fix: refresh generated tooltips when the accessible name changes - #92
Open
FutuRiata wants to merge 1 commit into
Open
fix: refresh generated tooltips when the accessible name changes#92FutuRiata wants to merge 1 commit into
FutuRiata wants to merge 1 commit into
Conversation
The effect that copies a button's accessible name into its title returns
early on every button that already has one:
if (button.title) {
return;
}
That leaves the MutationObserver watching aria-label with nothing to do.
The first pass gives each unlabelled button a title; every later pass hits
the guard and returns. A generated tooltip is written once and never again.
Toggle buttons are where it shows. ShapeInspector.tsx:480,483:
aria-label={locked ? "Unlock shape" : "Lock shape"}
aria-label={shape.hidden ? "Show shape" : "Hide shape"}
After one click the tooltip says the opposite of what the button does:
hovering a locked shape reads "Lock shape" while clicking unlocks. The
sketch image lock in SketchWorkspace.tsx:1241 behaves the same way.
Dropping the guard is not an option. Measured by walking the JSX with the
TypeScript parser: of 152 <button> elements, 53 set title in the markup,
and on 23 of those the title deliberately differs from the accessible name.
The six view cube buttons and the workplane button append the keyboard
shortcut ("Top view (5)" against aria-label "Top view", "Place workplane
(W)"); the shape menu entry explains the drag gesture in a full sentence;
the colour swatches show the hex code while the accessible name is
"Set color #d41721"; the transform and rotate handles carry a title and no
accessible name at all. Overwriting those would be a regression.
So the helper remembers what it wrote, in a WeakMap keyed by the element,
and only refreshes a title that still matches its own last value. A title
that came from the markup, or one that changed behind its back, is left
alone. Buttons with a markup title take the same early exit as before, so
the observer's hot path is unchanged.
The logic moves to src/lib/buttonTooltips.ts to make it testable; the effect
in SketchForgeEditor is three lines now. 269 tests pass, the 264 existing
ones plus five new.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TAoEuBEJyJtkEGapsp6G22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The effect in
SketchForgeEditor.tsxthat copies a button's accessible name into itstitlereturns early on every button that already has one:The
MutationObserverright below it watchesaria-labelon the whole subtree, so the intent to keep the tooltip in step with the name is clearly there. But the guard makes the observer a no-op: the first pass gives each unlabelled button a title, and every later pass hits the guard and leaves. A generated tooltip is written once and never again.Where it shows. Toggle buttons, whose accessible name is the thing that changes.
ShapeInspector.tsx:480,483:After one click the tooltip states the opposite of what the button will do: hovering the lock on an already-locked shape reads "Lock shape", and clicking it unlocks. The sketch image lock in
SketchWorkspace.tsx:1241behaves the same way. Found in a production build, by hovering, not by reading code.Why the guard cannot simply go. I walked the JSX with the TypeScript parser rather than trusting grep. Of 152
<button>elements inapps/web/src, 53 settitlein the markup, and on 23 of those the title deliberately differs from the accessible name:titleWorkplaneViewport.tsx:5048-5053(view cube, 6 buttons)Top view (5)Top viewWorkplaneViewport.tsx:5077Place workplane (W)Place workplaneWorkplaneViewport.tsx:5059,5064Show controlsShow camera controlsSketchForgeEditor.tsx:9983Add box at the sketch origin, or drag it onto the sketchBox(from the span)SketchForgeEditor.tsx:10032,10035UndoSketch undoShapeInspector.tsx:528#D41721Set color #d41721SketchWorkspace.tsx:1238Lock image (L)Lock sketch imageSketchWorkspace.tsx:1247Unlock image before deletingDelete sketch imageTransformOverlay.tsx:215,251Rotatepage.tsx:1771,1873Project optionsProject options for <name>Overwriting those would lose every keyboard shortcut hint in the viewport, so the fix has to tell "this tooltip was generated" from "this tooltip was authored".
What this does. The helper remembers what it wrote in a
WeakMapkeyed by the element and refreshes only a title that still matches its own last value. A title that came from the markup, or one that changed behind its back, is left alone. Buttons with a markup title take the same cheap early exit as before, so the observer's hot path is unchanged, and nothing is added to the DOM.The logic moves into
src/lib/buttonTooltips.tsso it can be covered by a unit test in the existing style; the effect inSketchForgeEditor.tsxis three lines now. If you would rather keep it inline, say so and I will fold it back — theWeakMapis the only part that matters.Transcribing the shipped
applyTitles()body verbatim into a scratch test and asserting the refresh makes it fail onLock shapewhereUnlock shapeis expected, which is the regression the new test pins down.269 tests pass, the 264 existing ones plus five new;
tsc --noEmitis clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01TAoEuBEJyJtkEGapsp6G22