Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
requested review from
cvkxx,
ernestt,
kentonquatman and
rubyycheung
November 1, 2026 16:27
This was referenced Nov 1, 2026
github-actions
Bot
added
the
needs:spec-owner-review
Contributor
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering-owner decision before it can be approved. The head rewrites component:Tooltip FR9 so that, with anchorRef or a useTooltip caller's trigger, a plain onClick ancestor still receives the pinning press, and the current FR9 stop clause does not allow that. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA on this pull request, and the author may do so as an engineering owner. Whatever the decision, these still need changes:
- An own-text trigger inside a Table row that expands on click also expands the row. The press-pin guard is only in
useClickableContainer, not in the Table row handlers that share its checks. - The changeset says a clickable row around a
useTooltiptrigger does nothing, but a plainonClickrow still receives the press from the hook. - With #9232's commit included, this is 2,068 authored lines. Landing #9232 first and rebasing would leave only this change's 163 lines.
[Automated review]
| | FR6 | Escape closes a visible tooltip; Escape during text composition is ignored. Escape routing follows `family:overlay-dismissal`: a visible topmost tooltip in the page handles that Escape, so a layer underneath stays open for that press. Implementation gap: a tooltip rendered into another document, such as an iframe or a Picture-in-Picture window, with no layer beneath it is never closed by Escape (Current-state impact). `spec:AST-003` FR4, accepted and not yet implemented, makes an unpinned tooltip passive and gives a pinned one Escape first (Current-state impact). Outside `pressTrigger="toggle"`, pressing an uncontrolled tooltip's trigger closes it; a press that focuses a text-entry trigger reopens it through FR4. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the tooltip shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled tooltip answers Escape by calling `onOpenChange(false)` without hiding, and is not closed by pressing the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, any focus FR4 opens on, keyboard focus or a click into a text-field trigger, still opens the tooltip and reports `onOpenChange(true)`, and Escape then only reports `onOpenChange(false)`, so it stays until focus or the pointer leaves the trigger or the tooltip, or the caller re-renders with a new `onOpenChange` (Current-state impact). `isEnabled={false}` disables triggers and drops a pending open. | Current source and tests | | ||
| | FR8 | `onOpenChange` reports each show and hide. `hasHoverIndication` (`auto`: a single string or number child) adds a hover cue to the inline trigger. | Current source and tests | | ||
| | FR9 | With `pressTrigger="toggle"` and a trigger whose role is `button` (a `<button>` or `role="button"`), a click, a completed tap, Enter, or Space on the trigger pins the tooltip open, and the same press on a pinned tooltip closes it. Hover and keyboard focus still preview it per FR3 and FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned tooltip stays open when the pointer leaves the trigger and the surface; Escape (FR6), a second press, a press outside the trigger and the surface, or focus moving outside them closes it. In a controlled tooltip a press reports the toggled value through `onOpenChange` and does not change the state itself (FR7). A press that pins or closes the layer stops at its trigger: it does not reach a clickable row, card, or container around it, or the control a label names. With `anchorRef`, or `useTooltip` with a caller's trigger, the caller's own handlers still receive the press: the anchor's `onClick` runs, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it; Astryx clickable rows, cards, and containers ignore it. A trigger whose role is not `button` gets a development warning and keeps tap-only opening: a tap opens the tooltip as `touchTrigger="tap"` does, and no press pins it. A trigger whose tooltip repeats its own visible text, such as a truncated Text that is its own tab stop, may take `pressTrigger="toggle"` without being a button: a press pins it as on a button trigger, and it gets no development warning, no `aria-expanded` or `aria-controls`, and no announcement, since its text is already its content. Implementation gap: no component reaches the own-text clause yet (Current-state impact). | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The new sentence "With anchorRef, or useTooltip with a caller's trigger, … a plain onClick element around the anchor receives it" is a carve-out from the current FR9 stop clause ("does not reach a clickable row, card, or container around it"). An engineering owner needs to approve it, or the anchorRef and hook paths need to conform.
| if (disabled) { | ||
| // A press that pinned or closed a hover layer stops at its trigger: the | ||
| // container does not act on it (component:Tooltip FR9). | ||
| if (disabled || isPressPinClick(event.nativeEvent)) { |
There was a problem hiding this comment.
The guard only covers the hook's own onClick. The Table row-expansion and tree-data row clicks share this hook's checks through hasInteractiveAncestor and miss it, so an own-text trigger in a clickable Table row both pins and expands the row. Moving the check into the shared predicate, or adding it to both row handlers, keeps the three surfaces aligned.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Tooltip (and `useTooltip`) take `pressTrigger` (type `TooltipPressTrigger`): `none` (the default, unchanged) or `toggle`. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the tooltip open and the same press closes it, while hover and focus still preview it. A pinned tooltip stays until Escape, a second press, a press outside, or focus leaving, and it takes Escape before the layer beneath it. The press stops at its trigger, so a clickable row, card, or label around it does nothing, and the trigger's own handlers still run. With `anchorRef`, Tooltip renders nothing around the trigger to stop it: the anchor's own `onClick` runs, Astryx clickable rows, cards, and containers ignore the press, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it. The trigger reports `aria-expanded` and `aria-controls`, and the tooltip's text is announced once when it pins, inside a modal too (`component:Tooltip` FR9 and FR10). A trigger that is not a button gets a development warning and keeps tap-only opening; beside `pressTrigger="toggle"`, `touchTrigger` is ignored. |
There was a problem hiding this comment.
For useTooltip, "a clickable row, card, or label around it does nothing" is not what the hook does: a plain onClick row still receives the press (the head's useTooltip.doc.mjs says so). Naming useTooltip beside anchorRef in the exception sentence fixes it.
`component:Tooltip` FR9 and FR10, `spec:AST-003` FR4: - `triggerBehavior` on Tooltip and useTooltip, with the type TooltipTriggerBehavior: `adaptive`, the default and the released behavior (a tap opens the tooltip unless the trigger acts; an explicit `touchTrigger` still decides taps under it until the next minor); `hover` (touch never opens it); or `toggle`. Hover and focus preview it under every value. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the tooltip and the same press closes it. - A pinned tooltip stays until Escape, a second press, a press or scroll outside (the touch trigger's outside dismissal, which the pin arms: the pin owns the layer's touch trigger, so useTooltip makes one usePressPin call), or focus moving onto an element outside the trigger and the tooltip (the pin listens for it on the trigger, whatever `focusTrigger` is); a press on the tooltip's own text keeps it. It takes Escape before the layer beneath it. A `triggerBehavior` that changes after mount wires or unwires the trigger, and a tooltip whose `triggerBehavior` turns to `adaptive` while pinned closes on leave again. - The press stops at its trigger. With `anchorRef`, the caller's own handlers still receive the press: the anchor's `onClick` runs, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it; Astryx clickable rows, cards, and containers ignore it (FR9). The trigger carries aria-expanded and aria-controls on a button trigger, and the tooltip's rendered text is announced once when it pins, through useAnnounceRenderedText (`spec:AST-086` FR12). - A non-button trigger gets one development warning and keeps tap-only opening (`Layer/usePressPin.ts`, the hover layers' shared press pin). - LayerTriggerBehavior and the touch trigger's armOutsideDismiss (useTouchTriggerInternal) stay internal: neither is exported from the Layer entry point, and the public useTouchTrigger return is unchanged.
`component:Tooltip` FR5 and FR9's `triggerBehavior` verification row: - Tooltip.test.tsx: `adaptive` by default and explicitly, `hover`, and `toggle`, each under a mouse hover and keyboard focus (both preview it) and a touch and a pen tap (which opens it, never opens it, or pins it); an explicit `touchTrigger` decides taps beside `adaptive` and is ignored beside `hover` and `toggle`. - TouchTrigger.a11y.chromium.spec.ts: a real finger and a pen-typed press on the new TriggerBehaviors story: a tap never opens `hover`, opens `adaptive`, and pins `toggle`. - The Tooltip record's `triggerBehavior` verification row names this evidence.
…out toggle state `component:Tooltip` FR9's own-text clause, on the shared press pin (`Layer/usePressPin.ts`): - usePressPin's internal `repeatsTriggerText` option, passed through useTooltipInternal (useTooltip with Core's internal options), pins a trigger whose tooltip repeats its own visible text, such as a truncated Text that is its own tab stop, without a button trigger: no development warning, no aria-expanded or aria-controls, and no announcement. - Astryx clickable containers ignore a press-pin click (`isPressPinClick` in useClickableContainer), so such a trigger, which is not a nested button, does not activate the card or row around it when no component wrapper stops the press. - No component passes the option yet; the record keeps that implementation gap.
vjeux
force-pushed
the
feat/tooltip-own-text-pin
branch
from
November 2, 2026 00:43
d35f344 to
f3f40ea
Compare
github-actions
Bot
removed
the
needs:design-review
4 tasks done
There was a problem hiding this comment.
This needs an engineering-owner decision before it can be approved: the head adds a sentence to component:Tooltip FR9 under which a plain onClick element around an anchorRef or useTooltip trigger still receives the pinning press, and the current FR9 stop clause does not allow that. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA on this pull request (the author may do so as an engineering owner), or asks for the anchorRef and hook paths to conform. Whatever the decision, these still need changes:
- An own-text trigger pins and also activates an Item or TreeList row with
onClickor a Table row that expands on click, and an anchoredrole="button"trigger activates an Item row, because onlyuseClickableContainerreads the press-pin mark. - The re-wire Effect in
usePressPin.tsleavesrepeatsTriggerTextout of its settle key and dependencies, so turning the option on after mount never wires the trigger. - With #9232 and #9406 included this is 2,286 authored lines; landing them first and rebasing onto
main, which also clears the conflicts inTooltip.spec.md,useTooltip.tsxanduseTooltip.doc.mjs, leaves this change's 163 lines.
[Automated review]
| if (disabled) { | ||
| // A press that pinned or closed a hover layer stops at its trigger: the | ||
| // container does not act on it (component:Tooltip FR9). | ||
| if (disabled || isPressPinClick(event.nativeEvent)) { |
There was a problem hiding this comment.
This is the only place the press-pin mark is read. An Item row with onClick (Item.tsx handleContainerClick), a TreeList row with onClick (TreeListItem.tsx handleClick) and the Table row-expansion and tree-data rows don't go through this onClick, so an own-text trigger pins and also activates those rows, and an anchored role="button" trigger activates an Item row. Reading the mark in the walk they share (or in each handler) keeps FR9's stop clause for all of them.
| // it here: the host's ref callback does not run again for that change. | ||
| useEffect(() => { | ||
| wire(offeredRef.current); | ||
| }, [triggerBehavior, wire]); |
There was a problem hiding this comment.
wire reads repeatsTriggerText, but neither this Effect's dependencies nor the settle key (element and triggerBehavior) include it, so a tooltip whose repeatsTriggerText turns true after mount never wires its trigger. Adding it to both fixes that.
| | FR6 | Escape closes a visible tooltip; Escape during text composition is ignored. Escape routing follows `family:overlay-dismissal`: a visible topmost tooltip in the page handles that Escape, so a layer underneath stays open for that press. Implementation gap: a tooltip rendered into another document, such as an iframe or a Picture-in-Picture window, with no layer beneath it is never closed by Escape (Current-state impact). `spec:AST-003` FR4, accepted and not yet implemented, makes an unpinned tooltip passive and gives a pinned one Escape first (Current-state impact). Outside `triggerBehavior="toggle"`, pressing an uncontrolled tooltip's trigger closes it; a press that focuses a text-entry trigger reopens it through FR4. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the tooltip shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled tooltip answers Escape by calling `onOpenChange(false)` without hiding, and is not closed by pressing the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, any focus FR4 opens on, keyboard focus or a click into a text-field trigger, still opens the tooltip and reports `onOpenChange(true)`, and Escape then only reports `onOpenChange(false)`, so it stays until focus or the pointer leaves the trigger or the tooltip, or the caller re-renders with a new `onOpenChange` (Current-state impact). `isEnabled={false}` disables triggers and drops a pending open. | Current source and tests | | ||
| | FR8 | `onOpenChange` reports each show and hide. `hasHoverIndication` (`auto`: a single string or number child) adds a hover cue to the inline trigger. | Current source and tests | | ||
| | FR9 | With `triggerBehavior="toggle"` and a trigger whose role is `button` (a `<button>` or `role="button"`), a click, a completed tap, Enter, or Space on the trigger pins the tooltip open, and the same press on a pinned tooltip closes it. Hover and keyboard focus still preview it per FR3 and FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned tooltip stays open when the pointer leaves the trigger and the surface; Escape (FR6), a second press, a press outside the trigger and the surface, or focus moving outside them closes it. In a controlled tooltip a press reports the toggled value through `onOpenChange` and does not change the state itself (FR7). A press that pins or closes the layer stops at its trigger: it does not reach a clickable row, card, or container around it, or the control a label names. With `anchorRef`, or `useTooltip` with a caller's trigger, the caller's own handlers still receive the press: the anchor's `onClick` runs, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it; Astryx clickable rows, cards, and containers ignore it. A trigger whose role is not `button` gets a development warning and keeps tap-only opening: every completed tap opens the tooltip, and no press pins it. A trigger whose tooltip repeats its own visible text, such as a truncated Text that is its own tab stop, may take `triggerBehavior="toggle"` without being a button: a press pins it as on a button trigger, and it gets no development warning, no `aria-expanded` or `aria-controls`, and no announcement, since its text is already its content. Implementation gap: no component reaches the own-text clause yet (Current-state impact). | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The new sentence "With anchorRef, or useTooltip with a caller's trigger, … a plain onClick element around the anchor receives it" is an exception to the current stop clause ("does not reach a clickable row, card, or container around it"). It needs an engineering owner's /approve-spec for this head, or the anchorRef and hook paths should conform.
This branch was successfully deployed
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.
Status: draft, stacked on #9406, which stacks on #9232. It contains their commits (3870f5e, 169e82b) and this change (f3f40ea) until they merge. Every gate under Testing finished at this head, on main a68e0e3.
Review only this pull request's change: vjeux/astryx@169e82b...f3f40ea
User need
A truncated Text that is its own tab stop has a tooltip that repeats its full text. Under
triggerBehavior="toggle",component:TooltipFR9 lets that trigger pin its tooltip without being a button, so touch and keyboard users can keep the full text open. It gets no toggle state and no announcement, since its text is already the trigger's content.Problem and solution fit
repeatsTriggerTextoption on the shared press pin, passed throughuseTooltipInternal, pins such a trigger on a press. It gets no development warning, noaria-expandedoraria-controls, and no announcement (FR9's own-text clause).useClickableContainerignores a press-pin click, so the card or row around the trigger stays inactive even where no component wrapper stops the press.Current specification
component:TooltipFR9, its own-text clause, current at 1d8696a (#9175). The prop's name and values,triggerBehavior('adaptive' | 'hover' | 'toggle', default'adaptive'), are the records pull request #9379's, merged as a68e0e3; this pull request is built on it, and the record lines it changes already use #9379's wording.Public delta and caller burden
None. The option is internal (
useTooltipInternalis@internal), no component passes it yet, anduseTooltipkeeps its public signature.API changes
None.
Design and alternatives
isPressPinClick). Every Astryx clickable surface built onuseClickableContainerignores that press: ClickableCard, Item, SelectableCard, TreeListItem, TokenLink, FileInput and Overlay.Evidence
useClickableContainer, the new ClickableCard case fails, because the card opens.Layer/usePressPin.test.tsx:Why it is split
#9232 was over the 2,000-line review threshold, and this unit separates cleanly. No component passes the option. The guard serves only this trigger, since Astryx clickable containers already skip a click on a nested button, the only kind of trigger #9232's toggle wires. This pull request is 5 files, +137/−26. All five (#9232, #9406, this one, #9291 and #9246) ship in the same release.
Scope
Writing rubric results
The rubric in
docs/contributing/spec-writing.md, over the changed parts ofTooltip.spec.md:Testing
Local gates at f3f40ea (on the evidence pull request's 169e82b and #9232's 3870f5e, on main a68e0e3):
pnpm check:repo,pnpm check:knowledge,pnpm check:changesets,pnpm typecheck(0 errors),pnpm lint(0 errors), CI-strict ESLint on the changed files, the private-attribute baseline check, the root build, and the Storybook build.pnpm test:ui: 17,559 tests pass.pnpm test:node: 15,664 pass. Besides the 2 releasecache.test.mjsfailures, which are independent of this change (fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520), 8 tests incomponent-size.test.mjs,doctor-integration.test.mjsandcomponent.test.mjshit their 30-second timeout on a machine at a load average near 66; those three files pass on a rerun at this head (78 of 78), and this change touches none of their code.TouchTrigger.a11y.chromium.spec.tspasses in Chromium, twice (42 of 42)..a11y.browser.spec.tsspecs pass ten times each on one worker with 4 CPUs: 220 of 220.