feat: let a Tooltip trigger be a control - #29
Merged
Merged
Conversation
`toggleable` gives the trigger role="button" with an aria-expanded that tracks the content, and makes Enter and Space toggle it. Both keys are prevented from their defaults, since Space scrolls the page and Enter submits an enclosing form. Off by default, so no existing trigger changes. A trigger whose content only supplements what is already on screen is not a control and should not say it is, which is why this is opt-in rather than the other way round. A glossary term is the opposite case: the definition is summoned deliberately, and without hover, activation is the only way to it. Both continuum-hub and backend.ai-go had built this same contract around a glossary term (tabIndex, role="button", aria-describedby, aria-expanded, Enter and Space) before this component existed. Adopting it cost the first of them that affordance, which is not a reasonable price for sharing code, and would have cost the second the same. Six tests, each checked against the perturbation that should break it: dropping the role, dropping aria-expanded, dropping the preventDefault, wiring the handler unconditionally, and handling Enter without Space.
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
1 participant
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.
Tooltiptakestoggleable. With it the trigger carriesrole="button"and anaria-expandedthat tracks the content, and Enter and Space toggle it. Off by default, so nothing that exists today changes.Why
A trigger whose content only supplements what is already on screen is not a control and should not announce itself as one, which is why this is opt-in and not the reverse. A glossary term is the opposite case: the definition is something a reader summons deliberately, and where there is no hover at all, activation is the only route to it.
This is not hypothetical. Both
continuum-hubandbackend.ai-gohave aTermcomponent for glossary phrases, both written before this package existed, and both built the same trigger contract around it:tabIndex={0},role="button",aria-describedby,aria-expandedbound to the open state, Enter and Space toggling, Escape closing. Two products, the same shape, arrived at independently.continuum-hub adopted this component in lablup/continuum-hub#1057 and lost that affordance, because the wrapper has no seam for it. I recorded the loss as the price of composing rather than reimplementing. That was the wrong call: the package is ours to change, and a shared component that costs accessibility to adopt will be worked around rather than adopted.
What it does
role="button"andaria-expandedon the wrapper, and a keydown handler for Enter and Space that flips visibility. Both keys are prevented from their defaults, since Space scrolls the page and Enter submits an enclosing form. Everything else is untouched: hover, focus, Escape and the placement all behave as they did, andaria-expandedreports the state however it was reached, including a plain hover.No pinning. Toggling open then moving the pointer away closes it, which is what both existing implementations do.
The prop docs say not to combine it with
tabIndex={-1}. That is for wrapping an element that is already focusable, which then owns its own role.Tests
Six, each verified against the perturbation that should break it: dropping the role, dropping
aria-expanded, dropping thepreventDefault, wiring the handler unconditionally rather than behind the flag, and handling Enter without Space. One of them asserts the default case claims nothing: no role, noaria-expanded, and Space left to the page.pnpm run verifypasses.Refs lablup/continuum-hub#996