CORE-2871: Make the TooltipGroup trigger a button that does something - #161
OpenStaxClaude wants to merge 4 commits into
Conversation
An accessibility evaluation flagged the info-icon trigger as a name/role/value failure: it is exposed as role=button, but pressing it did nothing useful. react-aria's useTooltipTrigger binds onPointerDown and onKeyDown to close the tooltip, so tabbing in opened it and Enter/Space then dismissed it, and on touch -- where hover never fires -- the tap closed it on pointerdown and the content was unreachable. TooltipGroup now owns the trigger state and presses toggle it. Because those library handlers run before onPress, the press records state at onPressStart rather than reading one react-aria has already flipped. Also fixes isOpen being spread into Tooltip instead of TooltipTrigger. react-aria's Tooltip builds its own state when passed isOpen, detached from the trigger's, so in controlled mode aria-describedby was never emitted at all and hover and Escape-to-dismiss stopped working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things the review caught on the press-to-toggle change. aria-expanded: the trigger now controls persistent content, and aria-describedby only supplies the description once the tooltip is already open — it says nothing about a control that can be opened and closed. The button reports its state. Duplicate onOpenChange: react-aria's useTooltipTrigger closes the tooltip from its own pointerdown/keydown handler, which runs through the same onOpenChange we hand TooltipTrigger, before our onPress finishes the toggle. A press on an open trigger therefore reported the close twice. setOpen now drops a call that would not change the value, which is the rule useControlledState already applies internally and also covers a press that reaches onPress without the library's handlers having fired first. Both are covered by tests that fail without the corresponding change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The touch behaviour was the part of this change I had only reasoned about, on the grounds that jsdom pinned the tooltip open. That was the wrong conclusion. The cause is that jsdom has no PointerEvent, and without one react-aria takes fallback branches that cannot represent touch at all: useHover binds onMouseEnter with a hardcoded 'mouse' pointer type, so a simulated tap opened the tooltip by "hover" and any passing test would have passed for the wrong reason. Installing a minimal PointerEvent polyfill puts useHover and usePress on the same branches a real browser takes, and triggerHoverStart then bails on pointerType 'touch' as it does on a device. The tap sequence is driven directly so the assertions are about the press and nothing else. Two tests: a tap opens the tooltip and the next one closes it, with aria-describedby, aria-expanded and a single onOpenChange call each way; and a companion asserting a touch pointerenter does not open it, which is what makes the first test meaningful rather than a restatement of the hover behaviour. The first fails without the onPress toggle, so it pins the fix. Pointer sizes are non-zero deliberately — isVirtualPointerEvent reads a zero-sized pointer as a screen reader and would route the press through usePress's virtual-click path instead of the touch one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RoyEJohnson
left a comment
There was a problem hiding this comment.
Address Copilot's latest comment.
defaultOpen was added by this change and had no tests. Four, in a nested block: - it starts open, with aria-describedby and aria-expanded following - false starts closed - it hands control to the user after the initial render, which is the behaviour that distinguishes it from isOpen - and the counterpart that gives that one its meaning: isOpen pins the state, so a dismiss the caller ignores must not close the tooltip Checked against the two ways this can plausibly break. Dropping the useState seed — the mistake of assuming defaultOpen is consumed by the library, which is exactly what went wrong for isOpen — fails the first and third. Treating defaultOpen as controlling rather than seeding fails the third. Neither is caught by the existing tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Right call — Four tests, and the interesting part is the pairing rather than the count:
I checked these against the two ways the prop can plausibly break rather than just watching them go green:
The first is worth calling out because it is precisely the mistake this PR already fixed for Full suite 788 tests, lint and typecheck clean. Still in draft per your note on the ticket — say the word and I will flip it. |

Jira: https://openstax.atlassian.net/browse/CORE-2871
What the evaluation found
A Level Access manual evaluation flagged the info-icon tooltip trigger (the "Multiple attempts" one on the assignment settings screen) as a WCAG 4.1.2 name/role/value failure: "It is functioning as a tooltip but incorrectly given as button."
Their recommendation was
aria-describedbyon the trigger plusrole="tooltip"on the tooltip element. I renderedTooltipGroupagainst the installedreact-aria-components@1.10.1and read the DOM before changing anything: both of those were already correct, and the screenshot attached to the ticket confirms it (Description reads "UNLIMITED ATTEMPTS If unlimite…",aria-describedbyis on the button). So the recommendation as written had nothing to fix.What was actually broken
The line in "Details of the issue" is the real defect.
useTooltipTriggerbinds bothonPointerDownandonKeyDowntoonPressStart, which closes the tooltip:role="button"promises — and it is dismissed. Nothing is ever activated.useHoverignores touch, and the tap closes on pointerdown, so the content is simply unreachable on a touch device.That is a role advertising an action the control does not have.
The change
TooltipGroupnow owns the trigger state and a press toggles it, so the button role describes real behaviour and the content is reachable by keyboard and by touch. Those library handlers run beforeonPress, so the press records the state atonPressStartrather than reading one react-aria has already flipped — there is a comment on the component saying so, because it looks removable and is not.Because the trigger now holds persistent state, it exposes
aria-expanded.aria-describedbysupplies the description once the tooltip is already open; it says nothing about a control that can be opened and closed, which is the "value" half of 4.1.2. Noaria-controls— react-aria keeps the tooltip id internal and screen-reader support for it is thin.onOpenChangereports each transition once. react-aria's close-on-pointerdown/keydown runs through the same callback beforeonPresscompletes the toggle, sosetOpendrops a call that would not change the value — the ruleuseControlledStatealready applies internally. Both of those came out of review.Also fixed, found along the way:
isOpenwas spread intoTooltipinstead of handed toTooltipTrigger. react-aria'sTooltipdoesstate = props.isOpen != null || props.defaultOpen != null || !contextState ? localState : contextState, so passing it to the tooltip element builds a second state the trigger knows nothing about — the trigger's state stays closed,aria-describedbyis never emitted at all, and hover and Escape-to-dismiss stop working.isOpenkeeps its public meaning and now drives the trigger;defaultOpenandonOpenChangeare accepted alongside it. Our own snapshot test went through that path, which is why the committed snapshot showed a trigger with noaria-describedby— the one-line snapshot change in this PR is that attribute appearing.ariaLabelis unchanged but now documented in the story. TheMore informationdefault repeats across every instance on a screen; naming the thing the tooltip is about is the caller's job. No assignments call site passes it today — that is the companion PR.Tests
New
name, role and stateblock inTooltip.spec.tsx:aria-describedbymatches the tooltip'sidwhen open (including via theisOpenprop, as a regression test), Enter and Space toggle rather than only dismissing, Escape closes,aria-expandedtracks the state both uncontrolled and viaisOpen,onOpenChangereports each transition exactly once, and the accessible name defaults and overrides.Touch is covered too, in a nested
touchblock. My first pass called it untestable because jsdom pinned the tooltip open; the real cause is that jsdom has noPointerEvent, and without one react-aria falls back to branches that cannot represent touch —useHoverbindsonMouseEnterwith a hardcoded'mouse'pointer type, so a simulated tap opened the tooltip by hover and would have passed for the wrong reason. A minimalPointerEventpolyfill (the checks run at render, not module load) putsuseHoverandusePresson the same branches a real browser takes, withtriggerHoverStartignoring touch as it does on a device. One test drives the tap sequence and asserts open → toggle shut witharia-describedby,aria-expandedand a singleonOpenChangeeach way; a companion asserts a touchpointerenterdoes not open it, which is what keeps the first test meaningful. The first fails without theonPresstoggle.defaultOpenhas its own block, since this PR introduced it: it starts open witharia-describedby/aria-expandedset,falsestarts closed, and — the behaviour that separates it fromisOpen— it hands control to the user after the initial render, paired with a test thatisOpenpins the state when the caller ignores the change. Both halves of that pair are load-bearing: dropping theuseStateseed (assuming react-aria consumes the prop, which is the mistake this PR fixed forisOpen) fails two of them, and treatingdefaultOpenas controlling fails a third.Full suite green (788 tests), lint and typecheck clean.
Notes for review
1.24.2, which is bumped but not yet tagged, so this entry rides in[Unreleased]. A behaviour change to a public component may deserve1.25.0instead — your call, happy to bump.@playwright/testis indevDependenciesbut has no config, no specs, and CI runs onlylint+test— standing up a browser harness is infrastructure, not a test, and shouldn't land inside an accessibility fix. Happy to file it separately if the team wants it.Follow-up
The flagged content is two headed sections of prose, and
role="tooltip"flattens that into a single unnavigable description string. The right answer for long-form content is a disclosure/popover rather than a tooltip; that is being tracked as a separate ticket rather than widened into this PR.🤖 Generated with Claude Code