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 09:07
4 tasks done
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.
Not ready yet: one owner decision and three fixes remain. An engineering owner needs to decide whether @astryxdesign/core should publish the new HoverCardPressTrigger and TooltipPressTrigger types, which no current record names; any engineering owner, the author included, can record that by commenting /approve-spec <full head SHA>.
- With
focusTrigger="never", a pinned card or tooltip stays open after focus moves away, contrary to FR9; letting the shared press pin listen forfocusouton the trigger it wires fixes both components. - The diff still contains the unmerged #9232, so land it first and rebase until this pull request holds only the HoverCard change.
- Both changesets say the press stops at the trigger for
useHoverCardanduseTooltiptoo, but with the hooks a plainonClickrow still receives it.
[Automated review]
| const handleFocusOut = useCallback( | ||
| (e: FocusEvent) => { | ||
| // A pinned card closes when focus leaves the trigger and the card. | ||
| if (handlePressFocusLeave(e)) { |
There was a problem hiding this comment.
This is the only way a pinned card learns that focus left, but the focusout listener that calls it is attached only when focusTrigger is always, or auto on a focusable trigger (lines 619-625). With focusTrigger="never", pressing Tab after pinning leaves the card open with aria-expanded="true", contrary to FR9's "focus moving outside them closes it". useTooltip.tsx:581-585 has the same gating.
| } | ||
| return; | ||
| } | ||
| element.addEventListener('click', handlePress); |
There was a problem hiding this comment.
Suggest wiring focusout here too, beside click and keydown, so the pin closes on focus leaving whatever the host's focusTrigger is, and the hosts can drop their trigger-side handlePressFocusLeave calls.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] HoverCard (and `useHoverCard`) take `pressTrigger` (type `HoverCardPressTrigger`): `none` (the default, unchanged) or `toggle`. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the card open and the same press closes it, while hover and focus still preview it. A pinned card stays until Escape, a second press, a press outside, or focus leaving; a press on its own content keeps it, 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. The trigger reports `aria-expanded` and `aria-controls`, and the card's `label` (else its text) is announced once when it pins, inside a modal too (`component:HoverCard` FR9 and FR10). On a trigger that is not a button, a labelled card keeps its own popup state and tap-only opening; beside `pressTrigger="toggle"`, `touchTrigger` is ignored. |
There was a problem hiding this comment.
"The press stops at its trigger, so a clickable row, card, or label around it does nothing" holds for the HoverCard component. It does not hold for useHoverCard, where a plain onClick row still receives the press (as useHoverCard.doc.mjs says). Suggest scoping this sentence to the component.
| '@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. 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.
Same here: useTooltip callers inside a plain onClick row still get the row's click on a pinning press, and only the Tooltip component stops it.
`component:Tooltip` FR9 and FR10, `spec:AST-003` FR4: - `pressTrigger` (`none`, the default, or `toggle`) on Tooltip and useTooltip, with the type TooltipPressTrigger. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the tooltip and the same press closes it; hover and focus still preview 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 `pressTrigger` that changes after mount wires or unwires the trigger, and a tooltip whose `pressTrigger` turns to `none` 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; a trigger whose tooltip repeats its own text pins without toggle state (`Layer/usePressPin.ts`, the hover layers' shared press pin). - LayerPressTrigger 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:HoverCard` FR9, FR10 and AR3, on the shared press pin (`Layer/usePressPin.ts`): - `pressTrigger` (`none`, the default, or `toggle`) on HoverCard and useHoverCard, with the type HoverCardPressTrigger. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the card and the same press closes it; hover and focus still preview it. - A pinned card stays until Escape, a second press, a press or scroll outside, or focus moving onto an element outside the trigger and the card (the pin listens for it on the trigger, whatever `focusTrigger` is); a press on its own content keeps it. It takes Escape before the layer beneath it, controlled or not. useHoverCard makes one usePressPin call, and a card whose `pressTrigger` turns to `none` while pinned closes on leave again. - The press stops at its trigger. The trigger carries aria-expanded and aria-controls on a button trigger; on any other trigger a labelled card keeps its own (FR3). When a press pins a card with a `label`, the press handler says the label once through useAnnounce; otherwise the card's rendered text is announced once through useAnnounceRenderedText.
4 tasks done
vjeux
force-pushed
the
feat/hover-card-press-trigger
branch
from
November 1, 2026 13:13
36b7102 to
5015adf
Compare
There was a problem hiding this comment.
Not ready yet: two engineering-owner decisions and four fixes remain. An engineering owner needs to approve or reject the new anchorRef exception this pull request writes into component:Tooltip FR9 (current FR9 says a pinning press never reaches a row or container around the trigger), and to decide whether @astryxdesign/core should publish HoverCardPressTrigger and TooltipPressTrigger; any engineering owner, the author included, can record approval by commenting /approve-spec <full head SHA>.
- Land #9232 first and rebase, so this pull request holds only the HoverCard change instead of 2,716 authored lines.
- Both changesets say the press stops at the trigger for
useHoverCardanduseTooltiptoo, but a plainonClickrow around a bare hook trigger still activates; scope that sentence to the components. - A controlled labelled HoverCard announces its label as soon as a press asks it to open, even when the owner keeps it closed; announce only once it pins.
- Add HoverCard stop cases inside a ClickableCard, an Item row and a
<label>, which its verification row now claims.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] HoverCard (and `useHoverCard`) take `pressTrigger` (type `HoverCardPressTrigger`): `none` (the default, unchanged) or `toggle`. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the card open and the same press closes it, while hover and focus still preview it. A pinned card stays until Escape, a second press, a press outside, or focus leaving; a press on its own content keeps it, 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. The trigger reports `aria-expanded` and `aria-controls`, and the card's `label` (else its text) is announced once when it pins, inside a modal too (`component:HoverCard` FR9 and FR10). On a trigger that is not a button, a labelled card keeps its own popup state and tap-only opening; beside `pressTrigger="toggle"`, `touchTrigger` is ignored. |
There was a problem hiding this comment.
"The press stops at its trigger, so a clickable row, card, or label around it does nothing" holds for the HoverCard component. With the bare useHoverCard hook a plain onClick row still receives the press (as useHoverCard.doc.mjs says). Suggest scoping this sentence to the component and saying what a hook caller does.
| '@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.
Same for useTooltip: a plain onClick row around a bare hook trigger still gets the pinning press; only the Tooltip component stops it at a child trigger.
| if (current.isOpen !== undefined) { | ||
| if (!current.isOpen) { | ||
| armOutsideDismiss(); | ||
| announceName(); |
There was a problem hiding this comment.
For a controlled layer this announces the label before the owner answers onOpenChange(true). If the owner keeps isOpen={false}, a screen-reader user hears the card's name while nothing opens and the trigger says aria-expanded="false". FR10 announces it "when the card pins"; consider announcing once the controlled card actually shows pinned, with a test where the owner declines.
| | 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`, 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 added anchorRef sentence relaxes current FR9 ("does not reach a clickable row, card, or container around it"). That is a requirement change, so it needs an engineering owner's approval (/approve-spec <full head SHA>) or the anchored Tooltip has to stop the press as FR9 says today.
| | FR3, AR1 | `HoverCard.test.tsx` role and trigger-attribute tests | Labelled and unlabelled; supported and unsupported roles | Wrong role, or trigger attributes that are not replaced and restored as stated, fail. | `audit:HoverCard/accessibility` | | ||
| | FR4–FR6, AR2 | `HoverCard.test.tsx` hover, Escape, refocus, and touch tests; `useTouchTrigger.test.ts` and `touchTriggerTargets.test.tsx` disabled-control cases; tap-destination cases an evidence gap until built | Hover, focus, tap, Escape from trigger and content; a trigger inside a link or a clickable card; a disabled control inside a clickable row | A card that cannot be reached or dismissed, or that opens on a tap that already acts, fails. | `audit:HoverCard/behavior` | | ||
| | FR7 | `HoverCard.test.tsx` isEnabled, isDefaultOpen, and onOpenChange tests; `layerDismissalFamilies.test.tsx` controlled HoverCard case | Disabled, default open | Wrong state reporting fails. | `audit:HoverCard/behavior` | | ||
| | FR9, FR10, AR3 | `usePressPin.test.tsx` HoverCard suites (click, Enter, Space, and tap pin; second press, press outside, and focus leaving close; pointer leave and presses on the card keep the pin; labelled and unlabelled announcements; a non-button trigger) and stop-at-trigger cases; `layerDismissalFamilies.test.tsx` pinned HoverCard over a Dialog, uncontrolled and controlled, closed by Escape | Click, Enter, Space, and tap pin; second press, press outside, focus leaving, and Escape close; pointer leave and presses on the card keep the pin; labelled and unlabelled; non-button trigger; a toggle trigger inside a ClickableCard, an Item row, and a `<label>` | A press that does not pin or close as stated, a pin lost on pointer leave, a missing or stale `aria-expanded` or `aria-controls`, a second announcement, a non-button trigger without the warning or its tap fallback, or a pinning press that also activates the card, row, or labelled control around the trigger, fails. | `audit:HoverCard/behavior` | |
There was a problem hiding this comment.
The "stop-at-trigger cases" in usePressPin.test.tsx render Tooltip only; this row's ClickableCard, Item row and <label> states have no HoverCard test. They pass today, so three HoverCard cases would back the claim.
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 #9232. It contains #9232's commit (26d7af9) and this change (5015adf) until #9232 merges. Every local gate has finished at 5015adf; results under Testing.
Review only this pull request's change: vjeux/astryx@26d7af9...5015adf
User need
A hover card that is the only home of its information (a profile card on an avatar button, the details behind an icon-only trigger) can't be kept open by touch, keyboard or screen-reader users. Hover opens it and leaving closes it; nothing pins it.
Problem and solution fit
pressTrigger="toggle"makes the trigger a toggle, as it does for Tooltip in feat(Tooltip): pressTrigger="toggle" pins the tooltip on a press #9232. On a button trigger, a click, a tap, Enter or Space pins the card, and the same press closes it. Hover and focus still preview it.spec:AST-003FR4, DEC-6).Current specification
component:HoverCardFR9, FR10, AR3 and DEC-1;spec:AST-003FR4 and DEC-6. All are current at 1d8696a (#9175).Public delta and caller burden
pressTrigger: 'none' | 'toggle'onHoverCardanduseHoverCard, with the typeHoverCardPressTrigger. The defaultnonechanges nothing.toggleon a button trigger (a<button>orrole="button"), a click, a tap, Enter or Space pins the card, and the same press closes it.focusTriggeris.onOpenChangeand doesn't change itself.aria-expanded(true only while pinned) andaria-controlswhile the card shows. When a press pins a card that has alabel, the press handler says that label once; otherwise the card's rendered text is announced once by the shared rendered-text announcer (spec:AST-086FR12), which keeps blocks apart and leaves hidden content out. Both reach a polite live region throughuseAnnounce, so inside a modal they're heard in the modal's own region (spec:AST-086FR14).aria-expanded/aria-controls(FR3) and tap-only opening, with a development warning.onClickancestor still receives it; the component stops it.pressTriggerturns tononewhile pinned closes on leave again.pressTrigger="toggle",touchTriggeris ignored.API changes
Added public rows, decided by
component:HoverCardFR9 and FR10 (#9175). They go to human review underarchitecture:public-component-apiINV25.@astryxdesign/coreHoverCardProps.pressTrigger?: HoverCardPressTrigger(default'none')<HoverCard content={card} label="Profile" pressTrigger="toggle"><button type="button">Ada</button></HoverCard>@astryxdesign/coreHoverCardOptions.pressTrigger?: HoverCardPressTriggeruseHoverCard({pressTrigger: 'toggle'})@astryxdesign/coretype HoverCardPressTrigger = 'none' | 'toggle'const mode: HoverCardPressTrigger = 'toggle';Design and alternatives
Layer/usePressPin.ts, which owns the layer's touch trigger, souseHoverCardmakes oneusePressPincall (architecture:react-component-runtimeINV16).labelis the dialog's name, a string and often shorter than the content, so the press handler says it throughuseAnnouncewith no Effect; a card without one is announced from its rendered text through the shared announcer (architecture:react-component-runtimeINV10).isPressPinTrigger).Evidence
layerDismissalFamilies.test.tsxfail (2 of 2).pressTriggerturns tononenever closes on leave; with fc05aff, afocusTrigger="never"card stays pinned when focus moves away.Layer/usePressPin.test.tsx(14 HoverCard tests): click, tap, Enter and Space pin; a second press, a press outside and focus leaving close it; pointer leave and presses on the card keep the pin, including a press on its own text that moves focus to no element;pressTriggerturning tononewhile pinned; focus moving away underfocusTrigger="never"; a labelled card saying its label once when a press pins it, controlled too; an unlabelled card saying its own text once, paragraphs apart; the stop; a labelled card on a non-button trigger keeping its own popup state.layerDismissalFamilies.test.tsx: an uncontrolled and a controlled pinned HoverCard over a Dialog each take Escape first, and the Dialog stays open.Layer/usePressPin.calls.test.tsx:useHoverCardmakes oneusePressPincall per render, and every touch-trigger call comes from inside it.hooks/useAnnounceRenderedText.adopters.test.tsx: an unlabelled HoverCard pinned by a press hands its text to the shared announcer and announces none of it itself; the registry check listsuseHoverCardas an adopter.TouchTrigger.a11y.chromium.spec.ts: under a mouse in Chromium, a pinned unlabelled card is announced as "Ada Lovelace Analyst, Payments", its paragraphs apart, and a click on its text keeps the pin (newHoverCardTogglestory).verified_by).Why it is split
The HoverCard wiring (about 580 lines, under
component:HoverCardFR9, FR10 and AR3) is its own pull request, stacked between #9232 and #9246, by the maintainers' ruling. All three ship in the same release. This one is 15 files, +607/−73: HoverCard's wiring, its tests and story, its record and docs, its changeset, and the shared hook's notes naming HoverCard.Scope
Writing rubric results
The rubric in
docs/contributing/spec-writing.md, over the changed parts ofHoverCard.spec.md:Testing
Local gates at 5015adf (on #9232's 26d7af9, on main a55352c):
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.useHoverCard.doc.mjs, which is unformatted on main too and outside the repository's formatting scope.pnpm test:ui: 17,444 tests pass.pnpm test:node: 15,102 pass. Its 2 failures, in the releasecache.test.mjs, 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).TouchTrigger.a11y.chromium.spec.tspasses in Chromium, twice (40 of 40), the mouse cases included..a11y.browser.spec.tsspecs pass ten times each on one worker with 4 CPUs: 220 of 220.