Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
a157048 to
c56cb3a
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: no current spec covers AlertDialog's touch input rule as implemented here (every tap, press and click is held for 435 ms after the alert starts presenting, and while it fades out its content is inert and focus leaves it), so an engineering owner needs to approve or reject that rule and say which record states it, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independently of that decision, these need changes:
- The branch carries the unmerged #8084, #8342 and touch-foundation commits (about 37,000 authored lines in 214 files); once they land, rebase so this pull request is only the 7-file AlertDialog commit.
gestures/VelocityTracker.tsoffers four selectable release filters and no stillness reset, which contradictsarchitecture:interaction-modalityINV10 andspec:AST-100FR2.- AlertDialog's touch action tracking has no
presentationopt-out, whicharchitecture:public-component-apiINV16 requires unless a current AlertDialog record defines it. - Reopening the alert during its fade leaves focus on the document body, because the dismissal hold blurs the focused action and nothing restores it.
- Under React StrictMode, an alert mounted open loses its presentation hold: the unmount-only effect's cleanup releases it during replay and the transition effect never places it again.
- The description should give element counts for the hold's shield, as
architecture:component-style-authoringINV7 asks for added elements.
[Automated review]
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
Deployment failed for project astryx with the following error: |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: no current spec covers AlertDialog's touch input rule as implemented here (every tap, press and click is held for 435 ms after the alert starts presenting, and while it fades out its content is inert and focus leaves it), so an engineering owner needs to approve or reject that rule and say which record states it, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independently of that decision, these need changes:
- The branch carries the unmerged #8084, #8342 and touch-foundation commits (about 37,600 authored lines in 198 files); once they land, rebase so this pull request is only its 7-file AlertDialog commit.
- AlertDialog's touch action tracking has no
presentationopt-out, whicharchitecture:public-component-apiINV16 requires unless a current AlertDialog record defines it. - AlertDialog's import now carries the whole touch layer and the complete spring table, while
architecture:component-style-authoringINV6 asks for a lazy load under a coarse pointer or a stated justification, and only the table rows the alert uses. - Reopening the alert during its fade leaves focus on the document, because the dismissal hold blurs the focused action and nothing restores it.
- Under React StrictMode an alert mounted open loses its presentation hold, since the unmount-only effect's cleanup releases it during replay and the transition effect never places it again.
- The description should give element counts for the hold's shield, as
architecture:component-style-authoringINV7 asks for added elements.
[Automated review]
|
/approve-spec 25b54b3 |
There was a problem hiding this comment.
Requesting changes. The alert's touch motion, input holds and phone width follow component:AlertDialog FR6, FR7 and FR9, the Layer hold's toast and live-region exceptions follow spec:AST-003 FR16 and spec:AST-086 FR14, and the record edits are owner-approved for this head. One thing blocks:
- FR6's verification row and
spec:AST-094DEC-27 need the iOS device and Android emulator results at the head that lands, but Testing gives the emulator run as pending and the iPhone runs from the previous head, which DEC-31 does not carry because this head changesuseAlertMotion.tsand itsmainmerge changedLayer/useLayer.tsxandhooks/usePager.ts; stating each run for the landing head in Testing (device or emulator, browser, head, animation scale, result) settles it.
CI skipped every job on this draft head; marking it ready for review starts it.
Claims and files checked
- Changeset and description: under a finger the alert scales from 1.2 while it and the dimming fade in, on the theme's entrance timing: verified (
useAlertMotion.ts:41-44,presentat:108). - Changeset and description: it fades out with the dimming on the exit timing and stays open until the fade ends: verified (
ALERT_EXIT_TOKENSat:47,fadeFromHereat:90, the leaving state at:150). - Changeset and description: a close or a reopen midway turns back from where it is: verified (
fadeFromHerestarts from the values on screen; hook tests). - Changeset and description: reduced motion only cross-fades: verified (the animator's 200 ms coarse-pointer cross-fade; browser case).
- Changeset and description: touch input waits while it enters, keys and assistive technology still act: verified (viewport hold holds touch and pen only).
- Changeset and description: nothing reaches it while it leaves, a toast it raised stays live and its words are read: verified (
'layer'hold at:292,makeContentInertandisTextLiveRegionininputHold.ts, Escape dropped atAlertDialog.tsx:221-229). - Changeset and description: 320 px on a phone in every theme, a passed width wins, a tablet keeps 400: verified (
AlertDialog.tsx:213; unit width case). - Changeset and description: nothing changes with a mouse: verified (motion, holds and width are gated on a coarse primary pointer).
- Description: no new prop, export or data attribute: verified (
AlertDialog/index.tsunchanged;AlertDialog.public.test.ts). - Description: the motion plays theme curves and no measured spring: verified (no
animatecall; hook test). - Description: the head merges the base into the previous head with no conflict, then reads the pointer flag through
useCommittedRef: verified (history;useAlertMotion.ts). - Description: the browser spec has 19 cases, three Chromium-only: verified.
- Description: +2,787 / −77 in 17 files, product +475 / −36, tests +2,152 / −3: verified.
- Description: the private hook is not exported by any package entry, the new layer-root reader sits in its owner module, and the input hold keeps its exports: verified.
- Description: test pass counts and red-first results: not re-checked (CI skipped; no test ran for this review).
- Description: device results: as stated, the Android emulator run is pending and the iPhone runs are from the previous head (the blocking item).
- Files:
AlertDialog.spec.mdcarries the evidence gap above;AlertDialog.tsxanduseAlertMotion.tshave non-blocking comment-style suggestions;inputHold.ts,layerRoot.stylex.ts,layer-runtime.md,AlertDialog.doc.mjs, the changeset, the story, both sandbox files,AlertDialog.motion.interaction.browser.spec.ts,useAlertMotion.test.tsx,AlertDialog.test.tsx,AlertDialog.public.test.ts,inputHold.test.tsxandDialogTouchEntrance.interaction.browser.spec.tsare clear.
The entrance check read only CSS animations, through animationstart. AlertDialog's touch motion runs its Reduce Motion cross-fade as script animations on the dialog, so the check saw none. It now also reads each script animation started on the dialog, and an animation's duration ends at its last change of value: the animator's keyframes hold their end value after it.
The check dated the close by the fade's start time. WebKit dates that start at its last frame before the close's task, before the motion reads the value on screen, so the check could miss by a few thousandths. The call lies between that start time and the moment the fade is made, and the fade's first value must lie within what the screen showed between them, within the same tolerance.
|
/approve-spec 836dd4c |
There was a problem hiding this comment.
Requesting changes for the one evidence item below. The alert's touch motion, input holds and phone width follow component:AlertDialog FR6, FR7 and FR9, the Layer hold's toast and live-region exceptions follow spec:AST-003 FR16 and spec:AST-086 FR14, and the record edits carry an engineering owner's approval for this head.
- FR6's verification row and
spec:AST-094DEC-27 need the iOS device and Android emulator results at the head that lands, but Testing gives every run at the previous head and the runs at this head as pending, and DEC-31 cannot carry them because this head adds two test commits and itsmainmerge changedmotion/animator.tsandLayer/useLayer.tsx; stating each run for this head in Testing (device or emulator, browser, head, animation scale, result) settles it.
CI skipped every job on this draft head; marking it ready for review starts it, and the inline notes on comment length and record placement are non-blocking.
Claims and files checked
- Changeset: under a finger the alert scales into place from 1.2 while it and the dimming fade in, on the theme's entrance timing: verified (
useAlertMotion.ts:41-44,presentat:108-120). - Changeset: it fades out with the dimming on the theme's exit timing, and a close or reopen midway turns back from where it is: verified (
:47-50;fadeFromHereat:90-100starts from the values on screen; hook tests). - Changeset: while it appears a touch does nothing, and keys and assistive technology still act: verified (pointer-only presentation hold at
:251; hook and browser cases). - Changeset: while it leaves nothing reaches it, but a toast its action raised stays live and its words are read: verified (dismissal hold at
:292;isInNestedLayer,isTextLiveRegionandmakeContentInertininputHold.ts; Escape dropped atAlertDialog.tsx:221-229). - Changeset: with reduced motion it only cross-fades: verified (the animator's 200 ms coarse-pointer cross-fade; browser case).
- Changeset: on a phone the default width goes from 400 px to 320 px in every theme, and a passed width wins: verified (
AlertDialog.tsx:213; unit width case). - Changeset: with a mouse nothing changes: verified (motion, holds and width are gated on a coarse primary pointer).
- Description: the dialog stays open until the fade ends: verified (the derived leaving state at
useAlertMotion.ts:146-152). - Description: no new prop, export or data attribute, and callers decide nothing new beyond the width: verified (
AlertDialog/index.tsunchanged;AlertDialog.public.test.ts). - Description: one private hook drives the dialog and its backdrop on the shared animator and theme timing, and no measured spring plays: verified (no
animatecall; hook test). - Description: the head merges
maininto the previous head and adds two test commits; the earlier history is as described: verified (commit history). - Description: the browser spec has 19 cases, three of them Chromium-only: verified.
- Description: +2,899 / −99 lines in 18 files, product +475 / −36, tests +2,263 / −23: verified.
- Description: no package entry exports the hook, the layer-root reader sits in its owner module, and the input hold keeps its exports: verified.
- Description: unit, browser, accessibility, typecheck and lint results, and the red-first and mutation results: not re-checked (CI skipped on this draft head and no test could run for this review).
- Description: device and emulator results: stated at the previous head, pending at this head (the blocking item).
- Files:
AlertDialog.spec.mdcarries the evidence gap above;layer-runtime.md,AlertDialog.tsxanduseAlertMotion.tshave non-blocking notes;inputHold.ts,layerRoot.stylex.ts,AlertDialog.doc.mjs, the changeset, the story, both sandbox files,AlertDialog.motion.interaction.browser.spec.ts,useAlertMotion.test.tsx,AlertDialog.test.tsx,AlertDialog.public.test.ts,inputHold.test.tsx,Dialog.reduced-motion.a11y.browser.spec.tsandDialogTouchEntrance.interaction.browser.spec.tsare clear.
|
/approve-spec 6f7e747 |
There was a problem hiding this comment.
Requesting changes for one evidence item. The alert's touch motion, input holds and phone width follow component:AlertDialog FR6, FR7 and FR9, the Layer hold's toast exception follows spec:AST-003 FR16, and the record edits are owner-approved for this head.
- FR6's verification row (
AlertDialog.spec.md:231) andspec:AST-094DEC-27 need iOS device and Android emulator results at the head that lands, and DEC-31 cannot carry the earlier runs because test commits followed them and themainmerges changedmotion/animator.ts,Layer/useLayer.tsxanddocs/architecture/layer-runtime.md: please state each run for this head in Testing (device or emulator, browser, head, animation scale, result).
Non-blocking: architecture:layer-runtime INV16 could state the dismissal hold's nested-layer exception, and three new comments in AlertDialog.tsx and useAlertMotion.ts run past INV29's sentence length.
Claims and files checked
- Verified: under a finger the alert scales from 1.2 to 1 while it and the dimming fade in, on the theme's entrance duration and easing (
useAlertMotion.ts:41-44,:238-251; browser case "presents from scale 1.2"). - Verified: it dismisses with a fade alone on the exit tokens, and the dialog stays open until the fade ends (
useAlertMotion.ts:279-301; browser case "dismisses with a fade alone"). - Verified: a close or a reopen midway turns back from the values on screen (
fadeFromHere,useAlertMotion.ts:90-100; unit and browser cases). - Verified: Reduce Motion gives an opacity-only cross-fade within 200 ms (browser case "under reduced motion").
- Verified: pointer input waits during the entrance while keys still act, and the content is inert during the fade (
useAlertMotion.ts:251,:292; browser cases). - Verified: the dismissal hold leaves a nested layer, its path and a child live region live, so a toast stays tappable and its words are read (
inputHold.ts:145-157,:924-971;inputHold.test.tsx; browser toast cases). - Verified: a mouse keeps today's motion and width (
AlertDialog.tsx:211-216; browser mouse cases; desktop frames unchanged). - Verified: no prop, export or data attribute is added; only the phone default of
widthmoves (built API surface shows no row;AlertDialog.public.test.ts). - Verified: on a phone the default width goes from 400 to 320 px, and a caller's
widthstill wins (AlertDialog.tsx:213; 370 to 320 px on a 402 px screen). - Verified: the size and split of the change (+2,899 / −99 in 18 files; product code +475 / −36).
- Verified: the head merges
mainwith no conflict. - Verified: the changeset matches the code.
- Pending, as stated: device and emulator runs at this head (the blocking item).
- Files (18, each visited):
AlertDialog.tsx,useAlertMotion.ts,inputHold.ts,layerRoot.stylex.ts(clear);AlertDialog.spec.md(the evidence gap);layer-runtime.md(non-blocking note);AlertDialog.motion.interaction.browser.spec.ts,useAlertMotion.test.tsx,AlertDialog.test.tsx,AlertDialog.public.test.ts,inputHold.test.tsx,Dialog.reduced-motion.a11y.browser.spec.ts,DialogTouchEntrance.interaction.browser.spec.ts(clear);AlertDialog.doc.mjs, the changeset,AlertDialog.stories.tsx,caseIds.ts,cases.tsx(clear).
|
/approve-spec c44f975 |
There was a problem hiding this comment.
Changes requested for one record sentence; the code, the other record edits and the device evidence check out at this head.
- Regression:
docs/architecture/layer-runtime.md:882says every dismissal hold leaves a nested layer, its path and a child live region live, butmainhas since added afocus: 'kept-inside'dismissal hold to INV16 that makes the whole layer inert once focus leaves (a closing BottomSheet takes it), so please mergemainand scope the sentence to the defaultfocus: 'blur'hold.
The code composes with main: AlertDialog's, the input hold's and BottomSheet's unit suites pass on the clean merge. The new head needs an engineering owner's /approve-spec <full head SHA> again, and its device evidence follows spec:AST-094 DEC-31, since that merge changes Layer/inputHold.ts. Please also update Testing, which still lists this head's device runs as pending.
Claims and files checked
- Claim: under a finger the alert scales from 1.2 into place while it and the dimming fade in, on the theme's entrance timing. Verified:
useAlertMotion.ts:238-251, unit tests, Chromium probe (scale 1.04, opacity 0.78 at 120 ms). - Claim: it fades out with the dimming on the theme's exit timing, and a close or reopen midway turns back. Verified:
useAlertMotion.ts:254-306, unit and browser tests. - Claim: a touch does nothing while it appears, and keys and assistive technology still act. Verified: the pointer-only entrance hold,
AlertDialog.test.tsx, the browser key case. - Claim: nothing reaches it while it leaves, but a toast its action raised stays live and is read. Verified:
AlertDialog.tsx:221-229,inputHold.ts:924-975,inputHold.test.tsx, probe (Undo ran during the fade). - Claim: with reduced motion it only cross-fades. Verified: the animator's reduced-motion path and
Dialog.reduced-motion.a11y.browser.spec.ts. - Claim: on a phone the default width goes from 400 to 320 px, and a passed width wins. Verified:
AlertDialog.tsx:213, unit test, probe (370 to 320 px). - Claim: with a mouse nothing changes. Verified: the desktop frame is pixel-identical; unit test.
- Claim: no new prop, export or data attribute. Verified: the built API surface shows no row;
AlertDialog.public.test.ts. - Claim: device runs at this head are pending. Superseded: an engineering owner's later comment records them.
- Claim: the head merges
mainwith no conflict. Verified: clean merge, also with today'smain. - File
.changeset/alertdialog-touch-motion.md: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/caseIds.ts: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/cases.tsx: clear. - File
apps/storybook/stories/AlertDialog.stories.tsx: clear. - File
docs/architecture/layer-runtime.md: finding at line 882. - File
docs/specs/components/AlertDialog/AlertDialog.spec.md: clear. - File
packages/core/src/AlertDialog/AlertDialog.doc.mjs: clear. - File
packages/core/src/AlertDialog/AlertDialog.public.test.ts: clear. - File
packages/core/src/AlertDialog/AlertDialog.test.tsx: clear. - File
packages/core/src/AlertDialog/AlertDialog.tsx: clear (one comment-style suggestion). - File
packages/core/src/AlertDialog/__tests__/AlertDialog.motion.interaction.browser.spec.ts: clear. - File
packages/core/src/AlertDialog/useAlertMotion.test.tsx: clear. - File
packages/core/src/AlertDialog/useAlertMotion.ts: clear (two comment-style suggestions). - File
packages/core/src/Dialog/__tests__/Dialog.reduced-motion.a11y.browser.spec.ts: clear. - File
packages/core/src/Dialog/__tests__/DialogTouchEntrance.interaction.browser.spec.ts: clear. - File
packages/core/src/Layer/inputHold.test.tsx: clear. - File
packages/core/src/Layer/inputHold.ts: clear. - File
packages/core/src/privateAttributes/layerRoot.stylex.ts: clear.
|
Testing now states the runs at c44f975. The INV16 sentence and the comment lengths follow in a separate pull request after this one merges. That keeps the device runs and the owners' stamp valid at this head. |
A blur hold leaves a nested layer, its path, and a child live region live, as the line for Layer/inputHold.ts under Owns already says. INV16 now states it beside the leaving AlertDialog.
The width prop's JSDoc, the dimming note, and two Effect notes each had a sentence longer than the source-comment rule allows. Each is now two sentences with the same meaning. Two inputHold comments said an open modal holds its toast viewport, which reads as the input hold; they now say it hosts the viewport.
|
Both notes are now in this pull request at 1e2dd21: INV16 states the dismissal hold's nested-layer exception, and four comment sentences are split. The device runs repeat at this head. |
|
/approve-spec 1e2dd21 |
There was a problem hiding this comment.
Changes requested for one record sentence and the device runs this head needs; the code and the other record edits check out.
docs/architecture/layer-runtime.md:887still says every dismissal hold leaves a nested layer, its path and a child live region live, but INV16 gives a closing BottomSheet afocus: 'kept-inside'dismissal hold that makes the whole layer inert once focus leaves, so please scope the sentence to the blur hold as the new INV16 sentence already is.component:AlertDialogFR6's verification row andspec:AST-094DEC-27 need iOS Safari device and Android emulator results at this head, Testing lists them as pending, and DEC-31 cannot carry the earlier runs because this head'smainmerge changedLayer/inputHold.ts, so please state those runs at the head that lands.
The code composes with today's main: AlertDialog's, the input hold's, BottomSheet's and Toast's unit suites pass on the clean merge. A head that changes the record text needs an engineering owner's /approve-spec <full head SHA> again.
Claims and files checked
- Claim: under a finger the alert scales from 1.2 into place while it and the dimming fade in, on the theme's entrance timing. Verified:
useAlertMotion.ts:238-252, unit tests, Chromium probe (scale 1.06, opacity 0.71 about 100 ms in). - Claim: it fades out with the dimming on the theme's exit timing, and a close or reopen midway turns back. Verified:
useAlertMotion.ts:254-300, unit tests. - Claim: a touch does nothing while it appears, and keys and assistive technology still act. Verified: the pointer-only entrance hold,
useAlertMotion.test.tsx. - Claim: nothing reaches it while it leaves, but a toast its action raised stays live and is read. Verified:
AlertDialog.tsx:218-229,inputHold.ts:1059-1108,inputHold.test.tsx, probe (Undo ran during the fade). - Claim: with reduced motion it only cross-fades. Verified in code: the animator's opacity-only reduced-motion plan; the browser case was not re-run here.
- Claim: on a phone the default width goes from 400 to 320 px in every theme, and a passed width wins. Verified:
AlertDialog.tsx:213, unit test, probe (370 to 320 px). - Claim: with a mouse nothing changes. Verified: the desktop frame is pixel-identical; unit test.
- Claim: no new prop, export or data attribute. Verified: the built API surface shows no row;
AlertDialog.public.test.ts. - Claim: the head merges
mainwith no conflict, and main's kept-inside hold runs on a separate path. Verified: clean merge, also with today'smain;inputHold.ts:464-466. - Claim: INV16 now states the blur hold's nested-layer exception. Verified:
layer-runtime.md:673-675; the ownership line at:887is still unscoped. - Claim: device runs at this head are pending. Verified: true, and they are still needed.
- File
.changeset/alertdialog-touch-motion.md: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/caseIds.ts: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/cases.tsx: clear. - File
apps/storybook/stories/AlertDialog.stories.tsx: clear. - File
docs/architecture/layer-runtime.md: finding at line 887. - File
docs/specs/components/AlertDialog/AlertDialog.spec.md: clear; its FR6 row's device evidence is pending. - File
packages/core/src/AlertDialog/AlertDialog.doc.mjs: clear. - File
packages/core/src/AlertDialog/AlertDialog.public.test.ts: clear. - File
packages/core/src/AlertDialog/AlertDialog.test.tsx: clear. - File
packages/core/src/AlertDialog/AlertDialog.tsx: clear. - File
packages/core/src/AlertDialog/__tests__/AlertDialog.motion.interaction.browser.spec.ts: clear. - File
packages/core/src/AlertDialog/useAlertMotion.test.tsx: clear. - File
packages/core/src/AlertDialog/useAlertMotion.ts: clear. - File
packages/core/src/Dialog/__tests__/Dialog.reduced-motion.a11y.browser.spec.ts: clear. - File
packages/core/src/Dialog/__tests__/DialogTouchEntrance.interaction.browser.spec.ts: clear. - File
packages/core/src/Layer/inputHold.test.tsx: clear. - File
packages/core/src/Layer/inputHold.ts: clear. - File
packages/core/src/privateAttributes/layerRoot.stylex.ts: clear.
A kept-inside dismissal hold makes the whole layer inert once focus leaves. So the line for Layer/inputHold.ts under Owns now says that a blur dismissal hold leaves a nested layer, its path, and a child live region live, as INV16 does.
|
/approve-spec ccac987 |
There was a problem hiding this comment.
Changes requested: one copied helper and the device results this head needs. The record edits carry an engineering owner's approval for this head, and the motion, input holds and phone width match component:AlertDialog FR6, FR7 and FR9.
- Miss:
useAlertMotion.ts:58-67copies Popover'stimingOn(Popover/popoverSurfaceMotion.ts:83-86, the same fallback to no motion when a theme's timing does not resolve), so please export that fallback frommotion/themeTiming.ts, call it from both files and delete both copies. component:AlertDialogFR6's verification row andspec:AST-094DEC-27 need the iPhone Safari and Android emulator results stated for the head that lands, and the earlier runs do not carry here because this head'smainmerges changedLayer/inputHold.ts,Button/Button.tsxandDialog/Dialog.tsx.
Non-blocking: the docs say the motion runs "on any touch device", but it follows a coarse primary pointer, so a finger on a touchscreen laptop gets the mouse behavior, and "on a phone or tablet" would be accurate. A head that changes record text needs a new /approve-spec <full head SHA>.
Claims and files checked
- Claim: under a finger the alert scales from 1.2 to 1 while it and the dimming fade in, on the theme's entrance duration and easing. Verified:
useAlertMotion.ts:41-44,:108-120; a probe 700 ms into a slowed entrance read scale 1.06, opacity 0.70 and dimming 0.70. - Claim: it dismisses with a fade alone on the exit tokens and stays open until the fade ends. Verified:
useAlertMotion.ts:47-50,:279-300; a probe 700 ms into a slowed exit read it open at 0.89 with no scale. - Claim: a close or a reopen midway turns back from where it is. Verified:
fadeFromHere(useAlertMotion.ts:90-100); the hook's unit tests. - Claim: under Reduce Motion both are the animator's 200 ms cross-fade. Verified in code (the animator's reduced-motion plan); the browser case was not run here.
- Claim: pointer input waits while the alert enters, and its content is inert while it leaves. Verified: the
'viewport'and'layer'holds (useAlertMotion.ts:251,:292); a probe found Cancel and Delete inert during the exit. - Claim: the leaving hold leaves a nested layer, its path and a child live region live, so a toast above the alert keeps its taps and its words are read. Verified:
inputHold.ts:157,:196-202,:511,:804,:1059,:1083; the four new hold cases fail on the base and pass here; an Undo tap during the fade ran. - Claim: a mouse keeps today's motion and width. Verified: the desktop frame is pixel-identical at 400 px.
- Claim: no new prop, export or data attribute, and callers decide nothing new. Verified: the generated API surface has no row;
AlertDialog.public.test.ts. - Claim: the motion and input rules apply under a coarse primary pointer only. Verified:
useTouchPresentationreads(pointer: coarse);AlertDialog.tsx:214-217. - Claim: the default width on a phone goes from 400 to 320 px, and a passed width still wins. Verified:
AlertDialog.tsx:213; unit test; probe 370 to 320 px. - Claim: the motion needs no exit copy, no registry and no new private attribute. Verified: it animates the live dialog and adds no attribute or module-scope registry.
- Claim: one private hook drives the dialog and its backdrop with the shared animator and the theme's timing, and no spring plays. Verified:
useAlertMotion.ts:108-120; unit test "plays no spring". - Claim: a new reader in the existing layer-root owner finds a nested layer. Verified:
layerRoot.stylex.ts:40-42. - Claim: the input hold's exports are unchanged; only its layer scope changes. Verified: the new helpers are module-local.
- Claim: size is +2,902 / −100 in 18 files, product code +475 / −36 and tests +2,263 / −23. Verified against the clone's numstat.
- Claim: this head adds one record line to a merge of main that changes none of the pull request's files but changes Button and Dialog. Verified; that merge also changes Toast and ToastViewport, which the toast case uses.
- Claim: main's kept-inside hold runs on a separate path from the new exceptions. Verified:
inputHold.ts:464-466. - Claim: red-first, mutation, unit, accessibility, typecheck and lint runs pass. Re-run here: the four hold cases fail on the base and pass on the head; the AlertDialog, Layer, Dialog, Toast and BottomSheet unit suites, the core typecheck and ESLint on the changed files pass; the browser and accessibility specs were not re-run.
- Claim: device runs at this head are pending. True, and they are still required.
- Claim: the changeset repeats the claims above, ending "With a mouse nothing changes". Verified as above.
- Claim: the docs say the actions are tracked as one group under a finger or a pen while a mouse keeps each button's click. Verified: tracking requires a coarse pointer. The docs' "on any touch device" overstates where the motion runs (non-blocking).
- File
.changeset/alertdialog-touch-motion.md: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/caseIds.ts: clear. - File
apps/sandbox/src/app/(raw)/pages/alert-dialog-cases/cases.tsx: clear. - File
apps/storybook/stories/AlertDialog.stories.tsx: clear. - File
docs/architecture/layer-runtime.md: clear (owner-approved at this head). - File
docs/specs/components/AlertDialog/AlertDialog.spec.md: finding: the FR6 row (line 231) needs the device and emulator results at this head. - File
packages/core/src/AlertDialog/AlertDialog.doc.mjs: clear, with one non-blocking wording note. - File
packages/core/src/AlertDialog/AlertDialog.public.test.ts: clear. - File
packages/core/src/AlertDialog/AlertDialog.test.tsx: clear. - File
packages/core/src/AlertDialog/AlertDialog.tsx: clear. - File
packages/core/src/AlertDialog/__tests__/AlertDialog.motion.interaction.browser.spec.ts: clear (read, not run here). - File
packages/core/src/AlertDialog/useAlertMotion.test.tsx: clear. - File
packages/core/src/AlertDialog/useAlertMotion.ts: finding at lines 58-67 (copied helper). - File
packages/core/src/Dialog/__tests__/Dialog.reduced-motion.a11y.browser.spec.ts: clear (read, not run here). - File
packages/core/src/Dialog/__tests__/DialogTouchEntrance.interaction.browser.spec.ts: clear (read, not run here). - File
packages/core/src/Layer/inputHold.test.tsx: clear. - File
packages/core/src/Layer/inputHold.ts: clear. - File
packages/core/src/privateAttributes/layerRoot.stylex.ts: clear.
| | 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 | Browser tests under a coarse pointer in Chromium and WebKit, a theme-token curve oracle for the present and the dismissal, a busy-main-thread case, real-device clips on iOS Safari, and Android Chrome clips in the Android emulator (`spec:AST-094` DEC-27) | present, dismiss, close during present, reopen during dismiss, Reduce Motion, a fine pointer, a narrow viewport | a present or a tap-, key- or program-started dismissal that plays a measured role instead of the theme's entrance or exit duration and easing, 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. No test exists yet; FR6 is not shipped. | `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") | 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 the leaving alert, which FR6's dismissal brings. | `audit:AlertDialog/behavior` | | ||
| | FR6, PR2 | `useAlertMotion.test.tsx`; `AlertDialog.motion.interaction.browser.spec.ts` under a coarse pointer in Chromium and WebKit: a theme-token curve oracle for the present and the dismissal, a theme's own tokens, and a busy-main-thread case; real-device clips on iOS Safari, and Android Chrome clips in the Android emulator (`spec:AST-094` DEC-27) | present, dismiss, close during present, reopen during dismiss, Reduce Motion, a fine pointer, a narrow viewport | a present or a tap-, key- or program-started dismissal that plays a measured role instead of the theme's entrance or exit duration and easing, 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 and spec:AST-094 DEC-27 need the iPhone Safari and Android emulator results at the head that lands (normal, slow and removed animations for Android). Testing lists this head's runs as pending, and DEC-31 cannot carry the earlier ones: a main merge since then changed Layer/inputHold.ts, and this head's merge changed Dialog/Dialog.tsx and Button/Button.tsx, which AlertDialog.tsx imports. Please state each run in the description's text at the head that lands.
readThemeTimingOrInstant in motion/themeTiming.ts reads a theme timing, or plays no motion when the timing does not resolve. AlertDialog's motion and Popover's surface motion call it in place of their own copies, so a change to that fallback reaches both.
The motion follows a coarse primary pointer, so a finger on a touchscreen laptop gets the mouse behavior. The docs now say "on a phone or tablet" instead of "on any touch device".
|
/approve-spec 5a5e131 |
|
Decision: D-20261113-323 Records the Specs consistency stamp at 5a5e131 (AlertDialog FR6 dismissal, layer-runtime INV16, and the docs' coarse-pointer wording from AST-094 FR13). This pull request carries no owner device row. This head changes a shared motion module and the pull request's own files, so no device evidence carries: still required are a real-phone farm take against 25b54b3 and the Android set on an emulator at normal, slow and reduced-motion settings. |
There was a problem hiding this comment.
Changes requested: the code and the record edits are in order, and one piece of evidence the spec requires is still missing at this head.
- The FR6 verification row of
component:AlertDialogandspec:AST-094DEC-27 need real-iPhone Safari and Android emulator results for the head that lands, stated in text (device or emulator, browser, head, animation settings, result); the description still lists this head's device runs as pending, and DEC-31 cannot carry the earlier runs because this head changes the pull request's own files.
The record edits carry an engineering owner's approval for this head, and the unit tests for the alert, the input hold and the theme timing pass here. The two comment-wording suggestions from the last review still apply and stay non-blocking.
Claims and files checked
- Changeset, under a finger the alert scales into place from 1.2 while it and the dimming fade in on the theme's entrance timing: verified (
useAlertMotion.ts:226-239, the unit curve cases). - Changeset, on close it fades out with the dimming on the theme's exit timing, and a close or reopen midway turns back from where it is: verified (
useAlertMotion.ts:241-288, unit cases). - Changeset, a touch does nothing while it appears, and keys and assistive technology still act: verified (the
'viewport'hold; unit cases). - Changeset, nothing reaches it while it leaves, but a toast its action raised stays live and is read: verified (
AlertDialog.tsx:221-229,inputHold.ts:1059-1105; the nested-layer and live-region hold cases). - Changeset, reduced motion only cross-fades: verified (the animator's opacity-only plan; the browser spec's reduced-motion case covers it).
- Changeset and description, with no
widtha phone's default goes from 400 to 320 px in every theme and a passed width wins: verified (AlertDialog.tsx:213, the width unit case). - Changeset and description, nothing changes with a mouse: verified (every new path is gated on a coarse primary pointer; unit mouse case).
- Description, no new prop, export or data attribute: verified (the generated API surface has no row;
AlertDialog.public.test.ts). - Description, one fallback for an unresolved theme timing shared by the alert and Popover: verified (
motion/themeTiming.ts:311-319). - Description, the head merges main with no conflict, and that merge changes the layer runtime record and Button: verified.
- Description, size of +2,943 / −117 lines in 22 files: verified.
- Description, device runs at this head are pending: verified, and this is the finding above.
- Description, test tallies: partly verified (a 398-test unit subset passes here; the browser runs were not repeated).
- Files: all 22 changed files were read;
AlertDialog.spec.mdcarries the finding above,useAlertMotion.tsandAlertDialog.tsxcarry the wording suggestions, and the other 19 are clear.
| * The presentation plays the theme's entrance easing over its duration step | ||
| * for UIKit's measured 404 ms alert present. A tap opens an alert, and a tap |
There was a problem hiding this comment.
Non-blocking (architecture:knowledge-contracts INV29 style): "over its duration step for UIKit's measured 404 ms alert present" leaves the step unnamed. Suggested: "The present plays --ease-entrance over --duration-medium-max, the theme's step for UIKit's measured 404 ms present."
| // Only under the touch presentation does a press slide across the | ||
| // actions, and does the alert scale in and fade out. A desktop press stays | ||
| // the native button's, and a mouse keeps the theme's entrance. |
There was a problem hiding this comment.
Non-blocking (architecture:knowledge-contracts INV29 style): the first sentence carries two topics. Suggested: "Only the touch presentation tracks a press across the actions. It also scales the alert in and fades it out. A desktop press stays the native button's, and a mouse keeps the theme's entrance."
User need
On a phone, an alert should appear and leave as iOS's does, and be 320 px wide. Today it rises with Dialog's entrance, vanishes at once and is 400 px wide. A toast its action raises must stay usable while it leaves.
Problem and solution fit
--duration-medium-maxand--ease-entrance.--duration-exitand--ease-exit. The dialog stays open until the fade ends.Current specification
component:AlertDialogFR6, FR7 and FR9, underspec:AST-094FR7, FR35 and FR45. The toast followsspec:AST-003FR16 andspec:AST-086FR2.Overlap with #8084
#8084 implements the same FR6, FR7 and FR9 with a different design: CSS keyframes, an exit copy and a page-wide host registry. Only one of the two lands. This one fades the live dialog, so it needs no copy, no registry and no new private attribute.
Public delta and caller burden
widthstill wins.Design and alternatives
One internal hook drives the dialog and its
::backdropwith the shared motion animator, onreadThemeTimingand the entrance easing, as Popover's touch motion does. The measured springs stay as reference only. An exit copy was rejected: it adds an element, and the toast then needs a registry.Evidence
Scope
Components: AlertDialog, Layer. Popover's surface motion shares the new timing fallback.
Testing
The head, 5a5e131, merges main ca36b56 into d46da0f with no conflict. That merge changes
architecture:layer-runtime, which this pull request also changes, in other rows, andButton/Button.tsx, which AlertDialog imports. d46da0f adds two commits to ccac987. One moves the fallback for a theme timing that does not resolve intomotion/themeTiming.ts, which AlertDialog's and Popover's motion now call. The other says the motion runs on a phone or tablet. ccac987 adds one record commit to 8476269: the line under Owns forLayer/inputHold.tsnow names a blur dismissal hold. 8476269 merges main f5b5f96 into 1e2dd21 with no conflict. That merge changes none of this pull request's files. It changesButton/Button.tsxandDialog/Dialog.tsx, which AlertDialog imports. 1e2dd21 adds two commits to c4d74bc. One adds a sentence to INV16 ofarchitecture:layer-runtime. The other splits four comment sentences for length, and makes two comments say a modal hosts its toast viewport. c4d74bc merges main b890479 into c44f975 with no conflict. Main changedLayer/inputHold.ts, its test and INV16 ofarchitecture:layer-runtime, which this pull request also changes. Its newfocus: 'kept-inside'hold and this pull request's dismissal exceptions run on separate paths. c44f975 merges main 721dbf1 into 6f7e747 with no conflict, and 6f7e747 merges main 9d2ce46 into 836dd4c. 836dd4c merges main a4595ef into 25b54b3, then adds two test commits. Main's line forLayer/holdEntranceInput.tsinarchitecture:layer-runtimemet ours, and the merge keeps both. The entrance check ofdesign:reduced-motionnow reads script animations too, and the turn-back check dates the close at its call. 25b54b3 merges main 1740654 into b5a6854 with no conflict. Then the motion reads its pointer flag throughuseCommittedRef, asarchitecture:react-component-runtimeINV3 asks. b5a6854 is seven commits on main b537123: the hold change, the alert's motion, two record commits, the entrance easing and two sandbox commits. At the head, every run used 1 worker:pnpm test:a11y-contract:pr): 353 passed in Chromium. Before its check read script animations,Dialog.reduced-motionsaw none on the alert under Reduce Motion with a finger, and failed.pnpm check:knowledgeandpnpm check:changesetspass.Earlier, at 3c1ab6d on main b537123:
AlertDialog.motion.interaction.browser.spec.tshas 19 cases. On main with only the spec and the story, 16 fail and 3 pass: a key during the entrance, and both mouse cases. At 3c1ab6d, Chromium passes all 19, and WebKit in CI's container passes 16 and skips the three Chromium-only cases.--ease-standardback, the unit case "plays a theme's own entrance and exit tokens" fails.Device runs at the head 5a5e131, stated in text: the clips exist and are not attached. The owners' decision for this head asks for these runs. On one iPhone 14, both heads ran interleaved, two takes in normal motion and two under Reduce Motion each. Each take used a fresh page load. The phone read the sandbox's
version.jsonfor each head: 5a5e131 and 25b54b3. Between the two heads, this pull request's code differs only in tests, comment and doc wording, and one helper. Its theme-timing fallback moved intomotion/themeTiming.ts, with no behavior change. Every other code difference comes from main.Device runs at the prior head ccac987, stated in text: the clips exist and are not attached. On one iPhone 14, both heads ran interleaved, two takes in normal motion and two under Reduce Motion each. Each take used a fresh page load. The phone read the sandbox's
version.jsonfor each head: ccac987 and 25b54b3. Between the two heads, this pull request's code differs only in two tests and in comment wording. Every other code difference comes from main.Device runs at the prior head c44f975, stated in text: the clips exist and are not attached. On one iPhone 14, both heads ran interleaved, two takes in normal motion and one under Reduce Motion each. The phone read the sandbox's
version.jsonfor each head: c44f975 and 25b54b3. Between the two heads, this pull request's code differs only in two tests. Every other code difference comes from main. The level read is the dimming under the card, per stream frame of about 28 ms.Device runs at the prior head 6f7e747, stated in text: the clips exist and are not attached. Both heads ran interleaved on one iPhone 14, two takes each. The phone read the sandbox's
version.jsonfor each head: 6f7e747 and 25b54b3. Between the two heads, this pull request's code differs only in two tests. Every other code difference comes from main. Scale is the card's width against its settled width, and opacity is the title's contrast against rest, read from frames of about 30 ms.Device runs at the prior head 25b54b3, stated in text: the clips exist and are not attached. Each iPhone run first read the sandbox's
version.json, which names 25b54b3.Public and internal API delta
@astryxdesign/corechanges.packages/core/src/AlertDialog/useAlertMotion.ts, exportinguseAlertMotion. No package entry exports it.const motion = useAlertMotion({isOpen, isEnabled: isTouch && isInline !== true});in AlertDialog. The Dialog then getsisOpen={motion.isPresented}and the mergedmotion.ref.motion/animator.ts,motion/themeTiming.tsandLayer/inputHold.tsdo the work, and the hook only orders them. AlertDialog.tsx could hold it, but would double in size.hasLayerRootWithin(element)in the existing ownerprivateAttributes/layerRoot.stylex.ts, asarchitecture:public-component-apiINV23 allows.Layer/inputHold.tskeeps live an element that holds a nested layer, and makes its other children inert.isLayerRootreads one element; a walk with it would visit every node of the layer.readThemeTimingOrInstant(element, tokens)in the existing modulemotion/themeTiming.ts. No package entry exports it.useAlertMotion.tsandPopover/popoverSurfaceMotion.tstake the theme's timing from it, or no motion when that timing does not resolve.Layer/inputHold.ts: no export changes. Only its'layer'scope changes, asarchitecture:layer-runtimenow states.AlertOverAToastserves the toast cases. The sandbox caseinterruptionslets a device clip show both interruptions.Writing rubric results
component:AlertDialog: FR6, FR7 and FR9 lose their implementation-gap notes, and Current-state impact keeps only AR1's Tab gap. FR7 gains one clause for a toast (spec:AST-003FR16). The verification rows name the new tests. Its Owns list namesuseAlertMotion.tsand says why it stays a private file.architecture:layer-runtimegains one present-tense ownership line forLayer/inputHold.ts, and one INV16 sentence on the nested layers a blur hold leaves live. It nameshasLayerRootWithin, and says why the owner's other readers do not fit.