feat: wysiwyg toolbar for basic edits to a button's style - #4341
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds atomic layered-style option updates and expands the layered button preview with dynamic sizing, element hit testing, selection, resizing, snapping, line editing, and quick actions. ChangesLayered button editing
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Controls/ControlTypes/Button/LayeredButtonStyleEditor.ts (1)
202-243: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard dynamic option writes in
updateOptions
updateOptionsstill writes arbitrary string keys from the API straight onto the element object. That leaves__proto__writable and can mutate the prototype chain. A smallObject.hasOwncheck, or at least blocking__proto__,constructor, andprototype, would close it without affecting the existingopt:*composite fields.
🧹 Nitpick comments (1)
webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/elementHitTest.ts (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the squareCoords math instead of mirroring it.
groupChildBoundsduplicatesGraphicsLayeredButtonRenderer.#drawGroupElement's squareCoords formula (acknowledged in the comment). Since that renderer already exposescomputeContentBoundspublicly for exactly this "avoid duplicating layout math" reason, exposing a small public static equivalent for the group-child-bounds calculation (inshared-lib/lib/Graphics/LayeredRenderer.ts) and importing it here would remove the risk of the two implementations silently drifting apart.♻️ Sketch of the shared helper
export class GraphicsLayeredButtonRenderer { + static computeGroupChildBounds(bounds: DrawBounds, squareCoords: boolean): DrawBounds { + if (!squareCoords) return bounds + const size = Math.min(bounds.width, bounds.height) + return new DrawBounds(bounds.x + (bounds.width - size) / 2, bounds.y + (bounds.height - size) / 2, size, size) + } + static async `#drawGroupElement`(...): Promise<DrawBounds> { const drawBounds = parentBounds.compose(element.x, element.y, element.width, element.height) if (skipDraw) return drawBounds - if (element.squareCoords) { - const squareSize = Math.min(drawBounds.width, drawBounds.height) - return new DrawBounds(...) - } - return drawBounds + return this.computeGroupChildBounds(drawBounds, element.squareCoords) }// elementHitTest.ts import { GraphicsLayeredButtonRenderer } from '`@companion-app/shared/Graphics/LayeredRenderer.js`' // walk(element.children, GraphicsLayeredButtonRenderer.computeGroupChildBounds(bounds, element.squareCoords), false)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a403c11-2aa0-4223-b694-984158bb81a7
📒 Files selected for processing (18)
companion/lib/Controls/ControlTypes/Button/Layered.tscompanion/lib/Controls/ControlTypes/Button/LayeredButtonStyleEditor.tscompanion/lib/Controls/ControlTypes/Button/Preset.tscompanion/lib/Controls/IControlFragments.tscompanion/lib/Controls/StylesTrpcRouter.tsshared-lib/lib/Graphics/LayeredRenderer.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/QuickActionsToolbar.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/SelectionOverlay.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/__tests__/elementHitTest.test.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/__tests__/fitCanvasSize.test.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/__tests__/snapping.test.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/boundsFields.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/canvasSize.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/elementHitTest.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/snapping.tswebui/src/Buttons/EditButton/LayeredButtonEditor/StyleStore.tsxwebui/src/scss/_button-edit.scss
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
ooh nice! Before I look at the code (or try to run it), my only thought is maybe this toolbar should be vertically along the left edge? |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b621f7f-75b2-48e7-9c4d-38a59f2145e7
📒 Files selected for processing (3)
webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/QuickActionsToolbar.tsxwebui/src/scss/_button-edit.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsx
| const button = ( | ||
| <button | ||
| type="button" | ||
| title={disabledReason ? undefined : title} | ||
| className={`button-layer-quick-action${active ? ' active' : ''}`} | ||
| onClick={onClick} | ||
| disabled={disabled} | ||
| > | ||
| <FontAwesomeIcon icon={icon} size="sm" /> | ||
| </button> | ||
| ) | ||
|
|
||
| // A disabled button doesn't emit hover events, so the trigger span (not the button) carries the tooltip | ||
| if (!disabledReason) return button | ||
| return ( | ||
| <Tooltip.Root> | ||
| <Tooltip.Trigger render={<span className="button-layer-quick-action-tooltip">{button}</span>} delay={300} /> | ||
| <Tooltip.Popup side="right" arrow size="md"> | ||
| {disabledReason} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
sed -n '110,165p' webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/QuickActionsToolbar.tsx
# Find other tooltip usages/patterns in the repo for comparison
rg -n "Tooltip\.Trigger render=<span|aria-label=|title=" webui/src -g '*.tsx'
# If the repo vendors Base UI docs or references, inspect tooltip-related usage
rg -n "tooltip" webui -g '*.md' -g '*.tsx' -g '*.ts'Repository: bitfocus/companion
Length of output: 48599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local tooltip wrapper and its tests
sed -n '1,220p' webui/src/Components/Tooltip.tsx
printf '\n--- tests ---\n'
sed -n '1,180p' webui/src/Components/__tests__/Tooltip.test.tsx
# Compare with a similar icon-only button that already uses title + aria-label
sed -n '1,120p' webui/src/Buttons/EditButton/ControlClearButton.tsx
sed -n '1,120p' webui/src/Layout/PanelIcons.tsxRepository: bitfocus/companion
Length of output: 12065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/QuickActionsToolbar.tsxRepository: bitfocus/companion
Length of output: 4085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '180,245p' webui/src/Modules/ImportCustomModule.tsx
sed -n '120,170p' webui/src/Components/Button.tsxRepository: bitfocus/companion
Length of output: 3588
🌐 Web query:
Base UI tooltip trigger accessibility disabled button span render prop accessible name focus hover keyboard
💡 Result:
In Base UI, the interaction between tooltips and disabled elements is managed by distinguishing between the tooltip's own disabled prop and the DOM disabled attribute of the trigger element [1][2]. Handling Disabled Elements: - Tooltip disabled prop: When you set disabled={true} on the <Tooltip.Trigger>, it prevents the tooltip from opening via hover or focus, but it does not add the disabled attribute to the underlying DOM element [1][2]. This allows you to maintain the element's focusability or provide other interactions [3]. - Rendering a native disabled button: If you use the render prop to pass a native disabled button (e.g., <Tooltip.Trigger render={<button disabled />} />), the tooltip may fail to trigger because browsers do not fire mouse events on disabled elements [1][4]. - Best practice for accessibility: Providing tooltips on disabled elements is generally discouraged because disabled elements are often not focusable by keyboard users, and tooltips are not accessible to screen readers [1][4]. A recommended alternative is to use an "infotip" pattern (e.g., a <Popover> with an icon) next to the button [4]. This ensures the information is accessible to keyboard, mouse, and touch users [4]. - Workaround for "pseudo-disabled" states: If you need to keep a trigger focusable while visually indicating it is disabled, use aria-disabled="true" instead of the native disabled attribute [4]. This ensures the element remains interactive for focus/hover events, allowing the tooltip to function if desired [4]. Keyboard and Focus Behavior: - Base UI ensures that if a trigger is marked as disabled (via the component's disabled prop), it will neither open on hover nor on keyboard focus [5][6]. - Recent updates have unified the behavior so that if a trigger is disabled, it will properly close any currently open tooltip when navigated to [5]. Accessible Name: - Base UI maintains that a tooltip's content is supplemental; it should not replace the element's label [1]. The trigger element must have its own accessible name (e.g., via aria-label) to ensure screen reader users can understand the purpose of the element independently of the tooltip [1].
Citations:
- 1: https://base-ui.com/react/components/tooltip
- 2: https://github.com/mui/base-ui/blob/d81ec002/packages/react/src/tooltip/trigger/TooltipTrigger.tsx
- 3: [tooltip] Allow disabling a
Tooltip.Triggerwhen using detached triggers mui/base-ui#3468 - 4: [tooltip] Trigger does not work when rendering disabled elements mui/base-ui#3075
- 5: [tooltip] Fix
disabledprop on Triggers mui/base-ui#4049 - 6: [tooltip] Prevent opening when focusing a disabled Trigger mui/base-ui#3902
Keep the disabled reason reachable for keyboard and screen-reader users. The icon-only button loses its accessible name when disabledReason is set, and the tooltip sits on a plain <span>, so the explanation is mouse-only. Consider keeping an aria-label on the button and exposing the disabled message on a focusable trigger or separate info/Popover control.
|
@Julusian Good thinking. I changed the toolbar to vertical. And, after that, the whole are started to all look a little off balance, so I did some experimenting with the aspect ratio selection + the simple/advanced toggle. Let me know what you think! I also added a tooltip about why the tools get disabled, for various reasons. ui-updates.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29433d13-4fa9-47f6-bc98-4e77a7b0383e
📒 Files selected for processing (5)
webui/src/Buttons/EditButton/LayeredButtonEditor/LayeredButtonEditor.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/__tests__/fitCanvasSize.test.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/canvasSize.tswebui/src/scss/_button-edit.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/canvasSize.ts
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsx
|
I like it! Only one concern with the aspect ratio, that we do want to allow custom aspect ratios, for testing for those other surface types that we dont have a builtin predefined button for. Maybe that could be a 4th button that opens a popover? Something vaguely like the grid zoom popover. Might be enough to have a dropdown in it, and nothing else? For that basic/advanced toggle, perhaps instead of advanced it should be 'all properties' or something? I dont mind the current naming, just thinking out loud. |
Julusian
left a comment
Julusian
left a comment
There was a problem hiding this comment.
Im happy with the code of this
Giving it a quick try, some UX thoughts (which may want to be follow ups):
- Lines dont appear to have drag markers for the start+end. Could they?
- The hitboxes are correct for the bounds, but make for slightly confusing selecting. for example, having a 100% height text box with a single line of text, it feels like I should be able to click around the text to select the background. But I expect this to be non-trivial to do some drawing aware hitbox detection.
I like that naming better! I changed it to that, and also made the buttons not have conflicting colors with the separator:
Good point. Added a custom icon, that accepts entering custom values for w/h. 
Yeah I had kinda punted on lines. But it feels right to include them. I added separate overlay indicators since they have different behaviors. Let me know what you think. 
Yeah, I agree with this. I'm not quite sure how to tackle it yet though. Might something for a followup, or just needs some more brainstorming. |
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99eda4a1-a259-438b-a0e4-508e6858fe45
📒 Files selected for processing (9)
webui/src/Buttons/EditButton/LayeredButtonEditor/ElementsList.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/LayeredButtonEditor.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LineSelectionOverlay.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/QuickActionsToolbar.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/SelectionOverlay.tsxwebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/boundsFields.tswebui/src/Buttons/EditButton/LayeredButtonEditor/Preview/elementHitTest.tswebui/src/scss/_button-edit.scss
🚧 Files skipped from review as they are similar to previous changes (4)
- webui/src/Buttons/EditButton/LayeredButtonEditor/LayeredButtonEditor.tsx
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/elementHitTest.ts
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/SelectionOverlay.tsx
- webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsx
|
I've fixed the merge conflicts and was about ready to merge it, but I've found that the markers arent following rotation: I also discovered Another thing to think about is #4381 which wants to add an aspect ratio option for the neo. I am not sure how to handle that, I dont want to have too many options on the bar for everyone and this may want a bigger solution out of scope of this pr (surface driven presets shown within the popover?) |
I couldn't obviously see the code for this bit in the PR, but do the current aspect ratios have tooltips to say what they're for? Actually I've just spotted it looks like they're built from the current ASPECT_RATIO_OPTIONS data.
I'd agree we don't want a never-ending list of options on the bar, and personally I'm not convinced having more they'd probably stop being clear what they're for, but could there be some sort of pop-up/drop-down to let you see the original/full list (what does the pencil currently do for example) as from a quick look the Loupedeck should be/can be 90x60, so that's yet another ratio too and I'm sure there are more already I'm not aware of, and more to come in the future... Edit I clearly need to read better, could the pencil also have the dropdown to populate the custom field (or custom as another option in that or something)? On a related note, which I suspect wants a separate feature request, it also strikes me you probably want the aspect ratio to be sticky per grid location, e.g. if I'm doing a SD+, I want the touch buttons on each page to preview correctly for that by default, but the other buttons to be "normal", I don't want to have to keep switching ideally, and this gets more and more important as the aspect gets further and further away from 1:1. |
Yeah I think my ideal solution would be to start storing the sizes/schema of each surface that is connected, which we can then use to populate more options here. So then once you have connected a neo, the option would appear. That would give us the most futureproof behaviour here, and avoids baking in details about every surface type. But I dont want to do that in this PR, I would like to get this merged before it gets more conflicts.
I think it depends. If you are programming a SDS, you wouldnt want to have to choose that for all 32 buttons. And for a plus-xl, I think it could be argued either way. If we had #2945, then it would make sense to tie to whatever surface type was selected perhaps with an auto-mode. But in the current setup, I'm not sure what is best |
The rebase dropped main's additions to _button-edit.scss (the wysiwyg toolbar from #4341 and the value-feedback overrides from #4379) as part of resolving the convert-to-CSS modify/delete. Re-apply them in button-edit.css: the canvas workspace + quick-actions/aspect toolbars, the panel resize bar with its mode-toggle, the elementlist heading/body split, the fixed layered- overrides table layout and the portaled aspect-custom popover. New colours go through tokens (a button-editor toolbar palette + shadow-4/-6 for the subtle lifts); the rest reuse existing surface/border/primary tokens.
The rebase dropped main's additions to _button-edit.scss (the wysiwyg toolbar from #4341 and the value-feedback overrides from #4379) as part of resolving the convert-to-CSS modify/delete. Re-apply them in button-edit.css: the canvas workspace + quick-actions/aspect toolbars, the panel resize bar with its mode-toggle, the elementlist heading/body split, the fixed layered- overrides table layout and the portaled aspect-custom popover. New colours go through tokens (a button-editor toolbar palette + shadow-4/-6 for the subtle lifts); the rest reuse existing surface/border/primary tokens.
The rebase dropped main's additions to _button-edit.scss (the wysiwyg toolbar from #4341 and the value-feedback overrides from #4379) as part of resolving the convert-to-CSS modify/delete. Re-apply them in button-edit.css: the canvas workspace + quick-actions/aspect toolbars, the panel resize bar with its mode-toggle, the elementlist heading/body split, the fixed layered- overrides table layout and the portaled aspect-custom popover. New colours go through tokens (a button-editor toolbar palette + shadow-4/-6 for the subtle lifts); the rest reuse existing surface/border/primary tokens.
3 participants
This adds a toolbar with some basic features to help speed up the styling process, without having to dive into the options / do math.
The toolbar contains:
Right now, if an expression is enabled for any of the position/size elements, it disables the toolbar.
wysiwyg.mp4
Summary by CodeRabbit