Repository navigation
feat(Tooltip, HoverCard, cli): deprecate touchTrigger="tap" (DEP-0016), with its upgrade codemod - #9246
feat(Tooltip, HoverCard, cli): deprecate touchTrigger="tap" (DEP-0016), with its upgrade codemod#9246vjeux wants to merge 3 commits into
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
added
the
needs:spec-owner-review
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.
Changes requested: the deprecation overloads and the upgrade listing work as described, but the codemod and the DEP-0016 record need fixes, and the diff should be rebased onto #9232 once it merges (2,846 authored lines today).
- The codemod rewrites
<span role="button">,<svg role="button">and<Icon role="button">triggers topressTrigger="toggle", which changes what a click, Enter or Space does; leave those with a TODO and list them. - DEP-0016 is marked
deprecatedwith its not-built note removed whileFieldLabel, LabInfoTipand theTouchTriggersstory still passtouchTrigger="tap"; migrate Lab InfoTip here and keep a current-state note for the rest. - A
pressTriggerthat changes fromnonetotoggleafter mount never wires the trigger. usePressPinkeeps the trigger element in React state, anduseTooltipanduseHoverCarduse an Effect only to copy callbacks into a ref.
[Automated review]
| if (icons.has(name.name) || name.name === 'svg') { | ||
| return true; | ||
| } | ||
| if (name.name !== 'span') { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This accepts any Core Icon, <svg> or <span> without reading its attributes, so <span role="button" tabIndex={0} onClick={open}>, <svg role="button"> and <Icon role="button" /> are rewritten to pressTrigger='toggle' with no TODO. FR9 pins a role="button" trigger, so after the upgrade a click, Enter or Space pins the layer and the trigger gains aria-expanded, unlike tap. Leaving an element with a role, tabIndex, event handler or spread for a person keeps the rewrite behavior-preserving (DEP-0016 codemod row).
| @@ -96,7 +98,7 @@ in `Tooltip.doc.mjs`. | |||
| | codemod | An `astryx upgrade` codemod staged in `packages/cli/assets/codemods/transforms/next/`, with tests and fixtures, checked by the codemod verify workflow, and available in the release that deprecates `tap`: it rewrites static `touchTrigger="tap"` to `pressTrigger="toggle"` only where the trigger is statically a non-interactive icon or span, which FR9 keeps on tap-only opening, reports every other `touchTrigger="tap"` site (an own-text trigger, a button, a link, or an unknown element) with a manual-migration note and leaves it unchanged, removes static `touchTrigger="auto"`, and reports spreads and dynamic values it cannot prove (`spec:AST-017` FR29c, FR29d) | | |||
| | downstream | In the deprecating release, Core FieldLabel's info icon moves from `touchTrigger="tap"` to `component:InfoTip` with `pressTrigger="toggle"` (`component:FieldLabel` FR9), and Lab InfoTip and the CLI page templates migrate too, the templates to a button trigger with `pressTrigger="toggle"` so they carry no development warning (`spec:AST-017` FR29c); docs, stories, templates, and examples teach only `pressTrigger` | | |||
| | transition | no prior transition | | |||
| | state | `proposed` | | |||
| | state | `deprecated` | | |||
There was a problem hiding this comment.
FieldLabel.tsx:316, packages/lab/src/InfoTip/InfoTip.tsx:117 and apps/storybook/stories/Tooltip.stories.tsx:373 still pass touchTrigger="tap", which DEP-0016's downstream row moves in the deprecating release. With the not-built item deleted, nothing in the record says so; keep a current-state item for the sites that cannot move here.
| }, []); | ||
|
|
||
| // The wired trigger, as state: its ARIA follows the pin through effects. | ||
| const [trigger, setTrigger] = useState<HTMLElement | null>(null); |
There was a problem hiding this comment.
architecture:react-component-runtime INV1 keeps DOM nodes out of React state. triggerRef already holds the node; a boolean (wired or not) in state is enough for the ARIA Effects.
| if (element == null || current.pressTrigger !== 'toggle') { | ||
| setTrigger(null); | ||
| return; |
There was a problem hiding this comment.
pressTrigger is read only when the trigger ref callback runs, and that callback does not depend on it, so a Tooltip re-rendered from none to toggle never wires its button (no pin, no aria-expanded on click).
| useEffect(() => { | ||
| touchDismissRef.current = {arm: armOutsideDismiss, disarm: clearTapOpen}; | ||
| }, [armOutsideDismiss, clearTapOpen]); |
There was a problem hiding this comment.
This Effect synchronizes no external system (INV3); it only copies two callbacks into a ref read from handlers. Assigning the ref during render, as usePressPin does with optionsRef, removes it (same in useHoverCard.tsx:463).
b9be726 to
e1e56b2
Compare
|
Fixed in e1e56b2:
Not changed:
|
There was a problem hiding this comment.
Changes requested: the codemod and record fixes from the last round hold, but a few items remain. Please land #9232 first and rebase, since the combined diff is 3,451 lines, and then address these:
- Keep an implementation-gap note for FR9's own-text pin in
Tooltip.spec.md: no component or public option reaches it yet, and<Tooltip pressTrigger="toggle">on a text trigger warns instead of pinning. - Reword the changeset's "where nothing else changes": each trigger the codemod rewrites logs FR9's development warning.
- In
usePressPin.ts, stop thepressTriggerEffect from re-wiring on mount (it logs that warning twice per trigger), warn once per trigger, and drop the unmount-onlyunwireEffect that the trigger's ref already covers.
[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. 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. | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
This row no longer says FR9 is unbuilt, but its own-text clause (a trigger whose tooltip repeats its text pins without being a button, with no warning) is reachable only through repeatsTriggerText on the internal useTooltipInternal, and no component passes it. <Tooltip content="Quarterly revenue by region" pressTrigger="toggle">Quarterly revenue by region</Tooltip> warns and does not pin. Please keep an implementation-gap note for that clause until something reaches it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] `touchTrigger="tap"` on Tooltip, HoverCard, `useTooltip` and `useHoverCard` is deprecated in favor of `pressTrigger="toggle"` (DEP-0016); it keeps working as released, and beside `pressTrigger="toggle"` it is ignored. A `tap` call resolves to a `@deprecated` signature, so editors mark it with its replacement, and the docs name it. `astryx upgrade` rewrites static `touchTrigger="tap"` to `pressTrigger="toggle"` on an icon or span trigger with no `role`, `tabIndex`, event handler, or spread props, where nothing else changes, and removes static `touchTrigger="auto"`. Every other `tap` site keeps its prop with a TODO naming the manual migration, and the upgrade lists each one with its file and line among the manual migrations it cannot make: a button, a link, an element with a `role`, `tabIndex`, event handler, or spread props, a trigger whose layer repeats its own text, a `useTooltip` or `useHoverCard` options object, an element it cannot identify, a computed value, or spread props. A core codemod reports such a site by passing a `ManualMigrationRequired` to `api.report`, and still keeps its changes to the file. The table-comparison page template now uses the toggle; its modality rail becomes a button so it can pin its explanation. |
There was a problem hiding this comment.
"where nothing else changes": every trigger this rewrites is a non-button, so in development each now logs FR9's warning (twice per mount at this head). DEP-0016's migration row says production behaviour is kept and the warning appears in development; the release note should say the same.
| useEffect(() => { | ||
| wire(offeredRef.current); | ||
| }, [pressTrigger, wire]); |
There was a problem hiding this comment.
This also runs on mount, after the host's ref already called attach, so a non-button trigger reaches devWarn twice (and again on each re-attach). Skipping the call when pressTrigger matches the mode the trigger was last wired for, and warning once per trigger (useDevWarning or warnOnce), would keep the post-mount rewiring without the repeat (INV3).
| writeAttribute(trigger, 'aria-controls', isLayerOpen ? layerId : null); | ||
| }); | ||
|
|
||
| useEffect(() => unwire, [unwire]); |
There was a problem hiding this comment.
The hosts' ref already unwires on unmount: React calls it with null, and attachPressPin(null) reaches unwire through wire(null). This Effect names no system and repeats that detach, so it can go (INV3, INV6).
e1e56b2 to
c21b1fb
Compare
|
Fixed in c21b1fb:
|
There was a problem hiding this comment.
Changes requested: the deprecation metadata, the codemod and the upgrade listing match DEP-0016, but the diff still carries the stacked Tooltip and HoverCard pressTrigger commits (3,546 lines in all), and the codemod mishandles spread props. Please land #9232 and #9291 first and rebase so only the deprecation commit remains, then fix these two cases in decide() and add them to the codemod tests:
pressTrigger="toggle"followed by{...rest}andtouchTrigger="tap"losestouchTriggerwithout a TODO or listing, although a spreadpressTriggerwould override the static toggle.- A spread
touchTrigger="auto"is listed with a note telling the caller to passpressTrigger="toggle", which would change what a press does on a button trigger.
[Automated review]
| * @returns {'rewrite' | 'remove' | 'keep' | 'value' | 'press' | 'spread'} | ||
| */ | ||
| function decide(value, press, spreads) { | ||
| if (press === 'toggle') { |
There was a problem hiding this comment.
This branch runs before the spreads check, so <Tooltip pressTrigger="toggle" {...rest} touchTrigger="tap"> (and the same hook options) loses touchTrigger with no TODO or listing, although a pressTrigger in rest would override the static toggle. Checking spreads first keeps the header's promise that spread sites are reported. A spread touchTrigger="auto" also falls into SPREAD_COMMENT, whose note tells the caller to pass pressTrigger="toggle"; it needs its own outcome.
c21b1fb to
7290cb2
Compare
|
Fixed in 7290cb2:
Both cases are in the codemod tests, and both fail on c21b1fb. |
There was a problem hiding this comment.
Approval waits on a spec decision: Tooltip.spec.md FR9 now says a pinning press on an anchorRef trigger also reaches a plain onClick element around it, and useTooltip and useHoverCard triggers behave the same way, while the current FR9 in both the Tooltip and HoverCard records says the press stops at its trigger. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), or the press should stop in those paths too. Whatever is decided, these changes are still needed:
- Land #9232 and #9291 first and rebase, since the combined diff is 3,858 lines.
- Scope the "press stops at its trigger" sentence in
.changeset/press-trigger-toggle.mdand.changeset/hover-card-press-trigger.mdto the components, because a plainonClickrow around auseTooltiporuseHoverCardtrigger still receives the press. - Use
useDevWarningfor the non-button warning inusePressPin.tsinstead of the module-levelWeakSet.
[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`, 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.
This adds an exception the current FR9 does not have: with anchorRef, "a plain onClick element around the anchor receives" the pinning press, where current FR9 says the press "does not reach a clickable row, card, or container around it". useTooltip and useHoverCard callers get the same behaviour with no record line. This needs an engineering owner's /approve-spec, or the press should stop in these paths too.
| '@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.
"Tooltip (and useTooltip) … The press stops at its trigger, so a clickable row, card, or label around it does nothing" is not true for useTooltip: a <div onClick> row around a hook trigger receives the pinning press (the hook's JSDoc says so). Scope the sentence to the component and say a hook caller's plain onClick ancestor still receives the press, as the anchorRef sentence does.
| '@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.
Same as the Tooltip changeset: the stop applies to the HoverCard component only; a plain onClick row around a useHoverCard trigger receives the pinning press.
| export type LayerPressTrigger = 'none' | 'toggle'; | ||
|
|
||
| /** Triggers already warned that a press cannot pin them: one warning each. */ | ||
| const warnedTriggers = new WeakSet<Element>(); |
There was a problem hiding this comment.
utils/devWarning.ts routes per-instance warnings to useDevWarning. usePressPin already has offeredKind, isToggleMode and wiresState, so useDevWarning(options.component, message, isToggleMode && offeredKind === 1 && wiresState) can replace this set and the devWarn call in wire().
7290cb2 to
c151a25
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: Tooltip.spec.md FR9 now says a pinning press on an anchorRef or useTooltip trigger also reaches a plain onClick element around it, while the current FR9 says the press stops at its trigger, and useHoverCard behaves the same way under an unchanged HoverCard FR9. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the press should stop in those paths too. Whatever is decided, these changes are still needed:
- Land #9232, #9361 and #9291 first and rebase, since the combined diff is 3,870 lines.
- Make HoverCard FR9 match
useHoverCard: the same exception if it is approved, or a stop in the hook if not. - Scope the "press stops at its trigger" sentence in both
pressTriggerchangesets to the components, because a plainonClickrow around auseTooltiporuseHoverCardtrigger still receives the press.
[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.
This FR9 sentence ("With anchorRef, or useTooltip with a caller's trigger, … a plain onClick element around the anchor receives it") is an exception the current FR9 does not have: there the pinning press "does not reach a clickable row, card, or container around it". It needs an engineering owner's /approve-spec at this head, or the press should stop in these paths too.
| | FR6 | Escape closes a visible card through the shared dismissal stack when it is the topmost layer in the page, so a layer underneath stays open. Implementation gap: a card 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). When focus was inside the card, it returns to the trigger, and that refocus does not reopen the card. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the card shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled card answers Escape by calling `onOpenChange(false)` without hiding, and setting `isOpen={false}` while focus is inside the card returns focus to the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, focus on the trigger still opens the card, and Escape then only reports `onOpenChange(false)` (Current-state impact). `isEnabled={false}` disables the triggers. `onOpenChange` reports each show and hide. | Current source and tests | | ||
| | FR8 | Content renders only while open; server markup and the first client render match, including a default-open card. `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 card open, and the same press on a pinned card closes it. Hover and focus still preview it per FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned card stays open when the pointer leaves the trigger and the card, and a press on its own content keeps it pinned; Escape (FR6), a second press on the trigger, a press outside the trigger and the card, or focus moving outside them closes it. In a controlled card 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. A trigger whose role is not `button` gets a development warning and keeps tap-only opening: a tap opens the card as `touchTrigger="tap"` does, and no press pins it. | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
HoverCard FR9 still says the pinning press stops at its trigger, and the public contract gives useHoverCard the same pressTrigger, but useHoverCard passes the press to a plain onClick row (its JSDoc: "a hook caller stops a plain onClick ancestor itself"). Add the same clause as Tooltip FR9 if that exception is approved, or stop the press in the hook.
| '@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.
"Tooltip (and useTooltip) … The press stops at its trigger, so a clickable row, card, or label around it does nothing" is not true for useTooltip: a <div onClick> row around a hook trigger receives the pinning press. Scope the sentence to the component and say a hook caller's plain onClick ancestor still receives it unless the trigger's handler stops it.
| '@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.
Same as the Tooltip changeset: the stop applies to the HoverCard component only; a plain onClick row around a useHoverCard trigger receives the pinning press.
c151a25 to
2679684
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: Tooltip.spec.md FR9 now says a pinning press on an anchorRef or useTooltip trigger still reaches a plain onClick element around it, and useHoverCard behaves the same way, while the current FR9 in both the Tooltip and HoverCard records says the press stops at its trigger. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner); if it is approved, HoverCard FR9 should say the same for useHoverCard, and if not, the press should stop on those paths too. Whatever is decided, these changes are still needed:
- Land #9232, #9406, #9361 and #9291 first and rebase, since the combined diff is 4,221 lines and the head now conflicts with
mainin three Tooltip files. - Scope the "press stops at its trigger" sentence in
.changeset/hover-card-trigger-behavior.mdto theHoverCardcomponent, as the Tooltip changeset already does, because a plainonClickrow around auseHoverCardtrigger still receives the press.
[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 `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.
This FR9 sentence ("With anchorRef, or useTooltip with a caller's trigger, … a plain onClick element around the anchor receives it") is an exception the current FR9 does not have: there the pinning press "does not reach a clickable row, card, or container around it". It needs an engineering owner's /approve-spec at this head, or the press should stop on these paths too; if approved, HoverCard FR9 should say the same for useHoverCard.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] HoverCard (and `useHoverCard`) take `triggerBehavior` (type `HoverCardTriggerBehavior`): `adaptive`, the default and the released behavior, where hover and focus preview the card and a tap opens it unless the trigger performs an action of its own; `hover`, where hover and focus preview it and touch never opens it; or `toggle`, where hover and focus still preview it and a click, a tap, Enter, or Space on a button trigger pins it open, and the same press closes 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; `touchTrigger` decides taps only under `adaptive`; `hover` and `toggle` ignore it. |
There was a problem hiding this comment.
"HoverCard (and useHoverCard) … The press stops at its trigger, so a clickable row, card, or label around it does nothing" is not true for useHoverCard: a plain <div onClick> row around a hook trigger receives the pinning press (the hook's JSDoc says so). Scope the sentence to the component and add, as the Tooltip changeset does, that with useHoverCard a plain onClick element around the trigger receives the press unless the trigger's onClick stops it.
2679684 to
11b54aa
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: Tooltip.spec.md and HoverCard.spec.md FR9 now say a pinning press from an anchorRef, useTooltip or useHoverCard trigger still reaches a plain onClick element around it, while the current FR9 in both records says the press stops at its trigger. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the press must stop on those paths too. Independently, please land #9232, #9406, #9361 and #9291 and rebase so only the deprecation commit remains: the diff is 4,479 lines, and it now conflicts with main in both records.
[Automated review]
11b54aa to
f4c46db
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: Tooltip.spec.md and HoverCard.spec.md FR9 now say a pinning press from an anchorRef, useTooltip or useHoverCard trigger still reaches a plain onClick element around it, while the current FR9 in both records says the press stops at its trigger. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the press must stop on those paths too. Independently of that decision, please fix:
- Land #9232, #9406, #9361 and #9291 and rebase, so only the deprecation commit remains (the diff is 4,479 lines).
- The HoverCard non-button test in
usePressPin.test.tsxspies onconsole.warnbut never asserts the warning or a tap opening, whichHoverCard.spec.md's verification row now cites. - The re-wire Effect in
usePressPin.tsand its settled key leave outrepeatsTriggerText, so turning that option on after mount never wires the own-text pin.
[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 `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.
This FR9 sentence ("With anchorRef, or useTooltip with a caller's trigger, … a plain onClick element around the anchor receives it") is an exception the current FR9 does not have: there, a pinning press "does not reach a clickable row, card, or container around it". It needs an engineering owner's /approve-spec at this head, or the press must stop on these paths too. HoverCard.spec.md:171 adds the same clause for useHoverCard.
|
|
||
| describe('HoverCard triggerBehavior="toggle" on a trigger that is not a button', () => { | ||
| it('keeps the labelled card’s own aria-expanded and aria-controls (FR3), and warns', () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); |
There was a problem hiding this comment.
warn is spied on but never asserted, and the test never taps, so it would pass if HoverCard stopped warning or lost its tap-only opening on a non-button trigger. HoverCard.spec.md:260 cites this suite for exactly that ("a non-button trigger without the warning or its tap fallback … fails"). Assert the warning, as the Tooltip case at line 367 does, and that a tap opens the card.
|
|
||
| // A `triggerBehavior` that changes after the trigger mounted wires or unwires | ||
| // it here: the host's ref callback does not run again for that change. | ||
| useEffect(() => { |
There was a problem hiding this comment.
wire reads repeatsTriggerText (line 477), but this Effect re-runs only on triggerBehavior, and settledRef keys on {element, mode}, so a host that turns repeatsTriggerText on after mount never gets the own-text pin (the click pins nothing and the trigger logs the non-button warning). Add it to the settled key and the dependencies.
…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. The option joins the settle key and the re-wire Effect, so turning it on after mount wires the trigger. - Astryx clickable containers and rows ignore a press-pin click: the mark lives in `Layer/pressPinClick.ts`, which useClickableContainer, Item rows, TreeList rows, and the Table row-expansion and tree-data row clicks read. So a trigger that is not a nested button, an own-text trigger or an anchored `role="button"` element, doesn't 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. - With the gap closed, Tooltip's FR9 gap note, its current-state item, and the Item-row exception in the changeset, docs, and JSDoc go.
f4c46db to
bc63546
Compare
|
Updated in bc63546, restacked on #9291's 8fa3613 (on #9361, on main, which now carries #9232 and #9406).
This head needs a spec owner's |
There was a problem hiding this comment.
Approval waits on a spec decision: HoverCard.spec.md FR9 now says that with useHoverCard, a plain onClick element around the trigger receives the pinning press, while the current HoverCard FR9 says that press stops at its trigger. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise the press from a useHoverCard trigger needs to stop before a plain onClick ancestor too. The matching Tooltip clause for anchorRef and useTooltip is already current and needs nothing.
Whatever that decision, these need changes:
- The diff is 2,924 authored lines because it carries the separable #9361 and #9291, so land those and rebase until only the deprecation commit remains.
- The codemod neither rewrites nor lists
<Tooltip touchTrigger="auto" {...props}>oruseTooltip({touchTrigger: 'auto', ...options}), where the later spread can carrytap, so list those sites as it already lists a spread-only layer (migrate-touch-trigger-tap-to-trigger-behavior.mjs:382).
The HoverCard non-button test and the repeatsTriggerText re-wire from the last round are fixed.
[Automated review]
| // toggle, so no rewrite or removal here is provably the same. `auto`, the | ||
| // default, is left as it is; anything else is left for a person. | ||
| if (spreads) { | ||
| return value === 'auto' ? 'keep' : 'spread'; |
There was a problem hiding this comment.
A spread that comes after a static touchTrigger="auto" overrides it and can carry tap, yet this returns keep, and the spread-only pass below skips the layer because it has its own touchTrigger. So <Tooltip touchTrigger="auto" {...props}> and useTooltip({touchTrigger: 'auto', ...options}) get no TODO and no listing, while <Tooltip {...props}> is listed. Keep auto silently only when the static touchTrigger follows every spread; otherwise list the site with the spread note (system spec AST-017 FR29c and FR29d, DEP-0016's codemod row). The test case at __tests__/…test.mjs:266 asserts the silence today.
| | FR6 | Escape closes a visible card through the shared dismissal stack when it is the topmost layer in the page, so a layer underneath stays open. Implementation gap: a card 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). When focus was inside the card, it returns to the trigger, and that refocus does not reopen the card. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the card shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled card answers Escape by calling `onOpenChange(false)` without hiding, and setting `isOpen={false}` while focus is inside the card returns focus to the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, focus on the trigger still opens the card, and Escape then only reports `onOpenChange(false)` (Current-state impact). `isEnabled={false}` disables the triggers. `onOpenChange` reports each show and hide. | Current source and tests | | ||
| | FR8 | Content renders only while open; server markup and the first client render match, including a default-open card. `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 card open, and the same press on a pinned card closes it. Hover and focus still preview it per FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned card stays open when the pointer leaves the trigger and the card, and a press on its own content keeps it pinned; Escape (FR6), a second press on the trigger, a press outside the trigger and the card, or focus moving outside them closes it. In a controlled card 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 `useHoverCard` and a caller's trigger, the caller's own handlers still receive the press: the trigger's `onClick` runs, and a plain `onClick` element around the trigger receives it unless the trigger'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 card, and no press pins it. | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The sentence "With useHoverCard and a caller's trigger … a plain onClick element around the trigger receives it unless the trigger's onClick stops it" is new to HoverCard FR9; the current FR9 says the pinning press "does not reach a clickable row, card, or container around it". It needs an engineering owner's /approve-spec <full head SHA> at this head.
| // id would reference nothing. DateInput gates it the same way. The | ||
| // trigger's own aria-controls (if any) is preserved either way. | ||
| if (hoverCard.isOpen) { | ||
| if (!ownsToggleState) { |
There was a problem hiding this comment.
Non-blocking: this branch and the one at line 406 are empty apart from // The press pin sets it. Wrapping both attribute writes in one if (ownsToggleState) { … } says the same thing in the structure, and the comments can go.
bc63546 to
6e420cf
Compare
|
Updated in 6e420cf, restacked on #9291's b5931d2 (on #9361, on main, which now carries #9232 and #9406).
This head needs a spec owner's |
There was a problem hiding this comment.
Approval waits on a spec decision: HoverCard.spec.md FR9 now says that with useHoverCard, a plain onClick element around the trigger receives the pinning press, while the current HoverCard FR9 says that press stops at its trigger. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise the press from a useHoverCard trigger needs to stop before a plain onClick ancestor too.
Whatever that decision, these need changes:
- The diff is 3,029 authored lines because it carries the separable #9361 and #9291, so land those and rebase until only the deprecation commit remains.
- The codemod neither changes nor lists
useTooltip({placement: 'below', ...options})oruseHoverCard({...options}), where the spread can carrytouchTrigger: 'tap', so list those calls as it already lists<Tooltip {...props}>(migrate-touch-trigger-tap-to-trigger-behavior.mjs:612).
The earlier gaps, a touchTrigger="auto" followed by a spread going unlisted and HoverCard's empty branches, are fixed.
[Automated review]
| const touch = properties.find( | ||
| (/** @type {any} */ p) => keyOf(p) === OLD_NAME, | ||
| ); | ||
| if (touch == null) { |
There was a problem hiding this comment.
Hook options with a spread and no touchTrigger of their own return here, so useTooltip({placement: 'below', ...options}) and useHoverCard({...options}) get no TODO and no listing, while the JSX pass lists <Tooltip {...props}> (line 568). The spread can carry touchTrigger: 'tap'; DEP-0016's codemod row and spec:AST-017 FR29d ask for spreads the codemod cannot prove to be reported. Leaving the spread note and reporting it here, with a test beside the one at __tests__/…test.mjs:278, would close it.
| | FR6 | Escape closes a visible card through the shared dismissal stack when it is the topmost layer in the page, so a layer underneath stays open. Implementation gap: a card 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). When focus was inside the card, it returns to the trigger, and that refocus does not reopen the card. | WCAG 1.4.13; tests | | ||
| | FR7 | `isOpen` forces the card shown or hidden, and `isOpen={true}` suppresses hover and focus hiding; a controlled card answers Escape by calling `onOpenChange(false)` without hiding, and setting `isOpen={false}` while focus is inside the card returns focus to the trigger. `isDefaultOpen` opens it on mount, still dismissible. Implementation gap: with `isOpen={false}`, focus on the trigger still opens the card, and Escape then only reports `onOpenChange(false)` (Current-state impact). `isEnabled={false}` disables the triggers. `onOpenChange` reports each show and hide. | Current source and tests | | ||
| | FR8 | Content renders only while open; server markup and the first client render match, including a default-open card. `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 card open, and the same press on a pinned card closes it. Hover and focus still preview it per FR4; on touch no preview opens and a tap pins it, while a touch that becomes a scroll pins nothing. A pinned card stays open when the pointer leaves the trigger and the card, and a press on its own content keeps it pinned; Escape (FR6), a second press on the trigger, a press outside the trigger and the card, or focus moving outside them closes it. In a controlled card 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 `useHoverCard` and a caller's trigger, the caller's own handlers still receive the press: the trigger's `onClick` runs, and a plain `onClick` element around the trigger receives it unless the trigger'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 card, and no press pins it. | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The sentence "With useHoverCard and a caller's trigger, … a plain onClick element around the trigger receives it unless the trigger's onClick stops it" is new to HoverCard FR9; the current FR9 says the press "does not reach a clickable row, card, or container around it". It needs an engineering owner's /approve-spec at this head.
`component:HoverCard` FR9, FR10 and AR3, on the shared press pin (`Layer/usePressPin.ts`): - `triggerBehavior` on HoverCard and useHoverCard, with the closed-union type HoverCardTriggerBehavior (exported): `adaptive`, the default and the released behavior (a tap opens the card 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 card and the same press closes 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 `triggerBehavior` turns to `adaptive` 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 (a controlled card says it each time its owner shows it pinned, as an unlabelled card's text is said, and nothing for a press the owner declines); otherwise the card's rendered text is announced once through useAnnounceRenderedText. - HoverCard.test.tsx: `adaptive` by default and explicitly, `hover`, and `toggle`, each under a mouse hover, focus, a tap, and a mouse click and a tap on a button trigger (which only the toggle pins), with `touchTrigger` beside each value (HoverCard FR5 and FR9's `triggerBehavior` row); and `useHoverCard` with a caller's trigger: its own onClick runs, an Astryx ClickableCard around it does not activate, and a plain onClick element around it receives the press unless the trigger's onClick stops it. - HoverCard FR9 states that `useHoverCard` clause, as Tooltip FR9 does for `anchorRef` and `useTooltip`; the changeset scopes the stop at the trigger to the HoverCard component. - HoverCard.test.tsx also stops a toggle press at its trigger inside an Astryx ClickableCard, an Item row, and a `<label>`. The HoverCard record links `Layer/usePressPin.ts` and `spec:AST-086` FR12 and FR14. - The HoverCard Space test presses Space down and up, as meta#9406's keyup timing needs. The API report lists HoverCard's three new rows.
…), with its upgrade codemod `component:Tooltip` DEP-0016, `spec:AST-017` FR29a and FR29c: - `touchTrigger="tap"` resolves to a `@deprecated` overload of Tooltip, HoverCard, useTooltip and useHoverCard, so editors mark it with its replacement; the docs and JSDoc name DEP-0016. It keeps working as released. - The `astryx upgrade` codemod, an opt-in early migration when run explicitly, rewrites static `touchTrigger="tap"` to `triggerBehavior="toggle"` on an icon or span trigger with no role, tabIndex, event handler, or spread props (a trigger whose layer repeats its own text is reported), rewrites static `touchTrigger="none"` to `triggerBehavior="hover"` (in place of an explicit `adaptive` too), and removes static `touchTrigger="auto"` where no props are spread, and any `touchTrigger` beside a static `triggerBehavior="hover"` or `"toggle"`, which ignore it; every other tap site, one beside spread props and a static toggle included, and a `none` beside spread props or a computed `triggerBehavior`, keeps its prop with a TODO and a row in the upgrade's manual-migration listing, with its file and line. A core codemod reports such a site by passing a ManualMigrationRequired to api.report, keeping its changes to the file. - The table-comparison page template's modality rail is a button with the toggle.
6e420cf to
20a434e
Compare
|
Updated in 20a434e, restacked on #9291's bdde69a.
This head needs a spec owner's |
|
Closing: touchTrigger stays as its own option; it answers whether a tap may be consumed to show the content, a different purpose from triggerBehavior. |
Status: draft, stacked on the HoverCard pull request (#9291, bdde69a) and #9361 (ed6c6cf), on main 9ae589b, which carries #9232 and #9406. Until they merge, this branch carries their commits and this change (20a434e). This change edits
Tooltip.spec.mdandHoverCard.spec.md, so it needs its own/approve-specat this head (pending). Every gate under Testing ran at this head or at 412589cc98, which differs only in the codemod, its test, and one comment line inuseHoverCard.tsx.Review only this pull request's change: vjeux/astryx@bdde69a...20a434e
User need
DEP-0016 deprecates
touchTrigger="tap"in favor oftriggerBehavior="toggle", which #9232 (Tooltip) and the HoverCard pull request (#9291) add. Callers need three things to move: to see the deprecation where they writetap, a mechanical migration for the sites where nothing else changes, and a list of every site they must migrate by hand. Without those, a release would ship the new option with no deprecation plan.Problem and solution fit
touchTrigger="tap"call resolves to a@deprecatedsignature ofTooltip,HoverCard,useTooltipanduseHoverCard, so editors mark it with its replacement.auto,none, no value, and props a wrapper forwards resolve to signatures that are not deprecated. The docs (English and Chinese) and the JSDoc name DEP-0016.astryx upgradecodemod, an opt-in early migration when a caller runs it explicitly, movestouchTriggertotriggerBehaviorahead of the removal, each rewrite keeping its behavior. It rewrites statictouchTrigger="tap"totriggerBehavior="toggle"only on an icon or span trigger (Core'sIcon, an<svg>, a<span>of text and icons, or a lone string the layer wraps in a span), with norole,tabIndex,contentEditable, event handler or spread props on it or inside it. There the result is observably the same, since FR9 keeps a non-button toggle trigger on tap-only opening. It rewrites statictouchTrigger="none"totriggerBehavior="hover", in place of an explicittriggerBehavior="adaptive"too, since underadaptivethe explicitnonedecided taps. It also removes statictouchTrigger="auto", the default, where no props are spread (beside spread props, which may set either prop, it leavesautoas it is), and anytouchTriggerbeside a statictriggerBehavior="hover"or"toggle", which ignore it.tapsite keeps its prop with a TODO naming the manual migration, andastryx upgradelists each one with its file and line among the manual migrations it cannot make (spec:AST-017FR29c). That covers a button, a link, an element with arole,tabIndex, event handler or spread props (FR9 would pin arole="button"one), a trigger whose layer repeats its own text, auseTooltip/useHoverCardoptions object (its trigger isn't visible there), an element it cannot identify, a computed value, and spread props, even beside a statictriggerBehavior="toggle", which a spreadtriggerBehaviorwould override. AtouchTrigger="none"beside spread props or a computedtriggerBehavioris listed the same way. The listing repeats on every run until the site is migrated.ManualMigrationRequiredtoapi.reportand keep its changes to the file. Before, the only way to report one was to throw, which left the file unchanged and would have dropped the same file's rewrites.<button type="button" aria-label="Model modalities">around its icons, withtriggerBehavior="toggle", so it pins its explanation and carries no development warning.Current specification
component:TooltipDEP-0016 (its warning, migration, codemod and downstream rows),component:HoverCard(which applies DEP-0016), andspec:AST-017FR29a, FR29c and FR29d. All are current at 1d8696a (#9175). The prop's name and values,triggerBehavior('adaptive' | 'hover' | 'toggle', default'adaptive'), and DEP-0016's rows are the records pull request #9379's, merged as a68e0e3: DEP-0016 keeps its shape, only its target istriggerBehavior="toggle", andtouchTriggerremains supported until the next minor. DEP-0016's codemod row is #9417's, merged as 0de0e75: the codemod is an opt-in early migration, it rewritesnonetohoveras well, and 0.8.0's upgrade runs it again with CLN-0024's removal. This pull request is built on both; the record lines it changes use their wording.Public delta and caller burden
touchTrigger="tap"keeps working as released and is ignored besidetriggerBehavior="hover"or"toggle". There's no runtime warning (spec:AST-017FR29, FR29a).astryx upgrade, then migrate each listed site by hand:triggerBehavior="toggle"on a button whose only job is the layer; otherwise keeptapuntil the next minor (CLN-0024).API changes
@astryxdesign/coreTooltip,HoverCard,useTooltip,useHoverCard: three overload signatures each.touchTrigger?: 'auto' | 'none';touchTrigger: 'tap'is@deprecated(DEP-0016); the full props or options type for forwarded values. The implementation and every accepted value are unchanged.<Tooltip touchTrigger="tap" …>is marked deprecated in editors@astryxdesign/climigrate-touch-trigger-tap-to-trigger-behaviorin the next release'sastryx upgradeastryx upgrade --from <version> --apply@astryxdesign/cliCodemodTransformApi.report/AstryxCodemodApi.report(signature unchanged): underastryx upgrade, a core transform that passes aManualMigrationRequiredreports a manual migration the upgrade lists, and the file keeps the transform's resultapi.report(new ManualMigrationRequired('Pass triggerBehavior="toggle" by hand.', {line}))Design and alternatives
@deprecatedfor a single literal of a union, so the metadata rides an overload:tapresolves to a deprecated signature, and the overload that takes the full props keeps forwarded values unmarked and type-correct.api.report, so no new channel or receipt field is added.Evidence
touchTriggerbeside a statictriggerBehavior="hover"stays (a dead prop:hoverignores it), and its case fails.tapbeside a static toggle and spread props was dropped with no TODO or listing, and atouchTrigger="auto"beside spread props was listed with a note to pass the toggle.touchTrigger="none", the threenonecases fail (the rewrite tohover, the spread and computed TODOs, and the hook's mapping).tapwould be rewritten instead of reported.Layer/__tests__/touchTriggerTap.deprecated.test.ts: through the TypeScript language service, everytapcall (both components, both hooks) is marked deprecated, andauto,none, no value, and forwarded props are not, with no type errors.Icon;autoremoved;nonerewritten tohover, also in place of an explicitadaptive, and left with a TODO beside spread props or a computedtriggerBehavior; own-texttapreported, not rewritten; atouchTriggerbeside a statichoverortoggle, which ignore it, removed; computed, competing and spread TODOs; hook reports; idempotent;role,tabIndex, handler or spread props, or one inside it, is reported, not rewritten;tapbeside spread props is reported even next to a static toggle, which a spreadtriggerBehaviorwould override, andtouchTrigger="auto"beside spread props is left as it is, with no TODO or listing;ManualMigrationRequiredwith its line, on every run.api/upgrade/run/outcomes.test.mjs: manual migrations a transform reports throughapi.reportare listed with their lines (src/sites.ts:2 - manual migration needed (…)), and the file keeps the transform's change.touchTrigger, auseTooltiporuseHoverCardcall whose options are not written at the call, a Tooltip or HoverCard whose props are spread with notouchTriggerof its own, a hook options literal that spreads or computes values with notouchTriggerof its own, and a statictouchTrigger="auto"with a spread after it, which can carrytap, get a TODO and a listing, since the codemod cannot check them. A staticautoafter every spread stays as it is. Each note that leaves atapoffers the record's three manual migrations, plus keepingtapuntil the next minor.@file/@input/@output/@positionform; the code and its tests carry the per-site rules.deprecated.tapuntil their own changes in the same release move them: Core FieldLabel's info icon, Lab InfoTip (its promotion to Core, feat(core): InfoTip moves to Core as the info preset over Tooltip's press toggle #9238, uses the toggle), and the Tooltip touch-triggers story.Scope
Writing rubric results
The rubric in
docs/contributing/spec-writing.md, over the changed parts ofTooltip.spec.md:Testing
Local gates at this head, 20a434e (on #9291's bdde69a, on #9361's ed6c6cf, on main 9ae589b):
pnpm check:knowledge,pnpm check:changesets,pnpm check:i18n-runtime, coretsc(0 errors),ASTRYX_STRICT_LINT=1 pnpm lint(0 errors), and Prettier on the touched files. The touched.doc.mjsfiles stay unformatted, as on main, and load.pnpm test:node: 16,066 pass, with 2 expected failures.pnpm buildandpnpm api:report --package corewrite exactly this pull request's overload rows forTooltip,HoverCard,useTooltip, anduseHoverCard. Apart from them, the regenerated file differs from the committed one only by main's own TextArea and TextInput rows (feat(TextInput,TextArea): tell a touch keyboard what the field holds #8166), which main's report also lacks.pnpm test:ui(742 files, 18,531 tests), the Storybook build,TouchTrigger.a11y.chromium.spec.tsin Chromium (22 of 22, twice), and Tooltip's two WebKit.a11y.browser.spec.tsspecs ten times each (220 of 220, in the CI profile: the Playwright 1.61.1 image, 4 CPUs,CI=1,LP_NUM_THREADS=4, one worker).