Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval of this change waits on an engineering owner's spec decision, and the diff needs to shrink to the AlertDialog fix. The touch behavior for AlertDialog (Cancel and the action tracked as one group under a finger, with AlertDialog painting its buttons' touch press) and the draft spec:AST-094 it relies on have no approved spec yet, so an engineering owner needs to approve or amend them by commenting /approve-spec <full head SHA> on this pull request; the author may record that decision if they are an engineering owner. As written, AST-094's FR11 (two press profiles) and FR13 (no component-owned media query) contradict the AlertDialog implementation, so that decision has to settle which side changes. Independently of the spec decision:
- Rebase onto
mainonce the stacked pull requests land, so this one carries only its AlertDialog commit (today it is 23,663 authored lines). - Run the interaction proofs inside the existing core test lane, or land an AST-030 amendment that admits
pr-interactionfirst (AST-030 FR10/FR11). - Add
## Writing rubric resultsfor spec:AST-094 to the description, and drop or fix its citation of spec:AST-090, which is not in the repository. - Add a pen case for the action-group tracker, or limit it to touch.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself (its own scope step below) to run Playwright proofs for core components. spec:AST-030 FR11 says new coverage inside an existing surface "MUST join those owners and MUST NOT create a separately routed lane or required context", and FR10 says a new test type "is not a new surface by itself". Run these proofs inside the existing core test owner, or land an owner-approved AST-030 amendment that admits the lane first.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default here, so AlertDialog changes pen behavior too, while its docs and changeset only describe touch screens and no test drives a pen. architecture:interaction-modality asks pointer-dependent changes to verify "mouse, touch, and pen as applicable". Add a pen slide case to useActionGroupTracking.test.tsx, or pass pointerTypes: ['touch'] from AlertDialog.
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This fade timer is never cleared when tracking is disabled or the component unmounts, so it can write to a closed or detached button. architecture:react-component-runtime INV5 wants component-owned timers released on disable and unmount; a cleanup that clears fadesRef and removes the attribute the timer would have removed covers it.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| options: ActionGroupTrackingOptions, | ||
| ): LongPressGestureRecognizer { | ||
| const optionsRef = useRef(options); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3: "A new or changed Effect MUST name the external system it synchronizes." A one-line comment naming the recognizer delegate that reads optionsRef outside render would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository at this head or on main (it is still an open proposal), so a reader following this amendment finds nothing. Drop the AST-090 amendment here (and in Current-state impact) until that record lands, or describe it in the pull request instead.
PR Analysis Report
Modified ComponentsAlertDialog (@astryxdesign/core)
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/AlertDialog | +17.7KB |
+34B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | 0B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 504 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | View full report
24a7782 to
37860b7
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the new direction here: AlertDialog tracking Cancel and its action as one group under touch and pen, taking its actions' touch press over from Button, and the draft spec:AST-094 requirements this branch adds, including where FR11 and FR13 meet the alert's behavior. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, these need changes:
- Land the dependency stack separately and rebase so this pull request carries only the AlertDialog commit (24,788 authored lines today).
- Run the interaction proofs in the existing Core test lane, or admit the new
pr-interactionlane through an AST-030 amendment first (FR10, FR11). - Add a pen test or drop pen from the hook's default, clear pending fade timers when tracking stops, and comment what each new effect synchronizes.
- Report the Writing rubric results for the new record and fix its references to AST-090,
utils/touchPresentation.tsand the missing motion files.
[Automated review]
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pen is tracked by default here, but no test drives a pen through the group tracking. architecture:interaction-modality asks pointer-dependent behavior to verify mouse, touch and pen as applicable: add a pen case (a slide and a lift off both), or default to ['touch'].
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| const release = (element: HTMLElement) => { | ||
| clearFade(element); | ||
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { | ||
| fadesRef.current.delete(element); | ||
| if (element.getAttribute(PRESSED_ATTRIBUTE) === 'fading') { | ||
| element.removeAttribute(PRESSED_ATTRIBUTE); | ||
| } | ||
| }, PRESS_FADE_MS), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Each release starts a fade timer that is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking; architecture:react-component-runtime INV5 wants node-owned timers released when disabled or unmounted. Clear every pending fade on disable and unmount.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| useIsomorphicLayoutEffect(() => { | ||
| optionsRef.current = options; | ||
| }); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3: a new Effect names the external system it synchronizes. One comment saying this keeps the recognizer's delegate reading the latest options would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
spec:AST-030 FR11: new coverage inside an existing surface joins that surface's lane and does not create a separately routed lane; FR10 admits a new surface only through an owner-approved amendment first. This job routes itself with its own scope check.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
No spec:AST-090 exists in the repository, here or on main, so FR7's proposed departure points at nothing an owner can read. Cite the record that exists, or drop the reference.
37860b7 to
0d3aae6
Compare
There was a problem hiding this comment.
This needs an owner decision before it can be approved: the AlertDialog touch group tracking has no current spec behind it, so approval needs an engineering owner to approve the governing spec, the proposed component:AlertDialog requirement and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and to settle whether AST-094's FR11 and FR13 or the alert's own press profile and coarse-pointer media query should change. An engineering owner records that decision by commenting /approve-spec <full head SHA>, and the author may do so if they are an engineering owner. Independently of that decision, please:
- land the gesture, motion and harness dependencies through their own pull requests and rebase this one onto them;
- run the interaction proofs in the existing Core test lane instead of a separately routed
pr-interactionjob, or amend AST-030 first; - clear pending press-fade timers when tracking turns off or the dialog unmounts, and add a pen test or limit tracking to touch;
- add AST-094's Writing rubric results to the description and fix the references to AST-090,
utils/touchPresentation.ts,uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts, none of which exist.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself with its own scope check, which spec:AST-030 FR11 rules out: new coverage inside an existing surface joins that surface's test lane and must not create a separately routed lane or required context. Run these proofs inside the Core test lane, or land an AST-030 amendment admitting a new surface first (FR10).
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This fade timer is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking, while architecture:react-component-runtime INV5 asks for component-owned timers to be released when disabled or unmounted. Clearing every pending fade, and removing the attribute it would have removed, when tracking turns off and on unmount would fix it.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default here, but no test drives a pen and the docs and changeset mention only touch screens. architecture:interaction-modality asks pointer-dependent changes to verify mouse, touch and pen as applicable: a pen slide and a pen lift-off-both case would cover it, or AlertDialog could pass pointerTypes: ['touch'].
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| options: ActionGroupTrackingOptions, | ||
| ): LongPressGestureRecognizer { | ||
| const optionsRef = useRef(options); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; a one-line comment saying what this keeps current would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 at this revision or on main, so this departure (and the amendment at line 235) points at nothing. Cite the record that holds these rules, or drop the reference.
| ## Touch presentation | ||
|
|
||
| Whether a component presents itself the touch (UIKit) way is answered once, | ||
| in `../utils/touchPresentation.ts`: |
There was a problem hiding this comment.
../utils/touchPresentation.ts does not exist, and neither do TOUCH_QUERY, COMPACT_TOUCH_QUERY or useTouchPresentation; point component authors at what exists, or remove this section until it lands.
There was a problem hiding this comment.
0d3aae6 to
a79b8e8
Compare
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: the AlertDialog touch group tracking has no current spec behind it, so an owner has to approve or reject the proposed component:AlertDialog requirement, including whether callers can turn it off under the current touch-default rule, and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and say which side changes where AST-094's FR4, FR11 and FR13 disagree with this branch. An engineering owner records that decision by commenting /approve-spec <full head SHA>, and the author may do so if they are an engineering owner. Independently of that decision, please:
- land the gesture, motion, theme-role, resolver and harness dependencies through their own pull requests and rebase this one onto them, settling the theme roles and the
pr-interactionjob there againstdesign:reduced-motion,spec:AST-012FR2 andspec:AST-030FR11; - keep the compact-touch line at width below
md(768 px) or height below 500, asspec:AST-012FR9 now requires, instead of 744 px; - load AlertDialog's touch tracking lazily or justify its 17 KB gzip cost (
architecture:component-style-authoringINV6); - clear the press-fade timers on disable and unmount, add a pen test or limit tracking to touch, add AST-094's Writing rubric results, and fix the references to AST-090,
uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts.
[Automated review]
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
spec:AST-012 FR9 (current on main) defines compact as width below the effective md point (768 by default) or height below 500, and says no resolver keeps size numbers of its own; DEC-6 rejects matching platform size classes. At 744 px a coarse-pointer screen 744–767 px wide gets the popover where the definition gives the sheet. Use (width < 768px), (height < 500px) until the theme exports the definition.
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { | ||
| fadesRef.current.delete(element); | ||
| if (element.getAttribute(PRESSED_ATTRIBUTE) === 'fading') { | ||
| element.removeAttribute(PRESSED_ATTRIBUTE); | ||
| } | ||
| }, PRESS_FADE_MS), | ||
| ); |
There was a problem hiding this comment.
This timer is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking; architecture:react-component-runtime INV5 asks component-owned timers to be released when disabled or unmounted. Clear fadesRef (and the attribute it would remove) when tracking turns off and on unmount.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default and AlertDialog passes no pointerTypes, but no test drives a pen and the docs and changeset mention only touch screens. architecture:interaction-modality asks pointer-dependent changes to verify mouse, touch and pen as applicable: add a pen slide and a pen lift-off-both case, or pass ['touch'].
| import {themeProps} from '../utils/themeProps'; | ||
| import {useTranslator} from '../i18n'; | ||
| import {useMediaQuery} from '../hooks'; | ||
| import {useActionGroupTracking} from '../hooks/useActionGroupTracking'; |
There was a problem hiding this comment.
This import brings the gesture host, the arena, the pan, tap and hover recognizers, the velocity tracker and the whole UIKIT_GESTURES table into every AlertDialog import: 30,347 to 47,413 bytes gzip (core modules, packages external). architecture:component-style-authoring INV6 asks touch-only JavaScript over 2 KB to load lazily on a coarse pointer or be justified, and shared tables to ship only the rows a component uses.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself with its own scope check, which spec:AST-030 FR11 rules out: new coverage inside an existing surface joins that surface's test lane and must not create a separately routed lane or required context. Run these proofs inside the Core test lane, or land an FR10 amendment first.
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
spec:AST-012 FR2 keeps when a closed vocabulary (width, height, pointer, contrast, motion) and says future environment fields require explicit additions to it; platform needs that amendment before it ships as a stable theme option.
| * Cross-Fade Transitions on (opacity CABasicAnimation 0.5 s, | ||
| * (0.42, 0, 0.58, 1)), Oct 23 2026 | ||
| */ | ||
| reducedMotionCrossFadeMs: 500, |
There was a problem hiding this comment.
design:reduced-motion DR2 (current) caps a touch cross-fade at 200 ms, and DR4 says core theme values offer no way to a longer reduced-motion animation. This 500 ms default, and the theme role that can retune it (the docs set it to 300), would give touch presentations a longer fade.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 at this revision or on main, so this departure (and the amendment at line 235) points at nothing an owner can read. Cite the record that holds these rules, or drop the reference.
a79b8e8 to
4a23248
Compare
|
Addressed at 4a23248:
Waiting in the reviewers' queue: the spec decision (component:AlertDialog FR6 and the draft AST-094), the |
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: AlertDialog's touch and pen group tracking has no current spec behind it, so an owner has to approve or reject the proposed component:AlertDialog requirement, including whether callers can turn it off under the current touch-default rule, and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and say which side changes where AST-094's FR4, FR11 and FR13 disagree with this branch. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, please:
- land the gesture, motion, theme-role, resolver and harness dependencies through their own pull requests and rebase this one onto them, settling the theme roles and the
pr-interactionjob there againstdesign:reduced-motion,spec:AST-012FR2 andspec:AST-030FR11; - keep the compact-touch line at width below
md(768 px) or height below 500, asspec:AST-012FR9 requires, instead of 744 px; - report AlertDialog's size, load its touch tracking lazily or justify its 16 KB gzip cost (
architecture:component-style-authoringINV6), and attach it only while the alert is open; - prove pens in the AlertDialog browser cases or track touch only, add AST-094's Writing rubric results, and fix the references to AST-090,
uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts.
[Automated review]
| if (element == null) { | ||
| return undefined; | ||
| } | ||
| return getGestureHost(element.ownerDocument).attach(element, recognizer, { |
There was a problem hiding this comment.
The recognizer attaches here whatever isEnabled says, and attaching installs the gesture host's twelve document and two window listeners (GestureHost.ts:463-490). Through useActionGroupTracking, every mounted AlertDialog keeps them while closed. architecture:react-component-runtime INV5 asks optional behavior to acquire local resources only while enabled; keying this attach on isEnabled (or attaching from AlertDialog only while open) would cover it.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default, and a pen lift relies on the host swallowing the browser's own trusted click after lifted.click() (GestureHost.ts:1018-1036, :1147-1163). The two pen cases run in jsdom, where no browser click follows, so they cannot catch a double activation. architecture:interaction-modality asks for real-browser evidence when behavior depends on pen synthesis and hit testing: a pen tap and a pen slide in AlertDialog.interaction.browser.spec.ts, or ['touch'] here, would settle it.
|
Agreed: one implementation. The local |
4a23248 to
65889f0
Compare
… tree; a claimed touch finds its pointer finger wherever fingers come from pointer events containsAlongFlatTree walks parentAcrossShadow, so a finger inside an open shadow root, now that targets come from the composed path, counts as under a scroller outside it: a scroll there takes it, its flick arms the scroll brake, and a touch there brakes. cancelIfClaimed matches a touch to its pointer finger whenever the host reads fingers from pointer events, not only under inputMode 'pointer-events', so a claimed pan's moves are cancelled where a browser that exposes no ontouchstart still sends Touch Events.
… flat tree, and a claimed touch finds its pointer finger wherever fingers come from pointer events) into meta#8320
…r along the flat tree, and a claimed touch finds its pointer finger wherever fingers come from pointer events) into meta#8293
…handlers type UseLongPressHandlers.start takes a point only again, as released: the per-press tolerance useMenuPress's trigger chooses as its press starts lives on the internal useLongPressDetector and LongPressDetectorHandlers, which the hooks entry does not export. useLongPress returns the same detector, typed as before.
…alog tracking base
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether AlertDialog's actions become one tracked group under every pointer: no current spec on main gives an alert's buttons the menu's lift-decides model (spec:AST-094 FR26–FR28 cover menus, and FR11 keeps a control's activation with the browser's click), and the always-on text-selection lock on the action row and the 200 ms release fade need that decision too. An engineering owner records it by commenting /approve-spec b2c6bd383d54678a9ed5df38255ea21a9dba314b on this pull request, and the author may do so as an engineering owner. Whatever the decision, please address these:
- Land the stacked pull requests and rebase, so this one carries only its AlertDialog files (it now changes 30,568 authored lines), and drop the unused
useIsScrollablehook. - A mouse pressed on Delete and moved onto Cancel shows both buttons pressed: Button's
:activeplus the group's paint. userEvent.clickon an open alert's Delete runs Cancel in jsdom, so consumer tests that click alert actions this way break.scripts/touch-support-bundle.test.mjs"is absent from the root entry" fails on this head, and the AlertDialog docs still say a mouse keeps each button's click.
[Automated review]
Main merged meta#8045 and meta#8046. The one conflict is utils/pressFeedback.ts: it takes main's merged meta#8046 text (the finger and button slots, the mouse and key press) and keeps this pull request's own additions, the one writer of the press attribute (writePress, paintPress, readPressPaint) and the header line naming them.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/busy.test.ts # internal/interaction-harness/src/busy.ts # internal/interaction-harness/src/frames.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
Main's button slot, merged with meta#8046, ends a press's release at once when the next press starts, by clearing the press attribute directly. On a row another painter holds, that clear erased the hold, which this pull request's one writer refuses. A mouse press on a held row, released, then a press elsewhere must leave the row held. Fails until the early end goes through the one writer.
finish() writes none through writePress, as this pull request's controller paints, so a held row stays its owner's when the next press ends a release early, under the finger and the button slots alike.
Both upstream heads are folded onto main 70ea859 (her meta#8045 frame evidence and meta#8046 coarse press): meta#8446 takes main's harness files, and meta#8073 carries the press controller onto meta#8046's slots. meta#8076 has no change of its own in either set of files, so the merge takes theirs with no hand resolution, and meta#8076 is clean against main again.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/busy.test.ts # internal/interaction-harness/src/busy.ts # internal/interaction-harness/src/frames.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts # packages/core/src/utils/pressFeedback.ts
meta#8076's new head carries main 70ea859 with meta#8073's writer of the press attribute on main's press controller, and its README names both writers. utils/pressFeedback.ts keeps that controller with this line's own changes on it: the pointer, drag, scroll and visibility listeners through the shared input stream (meta#8356) and touchcancel only while the primary pointer is coarse (meta#8270); the key path stays at the window's bubble phase, so a Space press the page cancels paints nothing. usePressTracker stays in its own module, one tracker for a component's life (meta#8315), so meta#8073's rebuilt-tracker hook and its test are not taken. The harness files are meta#8076's.
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…tion to its tracker meta#8046 made the controller paint a mouse press itself where the primary pointer is coarse (a tablet with a trackpad). On an action its group tracks (family:action-groups), the group's tracker paints that press, so the controller must paint nothing there, as it already does for a finger; it paints the press, then turns the tracker's paint to 'fading' at the release. A Space press on the same action still paints: the group tracks pointers only (guard).
… its tracker Where the primary pointer is coarse the controller paints a mouse press itself (meta#8046). It now skips an action a group's tracker paints for a pointer's press, a mouse's included, as it already did for a finger, so the two never write the press attribute over each other. A Space press on the action still paints: the group tracks pointers only.
…he keyboard keeps the buttons Her AlertDialog record (meta#8594) verifies FR8 with a press lifted outside the actions under a mouse, a pen and a finger, and with Enter and Space on each action. Add the two cases the tests lacked: a mouse press that slides off both actions highlights nothing and runs nothing at the lift, and under a coarse and a fine pointer Enter runs the action and Space cancels. A mutant that stops tracking a mouse's moves fails the first; one that drops the buttons' own click fails the second.
There was a problem hiding this comment.
AlertDialog's slide-to-the-lifted-action fix works under a finger and a mouse, but approval waits on an engineering owner deciding whether the lifted action's highlight keeps the shared 200 ms press fade or takes the 100 ms measured for UIKit's alert actions (spec:AST-094 FR1/FR2). An engineering owner records that decision by commenting /approve-spec 458e2d9e88dc59e321db7703e20c831db74f056b on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- Rebase onto the landed stack so this pull request carries only its 1,840-line AlertDialog change; it is 28,849 authored lines now.
- In a jsdom test,
userEvent.clickon the action now runs Cancel instead ofonAction, which would break consumers' tests. - Under a mouse, Delete keeps its
:activepress while the group lights Cancel, so two actions look pressed at once (family:action-groupsFR2). - The actions row's always-on
user-select: nonegoes pastspec:AST-094FR41's per-press suppression and a mouse's released behavior. - The root-entry case of
scripts/touch-support-bundle.test.mjsfails, opening the alert on a desktop logs a false "stays inactive" warning, the docs say a mouse keeps each button's click,useIsScrollableis unused, and the changeset's iOS Safari claim needs real-device evidence.
[Automated review]
| trackingRow: { | ||
| touchAction: 'pinch-zoom', | ||
| userSelect: 'none', | ||
| WebkitUserSelect: 'none', | ||
| }, |
There was a problem hiding this comment.
This keeps the actions row unselectable for every pointer for as long as the alert is open. spec:AST-094 FR41 suppresses selection for the press only and keeps a mouse's released behavior, and FR43 rules out added stylesheet bytes. The recognizer already locks selection per press for a finger or a pen (useActionGroupTracking.ts:166), so userSelect and WebkitUserSelect can go, with touch-action left to the gesture host.
| // actions keep their default styles otherwise. | ||
| manageStyle: false, | ||
| }); | ||
| const actionTrackingProps = isTracking ? pressGroupItemProps : undefined; |
There was a problem hiding this comment.
With a mouse, a press on Delete slid onto Cancel keeps Delete's own :active press (Button.tsx:128-131, scale 0.98) while the group paints Cancel data-astryx-press="on", so both actions look pressed at once, against family:action-groups FR2 ("at most one at a time"). The group's paint should be the only press on a tracked item.
| const DETACHED: RefObject<Element | null> = {current: null}; | ||
|
|
||
| /** Every pointer a group tracks (`family:action-groups` FR5). */ | ||
| const EVERY_POINTER: ReadonlyArray<PointerKind> = ['touch', 'pen', 'mouse']; |
There was a problem hiding this comment.
Tracking the mouse changes what a consumer's jsdom test sees: userEvent.click on Delete now runs Cancel (onOpenChange(false), no onAction), because jsdom lays nothing out and the tracker's hit test (actionGroupTracker.ts:309-324) matches the first member's empty box at the origin; on the base the same test calls onAction. Leaving the press to the browser's click when no member has a laid-out box would keep those tests working.
| usage: { | ||
| description: | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nOn a touch screen the two actions are tracked as one group under a finger or a pen, the way iOS tracks the actions of an alert: the action under the finger highlights as soon as it is touched, the highlight follows the finger from one action to the other, and lifting the finger runs the action under it. Lifting it off both actions runs nothing. A mouse, the keyboard and assistive technology keep the click of each button.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', |
There was a problem hiding this comment.
"A mouse, the keyboard and assistive technology keep the click of each button" (and line 199's "mouse/keyboard/AT keep each button click") no longer holds: the group tracks 'mouse' too (useActionGroupTracking.ts:85), as the changeset says.
| } else if (!this.isCoarse) { | ||
| this.warnFinePointer(); |
There was a problem hiding this comment.
An action-group attach takes this branch too, so opening an AlertDialog under a fine pointer logs "It stays inactive until the pointer is coarse" although the host serves the group there (family:action-groups FR5). Skip the warning for options.actionGroup, as for perEventPointer.
| * @position Internal hook for a touch-tracked surface whose content scroller | ||
| * keeps the browser's pan only where it overflows (AlertDialog, the action | ||
| * sheet card) |
There was a problem hiding this comment.
Nothing imports this hook, though the header names AlertDialog and an action sheet as consumers. Please drop it and its test from this pull request.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] AlertDialog: its two actions are tracked as one group under every pointer, a finger, a pen or a mouse, the way iOS tracks an alert's actions. A press that lands on one action and slides to the other now runs the action it lifts on. Before, iOS Safari ran the action the finger first touched (Delete, when the finger slid to Cancel), and other browsers ran nothing and left the alert open. The pressed action highlights as soon as it is pressed instead of 150 ms later, the highlight follows the pointer between the actions, and lifting off both still runs nothing; a second finger or the window losing focus ends the press with nothing run. Only the actions are tracked: a press on the title or the message is not, and their text stays selectable with a mouse. The keyboard and assistive technology keep each button's own activation. |
There was a problem hiding this comment.
"Before, iOS Safari ran the action the finger first touched" is a claim about iOS Safari behavior, and spec:AST-094 Platform support asks for real-device evidence for every such claim. Attach real-device evidence or drop the sentence.
Depends on #8360, #8638 and #8637. Review only this PR's change: vjeux/astryx@27778e7...458e2d9
Why
On a phone, someone touches Delete in an alert and slides to Cancel before lifting: iOS Safari runs Delete, where the touch started, and the pressed look shows 150 ms late. An alert guards an irreversible action, so running the wrong button is its worst failure.
What led to it
The UIKit-parity work, which brings Astryx's touch behavior to iOS's, compared AlertDialog with the iOS 26.2 alert. Each button tracks its own press: the click target is fixed when the touch starts, and a 10 px move ends the press. iOS treats an alert's buttons as one group: the highlight follows the finger, and the button under it at the lift runs.
family:action-groups(#8622;spec:AST-094DEC-17) asks every action group to do the same, and AlertDialog's record makes it FR8 (#8594).Options and tradeoffs
Why this choice
Option 3 fixes every group the same way, as the contract requires. AlertDialog attaches the tracker (
hooks/useActionGroupTracking.ts, internal) to its button row while open; the row allows only pinch-zoom and selects no label. Open for the owner: UIKit fades the highlight in 100 ms (shared: 200 ms) and runs the handler once the alert is gone (~0.43 s); hereonActionruns at the lift. Known public-API gaps (spec:AST-107FR7): the tracker and the selection lock.How to check it
On a phone, the iOS simulator or Chrome's touch emulation,
Core/AlertDialogDelete story:Run on the iOS 26.2 simulator and the Android 15 emulator (steps 1–2), in Chromium (3–4; the simulator has one finger), and as unit and browser tests (all five): each behaved as written. On main, step 1 runs Delete and 7 of the browser tests fail.
Evidence
iOS 26.2 simulator, Mobile Safari, main first:
Chromium, a mouse and a second finger (storyboard; WebKit):
Android 15 emulator, Chrome 124, main on the left:
touch-support-bundle.test.mjs(test(core): a touch component's import carries its gesture code, and nothing else does #8671 fixes it).