Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
There was a problem hiding this comment.
This pull request is on hold for an owner decision: AlertDialog's touch motion rests only on draft spec:AST-094 (FR4, FR5, FR7, FR8 and DEC-1), and FR4 narrows the current architecture:theme-tokens INV3, so an engineering owner has to approve or amend those requirements. An engineering owner records the decision by commenting /approve-spec <full head SHA> on the pull request, and the author may do so if they are an engineering owner. Separately from that decision:
- Land the stacked pull requests first and rebase, so this diff is only the AlertDialog motion commit instead of 25,379 authored lines.
- While the alert fades out under touch, keyboard and assistive-technology activations still run
onAction, and Escape callsonOpenChange(false)again; make the leaving alert inert. - Under React StrictMode, an AlertDialog that mounts open never starts its presentation and stays invisible and modal; the status effect should own and clean up its animation frame.
- The
adaptivethreshold change contradictscomponent:SelectorDEC-1; split it out or keep the current query. - The
lintjob failed on this head with an ESLint out-of-memory error and needs a rerun.
[Automated review]
| ref={ref} | ||
| isOpen={isOpen} | ||
| ref={mergedRef} | ||
| isOpen={motion.isPresented} |
There was a problem hiding this comment.
Under a coarse pointer this keeps the native dialog open for the 404 ms fade (504 ms backstop), but only touch and pen are inert (useActionGroupTracking.ts:186). The still-focused Delete takes a second Enter or AT activation and runs onAction again (:271), and Escape calls onOpenChange(false) again. Consider making the content inert while isLeaving, or dropping onAction, cancel and dismissal once isOpen is false.
There was a problem hiding this comment.
Fixed in 6dca4bf: while the alert leaves (closed by the caller, still presented during its fade), AlertDialog drops a click, Enter or assistive-technology activation on an action, Cancel, and Escape, as UIKit delivers no input during a dismissal. Focus stays where it is, so Dialog returns it to the opener. The new case 'on iOS, while the alert leaves, runs nothing' failed before the fix (onAction ran).
| const was = statusRef.current; | ||
| const now: Status = !isPresented ? 'hidden' : isOpen ? 'open' : 'leaving'; | ||
| statusRef.current = now; | ||
| if (!isEnabled || was === now) { |
There was a problem hiding this comment.
Under StrictMode replay of a mount-open alert (for example useImperativeAlertDialog's first show()), statusRef is already 'open', so this returns. Meanwhile the cleanup at :211 has cancelled the presentation frame, so the panel stays at opacity 0 and scale 1.2 and touches stay inert. Giving this effect its own cleanup (cancel the frame, restore statusRef) would make the replay re-run the opening (architecture:react-component-runtime INV3).
There was a problem hiding this comment.
The presentation no longer waits for an animation frame (it starts in the effect that opens the dialog), so a StrictMode replay has nothing to cancel, as the later review notes. 6dca4bf adds the StrictMode mount-open case: the alert presents and takes touches once the presentation completes.
| // and large touch displays stay anchored, phones in either orientation get | ||
| // the sheet. The query is the shared touch presentation's | ||
| // (utils/touchPresentation.ts). | ||
| export const COMPACT_TOUCH_PRESENTATION_QUERY = COMPACT_TOUCH_QUERY; |
There was a problem hiding this comment.
This moves adaptive from (max-width: 768px) and (pointer: coarse) to (width < 744px) or (height < 500px). Current component:Selector DEC-1 records that "adaptive keeps its width-and-pointer query". The change belongs in its own pull request with an owner decision, or this should keep the current query.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
f3b37d2 to
6e2412f
Compare
There was a problem hiding this comment.
This pull request is on hold for an owner decision: AlertDialog's touch motion rests only on draft spec:AST-094 (FR4, FR5, FR7, FR8 and DEC-1), and FR4 narrows the current architecture:theme-tokens INV3, so an engineering owner needs to approve or amend those requirements. 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. Separately from that decision:
- Land the stacked pull requests first and rebase, so this diff is only the AlertDialog motion commit instead of 26,547 authored lines.
- While the alert fades out under touch, keyboard and assistive-technology activations still run
onActionand Escape callsonOpenChange(false)again; make the leaving alert inert. - Under React StrictMode, an AlertDialog that mounts open never starts its presentation and stays invisible and unresponsive to touch; let the status effect own and clean up its animation frame.
- The
adaptivethreshold change contradictscomponent:SelectorDEC-1; split it out or keep the current query. - The
lintjob runs out of memory on this head while the base passes.
[Automated review]
6e2412f to
b635c03
Compare
There was a problem hiding this comment.
This pull request is on hold for an owner decision: AlertDialog's touch motion rests only on draft spec:AST-094 (FR4, FR5, FR7, FR8 and DEC-1), and FR4 narrows the current architecture:theme-tokens INV3, so an engineering owner needs to approve or amend those requirements. 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. Separately from that decision:
- Land the stacked pull requests first and rebase, so this diff is only the AlertDialog motion commit instead of 27,525 authored lines.
- While the alert fades out under touch, keyboard, click and assistive-technology activations still run
onAction, and Escape callsonOpenChange(false)again; make the leaving alert inert. - The
adaptivethreshold change contradictscomponent:SelectorDEC-1; split it out or keep the current query. - The
lintjob runs out of memory on this head whilemainpasses. - Add a StrictMode mount-open test for the new motion effect; the StrictMode presentation issue raised on the earlier head no longer applies.
[Automated review]
b635c03 to
1c55503
Compare
There was a problem hiding this comment.
This pull request is on hold for an owner decision: AlertDialog's iPhone and iPad motion rests only on draft spec:AST-094 (FR5, FR7, DEC-1), and that record's FR4 and DEC-1 forbid the theme-tunable touch motion this head adds through defineTheme({motion: {springs, durations, curves}}), so an engineering owner needs to settle which holds, and whether the two non-component experimental subpaths are an acceptable boundary under spec:AST-017 FR21. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once the record says so, and the author may do so if they are an engineering owner. Separately from that decision:
- While the alert fades out on iOS, keyboard and assistive-technology activations still run
onActionand Escape callsonOpenChange(false)again; make the leaving alert inert. - Under Reduce Motion the iOS fades last 404 ms on a spring, but
design:reduced-motionDR2 allows an opacity-only change of 200 ms or less, and DR4 bars theme values that lengthen it. - The compact test's 744 px width contradicts
spec:AST-012FR9 (the theme'smdpoint or a height under 500), and theplatformadaptation condition extends the vocabulary FR2 closes. - Land the stacked pull requests first and rebase, so this diff is only the AlertDialog motion commit instead of 31,379 authored lines.
- Add a StrictMode mount-open test for the new motion effect.
[Automated review]
| ref={ref} | ||
| isOpen={isOpen} | ||
| ref={mergedRef} | ||
| isOpen={motion.isPresented} |
There was a problem hiding this comment.
While the alert fades out on iOS, Dialog stays an open modal here, but only the touch tracker consults the inert window (:250). A second Enter or assistive-technology activation of the still-focused Delete runs onAction again (:285), and Escape calls onOpenChange(false) on an alert the caller already closed. Making the content inert while isPresented && !isOpen, or dropping the handlers once isOpen is false, would close that window.
There was a problem hiding this comment.
Fixed at 7c77707: while the alert fades out, a click on an action or Cancel, Enter, an assistive-technology activation and Escape do nothing (isLeaving guards the three handlers). AlertDialog.test.tsx covers it.
| const handle = panel.animate( | ||
| {opacity: 1, scale: 1}, | ||
| {spring: springs.alertPresent}, | ||
| {crossfade: springs.alertPresent}, |
There was a problem hiding this comment.
Under Reduce Motion this fades on springs.alertPresent, a 404 ms curve, and the exit at :252 does the same on springs.alertDismiss. design:reduced-motion DR2 (current) requires an opacity-only change that finishes in 200 ms or less on a coarse pointer, and DR1 rules out springs.
| 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) defines compact as width below the theme's effective md point or height below 500, used by this resolver and by Selector's adaptive, with no size numbers of their own. 744px keeps a number of its own and gives the popover at 744–767 px where FR9 gives the sheet.
| * 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 (current) closes the when vocabulary to width, height, pointer, contrast and motion: "Future environment fields require explicit additions to this closed vocabulary." platform needs that amendment first, or should come out of this pull request; AlertDialog's motion does not use it.
1c55503 to
6dca4bf
Compare
|
Addressed at 6dca4bf:
Waiting in the reviewers' queue: the spec decision (draft AST-094 FR4, FR5, FR7, FR8, DEC-1 and theme-tokens INV3; the alert's springs and durations are now theme motion roles) and the |
There was a problem hiding this comment.
This pull request is on hold for an owner decision: AlertDialog's iPhone and iPad motion rests only on draft spec:AST-094 (FR5, FR7, DEC-1), and that record's FR4 and DEC-1 forbid the theme-tunable touch motion this head adds through defineTheme({motion: {springs, durations, curves}}), so an engineering owner needs to settle which holds, and whether the two non-component experimental subpaths are an acceptable boundary under spec:AST-017 FR21. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request once the record says so, and the author may do so as an engineering owner. Separately from that decision:
- Under Reduce Motion the iOS fades last 404 ms on a spring, but
design:reduced-motionDR2 allows an opacity-only change of 200 ms or less, and DR4 bars theme values that lengthen it. - The changeset and docs say no click, key or assistive-technology activation reaches the alert during either transition, but during the entrance they still run
onActionandonOpenChange; gate them until the presentation completes or narrow the text. - The compact test's 744 px width contradicts
spec:AST-012FR9 and the compact definitionmainnow exports, and theplatformadaptation condition extends the vocabulary FR2 closes. - Land the stacked pull requests first and rebase, so this diff is only the AlertDialog motion commit instead of 31,567 authored lines.
[Automated review]
| // or an assistive-technology activation on an action, and Escape, do | ||
| // nothing until it is hidden. Focus stays where it is, so Dialog returns it | ||
| // to the opener when the fade ends. | ||
| const isLeaving = motion.isDriving && motion.isPresented && !isOpen; |
There was a problem hiding this comment.
This drops clicks, keys and Escape only while the alert leaves. During the entrance only the touch tracker consults the 435 ms window (:270), so a keyboard or assistive-technology activation of Delete still runs onAction, and Cancel or Escape still calls onOpenChange(false), while .changeset/alertdialog-touch-motion.md and the usage docs say nothing reaches the alert during either transition. Either gate these handlers on the same window as acceptsTouches() (with a keyboard-during-entrance test), or narrow the changeset, docs and file header to touches.
6dca4bf to
3b4bd41
Compare
There was a problem hiding this comment.
This pull request is on hold for an engineering-owner decision: AlertDialog's touch entrance and exit, the exit ignoring keys and assistive-technology activation, the alert.present and alert.dismiss rows added to current AST-094 FR4, and tracking a mouse or pen across the actions where AST-094 FR19 says no recognizer attaches are not yet settled by any current spec. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA once the AlertDialog record states what is approved, or by approving a spec pull request that states it; the author may do so as an engineering owner. Independent of that decision, please:
- split the 32,721 changed lines so this pull request carries only the AlertDialog slice once its parent stack lands;
- run the alert's busy-main-thread proof in WebKit through a 1 s block, as AST-094 FR5 requires, or drop the changeset's busy-page promise;
- make the usage doc and changeset (which say a mouse keeps each button's click), FR6 (which omits
presentation="dialog") and the DEC-11 test (which sets variables no alert role reads) match the code.
[Automated review]
| // transition is taken and dropped (`isInert`) rather than reaching the | ||
| // button under it. | ||
| const actionsRef = useRef<HTMLElement>(null); | ||
| const isTracking = motion.isPresented && isInline !== true; |
There was a problem hiding this comment.
isTracking has no pointer condition, so on a desktop the action row is tracked and a mouse press on Delete released over Cancel runs Cancel. Current AST-094 FR19 says no recognizer attaches where the primary pointer is fine, and the usage doc says a mouse keeps each button's click; this waits on the owners' answer.
| test.skip( | ||
| browserName !== 'chromium', | ||
| 'screencast frame evidence is Chromium-only in Playwright (WebKit: the iOS simulator rig)', | ||
| ); |
There was a problem hiding this comment.
AST-094 FR5's verification row asks for this proof in Chromium and WebKit through a 1 s block; this skips WebKit and blocks 250 ms (line 755), while the interaction harness README lists WebKit screencast frames as supported.
There was a problem hiding this comment.
At 7c77707 the case blocks 1 s, and WebKit's evidence is the iOS simulator's 1 s block in Safari, linked in the description.
There was a problem hiding this comment.
Thanks — the diff now names the case as a 1 s main-thread block (blockMainThread(page, 1000)) in Chromium, and the PR body links the same 1 s block on the iOS simulator in Safari as WebKit's evidence. I can't change this review's verdict here; the next full review should weigh whether that simulator evidence satisfies FR5's WebKit proof.
| ['--motion-springs-alert-present' as string]: '0.8s 0', | ||
| ['--motion-springs-alert-dismiss' as string]: '0.8s 0', |
There was a problem hiding this comment.
No alert role reads these variables: alert.present and alert.dismiss are carried by --motion-alert-present and --motion-alert-dismiss (motion/roles.ts), so this test passes whether or not the roles stay internal.
| | FR3 | The anatomy is title, description, then the buttons: Cancel then action on wider screens, the action stacked above Cancel on small screens, Cancel keeping initial focus either way; the action may show a loading state while `isActionLoading`. | shipped source and focused tests | verify | | ||
| | FR4 | `isInline` renders the same anatomy without modality and without the alert-dialog role. | shipped source and public docs | verify | | ||
| | FR5 | `useImperativeAlertDialog` returns `show`, `hide`, and the element to render, and owns `isOpen`/`onOpenChange` for its caller. | shipped source and focused tests | verify | | ||
| | FR6 | On phones (compact width with a touch pointer), alerts are 320 px wide in every theme, matching iOS, with no new theme role; elsewhere the default width is 400. | owner decision, 2026-10-26 | settled | |
There was a problem hiding this comment.
presentation="dialog" keeps 400 px on a phone (AlertDialog.tsx:251-256), which this unconditional 320 px does not allow; scope it to presentation="adaptive". Its verification row at line 191 has 2 cells in a 5-column table.
| 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\nUnder a finger, on any touch device, the alert also appears and leaves the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, and it fades out with the dimming when it closes. While it appears a touch does nothing (keys and assistive technology still act), and while it leaves nothing reaches it: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it only cross-fades. With a mouse it keeps the theme entrance and closes at once.\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.
This says a mouse keeps each button's click, but the code tracks a mouse across both actions (AlertDialog.tsx:277, AlertDialog.test.tsx:755), and line 184 says the actions are tracked "under every pointer".
There was a problem hiding this comment.
This pull request is on hold for an engineering-owner decision: AlertDialog's touch entrance and exit, dropping keys and assistive-technology activation during that exit, the alert.present and alert.dismiss rows added to current AST-094 FR4, and tracking a mouse or pen across the actions where AST-094 FR19 says no recognizer attaches are not settled by any current spec. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA once the AlertDialog record states what is approved, or by approving a spec pull request that states it; the author may do this as an engineering owner. Independent of that decision, please:
- write the alert's opening state before paint, since an open from a timer or a network response can show it at full opacity for a frame before it scales in;
- land the parent stack first so this pull request carries only the AlertDialog change (32,872 changed lines today), and get main's
touch-support-bundleroot-entry test passing; - run the alert's busy-thread proof in WebKit through a 1 s block and add device evidence, as AST-094 FR5 and its platform support ask;
- make FR6, the usage docs and the DEC-11 test match the code.
[Automated review]
| // dialog can stay on the main thread (motion/animator.ts, known limits); | ||
| // that case is not yet proven here. | ||
| transitionEndsAtRef.current = Number.POSITIVE_INFINITY; | ||
| panel.set({opacity: 0, scale: presentScaleFrom(elementRef.current)}); |
There was a problem hiding this comment.
These start values are written in a passive Effect, after the commit where Dialog has already applied display: flex; opacity: 1 and this alert has turned the theme keyframes off. For an open that is not a discrete event (a timer, a network response, useImperativeAlertDialog().show() after an await) the browser can paint that frame first, so the alert flashes at full opacity outside the top layer before it scales in (architecture:react-component-runtime INV1). Holding opacity 0 until the presentation starts, or writing these in a layout Effect, would close the gap; a timer-driven open test would pin it.
| // an animation at its transaction's commit, and the animator promotes | ||
| // it in this task. Chromium runs it on the compositor from that first | ||
| // frame through a busy main thread (the busy-thread browser proof). In | ||
| // iOS Safari an animation created in the task that first displays a |
There was a problem hiding this comment.
This is the case motion/animator.ts:100-101 documents as staying on the main thread in iOS Safari, while the changeset promises the animation keeps running on a busy page. AST-094 FR5's verification row and its platform support ask for the busy-thread proof in WebKit through a 1 s block and real-device evidence; the alert's proof skips WebKit and blocks 250 ms.
| // transition is taken and dropped (`isInert`) rather than reaching the | ||
| // button under it. | ||
| const actionsRef = useRef<HTMLElement>(null); | ||
| const isTracking = motion.isPresented && isInline !== true; |
There was a problem hiding this comment.
isTracking has no pointer condition, so on a desktop a mouse press on Delete released over Cancel runs Cancel. Current AST-094 FR19 says no recognizer attaches where the primary pointer is fine, and family:action-groups FR5, cited here and in the gesture host, is not a record on main or at this head. This waits on the owner question about fine pointers.
| // or an assistive-technology activation on an action, and Escape, do | ||
| // nothing until it is hidden. Focus stays where it is, so Dialog returns it | ||
| // to the opener when the fade ends. | ||
| const isLeaving = motion.isDriving && motion.isPresented && !isOpen; |
There was a problem hiding this comment.
While the alert leaves, this also drops Enter, Escape and assistive-technology activation for the 404 ms fade. AST-094 FR35 and DEC-16 hold only pointer input, and only during an entrance; no record states this exit behavior yet.
| | FR3 | The anatomy is title, description, then the buttons: Cancel then action on wider screens, the action stacked above Cancel on small screens, Cancel keeping initial focus either way; the action may show a loading state while `isActionLoading`. | shipped source and focused tests | verify | | ||
| | FR4 | `isInline` renders the same anatomy without modality and without the alert-dialog role. | shipped source and public docs | verify | | ||
| | FR5 | `useImperativeAlertDialog` returns `show`, `hide`, and the element to render, and owns `isOpen`/`onOpenChange` for its caller. | shipped source and focused tests | verify | | ||
| | FR6 | On phones (compact width with a touch pointer), alerts are 320 px wide in every theme, matching iOS, with no new theme role; elsewhere the default width is 400. | owner decision, 2026-10-26 | settled | |
There was a problem hiding this comment.
The code keeps 400 px on a phone under presentation="dialog" (AlertDialog.tsx:255-256), which the restoring value of architecture:public-component-api INV16 needs, but this row makes 320 px unconditional. Scope it to the adaptive presentation, and fill the empty cells of its verification row at line 191.
| 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\nUnder a finger, on any touch device, the alert also appears and leaves the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, and it fades out with the dimming when it closes. While it appears a touch does nothing (keys and assistive technology still act), and while it leaves nothing reaches it: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it only cross-fades. With a mouse it keeps the theme entrance and closes at once.\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.
This says the actions are tracked under a finger or a pen and that a mouse keeps each button's click, but the code tracks every pointer (AlertDialog.tsx:277, useActionGroupTracking.ts:93-94), as this file's presentation description and the changeset say. The dense description at line 206 repeats it.
| it('takes no theme variable: `--motion-springs-alert-*` on its theme changes neither the spring it plays nor the touch window', async () => { | ||
| render( | ||
| <div | ||
| data-astryx-theme="slow" |
There was a problem hiding this comment.
No alert role reads these variables: alert.present and alert.dismiss are carried by --motion-alert-present and --motion-alert-dismiss (motion/roleValue.ts:29-36), so this test passes whatever the hook reads.
| | `springs.menuClose` | damping ratio 0.85, response 0.35 s | measured: UIMenu closing, iOS 26.2 simulator, 2026-10-23 | | ||
| | `springs.subMenuOpen` | damping ratio 0.85, response 0.4 s | measured: UIKit's own spring for UIMenu's sub-menu opening, iOS 26.2 simulator, 2026-10-24 | | ||
| | `springs.subMenuBack` | damping ratio 1, response 0.4 s | measured: UIKit's own spring for UIMenu's sub-menu header back, iOS 26.2 simulator, 2026-10-24 | | ||
| | `alert.present` | mass 1, stiffness 522.35, damping 45.71 | measured: UIAlertController alert appearing, iOS 26.2 simulator, 2026-10-23 | |
There was a problem hiding this comment.
FR4 and DEC-5 have a component's measured roles join through that component's own record and the generated role table; writing them into AST-094's own table adds a requirement to a current record, which waits on the open owner question.
There was a problem hiding this comment.
This pull request is on hold for an engineering-owner decision: AlertDialog's touch entrance and exit, dropping keys and assistive-technology activation while the alert leaves, and the alert.present and alert.dismiss rows added to AST-094's FR4 table have no current spec, because AST-094 leaves each overlay's presentation and exit to its own record and component:AlertDialog is still a draft. An engineering owner records the decision by stating it in the AlertDialog record and commenting /approve-spec <full head SHA>, or by approving a spec pull request that states it; the author may do this as an engineering owner. Independent of that decision, please:
- get
scripts/touch-support-bundle.test.mjspassing, since its root-entry case fails on this head (the description points to #8671 for that); - render the alert's opening state in the commit instead of repairing it in a passive effect (
useAlertMotion.ts:273); - add the WebKit busy-thread proof and the real-device evidence AST-094 asks for, or narrow the changeset's claim that the animation keeps running when the page is busy;
- land the stacked layers first so this diff carries only the AlertDialog change (33,172 changed lines today), and align the usage text, FR6 and the DEC-11 test with the code.
[Automated review]
| // dialog can stay on the main thread (motion/animator.ts, known limits); | ||
| // that case is not yet proven here. | ||
| transitionEndsAtRef.current = Number.POSITIVE_INFINITY; | ||
| panel.set({opacity: 0, scale: presentScaleFrom(elementRef.current)}); |
There was a problem hiding this comment.
The opening commit has already rendered the dialog at opacity: 1 with the theme keyframes off (AlertDialog.tsx:85-86 plus Dialog's open style), and these start values arrive only in this passive Effect, which architecture:react-component-runtime INV1 forbids. Rendering the start state in the commit, or writing it in a layout Effect, closes the window that an open from a timer or a network response leaves before paint.
| <div | ||
| data-astryx-theme="slow" | ||
| style={{ | ||
| ['--motion-springs-alert-present' as string]: '0.8s 0', |
There was a problem hiding this comment.
Since the rename the roles read --motion-alert-present and --motion-alert-dismiss (motion/roleValue.ts:29-36), so setting --motion-springs-alert-* here cannot make this test fail. Setting the roles' own variables makes it test DEC-11.
| | FR3 | The anatomy is title, description, then the buttons: Cancel then action on wider screens, the action stacked above Cancel on small screens, Cancel keeping initial focus either way; the action may show a loading state while `isActionLoading`. | shipped source and focused tests | verify | | ||
| | FR4 | `isInline` renders the same anatomy without modality and without the alert-dialog role. | shipped source and public docs | verify | | ||
| | FR5 | `useImperativeAlertDialog` returns `show`, `hide`, and the element to render, and owns `isOpen`/`onOpenChange` for its caller. | shipped source and focused tests | verify | | ||
| | FR6 | On phones (compact width with a touch pointer), alerts are 320 px wide in every theme, matching iOS, with no new theme role; elsewhere the default width is 400. | owner decision, 2026-10-26 | settled | |
There was a problem hiding this comment.
The code keeps 400 under a finger with presentation="dialog" and honours an explicit width (AlertDialog.tsx:255-256); FR6 could say it sets the default and name that exception. The FR6 verification row below fills 2 of the table's 5 columns.
| 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\nUnder a finger, on any touch device, the alert also appears and leaves the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, and it fades out with the dimming when it closes. While it appears a touch does nothing (keys and assistive technology still act), and while it leaves nothing reaches it: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it only cross-fades. With a mouse it keeps the theme entrance and closes at once.\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.
This still says a mouse keeps the click of each button, but the head tracks a mouse across the actions (the "tracks a mouse too" test), and the presentation description says the actions are tracked under every pointer.
There was a problem hiding this comment.
This pull request is on hold for an engineering-owner decision. AlertDialog's touch entrance and exit, and its dropping of keys and assistive-technology activation while it fades out, rest only on the draft component:AlertDialog record, because current AST-094 leaves each overlay's presentation and exit to that record. The two alert rows added to AST-094's FR4 table also go against its DEC-5. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request for the FR4 rows, and by approving the AlertDialog behavior here or making that record current; the author may do this as an engineering owner. Independent of that decision, please:
- get
scripts/touch-support-bundle.test.mjspassing: its root-entry case fails on this head and passes on main, and #8671 is still open; - land the parent stack first, so this diff carries only the AlertDialog change (33,046 authored lines today);
- add the WebKit busy-main-thread proof and the device evidence AST-094 FR5 asks for, or start the entrance in the next animation frame as
animator.tsadvises; - make the usage docs say the actions are tracked for a mouse too.
[Automated review]
| 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\nUnder a finger, on any touch device, the alert also appears and leaves the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, and it fades out with the dimming when it closes. While it appears a touch does nothing (keys and assistive technology still act), and while it leaves nothing reaches it: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it only cross-fades. With a mouse it keeps the theme entrance and closes at once.\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.
This says the actions are tracked "under a finger or a pen" and that "A mouse, the keyboard and assistive technology keep the click of each button", but useActionGroupTracking defaults to every pointer and AlertDialog passes no pointerTypes. The test "tracks a mouse too (family:action-groups FR5)" confirms it, and family:action-groups FR5 applies the tracking to every web pointer. A mouse press on Delete released over Cancel runs Cancel. Suggest: "Under every pointer, mouse included, the two actions are tracked as one group …; the keyboard and assistive technology keep each button's own activation." The dense text at line 199 needs the same change.
| if (now === 'open' && was === 'hidden') { | ||
| // The Dialog opened its modal in this same flush (its effect runs | ||
| // first), and nothing has painted it yet. The alert starts at its | ||
| // starting values and moves from rest with no start time: the browser | ||
| // starts it at the commit that first shows it, as Core Animation starts | ||
| // an animation at its transaction's commit, and the animator promotes | ||
| // it in this task. Chromium runs it on the compositor from that first | ||
| // frame through a busy main thread (the busy-thread browser proof). In | ||
| // iOS Safari an animation created in the task that first displays a | ||
| // dialog can stay on the main thread (motion/animator.ts, known limits); | ||
| // that case is not yet proven here. | ||
| transitionEndsAtRef.current = Number.POSITIVE_INFINITY; | ||
| panel.set({opacity: 0, scale: presentScaleFrom(elementRef.current)}); | ||
| backdrop.set({opacity: 0}); | ||
| const handle = present(panel, backdrop); | ||
| presentRef.current = handle; | ||
| armTouchWindow(handle); | ||
| return; |
There was a problem hiding this comment.
The presentation is created in the same passive-effect flush that runs Dialog's showModal(). motion/animator.ts records that in iOS Safari "an animation created in the task that first displays a dialog stays on the main thread: start it in the next animation frame", and the comment here says that case "is not yet proven". AST-094 FR5 requires the motion to keep advancing through a main-thread block, and its Platform support asks for a WebKit busy proof and device evidence. The alert's busy test skips WebKit, while the changeset promises the animation keeps running on a busy page. Could the presentation start in the next frame, with the StrictMode-safe cancel, plus a WebKit busy proof or a device clip?
| | `springs.alertPresent` | mass 1, stiffness 522.35, damping 45.71 | measured: UIAlertController alert appearing, iOS 26.2 simulator, 2026-10-23 | | ||
| | `springs.alertDismiss` | mass 1, stiffness 522.35, damping 45.71 | measured: UIAlertController alert dismissing, iOS 26.2 simulator, 2026-10-23 | |
There was a problem hiding this comment.
These rows put the alert's measured springs into AST-094's own FR4 table. FR4 and DEC-5 list only the roles this record names, and send roles a component's own record measures through the generated motion role table. This needs an engineering owner's decision (/approve-spec <full head SHA>), or the rows move to the AlertDialog record's measurement.
There was a problem hiding this comment.
This pull request is on hold for an engineering-owner decision: AlertDialog's touch entrance and exit, and its dropping of keys and assistive-technology activation while it fades out, rest only on the draft AlertDialog record, because current AST-094 leaves each overlay's presentation and exit to that record, and the two alert rows added to AST-094's FR4 table go against its DEC-5. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request for the FR4 rows, and by approving the AlertDialog behavior here or making that record current; the author may do this as an engineering owner. Independent of that decision, please:
- get
scripts/touch-support-bundle.test.mjspassing: its root-entry case fails on this head and passes at the base, and #8671 is still an open draft; - land the parent stack first, so this diff carries only the AlertDialog change (31,326 authored lines today);
- add the WebKit busy-main-thread proof and device evidence AST-094 FR5 asks for, or start the entrance in the next animation frame as
animator.tsadvises; - make the usage docs say the actions are tracked for a mouse too.
[Automated review]
| 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\nUnder a finger, on any touch device, the alert also appears and leaves the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, and it fades out with the dimming when it closes. While it appears a touch does nothing (keys and assistive technology still act), and while it leaves nothing reaches it: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it only cross-fades. With a mouse it keeps the theme entrance and closes at once.\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.
This says the actions are tracked under a finger or a pen and that "A mouse, the keyboard and assistive technology keep the click of each button", but useActionGroupTracking tracks every pointer by default (EVERY_POINTER) and AlertDialog passes no pointerTypes; the test "tracks a mouse too (family:action-groups FR5)" passes, and both changesets say "every pointer". A mouse press on Delete released over Cancel runs Cancel. Suggest: the two actions are tracked as one group under every pointer, mouse included; the keyboard and assistive technology keep each button's own activation. The dense description at line 199 says the same thing.
| transitionEndsAtRef.current = Number.POSITIVE_INFINITY; | ||
| panel.set({opacity: 0, scale: presentScaleFrom(elementRef.current)}); | ||
| backdrop.set({opacity: 0}); | ||
| const handle = present(panel, backdrop); |
There was a problem hiding this comment.
The entrance is created here, in the same effect flush as Dialog's showModal(). animator.ts:100-101 records that iOS Safari keeps "an animation created in the task that first displays a dialog" on the main thread and says to start it in the next animation frame, and the comment above (line 274) says this case "is not yet proven here". AST-094 FR5 and its Platform support ask for the busy-main-thread proof in WebKit and real-device evidence, and the busy test skips WebKit, while the changeset promises the animation keeps running on a busy page. Either start the presentation in the next frame (keeping the StrictMode-safe start) and add the WebKit proof and a device clip, or narrow the changeset's promise.
There was a problem hiding this comment.
Fixed at 8752ee3. The presentation now starts in the next animation frame (the view's requestAnimationFrame, cancelled on cleanup), after the task that shows the dialog. The unit case "starts the presentation in the next animation frame, not in the task that shows the dialog" fails on the old code.
| | `springs.alertPresent` | mass 1, stiffness 522.35, damping 45.71 | measured: UIAlertController alert appearing, iOS 26.2 simulator, 2026-10-23 | | ||
| | `springs.alertDismiss` | mass 1, stiffness 522.35, damping 45.71 | measured: UIAlertController alert dismissing, iOS 26.2 simulator, 2026-10-23 | |
There was a problem hiding this comment.
These rows put a component's measured pair into AST-094's own FR4 table. FR4 says "further roles a component's own record measures join through the generated motion role table (DEC-5)", and DEC-5 says FR4 lists the roles this record names and cites the generated table for the roles a component's record measures. This is a normative change to a current record, so it needs an engineering owner's decision (an exact-head /approve-spec) or the rows should move to the alert's own record and the generated table.
There was a problem hiding this comment.
Fixed at 8752ee3. AST-094's record is untouched. AlertDialog's own record names the two springs in its Motion roles table, and the generated motion role table carries them.
There was a problem hiding this comment.
Requesting changes. The touch entrance and exit, the entrance hold, the action-group tracking and the 320 px phone width all match the current AlertDialog record (FR6–FR9), so no spec decision is pending; what remains is scope, evidence and two fixes.
- Land #8083 first, or retarget onto it, because this diff carries its 1,975-line tracking commit and puts the pull request at 3,870 authored lines.
- Add the WebKit busy-main-thread proof and the iOS Safari and Android Chrome device clips that AST-094 FR5 and its Platform support require, keep that wording in the FR6/PR2 verification row, and until then limit the changeset's busy-page sentence to what the Chromium test proves.
- Register the alert's two springs on their own, since
registerSpringsTable()inuseAlertMotion.tsships the 18 other role springs to AlertDialog and the alert's springs to every other component that registers roles, against INV6 of the style-authoring architecture. - Correct PR1 in the AlertDialog record so it names the two media-query listeners that
useTouchPresentation()adds to every mounted AlertDialog.
Two optional comment suggestions are inline.
[Automated review]
| present: SpringDescription; | ||
| dismiss: SpringDescription; | ||
| } { | ||
| registerSpringsTable(); |
There was a problem hiding this comment.
registerSpringsTable() registers all 20 role springs, so AlertDialog's bundle carries 18 rows it never reads, and adding the alert rows to SPRING_ROLES puts them in Slider's, SegmentedControl's and every animated-layout bundle. Architecture INV6 (component style authoring) asks that a tuning table ship a component only the rows it uses. Registering the alert's two springs through their own register function keeps both sides clean.
There was a problem hiding this comment.
Moot at 8e275a2. The owner ruled the alert's motion non-gesture (DEC-4), so it plays the theme's dialog motion tokens. No spring is registered, and both spring roles are gone.
| | FR7 | Browser tests under a coarse pointer, with keyboard and assistive-technology input | a tap during the presentation, Enter and Escape during it, a screen reader's activation, a tap during the dismissal; a mouse click during the presentation under a fine pointer | a held key or assistive-technology action, content made inert during the entrance, a tap that reaches a leaving alert, or a mouse click held under a fine pointer fails. No test exists yet; FR7 is not shipped. | `audit:AlertDialog/behavior` | | ||
| | FR8 | Pointer-stream tests with a mouse, a pen, and a finger, and keyboard tests | a press on Cancel slid onto the action and lifted, and a press lifted outside the actions, each with a mouse, a pen, and a finger; Enter and Space on each action | the press-start action running after a slide, any action running on a lift outside, or a changed keyboard or assistive-technology activation fails. No test exists yet; FR8 is not shipped. | `audit:AlertDialog/behavior` | | ||
| | FR9 | Browser tests at a phone width and at a desktop width, in the default and an alternate theme | a phone width and a desktop width, in the default and an alternate theme; a caller `width` at a phone width; a screen narrower than 320 px; a narrow window under a fine pointer; a coarse screen that is not compact | a phone-width alert other than 320 px in any theme without a caller `width`, a caller `width` overridden, a desktop width changed, an alert wider than the viewport, or a 320 px alert in a narrow fine-pointer window or on a non-compact coarse screen fails. No test exists yet; FR9 is not shipped. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the next-frame start, a close and a reopen mid-transition, the exit backstop); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the present and dismiss curves on the alert springs, the viewport start, reduced motion) and in Chromium frames through a blocked main thread; a human check on a real device | present, dismiss, close during present, reopen during dismiss, Reduce Motion, a fine pointer, a narrow viewport | a curve off its role, a start outside the viewport, a jump on interruption, a scale under Reduce Motion, changed motion under a fine pointer, or an added element fails. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
This row replaces "real-device clips on iOS Safari and Android Chrome" with "a human check on a real device" and limits the busy-main-thread case to Chromium. AST-094's Platform support still requires the busy-main-thread proof in WebKit and real-device evidence for iOS Safari and Android Chrome, so please keep the device-clip wording and attach that evidence.
There was a problem hiding this comment.
Restored at 8e275a2: the FR6/PR2 row names real-device clips on iOS Safari and Android Chrome again. Those clips and WebKit's busy-thread proof are still owed. The human check on this head's preview comes before this leaves draft.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] AlertDialog: under a finger the alert appears and leaves the way an iOS alert does. It scales down into place from 1.2 while it and the dimming fade in. It fades out with the dimming when it closes. Both run on the alert's measured spring and keep running on a busy page. With reduced motion it only cross-fades. While it appears or leaves, a touch on its actions does nothing. While it leaves, a click, a key or an assistive-technology activation on it does nothing. On a phone, an alert with no `width` prop is now UIKit's 320 px wide in every theme, not 400 px. A `width` you pass still wins. This motion replaces the Dialog's entrance and exit under a finger. With a mouse nothing changes. |
There was a problem hiding this comment.
"keep running on a busy page" is proven only in Chromium: the busy-main-thread test skips WebKit, and the iOS Safari next-frame start has no device evidence yet. Until it does, consider limiting this sentence to what the tests show.
There was a problem hiding this comment.
Taken at 8e275a2: the changeset no longer claims a busy page.
| small-screen media query behind the button order, joined by the press | ||
| tracking `family:action-groups` FR5 gives its actions once FR8 ships; Dialog | ||
| owns the modal lifecycle. | ||
| - **PR1 — Dialog-owned resources.** AlertDialog's own listeners are the |
There was a problem hiding this comment.
AlertDialog.tsx now calls useTouchPresentation(), which subscribes two more matchMedia listeners (the touch query and the compact query) per mounted AlertDialog. PR1 should name them beside the small-screen query and the press tracking.
There was a problem hiding this comment.
Fixed at 8e275a2: PR1 names the touch presentation's two queries, beside the small-screen query and the press tracking.
| motion: AlertMotion; | ||
| } | ||
|
|
||
| /** AlertDialog's touch layer: the motion. */ |
There was a problem hiding this comment.
Optional: this layer wraps a single useAlertMotion call. Calling useAlertMotion({isOpen, isEnabled: isTouch && !isInline}) in AlertDialog and deleting this file would remove the need for the comment.
There was a problem hiding this comment.
Taken at 8e275a2: AlertDialogTouch.ts is gone, and AlertDialog calls useAlertMotion itself.
| // Under a touch presentation the alert appears and leaves the way UIKit's | ||
| // does, and stays presented while it fades out. Under a fine pointer none | ||
| // of it attaches. |
There was a problem hiding this comment.
Optional: this restates the lines below. Keeping only the reason is enough: "The alert stays presented while it fades out."
|
/approve-spec 8e275a2 |
|
/approve-spec 09ba1f9 |
There was a problem hiding this comment.
Requesting changes. The owner-approved AlertDialog record now covers the touch look, and the entrance hold and the 320 px phone width match it. But the approved touch motion still conflicts with two current system specs, and a few fixes and proofs are missing.
- Under a finger the fade-out plays on
--duration-exit-dialog, so the alert disappears at once while that pointer exit switch is off (in every theme today, because Core defines no such token), and current AST-090 FR14 and DEC-5 and AST-094 FR7 forbid that; amend those records in a spec pull request or let the touch exit play whatever the switch says. - The touch entrance and exit are timed on theme tokens (
--duration-medium-max,--ease-standard,--ease-exit) where current AST-094 FR1, FR4 and DEC-15 require measured motion roles, so either amend AST-094 or play the alert on roles. - Under Reduce Motion the dimming fades over the whole entrance (400 ms in Storybook's theme, 550 ms with default tokens) instead of the 200 ms that design:reduced-motion DR2 allows, and the touch exit has no cap either, so give both the reduced 200 ms fade and assert the backdrop's duration in the reduced-motion browser case.
- The changeset and the docs say the alert fades out on "your theme's dialog exit", but no theme can set that token today, so describe the close as immediate until the token ships.
- The busy-main-thread proof skips WebKit and blocks for only 250 ms, and the record defers Android Chrome real-device evidence until after merge, while AST-094 FR5 and its Platform support require the 1 s WebKit proof and real-device evidence before merge.
- The
useEffectinuseAlertMotion.tsthat clearsenteringRefwhenisEnabledturns false synchronizes no external system, andacceptsTouchesalready covers that case, so delete it.
Two optional comment suggestions are inline.
[Automated review]
| const fadeOut = stylex.keyframes({from: {opacity: 1}, to: {opacity: 0}}); | ||
|
|
||
| /** The theme's dialog exit duration: 0 ms while that exit is off. */ | ||
| const EXIT_DURATION = 'var(--duration-exit-dialog, 0ms)'; |
There was a problem hiding this comment.
Under a finger this makes the alert's fade-out depend on the pointer exit switch, so it closes at once while --duration-exit-dialog is 0 ms, which is every theme today because Core defines no such token. Current AST-090 FR14 says the exit switches "do not change that exit: a theme that leaves a kind off does not remove it" (DEC-5 rejects this), and AST-094 FR7 requires the exit UIKit plays. Either amend those records in a spec pull request, or let the touch exit play its fade whatever the pointer switch says.
There was a problem hiding this comment.
Taken at 6694005, on the spec owners' ruling: the touch exit fades on the exit tokens (--duration-exit, by default --duration-fast, and --ease-exit) even while a theme leaves its dialog exit off, as AST-090 FR14 and AST-094 FR7 ask. DEC-4 says so now.
| }, | ||
| '::backdrop': { | ||
| animationName: fadeIn, | ||
| animationDuration: durationVars['--duration-medium-max'], |
There was a problem hiding this comment.
This backdrop fade has no reduced-motion override. On a phone with Reduce Motion the alert fades in over 200 ms but the dimming takes the whole entrance (400 ms in Storybook's theme, 550 ms with default tokens), past design:reduced-motion DR2's 200 ms limit. Suggest Dialog's reduced fade under @media (prefers-reduced-motion: reduce) for this and for the touchExit animations. The reduced-motion browser case could also assert the backdrop's duration.
There was a problem hiding this comment.
Fixed at 6694005. The dimming takes Dialog's reduced 200 ms fade through dialog-level custom properties, since Dialog takes no media query inside ::backdrop. The exit stays within 200 ms, and the reduced-motion case asserts both: the dimming's 200 ms and a 300 ms exit capped at 200.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] AlertDialog: under a finger the alert appears the way an iOS alert does. It scales down into place from 1.2 while it and the dimming fade in, on your theme's dialog entrance duration and curve. When it closes, it and the dimming fade out on your theme's dialog exit, so it closes at once while that exit is off. With reduced motion it only fades in. While it appears, a touch on its actions does nothing. While it fades out, a click, a key or an assistive-technology activation on it does nothing. On a phone, an alert with no `width` prop is now UIKit's 320 px wide in every theme, not 400 px. A `width` you pass still wins. With a mouse nothing changes. |
There was a problem hiding this comment.
"fade out on your theme's dialog exit" promises something no theme can do yet. defineTheme's tokens accept only TokenName, which has no --duration-exit-dialog, and the browser proof turns the exit on with a raw :root style. Until the token ships, suggest: "When it closes, it disappears at once, as before."
There was a problem hiding this comment.
At 6694005 the alert fades out on every theme, so the changeset says "When it closes, it fades out with the dimming."
| page, | ||
| browserName, | ||
| }, testInfo) => { | ||
| test.skip( |
There was a problem hiding this comment.
AST-094 FR5's verification row asks for this proof through a 1 s block in Chromium and WebKit, and its Platform support lists the WebKit busy-main-thread proof. This case skips WebKit and blocks for 250 ms.
There was a problem hiding this comment.
At 6694005 the case blocks 1 s on 2 s motions in Chromium. Playwright has no WebKit screencast, so the WebKit 1 s block runs on the iOS simulator in Safari, which the verification row names; its evidence follows in this thread.
|
|
||
| // Synchronizes the touch window with a mouse taking over: a fine pointer | ||
| // acts at once. | ||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect writes only a ref and synchronizes no external system (architecture:react-component-runtime INV2, INV3). acceptsTouches already returns true while !isEnabled. Without the Effect, a finger that comes back mid-entrance still waits for the entrance to end, as FR35 asks. Suggest deleting it.
| onOpenChange(false); | ||
| }, [onOpenChange]); | ||
|
|
||
| // The alert stays presented while it fades out. |
There was a problem hiding this comment.
Optional (INV29): this comment does not describe useTouchPresentation() below it. Suggest moving it beside isOpen={motion.isPresented}, or deleting it.
| // not Dialog's rise, and the dimming fades in on the same tokens. Under | ||
| // reduced motion the alert only fades in, on Dialog's reduced fade. |
There was a problem hiding this comment.
Optional (INV29): the dimming does not use Dialog's reduced fade today. Once it does, suggest: "Under reduced motion the alert and the dimming fade in over Dialog's reduced 200 ms."
|
/approve-spec 6694005 |
There was a problem hiding this comment.
Requesting changes. The owner-approved AlertDialog record now covers the touch look, and the entrance hold, the 320 px phone width and the reduced-motion fades match it. Four things still need fixing before approval:
- The touch exit fades over
--duration-exiton--ease-exit(AlertDialog.tsx:71,:123), which current AST-090 FR14 says must never change a touch exit, so give the touch exit timing those tokens cannot reach or amend AST-090 FR14 and DEC-5 in a spec pull request. - The touch entrance and exit are timed on theme tokens where current AST-094 FR1, FR4 and DEC-15 require motion roles that a theme reaches only through its motion knobs, so either amend AST-094 or play the alert on roles.
- An alert that is open in server markup replays the 1.2 touch entrance after hydration on a touch device, because
styles.touchEntranceoverrides Dialog's settled open (AlertDialog.tsx:368), against AST-013 FR13, so use the settled look for that open and add a server-render-then-hydrate test. - AST-094's Platform support requires the WebKit busy-main-thread proof and real-device iOS Safari and Android Chrome evidence before merge, but the busy-thread case skips WebKit and the record defers iOS Safari to the merged head and covers Android only in the emulator.
Two verification rows in the record also overstate their test coverage (see the inline comments).
[Automated review]
| * The exit duration token, by default the standard dismissal duration. A | ||
| * theme's dialog exit switch does not change a touch exit. | ||
| */ | ||
| const EXIT_DURATION = `var(--duration-exit, ${durationVars['--duration-fast']})`; |
There was a problem hiding this comment.
Current spec:AST-090 FR14 says "The exit switches, --ease-exit, and --duration-exit do not change that exit" for a touch presentation, and FR12 belongs to the pointer block (AST-090: "FR1–FR13 govern dialog and layer surfaces in a pointer presentation"). Reading --duration-exit here, and --ease-exit at line 123, lets a theme retime the touch fade, and a --duration-exit of 0 ms closes the alert at once (useAlertMotion.ts:167-170). The browser case at AlertDialog.motion.interaction.browser.spec.ts:463 lengthens the touch exit through this token. Suggest timing the touch exit on a value the pointer exit tokens do not reach, or amending AST-090 FR14 and DEC-5 first.
There was a problem hiding this comment.
You're right that this changes the basis of the finding. The PR description says this branch builds on #9706 (merged), and the updated spec row now cites spec:AST-090 FR14 alongside spec:AST-094 FR7 for the dismissal playing the exit tokens even while the dialog exit switch is off. I can't change this review's verdict here, but the next full review should re-evaluate this thread against the amended FR14/DEC-5 rather than the version the finding quoted.
| '@media (prefers-reduced-motion: reduce)': fadeIn, | ||
| }, | ||
| '--_alert-dim-duration': { | ||
| default: durationVars['--duration-medium-max'], |
There was a problem hiding this comment.
Current spec:AST-094 FR4: "Every spring, duration and curve a touch presentation plays MUST be a motion role named for its use", and FR1 and DEC-15 let a theme reach touch motion only through its motion knobs. FR35 names the alert as a touch presentation. Timing the touch entrance on --duration-medium-max and --ease-standard contradicts those claims, which the approved AlertDialog record does not amend. Either AST-094 is amended to admit theme-timed overlay motion, or the alert plays roles.
There was a problem hiding this comment.
#9706 amends AST-094 (FR45, DEC-26): motion that a tap, a click, a key or a call starts plays the theme's duration and easing tokens on every pointer, and a touch presentation keeps its look. At 7c77707 the AlertDialog record cites FR45, and springs.alertPresent and springs.alertDismiss stay only as UIKit's measured reference.
There was a problem hiding this comment.
Fair point — the finding quoted the pre-amendment FR4/DEC-15 reading. The updated spec row now puts the presentation on the theme's entrance tokens citing spec:AST-094 FR45, and keeps springs.alertPresent / springs.alertDismiss only as UIKit's measured reference, with no motion playing them. I can't revise this review's verdict here; the next full review should re-evaluate this thread against amended FR45/DEC-26.
| {...mergeProps(themeProps('alert-dialog'), {className, style})} | ||
| xstyle={xstyle} | ||
| xstyle={[ | ||
| motion.look === 'entrance' && styles.touchEntrance, |
There was a problem hiding this comment.
For an alert open in server markup, Dialog applies styles.openSettled (animationName: 'none', Dialog.tsx:901), but this xstyle comes after it, so after hydration on a touch device the alert plays the 1.2 entrance. A server-render-then-hydrate probe on a coarse pointer gives the hydrated dialog the same entrance keyframes as a client mount; on a fine pointer it hydrates with none. That contradicts spec:AST-013 FR13, and the replay is not held for touches either (wasPresentedRef is set in the hydrating render). Suggest the settled look while the shared useSettledOpen reports the server-rendered open, as Dialog does at Dialog.tsx:516-517, plus a hydrate test.
There was a problem hiding this comment.
Fixed at 7c77707: an alert open in the server markup opens at rest after hydration, through the shared useSettledOpen as Dialog does, and takes touches at once there. The hydrate case in useAlertMotion.test.tsx fails on the previous hook.
| page, | ||
| browserName, | ||
| }, testInfo) => { | ||
| test.skip( |
There was a problem hiding this comment.
spec:AST-094 Platform support lists "the busy-main-thread proof in WebKit" as required browser evidence, and only presented hit testing may bring its real-device evidence after merge. Until the iOS-simulator run is attached, the WebKit half of FR5's proof is missing for this presentation.
There was a problem hiding this comment.
The 1 s block on the iOS simulator (Safari, iOS 26.2) is in the description: during the entrance and the dismissal the alert changed on 57–60 of each block's 59–61 frames. The harness's screencast is Chromium-only (startScreencast throws elsewhere), so the spec's case stays Chromium-only and names the simulator rig.
There was a problem hiding this comment.
Thanks — that fills the gap the finding pointed at, as described in the PR body: the iOS simulator (Safari, iOS 26.2) 1 s block during entrance and dismissal, with the alert changing on 57–60 of 59–61 frames per block, and the testing notes stating the screencast-based 1 s block case is Chromium-only. I can't change this review's verdict here; the next full review should weigh whether that simulator evidence plus the Chromium screencast case satisfies the WebKit half of FR5's proof.
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the merged head, beside it the Android Chrome emulator recording at the merged head (Chrome with Gboard; default, Remove animations and a slow profile) | present, dismiss, close during present, reopen during dismiss, Reduce Motion, a fine pointer, a narrow viewport | a curve off its role, a start outside the viewport, a jump on interruption, a scale under Reduce Motion, changed motion under a fine pointer, or an added element fails. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
Two points on this row. First, it moves iOS Safari's real-device pass to the merged head and covers Android Chrome only in the emulator, while current spec:AST-094 Platform support asks for real-device evidence for every iOS Safari and Android Chrome claim before merge. Second (non-blocking), its failure list still says "a jump on interruption … fails", although FR6 records that the alert restarts from rest and no listed test checks interruption.
There was a problem hiding this comment.
There was a problem hiding this comment.
You're right on both points as the diff now stands. The verification row now places the owner's real-device iOS Safari pass at the merged head and Android Chrome in the emulator under spec:AST-094 DEC-27, and the failure list no longer claims an interruption check — it now says no test checks a turn-back on interruption, which does not ship. I can't revise this review's verdict here, but the next full review should re-evaluate this thread against that updated row.
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the merged head, beside it the Android Chrome emulator recording at the merged head (Chrome with Gboard; default, Remove animations and a slow profile) | present, dismiss, close during present, reopen during dismiss, Reduce Motion, a fine pointer, a narrow viewport | a curve off its role, a start outside the viewport, a jump on interruption, a scale under Reduce Motion, changed motion under a fine pointer, or an added element fails. | `audit:AlertDialog/behavior` | | ||
| | FR7 | `useAlertMotion.test.tsx` touch-window cases; `AlertDialog.test.tsx` ("under a finger, while the alert fades out on the theme's dialog exit, runs nothing"); `AlertDialog.motion.interaction.browser.spec.ts` ("a tap while the alert presents or leaves reaches nothing") | a tap during the presentation, Enter and Escape during it, a screen reader's activation, a tap during the dismissal; a mouse click during the presentation under a fine pointer | a held key or assistive-technology action, content made inert during the entrance, a tap that reaches a leaving alert, or a mouse click held under a fine pointer fails. No test covers inert content while the alert leaves or a drag held during the entrance, which do not ship. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
Non-blocking: AlertDialog.test.tsx has no test named "under a finger, while the alert fades out on the theme's dialog exit, runs nothing". The closest is "under a finger, while the alert fades out, runs nothing: a click or Enter on an action, Cancel, or Escape", and that test fires no Enter.
There was a problem hiding this comment.
Fixed at 7c77707: the row names the test exactly, and the test's name no longer mentions Enter.
|
/approve-spec 7c77707 |
There was a problem hiding this comment.
Requesting changes. The touch entrance, the entrance hold, the fade-out exit and the 320 px phone width match the AlertDialog record, which an engineering owner approved at this head, and the earlier exit-token and hydration findings are resolved. Two things remain:
- The FR6/PR2 verification row takes the iOS Safari real-device pass and the Android Chrome emulator recording "at the merged head", but AST-094's Platform support requires real-device iOS Safari evidence before merge and its DEC-27 requires the emulator recording at the exact head, so attach both at this head (the iPhone pass covering the close fade and Reduce Motion) or amend AST-094 in its own spec pull request.
- Miss: the changeset (line 5) and the usage doc say a key does nothing while the alert fades out, but only activation and Escape are dropped and Tab still moves focus because the content is not inert (as FR7 records), so reword it to "a click, Enter, Space, Escape or an assistive-technology activation does nothing".
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] AlertDialog: under a finger the alert appears the way an iOS alert does. It scales down into place from 1.2 while it and the dimming fade in, on your theme's dialog entrance duration and curve. When it closes, it fades out with the dimming. With reduced motion it and the dimming only fade in, over 200 ms. While it appears, a touch on its actions does nothing. While it fades out, a click, a key or an assistive-technology activation on it does nothing. On a phone, an alert with no `width` prop is now UIKit's 320 px wide in every theme, not 400 px. A `width` you pass still wins. With a mouse nothing changes. |
There was a problem hiding this comment.
Miss: "a click, a key or an assistive-technology activation on it does nothing" is broader than what ships. Only activation and Escape are dropped (AlertDialog.tsx:291-308); the content is not inert while the alert leaves (FR7's own gap), so Tab still moves focus between Cancel and the action during the fade. Suggested wording: "a click, Enter, Space, Escape or an assistive-technology activation on it does nothing". The same applies to the usage text in AlertDialog.doc.mjs.
There was a problem hiding this comment.
Fixed at 45e9de5: the changeset and the usage doc now say a touch, a click, Enter, Space, Escape or an assistive-technology activation does nothing while it fades out. Tab still moves focus, as FR7 records.
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit, a server-rendered open that opens at rest); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the merged head, beside it the Android Chrome emulator recording at the merged head (Chrome with Gboard; default, Remove animations and a slow profile; `spec:AST-094` DEC-27) | present, dismiss with the dialog exit switch off and on, reopen during dismiss, an open in server markup, Reduce Motion, a fine pointer, a narrow viewport | a timing off the theme's tokens, a start outside the viewport, a dismissal without its fade, a scale or a dimming over 200 ms under Reduce Motion, an entrance replayed after hydration, changed motion under a fine pointer, or an added element fails. No test checks a turn-back on interruption, which does not ship. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
This row takes the iOS Safari device pass and the Android Chrome emulator recording "at the merged head". AST-094's Platform support requires real-device evidence for every iOS Safari claim before merge (only presented hit testing may follow merge), and its DEC-27 has the emulator prove Android "at the exact head under review". Attach both at this head and state them here, or amend AST-094 in its own spec pull request.
There was a problem hiding this comment.
Fixed at 45e9de5: the row now takes the owner's iOS Safari device pass and the Android emulator recording at the exact head. Both come at this head before merge.
|
/approve-spec 45e9de5 |
There was a problem hiding this comment.
Requesting changes. The touch entrance, the entrance hold, the fade-out exit and the 320 px phone width match the current AlertDialog record, whose edits an engineering owner approved at this head, and the changeset and usage doc wording from the last review is fixed. One thing remains:
- The FR6/PR2 verification row asks for the owner's real-device iOS Safari pass and the Android Chrome emulator recording at the exact head, as AST-094's Platform support and DEC-27 require, but neither is attached at this head yet, so attach the iPhone pass covering the close fade and Reduce Motion and the emulator recording with default animations, animations removed and a slow profile.
CI has not run on this head yet because the pull request is a draft.
[Automated review]
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit, a server-rendered open that opens at rest); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the exact head, and with it the Android Chrome emulator recording at the exact head (Chrome with Gboard; default, Remove animations and a slow profile; `spec:AST-094` DEC-27) | present, dismiss with the dialog exit switch off and on, reopen during dismiss, an open in server markup, Reduce Motion, a fine pointer, a narrow viewport | a timing off the theme's tokens, a start outside the viewport, a dismissal without its fade, a scale or a dimming over 200 ms under Reduce Motion, an entrance replayed after hydration, changed motion under a fine pointer, or an added element fails. No test checks a turn-back on interruption, which does not ship. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
This row now asks for the owner's real-device iOS Safari pass and the Android Chrome emulator recording at the exact head, which matches AST-094's Platform support and DEC-27, but neither is attached at this head yet. Please attach both (the iPhone pass covering the close fade and Reduce Motion; the emulator recording with default animations, animations removed and a slow profile) before this merges.
|
/approve-spec 10f4df6 |
There was a problem hiding this comment.
Requesting changes. The touch entrance, the fade-out exit and the 320 px phone width match the current AlertDialog record, whose edits are owner-approved at this head. Three things remain:
- Regression: since main gained the shared Layer input hold, AlertDialog's own entrance gate and leaving guards (
useAlertMotion.ts:132-160,AlertDialog.tsx:284-308) duplicate it, so drop the entrance gate and the tracker'sisInert(Dialog already holds touches through this alert's entrance), hold the exit withholdInput(dialog, {until, scope: 'layer'}), and keep only the Escape guard, as #8373 does. - Regression: the changeset's "While it appears, a touch on its actions does nothing" and the description's "A tap during the entrance already runs a button" describe behavior main already ships through Dialog's entrance hold, so drop that sentence from the changeset and correct the description.
- The FR6/PR2 verification row, with AST-094's Platform support and DEC-27, needs the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this exact head, and the description still lists both as to come.
CI has not run on this head because the pull request is a draft.
[Automated review]
| const enteringRef = useRef<number | null>(null); | ||
| const openingRef = useRef(0); | ||
| const acceptsTouches = useCallback( | ||
| () => !isEnabled || (isOpen && enteringRef.current == null), | ||
| [isEnabled, isOpen], | ||
| ); | ||
|
|
||
| // Synchronizes the dialog's touch entrance with an opening: its start | ||
| // scale, then the end of its CSS animations, which gives touches back. | ||
| // Dialog styles an open dialog in this commit, before its effect shows the | ||
| // modal. So the box and the entrance exist before the first paint. | ||
| const wasPresentedRef = useRef(false); | ||
| useIsomorphicLayoutEffect(() => { | ||
| const opened = isPresented && !wasPresentedRef.current; | ||
| wasPresentedRef.current = isPresented; | ||
| const element = elementRef.current; | ||
| if (!opened || !isEnabled || element == null) { | ||
| return; | ||
| } | ||
| element.style.setProperty( | ||
| SCALE_FROM_PROPERTY, | ||
| String(presentScaleFrom(element)), | ||
| ); | ||
| const opening = ++openingRef.current; | ||
| enteringRef.current = opening; | ||
| void allEnded(animationsOf(element)).then(() => { | ||
| if (enteringRef.current === opening) { | ||
| enteringRef.current = null; | ||
| } |
There was a problem hiding this comment.
Regression: since main gained the shared input hold, Dialog.tsx already calls holdEntranceInput(dialog) on this alert's own entrance, so this entrance wait never sees a touch. With acceptsTouches reduced to () => !isEnabled || isOpen, the browser case "a tap while the alert presents or leaves reaches nothing" still passes 3 of 3. Drop the entrance gate, and hold the exit with holdInput(dialog, {until: <exit animations finished>, scope: 'layer'}), as #8373 does.
There was a problem hiding this comment.
Kept as reviewed in this pull request, by the spec owner's call: one solution per pull request. Removing the entrance gate and holding the exit with the Layer input hold land in #8373, built on main right after this merges.
There was a problem hiding this comment.
Understood — keeping the entrance gate in this pull request per the spec owner's one-solution-per-PR call, with #8373 to drop it and hold the exit with the Layer input hold, is a recorded deferral rather than a refutation of the finding. I can't change this review's verdict here; the next full review should check that #8373 lands and removes the duplication instead of leaving both holds in place.
There was a problem hiding this comment.
Done at 058b372, by the spec owner's decision: the entrance gate is gone, since Dialog's hold covers the entrance, and the exit holds input with holdInput(dialog, {until: <exit animations finished>, scope: 'layer'}).
| highlightExpansion: alertController.actions.highlightExpansion, | ||
| // A mouse press acts as a native button's under any presentation. | ||
| pointerTypes: SLIDING_POINTERS, | ||
| isInert: () => !motion.acceptsTouches(), |
There was a problem hiding this comment.
Regression: this isInert check and the isLeaving guards below duplicate Layer/inputHold.ts, whose 'layer' scope holds every press and click during a dismissal and makes the content inert, which FR7 asks for. Keep only the Escape guard: inert content does not stop the dialog's own cancel.
There was a problem hiding this comment.
Same scope as above: this pull request keeps the guards it was reviewed with, and #8373 replaces them with the exit's 'layer' hold, keeping only the Escape guard.
There was a problem hiding this comment.
Understood — same deferral: this pull request keeps the guards it was reviewed with, and #8373 replaces them with the exit's 'layer' hold, keeping only the Escape guard. I can't change this review's verdict here; the next full review should check that #8373 lands and that the kept guards do not ship alongside the Layer hold.
There was a problem hiding this comment.
Done at 058b372: isInert and the leaving guards on the actions are gone. Only the Escape guard stays, because inert content does not stop the dialog's own cancel event. FR7 now ships whole.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] AlertDialog: under a finger the alert appears the way an iOS alert does. It scales down into place from 1.2 while it and the dimming fade in, on your theme's dialog entrance duration and curve. When it closes, it fades out with the dimming. With reduced motion it and the dimming only fade in, over 200 ms. While it appears, a touch on its actions does nothing. While it fades out, a touch, a click, Enter, Space, Escape or an assistive-technology activation on it does nothing. On a phone, an alert with no `width` prop is now UIKit's 320 px wide in every theme, not 400 px. A `width` you pass still wins. With a mouse nothing changes. |
There was a problem hiding this comment.
Regression: "While it appears, a touch on its actions does nothing" already ships on main through Dialog's entrance hold, and main's dialog-drawer-touch-entrance-hold changeset already announces it. On the base, a tap on Cancel 70 ms into the entrance runs nothing. Suggest dropping this sentence.
There was a problem hiding this comment.
Dropped at b73a802: the changeset no longer announces the entrance hold that main already ships.
There was a problem hiding this comment.
Requesting changes: the touch entrance, the fade-out exit and the 320 px phone width match the current AlertDialog record, whose edits are owner-approved at this head, but three things remain.
- Regression: AlertDialog's own entrance gate and leaving guards (
useAlertMotion.ts:132-160,AlertDialog.tsx:284-308) duplicate the shared Layer input hold that Dialog already applies to this entrance, so drop the entrance gate and the tracker'sisInert, hold the exit withholdInput(dialog, {until, scope: 'layer'}), and keep only the Escape guard, as #8373 does. - Regression: the description's user need ("A tap during the entrance already runs a button") and its item 2 describe an entrance-tap drop that main already ships through Dialog's entrance hold, so restate them around the look, the instant close, the width and the exit-time drop.
- The FR6/PR2 verification row, with AST-094's Platform support and DEC-27, needs the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this exact head; the cited iPhone pass ran on a base without the shared input hold, and the Android recording is still to come.
Non-blocking: the record's FR7 verification row lost its states, failure expectation and audit cells. CI has not run its jobs on this head because the pull request is a draft.
[Automated review]
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit, a server-rendered open that opens at rest); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the exact head, and with it the Android Chrome emulator recording at the exact head (Chrome with Gboard; default, Remove animations and a slow profile; `spec:AST-094` DEC-27) | present, dismiss with the dialog exit switch off and on, reopen during dismiss, an open in server markup, Reduce Motion, a fine pointer, a narrow viewport | a timing off the theme's tokens, a start outside the viewport, a dismissal without its fade, a scale or a dimming over 200 ms under Reduce Motion, an entrance replayed after hydration, changed motion under a fine pointer, or an added element fails. No test checks a turn-back on interruption, which does not ship. | `audit:AlertDialog/behavior` | | ||
| | FR7 | `DialogTouchEntrance.interaction.browser.spec.ts` ("a tap on an alert's action while it enters runs nothing, and once it has entered the same tap runs it"; "keys and assistive technology stay live while the dialog enters"); `useAlertMotion.test.tsx` touch-window cases; `AlertDialog.test.tsx` ("under a finger, while the alert fades out, runs nothing: a click on an action or Cancel, or Escape"); `AlertDialog.motion.interaction.browser.spec.ts` ("a tap while the alert presents or leaves reaches nothing") | |
There was a problem hiding this comment.
Non-blocking: this FR7 row ends after its Verification cell, so it has no representative states, no failure expectation (including the "No test covers inert content while the alert leaves" gap) and no audit:AlertDialog/behavior section. The row had all five cells before the rebase and on main; please restore the other three.
There was a problem hiding this comment.
Restored at d503154: the FR7 row has its representative states, its failure expectation (with the gap that no test covers inert content while the alert leaves, which does not ship) and its audit section again.
|
/approve-spec d503154 |
There was a problem hiding this comment.
Requesting changes. The touch entrance, the fade-out exit and the 320 px phone width match the current AlertDialog record, and an engineering owner approved the record's edits at this head. Two things remain:
- Regression: AlertDialog's own entrance gate and leaving guards (
useAlertMotion.ts:132-160,AlertDialog.tsx:284-308) duplicate the shared Layer input hold that Dialog already applies to this entrance; drop the entrance gate andisInert, hold the exit withholdInput(dialog, {until: <exit animations finished>, scope: 'layer'}), and keep only the Escape guard. - The FR6/PR2 verification row, AST-094's Platform support and DEC-27 need the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this exact head, but the description cites an iPhone pass at an earlier commit on an older base and lists the Android recording as still to come.
Deferring the input-hold move to #8373 still merges two paths for one rule, so the move belongs in this pull request. CI has not run its jobs on this draft yet.
[Automated review]
|
/approve-spec 058b372 |
There was a problem hiding this comment.
Requesting changes. The touch entrance, the fade-out exit with its content held by the shared Layer input hold, and the 320 px phone width match the current AlertDialog record, and an engineering owner approved the record's edits at this head. Two things remain:
- Regression:
useActionGroupTracking.ts:42-47still adds anisInertoption, with its wiring and test, that nothing passes now that the entrance gate is gone; remove it so the Layer input hold stays the one way a transition drops a press. - The FR6/PR2 verification row and AST-094's Platform support and DEC-27 need the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this head, but the description cites runs at earlier commits whose
AlertDialog.tsxanduseAlertMotion.tsdiffer from this head in the exit-time input path.
CI has not run its jobs on this draft yet.
[Automated review]
| /** | ||
| * Read when a press begins. While true, the press is still taken, so the | ||
| * browser makes no click of it, but it paints and runs nothing. UIKit drops | ||
| * touches during a presentation transition this way. | ||
| */ | ||
| isInert?: () => boolean; |
There was a problem hiding this comment.
No caller passes isInert at this head: AlertDialog's entrance gate is gone, and Dialog's entrance hold and the Layer input hold now drop presses during both transitions. Please remove this option, its wiring at line 155 and its test, so the Layer hold stays the one way a transition drops a press.
There was a problem hiding this comment.
Removed at 92e94d3: useActionGroupTracking.ts and its test are main's again, so the Layer input hold is the one way a transition drops a press.
There was a problem hiding this comment.
Requesting changes: the touch entrance, the held fade-out exit and the 320 px phone width match the current AlertDialog record, whose edits are owner-approved at this head, and the unused isInert tracker option is gone, but one item remains.
- The FR6/PR2 verification row and AST-094's Platform support and DEC-27 need the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this head, but both runs cited in the description predate this head's exit-time input path in
AlertDialog.tsxanduseAlertMotion.ts.
The motion spec's "dialog exit on" leg sets --duration-exit-dialog, which nothing reads yet, so it cannot fail (non-blocking). CI has not run its jobs on this draft yet.
[Automated review]
| for (const dialogExit of ['off', 'on'] as const) { | ||
| if (dialogExit === 'on') { | ||
| // A theme that turns the pointer dialog exit on: the touch exit stays. | ||
| await setToken(page, '--duration-exit-dialog', `${EXIT_MS}ms`); |
There was a problem hiding this comment.
Nothing reads --duration-exit-dialog yet (Dialog has no pointer exit), so this "on" iteration repeats the "off" one and cannot fail. Drop it, or note that it becomes live when Dialog reads the switch. Non-blocking.
There was a problem hiding this comment.
Kept as a forward check: the "on" leg pins that the touch exit stays when a theme turns the pointer dialog exit on. Today Dialog does not read --duration-exit-dialog, so it repeats the "off" leg, which carries FR6's claim; the leg becomes live once Dialog reads the switch.
There was a problem hiding this comment.
Fair point — keeping it as a forward check is reasonable: the "on" leg pins that the touch exit stays when a theme turns the pointer dialog exit on, the "off" leg carries FR6's claim today, and the leg becomes live once Dialog reads the switch. I can't change this review's verdict here; the next full review should re-evaluate it on that basis, and the becomes-live note should live in the test itself so a future reader knows the "on" leg cannot fail yet.
There was a problem hiding this comment.
Added at a3f77df: the test now says that Dialog does not read this switch yet, so the "on" leg repeats the "off" one until Dialog plays a pointer exit.
There was a problem hiding this comment.
Requesting changes: the touch entrance, the held fade-out exit and the 320 px phone width match the current AlertDialog record, whose edits are owner-approved at this head, and the motion spec now says why its "dialog exit on" leg repeats the "off" leg. One item remains.
- The FR6/PR2 verification row and AST-094's Platform support and DEC-27 need the owner's real-device iOS Safari pass and the Android Chrome emulator recording at this head, but both runs cited in the description predate the exit-time input path in
AlertDialog.tsxanduseAlertMotion.ts.
CI has not run its jobs on this draft yet.
[Automated review]
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit, a server-rendered open that opens at rest); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the exact head, and with it the Android Chrome emulator recording at the exact head (Chrome with Gboard; default, Remove animations and a slow profile; `spec:AST-094` DEC-27) | present, dismiss with the dialog exit switch off and on, reopen during dismiss, an open in server markup, Reduce Motion, a fine pointer, a narrow viewport | a timing off the theme's tokens, a start outside the viewport, a dismissal without its fade, a scale or a dimming over 200 ms under Reduce Motion, an entrance replayed after hydration, changed motion under a fine pointer, or an added element fails. No test checks a turn-back on interruption, which does not ship. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
This row asks for the owner's real-device iOS Safari pass and the Android Chrome emulator recording at the exact head (with AST-094 Platform support and DEC-27). The runs the description cites were taken on earlier commits, before the exit-time input path in AlertDialog.tsx and useAlertMotion.ts changed. Record both at this head (Android: normal, Remove animations and a slow profile) and cite them in the description.
There was a problem hiding this comment.
The Android recording is at 058b372, which already has the exit-time input path: AlertDialog.tsx and useAlertMotion.ts are byte-identical between it and this head (since then only an unused tracker option went and a test gained a comment). It covers the normal profile, Remove animations and a 4× CPU slow profile, and the description now says so. The spec owner's phone pass at this head is still to come.
There was a problem hiding this comment.
That addresses the Android half if the byte-identity claim holds: a recording at 058b372 over byte-identical AlertDialog.tsx and useAlertMotion.ts, covering the normal profile, Remove animations and a 4× CPU slow profile, with the description now saying so. The finding is not fully closed, though — the spec owner's phone pass at this head is still to come. I can't change this review's verdict here; the next full review should verify the byte-identity claim and that nothing else changed since 058b372 affects the recording, and confirm the iOS pass at this head.
There was a problem hiding this comment.
Requesting changes: the touch entrance, the held fade-out and the 320 px phone width match the AlertDialog record, whose edits are owner-approved at this head, but two items remain.
- Miss: while the alert fades out under a finger, the shared Layer hold it applies (
useAlertMotion.ts:182-185, thenmakeContentInertinLayer/inputHold.ts) marks every child of the<dialog>inert, including the ToastViewport host that lives in the leaving modal, so a toast such as the one the action raises cannot be tapped, focused or read for the whole fade, against AST-003 FR16; keep the toast host (and the dialog's live regions) out of the inert set and above the dismissal shield, with a test. - The owner's real-device iOS Safari pass and the Android Chrome emulator recording that AST-094 Platform support, DEC-27 and the FR6/PR2 row require at the exact head are still missing, since the cited runs come from earlier commits; please record both at the head that carries the fix above.
Two non-blocking notes: handleAction in AlertDialog.tsx no longer has a job, and the usage doc's "on any touch device" overstates the coarse-pointer gate.
[Automated review]
| const release = | ||
| element == null | ||
| ? null | ||
| : holdInput(element, {until: ended, scope: 'layer'}); |
There was a problem hiding this comment.
Miss: this 'layer' hold runs makeContentInert (Layer/inputHold.ts:707-716) on every child of the <dialog>, and the ToastViewport host is one of them while this alert is the latest open modal (Toast/ToastViewport.tsx:767-775). For the whole fade a toast, such as the one the action raises, cannot be tapped, focused or read by assistive technology, against AST-003 FR16 ("ToastViewport remains visible and operable above the current native modal"). Keeping the modal outlet's host (and the dialog's live regions) out of the inert set and above the shield, in the shared hold, fixes it for every dismissal caller.
There was a problem hiding this comment.
Fixed at 62c9b4a: a dismissal hold leaves the toast viewport's host live. It stays out of the inert content, a press on it passes the press hold and the gesture gate, and it re-enters the top layer above the shield. The new "Alert over a toast" browser case and three unit cases prove it.
| const handleAction = useCallback(() => { | ||
| void onAction(); | ||
| }, [onAction]); |
There was a problem hiding this comment.
Non-blocking: with the leaving guard gone, this wrapper only drops the click event before calling onAction; onClick={onAction}, as on the base, does the same.
There was a problem hiding this comment.
Done at 62c9b4a: the button passes onAction directly, as on main.
| 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\nThe two actions are tracked as one group under every pointer, the way iOS tracks the actions of an alert: the action under the pointer highlights as soon as it is pressed, the highlight follows the pointer from one action to the other, and the release runs the action under it. Releasing off both actions runs nothing. 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.', | ||
| '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\nThe two actions are tracked as one group under every pointer, the way iOS tracks the actions of an alert: the action under the pointer highlights as soon as it is pressed, the highlight follows the pointer from one action to the other, and the release runs the action under it. Releasing off both actions runs nothing. The keyboard and assistive technology keep the click of each button.\n\nUnder a finger, on any touch device, the alert also appears the way an iOS alert does: it scales down slightly into place while it and the dimming fade in, on the theme\'s dialog entrance. When it closes it fades out with the dimming. While it appears a touch does nothing (keys and assistive technology still act), and while it fades out its content is inert: a touch, a click, a key or an assistive-technology activation does nothing. With reduced motion it and the dimming only fade in, over 200 ms. With a mouse it keeps the theme entrance and closes at once.\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.
Non-blocking: "Under a finger, on any touch device" overstates the gate. The look plays under a coarse primary pointer (FR6, AST-094 FR13), so a finger on a touchscreen laptop keeps the mouse behavior; "On a phone or tablet" says what ships.
There was a problem hiding this comment.
Done at 62c9b4a: the doc now says "On a phone or tablet", and the dense line "Phone or tablet (coarse pointer)".
Under a touch presentation AlertDialog takes UIKit's alert look on the theme's motion tokens (component:AlertDialog FR6, DEC-4; spec:AST-094 FR45). It scales from 1.2 to 1 and fades in with its dimming on Dialog's entrance duration and curve. It leaves by a fade of the alert and its dimming on the theme's exit duration and easing (spec:AST-090 FR12), even while a theme leaves its dialog exit off (AST-090 FR14, AST-094 FR7). A close during the presentation, or a reopen during the fade, restarts from rest; FR6 records that gap. Under reduced motion the alert and its dimming only fade in, over the reduced 200 ms cross-fade, and the fade out stays within 200 ms. An alert open in the server markup opens at rest after hydration. While the alert fades out, the Layer module's input hold holds its input with the layer scope: its content is inert, and the hold's shield takes every press, so a tap, a click, a key or an assistive-technology activation does nothing (FR7). Escape, the dialog's own cancel event, is dropped until the alert is hidden. A reopen during the fade gives the content back, with focus on the action that had it. The toast viewport, which lives in the latest open modal, stays live and goes back above the hold's shield, so a toast the action raises takes a tap and is read during the fade (spec:AST-003 FR16). Dialog already holds a press while the alert appears. On a phone the alert is 320 px wide in every theme (FR9); a caller's width still wins. A fine primary pointer keeps today's motion, and a mouse press on the actions stays native.
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8095.
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------- | | ||
| | FR1–FR5 | `AlertDialog.test.tsx` ("calls onOpenChange(false) on Escape", "does not call onOpenChange when action is clicked", "does not call onAction on Escape", "does not disable the cancel button while the action is loading", "exposes a named group instead, with no aria-modal") and `useImperativeAlertDialog.test.tsx` ("opens on show()", "closes on hide()") | open, loading, inline, imperative | Escape not cancelling, the action closing by itself, or the inline path claiming the role fails. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx` (the touch look, the start scale, an exit that closes at once at 0 ms or at its end, the backstop, a reopen mid-exit, a server-rendered open that opens at rest); `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit (the presentation on the theme's entrance tokens, the fade on the exit tokens with the dialog exit switch off and on, the viewport start, reduced motion with its 200 ms dimming and exit) and in Chromium frames through a 1 s main-thread block; the same 1 s block on the iOS simulator in Safari; the owner's real-device pass on iOS Safari at the exact head, and with it the Android Chrome emulator recording at the exact head (Chrome with Gboard; default, Remove animations and a slow profile; `spec:AST-094` DEC-27) | present, dismiss with the dialog exit switch off and on, reopen during dismiss, an open in server markup, Reduce Motion, a fine pointer, a narrow viewport | a timing off the theme's tokens, a start outside the viewport, a dismissal without its fade, a scale or a dimming over 200 ms under Reduce Motion, an entrance replayed after hydration, changed motion under a fine pointer, or an added element fails. No test checks a turn-back on interruption, which does not ship. | `audit:AlertDialog/behavior` | |
There was a problem hiding this comment.
This row asks for the owner's real-device iOS Safari pass and the Android Chrome emulator recording at the exact head, as AST-094's Platform support does. The Android run is at an earlier commit, before this head changed the shared Layer hold, the modal outlet and the toast host, so neither item is at this head yet.
|
/approve-spec 62c9b4a |
Stack: #8084 → #8095
This pull request lands first; #8095 builds on it.
User need
On a phone, AlertDialog plays the theme's desktop entrance: it rises 16 px and grows from 95% while the dimming pops on, and it closes at once. The iOS alert shrinks into place from 1.2 while it and the dimming fade in, and fades out to close. Main already holds a tap during the entrance, through Dialog's input hold.
Problem and solution fit
spec:AST-003FR16).Current specification
component:AlertDialogFR6, FR7 and FR9, with DEC-4: an alert's motion is not a gesture, so it plays the theme's tokens on every pointer (spec:AST-094FR45, owner decision 2026-11-02). The touch exit plays even with the dialog exit switch off (spec:AST-090FR14,spec:AST-094FR7). An alert open in server markup opens at rest (spec:AST-013FR13). FR6's turn-back from mid-transition is a recorded gap with its own pull request; With #8342 merged, Dialog holds pointer input while the alert enters, and the leaving alert holds its input with the same Layer hold, so FR7 ships whole. AlertDialog keeps no input gate of its own, apart from an Escape guard. The toast viewport stays visible and operable above the current native modal (spec:AST-003FR16, FR18), also while the hold keeps a leaving modal's content inert;architecture:layer-runtimegains that sentence. It builds on #9706, #9771 and #8342, all merged.Public delta and caller burden
No API change. Under a touch presentation the look, the held input while the alert leaves and the phone width change. A mouse keeps today's motion, and a caller's
widthstill wins. The private modal-outlet registry gains a mark for global hosts; no public export changes.Design and alternatives
AlertDialog gives Dialog its own keyframes under a touch presentation, so the browser plays them on the compositor. The dimming inherits the alert's timing (a backdrop inherits from its dialog), which also carries Reduce Motion's 200 ms. The hook closes the dialog at the fade's end, and holds the dialog's input with the Layer module's layer-scope hold until then. Inert content does not stop the dialog's own cancel event, so AlertDialog drops Escape while it leaves.
The shared hold keeps the toast viewport live, so every caller of a dismissal hold gets it, not only AlertDialog. ToastViewport marks the host it moves into the latest open modal. A dismissal hold leaves marked hosts out of the inert content, and its press hold and the gesture host's gate leave a press on them alone; a presentation's hold still holds a press anywhere, as on main. The hold's shield is a top-layer element shown after the toast viewport, so the hold then sends the top-layer entry signal that a non-modal sheet already sends, and the toast viewport re-enters the top layer above the shield. Holding a narrower element inside the dialog would not do it: the shield still covers the viewport from the top layer, and the press hold listens on the window.
Evidence
useAlertMotion.tsis byte-identical between 058b372 and this head. This head changes the input hold's inert set and the toast viewport's top-layer order, andAlertDialog.tsxnow passesonActionstraight to the button, so a recording at this head, with the new story, is pending. The recording at 058b372 covers the normal profile, Remove animations and a slow profile (4× CPU throttling). Every frame was logged with the dialog's opacity, scale and position, the dimming's opacity, and each pointer event:Maintainer action
#10018 is the CI-side counterpart: the WebKit interaction lane's path filter in
.github/workflows/ci.ymlchanges frompackages/core/src/Layer/inputHold\.ts$topackages/core/src/Layer/(inputHold|modalOutlet)\.ts$. The input hold now importsLayer/modalOutlet.ts, so the gesture and pager fixtures bundle it, and a fork pull request can't change the workflow. Until #10018 merges and this branch updates,test:nodeshows two reds inci-interaction-webkit.test.mjs("keeps everything … bundles in scope", for the gesture fixture and the pager bundle), both onmodalOutlet.ts, and the WebKit lane does not run for a change to that file alone.Scope
Testing
Local gates at 62c9b4a, on main 252b45b:
CI=true pnpm lint(no errors), the build,pnpm typecheck, Storybook,pnpm test:ui(20,100),check:knowledgeandcheck:repopass. Inpnpm test:node16,448 passed, and only the twoci-interaction-webkit.test.mjscases named under Maintainer action failed, both onmodalOutlet.ts.api:report --checkreports drift already on main (DropdownMenu'spresentationdefault, the CLI'sComponentDeprecation), none of it from this change.Writing rubric results
component:AlertDialog: FR6 plays the theme's tokens, with the exit fade on even while the dialog exit switch is off; its turn-back clause carries an implementation gap, listed under Current-state impact. FR7 ships whole: its gap sentence and its Current-state entry are gone, and its verification row names the entrance and exit cases with all five cells. DEC-4, readiness,system_specs, Governing rules and the FR6/PR2 verification row match what ships. No motion role is added.architecture:layer-runtime: one addition to the modal-outlet paragraph, in the present tense: an input hold can make a leaving modal's content inert, the host stays live, and the hold announces its shield's entry, so toasts stay operable above the shield. It names what the code does, with no change history.ToastViewport.modalHost.test.tsx, already in the record'sverified_by.