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
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.
Requesting changes: the InfoTip promotion follows component:InfoTip, but the stacked pressTrigger work has defects to fix first, and the branch should be split.
- The docsite build fails: InfoTip's ShadCN route moves from
components/lab/info-tipwith no alias, so addregistry: {aliases: ['lab/info-tip']}toInfoTip.doc.mjs(as Drawer does) and refresh the route lock. - Land #9232 first and rebase, so this pull request carries only the InfoTip commit (about 1,600 of its 3,900 authored lines).
- A labelled HoverCard whose trigger is not a button loses
aria-controls/aria-expandedunderpressTrigger="toggle"(HoverCard.tsx:380), though FR9 keeps it ontapbehavior. - The pin announcement and the outside-press dismissal in
usePressPin.tsduplicateuseAnnounceRenderedText(which leaves outaria-hiddentext) anduseTouchTrigger's outside dismissal; reuse those owners. - DEP-0016 needs its
@deprecatedmetadata ontap, and FieldLabel and the Tooltip story still usetouchTrigger="tap".
[Automated review]
|
|
||
| // Under the toggle, the press pin owns aria-expanded (true only while | ||
| // pinned) and aria-controls (FR10); the popup role stays here. | ||
| const ownsToggleState = pressTrigger !== 'toggle'; |
There was a problem hiding this comment.
This hands aria-expanded and aria-controls to the press pin for every toggle trigger, but usePressPin wires only buttons. A labelled HoverCard on a link or span still opens on tap and now has neither attribute, unlike under touchTrigger="tap" (HoverCard FR3, FR9). Could this follow isPressPinTrigger(firstChild) instead of the prop?
| const isExpanded = (isOpen !== undefined ? isOpen : isPinned) && isLayerOpen; | ||
|
|
||
| // While pinned, a press or focus outside the trigger and the layer closes it. | ||
| useEffect(() => { |
There was a problem hiding this comment.
useTooltip and useHoverCard already call useTouchTrigger, whose armOutsideDismiss owns the press-outside close and arms from the opening handler. This second listener closes on pointerdown where that one closes on lift. Extending that owner and arming it from toggle() would keep one path.
| // Said once when the layer pins; a preview says nothing beyond the | ||
| // trigger's description. | ||
| const announcedRef = useRef(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
The callers feed this raw textContent, so aria-hidden text is read and blocks run together ("Ada Lovelace★Analyst"). spec:AST-086 FR12 and react-component-runtime INV10 route own-content announcements through useAnnounceRenderedText, with no Effect of the adopter's own.
| const mergedRef = useMergedRefs(ref, buttonRef); | ||
|
|
||
| // Under a coarse pointer the button takes taps out toward 44 px (AR1). | ||
| useEffect(() => { |
There was a problem hiding this comment.
The watch starts because the button attached. A callback ref merged with ref (React 19 ref cleanup) can start and stop it, and it handles node replacement, so this Effect can go.
| | 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.
deprecated here comes before DEP-0016's downstream is done: FieldLabel.tsx:316 still passes touchTrigger="tap", and Tooltip.stories.tsx:373 still teaches it (spec:AST-017 FR29b, FR29c).
|
Fixed in 0812a94: the hit-area watch now starts from a callback ref merged with |
There was a problem hiding this comment.
The InfoTip promotion follows component:InfoTip, but approval waits on an engineering owner's decision about two new @astryxdesign/core/Layer exports and on a few fixes in the stacked pressTrigger commit.
LayerPressTriggerandarmOutsideDismissonuseTouchTrigger's return become public API that no current record decides; an engineering owner records whether they stay public by commenting/approve-spec <head SHA>(the author may, if they are one), or they can be kept internal.- The pin announcement reads raw
textContentin an Effect of its own, soaria-hiddentext is spoken and paragraphs run together ("Revenue30-day rolling average."); announce throughuseAnnounceRenderedTextinstead (spec:AST-086FR12). - The
touchDismissRefEffects inuseTooltipanduseHoverCardonly copy callbacks into a ref; assign the ref during render and drop them. - Switching
pressTriggerfromnonetotoggleafter mount never wires the pin; re-attach when the prop changes and add a rerender test. - Land #9232 with these fixes, then rebase so this pull request carries only the InfoTip commit (about 1,600 of its 3,400 authored lines).
[Automated review]
| return; | ||
| } | ||
| announcedRef.current = true; | ||
| const text = optionsRef.current.announcement()?.trim(); |
There was a problem hiding this comment.
announcement() hands this raw textContent, so aria-hidden glyphs are spoken and blocks run together (an InfoTip with two paragraphs is announced "Revenue30-day rolling average."). spec:AST-086 FR12 and react-component-runtime INV10 route own-content announcements through useAnnounceRenderedText, with no Effect of the adopter's own.
| useEffect(() => { | ||
| touchDismissRef.current = {arm: armOutsideDismiss, disarm: clearTapOpen}; | ||
| }, [armOutsideDismiss, clearTapOpen]); |
There was a problem hiding this comment.
This Effect synchronizes no external system (react-component-runtime INV3); it only copies two callbacks into a ref. Assigning touchDismissRef.current during render, as useTouchTrigger does for hideRef, removes it; useHoverCard.tsx:450 has the same one.
| } | ||
|
|
||
| triggerRef.current = el; | ||
| attachPressPin(el); |
There was a problem hiding this comment.
The pin attaches only when this ref re-attaches, and the dependencies leave out pressTrigger, so a Tooltip rerendered from none to toggle never wires it: no aria-expanded, and a click does nothing. Could attach re-run when pressTrigger changes?
`component:Tooltip` FR9 and FR10, `spec:AST-003` FR4: - `triggerBehavior` on Tooltip and useTooltip, with the type TooltipTriggerBehavior: `adaptive`, the default and the released behavior (a tap opens the tooltip unless the trigger acts; an explicit `touchTrigger` still decides taps under it until the next minor); `hover` (touch never opens it); or `toggle`. Hover and focus preview it under every value. With `toggle` on a button trigger, a click, a tap, Enter, or Space pins the tooltip and the same press closes it. - A pinned tooltip stays until Escape, a second press, a press or scroll outside (the touch trigger's outside dismissal, which the pin arms: the pin owns the layer's touch trigger, so useTooltip makes one usePressPin call), or focus moving onto an element outside the trigger and the tooltip (the pin listens for it on the trigger, whatever `focusTrigger` is); a press on the tooltip's own text keeps it. It takes Escape before the layer beneath it. A `triggerBehavior` that changes after mount wires or unwires the trigger, and a tooltip whose `triggerBehavior` turns to `adaptive` while pinned closes on leave again. - The press stops at its trigger. With `anchorRef`, the caller's own handlers still receive the press: the anchor's `onClick` runs, and a plain `onClick` element around the anchor receives it unless the anchor's `onClick` stops it; Astryx clickable rows, cards, and containers ignore it (FR9). The trigger carries aria-expanded and aria-controls on a button trigger, and the tooltip's rendered text is announced once when it pins, through useAnnounceRenderedText (`spec:AST-086` FR12). - A non-button trigger gets one development warning and keeps tap-only opening (`Layer/usePressPin.ts`, the hover layers' shared press pin). - LayerTriggerBehavior and the touch trigger's armOutsideDismiss (useTouchTriggerInternal) stay internal: neither is exported from the Layer entry point, and the public useTouchTrigger return is unchanged.
…s press toggle InfoTip moves to Core: a named info-icon button wrapped in Tooltip with pressTrigger="toggle", so hover and keyboard focus preview the help and a click, tap, Enter, or Space pins it open. Its default name comes from the translation catalog, it forwards a ref to the button and takes xstyle, className, and style, and under a coarse pointer it takes taps out toward 44 px through the shared hit area. The Lab export becomes a deprecated re-export of the Core component.
There was a problem hiding this comment.
The InfoTip promotion follows component:InfoTip, but approval waits on an engineering owner's decision about the anchorRef exception this branch writes into component:Tooltip FR9, which today says a pinning press does not reach a clickable row, card, or container around its trigger. An engineering owner can approve that exception by commenting /approve-spec 16655a6e24dbd7f67bb74d88ea6866437c463431 on this pull request (the author may, as an engineering owner), or anchored and useTooltip triggers can stop the press too so FR9 stays as written. Either way, please land #9232 first and rebase, so this pull request carries only the InfoTip commit (about 1,600 of its 3,600 authored lines) and resolves the conflicts with main in Tooltip.spec.md, useTooltip.tsx and useTooltip.doc.mjs.
[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: the own-text clause is not built in Core yet (Current-state impact). | DEC-1; WCAG 1.4.13, 2.1.1 | |
There was a problem hiding this comment.
The anchorRef sentence added to FR9 lets a pinning press reach a plain onClick element around the anchor, while the current FR9 says the press "does not reach a clickable row, card, or container around it". An engineering owner needs to approve this exception (or anchored triggers need to stop the press too) before it can be relied on.
Stacks on #9232 (
triggerBehavior): only the last commit is this pull request's own.User need
Touch, keyboard, and screen-reader users can open the help behind an "i" but not keep it open to read it. Lab's InfoTip pins nothing, and its button is named by an untranslated "More information".
Problem and solution fit
This is part of making Astryx's touch components behave like native iOS ones. #9175 decided that InfoTip is Tooltip with
triggerBehavior="toggle"on a named button (DEC-1), the prop name #9379 records.Current specification
component:InfoTip(#9175, renamed by #9379) andcomponent:TooltipFR9–FR10. Its record edits close the gaps and need the owner's approval.Public delta and caller burden
InfoTip:content,label,size, arefto the button,xstyle,className, andstyle.InfoTipbecomes a deprecated re-export for one release, and the ShadCN registry keeps its Lab route as an alias.Design and alternatives
Evidence
aria-expanded,aria-controls), and pointer leave keeps it. The ref and style props reach the button, and the name comes from the catalog.Scope
Writing rubric results
InfoTip.spec.md: R1, R2, R4, R5, R6: none. R3: each verification cell names its test.Testing
pnpm test:ui, and the browser test (also 2-core pinned) pass; the node project fails only fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520's cache cases. The press, ref, catalog,toggle, and hit-area cases fail without their change.