Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
623b365 to
217625b
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec governs MobileNav holding touch input through its slide-in and slide-out (the MobileNav record is draft and AST-094 is not on main), so an owner needs to approve that behavior, and say whether architecture:public-component-api INV16 calls for a presentation value that keeps the fine-pointer behavior, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Separately, the diff against main still carries the unlanded stack (#8342, #8119, #8076, #8336 and their predecessors, about 36,000 of its 36,800 authored lines), so please land those first and update this branch so it carries only its last three commits.
- The MobileNav open/close effect acquires the presentation hold, but its cleanup does not release it (
architecture:react-component-runtimeINV3); releasing it there keeps every path in one place. MAX_EXIT_MS(550 ms) is not the end of the slide-out for themes with a slower--duration-medium, so the changesets' "until its slide-out ends" needs the bound stated, or AppShell should follow the theme's duration.
[Automated review]
| // slide-out ends; its content takes no input meanwhile. Then it | ||
| // closes and focus goes back to the opener. | ||
| const duration = resolveSlideDuration(drawerRef.current); | ||
| hold('layer', duration); |
There was a problem hiding this comment.
This effect acquires the presentation hold here (and at line 665), but its cleanup (lines 692–697) only clears the close timer; the release lives in the next run, the close timer, the mouse branch and the unmount-only effect. architecture:react-component-runtime INV3 asks the acquiring effect's cleanup to release what its setup acquires. Releasing releaseHoldRef in the cleanup, with a ref flag for the reopen refocus, would keep every path in one place.
| * AppShell.tsx): Activity's `display: none !important` would otherwise cut | ||
| * the slide-out off (#5701). | ||
| */ | ||
| export const MAX_EXIT_MS = 550; |
There was a problem hiding this comment.
resolveSlideDuration clamps to this and AppShell waits this plus 50 ms, but the drawer slides for the theme's --duration-medium, which defineTheme's motion scale does not bound. With medium: 700, a finger gets the page back 150 ms before the drawer finishes and AppShell cuts the last 100 ms, while both changesets say the hold lasts until the slide-out ends. Stating the bound, or giving AppShell the resolved duration, would make that true.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsAppShell (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/AppShell | +15.0KB |
0B |
| core/MobileNav | +15.1KB |
0B |
| core/SideNav | +15.2KB |
0B |
| core/TopNav | +15.1KB |
0B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | 0B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 739,916 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec governs MobileNav holding touch input through its slide-in and slide-out (the MobileNav record is still a draft, and AST-094 leaves each component's touch presentation to that component's own record), so an engineering owner needs to approve that behavior, and say whether architecture:public-component-api INV16 calls for a presentation value that keeps the fine-pointer behavior, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Separately, the diff against main still carries the unlanded stack (#8342, #8119 and their predecessors, about 37,500 of its 38,200 authored lines) and now conflicts with main, so please land those first and update this branch so it carries only its last three commits.
- The MobileNav open/close effect acquires the presentation hold but its cleanup does not release it (
architecture:react-component-runtimeINV3); releasing it there keeps every path in one place. MAX_EXIT_MS(550 ms) is not the end of the slide-out for themes with a slower--duration-medium, so the changesets' "until its slide-out ends" needs the bound stated, or AppShell should follow the theme's duration.packages/build/README.mdstill documents the undeclared-variable warning that the stack's own revert removed from the code.
[Automated review]
|
|
||
| Unsupported declarations fail the build by default with StyleX's replacement guidance. Set `propertyValidationMode` explicitly in `stylexOverrides` (modern API) or `stylexOptions` (legacy API) only when a migration needs the upstream `warn` or `silent` behavior. | ||
|
|
||
| A production build also checks the StyleX stylesheet for generated custom properties (`--<prefix><hash>`) that it reads but no stylesheet in the build declares, which browsers drop: a `.stylex.ts` export read in a style that is not a `defineVars`/`defineConsts` value, or one whose module never reached the build. It warns, naming the variable and the rule that reads it. Only stylesheets count as declarations, so a property the app sets at runtime (an inline style) is reported too: give it a name outside the StyleX class-name prefixes. |
There was a problem hiding this comment.
This paragraph documents a production-build warning that the stack's later revert ("astryxStylex keeps no undeclared-variable warning") removed together with its tests; nothing in packages/build/src produces it at this head, so an app author would expect a check that never runs. It should go with that revert.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved. No current spec governs MobileNav holding touch input through its slide-in and slide-out (the MobileNav record is still a draft, and AST-094 leaves each component's touch presentation to that component's own record), so an engineering owner needs to approve that behavior, and say whether architecture:public-component-api INV16 calls for a presentation value that keeps the fine-pointer behavior, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Separately, the diff against main still carries the unlanded stack (#8342, #8119 and their predecessors, about 31,400 of its 32,200 authored lines) and now conflicts with main, so please land those first and update this branch so it carries only its last three commits.
- The touch-only hold statically imports the gesture host, adding roughly 10.6 KB gzip to the MobileNav, AppShell, SideNav and TopNav imports;
architecture:component-style-authoringINV6 asks for it to load lazily under a coarse pointer, or for the cost to be justified in the description. - The open/close effect acquires the presentation hold, but its cleanup does not release it (
architecture:react-component-runtimeINV3). MAX_EXIT_MS(550 ms) is not the end of the slide-out for themes with a slower--duration-medium, so the changesets' "until its slide-out ends" needs the bound stated, or AppShell should follow the theme's duration.
[Automated review]
| type ScrollbarGutterHold, | ||
| } from '../hooks/scrollbarGutter'; | ||
| import {useScrollLock} from '../hooks/useScrollLock'; | ||
| import {holdPresentation} from '../Layer/presentationHold'; |
There was a problem hiding this comment.
This static import brings the whole gesture host (presentationHold.ts:65 imports getGestureHost) into every MobileNav import, and so into AppShell, SideNav and TopNav, for a hold that only runs where the pointer is coarse. That comes to about 10.6 KB gzip by an esbuild estimate on this head. architecture:component-style-authoring INV6 asks a coarse-pointer-only feature over 2 KB to load lazily or to justify the cost in the description. A dynamic import('../Layer/presentationHold') once TOUCH_QUERY matches (for example at mount, so the first open's hold is not late) would keep mouse users from paying for it.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: on main, the MobileNav spec is now current, and its FR5 and ORD3 close the dialog no later than 250 ms after a close request, while under a finger this change keeps the dialog open until the slide-out ends (410 ms by default, up to 550 ms) and drops taps during the slide-in, which no spec states. An engineering owner needs to update FR4, FR5 and ORD3 for a coarse pointer, or decline, and say whether architecture:public-component-api INV16 calls for a presentation value that keeps the fine-pointer behavior, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Separately, please land the stacked dependencies (#8342, #8119 and their predecessors) first so this branch carries only its own 10 files, and address the points below.
- The touch-only hold is imported statically, adding about 11 KB gzip to the MobileNav, AppShell, SideNav and TopNav imports;
architecture:component-style-authoringINV6 asks for it to load lazily under a coarse pointer, or for the cost to be justified in the description. - The open/close effect acquires the presentation hold, but its cleanup does not release it (
architecture:react-component-runtimeINV3). MAX_EXIT_MS(550 ms) is not the end of the slide-out for custom themes with a slower--duration-medium, so the doc comment and changeset should state that bound, or AppShell should follow the drawer's duration.
[Automated review]
| } else if (dialog.open && isTouch) { | ||
| // The dialog stays open, and so keeps the page inert, until the | ||
| // slide-out ends; its content takes no input meanwhile. Then it | ||
| // closes and focus goes back to the opener. | ||
| const duration = resolveSlideDuration(drawerRef.current); | ||
| hold('layer', duration); | ||
| closeTimeoutRef.current = setTimeout(() => { | ||
| releaseHoldRef.current?.(); | ||
| releaseHoldRef.current = null; | ||
| closeAndRestoreFocus(dialog, triggerElementRef); | ||
| setIsCloseHeld(false); | ||
| }, duration); |
There was a problem hiding this comment.
Under a coarse pointer the dialog now closes when the slide-out ends: 410 ms after the close request with the default --duration-medium, and up to 550 ms. On main, component:MobileNav FR5 and ORD3 close it "no later than 250 ms after the request", so this needs an engineering owner to amend those claims for a coarse pointer, or the touch close has to stay inside 250 ms.
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision: under a finger this change keeps MobileNav's dialog open until the slide-out ends (410 ms by default, up to 550 ms) and ignores taps during the slide-in, while the current MobileNav spec on main closes the dialog within 250 ms of a close request (FR5, ORD3) and lets a backdrop or Close tap request close at any time (FR4). An engineering owner needs to decide whether FR4, FR5 and ORD3 should change for a coarse pointer, updating the MobileNav spec if so, and whether architecture:public-component-api INV16 calls for a presentation value that keeps the mouse behavior; the decision is recorded by commenting /approve-spec <full head SHA> on this pull request, which the author can do as an engineering owner. Independently of that decision, please address the following:
- Land #8342, #8119 and their predecessors first, so this branch carries only its own 10 files instead of about 30,000 lines of stacked work.
- The touch-only hold is imported eagerly and adds about 11 KB gzip to the MobileNav, AppShell, SideNav and TopNav imports; load it lazily under a coarse pointer or justify the cost, as
architecture:component-style-authoringINV6 requires. - Release the hold in the open/close effect's cleanup (
architecture:react-component-runtimeINV3), and state the 550 ms cap where the changesets say the hold lasts until the slide-out ends.
[Automated review]
| type ScrollbarGutterHold, | ||
| } from '../hooks/scrollbarGutter'; | ||
| import {useScrollLock} from '../hooks/useScrollLock'; | ||
| import {holdInput} from '../Layer/inputHold'; |
There was a problem hiding this comment.
This eager import makes every MobileNav, AppShell, SideNav and TopNav bundle carry the gesture host (through Layer/inputHold.ts:65), about 11.1 KB gzip, although the hold only runs when (pointer: coarse) matches. architecture:component-style-authoring INV6 asks for a lazy load or a stated justification above 2 KB; a dynamic import('../Layer/inputHold') started once TOUCH_QUERY matches (for example at mount) would keep mouse users from downloading it.
| // slide-out ends; its content takes no input meanwhile. Then it | ||
| // closes and focus goes back to the opener. | ||
| const duration = resolveSlideDuration(drawerRef.current); | ||
| hold('layer', duration); |
There was a problem hiding this comment.
The hold taken here (and at line 664) is not released by this effect's cleanup, which only clears the close timer. architecture:react-component-runtime INV3 asks an effect's cleanup to release what its setup acquired; calling releaseHoldRef.current?.() there, with a separate ref for the reopen refocus, would keep the release beside the acquisition.
| releaseHoldRef.current = null; | ||
| closeAndRestoreFocus(dialog, triggerElementRef); | ||
| setIsCloseHeld(false); | ||
| }, duration); |
There was a problem hiding this comment.
Under a coarse pointer the close now runs when the slide-out ends: 410 ms after the request at the token default, and up to 550 ms. component:MobileNav FR5 and ORD3 on main bound the close at 250 ms after the request, so an engineering owner needs to amend those claims for a coarse pointer before this timing can be approved.
| * AppShell.tsx): Activity's `display: none !important` would otherwise cut | ||
| * the slide-out off (#5701). | ||
| */ | ||
| export const MAX_EXIT_MS = 550; |
There was a problem hiding this comment.
--duration-medium comes from a theme's motion.medium, which expandMotionScale does not cap, so with a theme slower than 550 ms the page and focus come back while the drawer is still moving, and AppShell hides it at 600 ms. Please state this cap here and in both changesets, which currently say the hold lasts until the slide-out ends.
|
Posted by Cindy's agent on her behalf. Review follow-ups:
If this stays idle, we'll fix 2 and 4 after about 30 minutes, keeping your authorship. |
|
Thanks. On it: 1, 2 and 4 land with my next push, within the hour, with the foundation fold.
|
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
meta#8360 now carries the review fixes of the foundation it holds. The two conflicts are this pull request's own files: - GestureHost.ts: meta#8360 counts the attachments that ask for an action group; here the fine-pointer group listener and the per-event recognizer set are gone (every pointer is served, FR43), so the merge keeps the count and drops those two. - GestureHost.test.ts: both sides added the same assertion to the second-finger case; the merge keeps this side's comment. meta#8360's action-group lifetime case counts a group's own pointerdown under a fine pointer, which this pull request replaces with one passive pointerdown on any element that uses a gesture (its own idle-listener case), so the merge leaves it out.
… attached, in this host's terms meta#8360's action-group lifetime case pins that an element stays a group while any attachment asks and stops with the last. Here every element that uses a gesture is served as a group under a fine pointer, so the same invariant reads as the element's one passive pointerdown: with two gestures attached, detaching either keeps it, and detaching the last removes it. It passes on this head.
meta#8360 carries this pull request's predecessors and the review fixes of the foundation it holds; this pull request's own diff is now its six commits (a second finger fails a one-finger pan wherever fingers come from, and the check reruns once that finger is delivered; a press's selection lock gets what it landed on; a pinch's second finger on another pan claims neither; the wider-band edge pan's real-touch proof; text). Two conflicts: - GestureHost.ts: the selection lock's target takes meta#8360's presented target, which is what the press landed on unless an element under it is moving, so it keeps this pull request's fix and meta#8293's correction. - .changeset/gesture-press-tracking.md: meta#8360's text, which names both writers of the press attribute.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
… lift and the browser's menu, keeps a kept press's finger, and goes when the page hides From the review of this pull request, three cases fail on its head: - an armed press installed the page listeners without the claiming ones, so a claimed finger's touchend stayed passive and the browser's menu opened during a claimed long press (spec:AST-094 FR21); - a kept press got its blocking touchmove during an armed press, but its moves were cancelled only where the primary pointer is coarse, so a held stepper on a touchscreen laptop lost its finger (FR11, DEC-21); - a page hidden mid-press cancelled the press but kept the page's listeners until the next lift anywhere (FR24).
…lift and the browser's menu, keeps a kept press's finger, and goes when the page hides - A finger's or a pen's armed press now brings the claiming listeners (the cancelable touchend and the contextmenu canceller) with the page listeners, as wherever the host installs, so a claimed lift clicks nothing and a claimed hold opens no browser menu (spec:AST-094 FR21). A mouse's press needs neither and adds neither. - A kept press keeps its finger under any pointer: its target blocks and its moves are cancelled once its press is delivered, not only where the primary pointer is coarse (FR11, DEC-21). - A page hidden mid-press releases the armed press as a blur does, so the page's listeners go with the cancelled press (FR24).
…'s selection An action group's press is a long press with no minimum duration, so it claims at the press's pointerdown, before any movement. The drag selection hold then cleared the page's selection, so a plain click on a group's button left its Copy or Cut with nothing to act on. The case fails on this head: the selection is gone after the pointerdown.
…age's selection The drag selection hold cleared the page's selection when any mouse or pen contact was claimed, but an action group's press claims at its pointerdown, before it has moved or selected anything, so a plain click on a group's button left its Copy or Cut with nothing to act on. A claim before any movement now keeps the page's selection: the hold still refuses a new one, and puts the page's back over a selection that appears while it runs. A claim after movement clears what its drag selected, as before (spec:AST-094 FR41). The case before the new one now waits for its hold to end a task after the lift, so no hold outlives that test.
…t every pointer does - GestureHost's header, the gestures README and coarsePointer.ts no longer describe the coarse-only host: a finger or a pen takes the touch-down lock under any primary pointer, a kept press's finger is kept during a fine pointer's press too, and recognizers attach for every pointer while the page listeners last a press (spec:AST-094 FR19, FR24, FR41, FR43). - The fine-pointer selection case asserted the replaced rule and passed only because it read iOS WebKit's inline lock without iOS WebKit: it now asserts the lock a fine pointer's finger and pen take, and that a mouse takes none. - FR41's sentence and its verification row say that a press claimed at its own pointerdown, before it moves, keeps the page's selection, while a drag that moved before its claim loses what it selected.
…ion, in the present tense The sentence said a drag that moved before its claim loses the selection it started, but the hold clears the page's selection then, whatever made it; and an unclaimed mouse drag kept "today's" behavior, a phrase that ages. It now says a claim after movement clears the page's selection, a press claimed at its own pointerdown keeps it, and an unclaimed mouse drag keeps its released behavior.
This pull request now sits on meta#8665, so a stack carrying it and meta#8665's other dependants (meta#8676) needs no second GestureHost resolution. Two hunks in GestureHost.ts's pointerdown, both this pull request's: - the second-finger check where fingers come from pointer events stays, followed by meta#8665's rule that a right click, a pen's barrel button or a macOS ctrl-click starts no press; - the check that runs again once the new finger is delivered stays, followed by meta#8665's selection lock call, which no longer takes the window.
Once a component in the root entry holds its layer's input (a popover's presentation hold, meta#8223), Layer/inputHold's static imports of gestures/GestureHost and gestures/nativeScroll put eight gesture modules (about 47 KB) in the root entry, against architecture:react-component-runtime INV13. Bundle a component that imports the input hold and require no gesture bytes. Fails while the input hold imports from gestures/.
…e host asks it Layer/inputHold registered a touch gate with the gesture host (getGestureHost(doc).addTouchGate(isInHeldLayer)) and walked the flat tree through gestures/nativeScroll, so any component that holds its layer statically carried the gesture system into its bundle and the root entry (architecture:react-component-runtime INV13). Invert it: the host asks isInHeldLayer for every touch itself (gestures may import Layer), the hold registers nothing, and it walks the flat tree through utils/flatTree, where parentAcrossShadow already lives. addTouchGate stays for other gates. A host test holds a held layer's touches with no gate (it fails if the host stops asking the hold).
There was a problem hiding this comment.
Approval is waiting on an engineering owner to settle two spec questions this branch raises: whether MobileNav, under a finger, should hold the leaving drawer's input from a close request until its dialog closes (no current claim covers it, since spec:AST-094 FR35 covers only the entrance, and the decision also settles whether architecture:public-component-api INV16 calls for a presentation value), and whether spec:AST-094 FR41 takes the new sentence that a mouse or pen drag a recognizer claims takes no text selection. An engineering owner records each decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of those decisions:
- Land #8342, #8119 and the pull requests beneath them first, so this branch carries only its own 10 files instead of about 31,800 lines of stacked work.
- Land #8671 (or move its test change into the scroll-lock pull request) first, since Core's root and hooks entries now carry gesture code that
scripts/touch-support-bundle.test.mjsrejects. - Let the slide-in hold end with the drawer's own transition when the slide-in is longer than 550 ms (
MobileNav.tsxline 378), as FR35 requires. - Have the
onOpenChangedocs (MobileNav.doc.mjslines 101, 206 and 314) say the drawer takes no touch while it slides in, since keys and assistive technology still work then. - Clear the timers that
after()starts (MobileNav.tsxlines 391–395) in the open/close effect's cleanup.
[Automated review]
| * load it can also run after it. | ||
| */ | ||
| async function slideInEnd(drawer: HTMLElement | null): Promise<unknown> { | ||
| const cap = after(MAX_EXIT_MS); |
There was a problem hiding this comment.
The race ends the hold at 550 ms whatever the transition, but the drawer slides for the theme's --duration-medium, which defineTheme({motion: {medium}}) accepts at any value and spec:AST-108 FR1–FR2 let tempo scale to 615 ms. With an 800 ms slide-in, a backdrop tap 690 ms after the open closes the drawer mid-entrance, which spec:AST-094 FR35 forbids. Keeping the cap only where getAnimations is missing, or deriving it from the computed transition, lets a running transition end the hold itself.
| async function after(ms: number): Promise<void> { | ||
| await new Promise(resolve => { | ||
| setTimeout(resolve, ms); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This setTimeout keeps no handle, so the open/close effect's cleanup releases the hold but cannot clear the timer: slideInEnd starts one on every touch open and the close branch another, and each still fires up to 550 ms after a reopen, a close or an unmount. architecture:react-component-runtime INV3 and INV5 ask the cleanup to release it; keeping the handles beside releaseHoldRef would do that.
| } else if (dialog.open && isTouch) { | ||
| // The leaving drawer's content takes no input until the dialog closes, | ||
| // at most MAX_CLOSE_DELAY_MS in, as under a mouse (FR5, ORD3); then | ||
| // focus goes back to the opener. | ||
| const delay = resolveCloseDelay(dialog); | ||
| hold('layer', after(delay)); | ||
| closeTimeoutRef.current = setTimeout(() => { | ||
| releaseHoldRef.current?.(); | ||
| releaseHoldRef.current = null; | ||
| closeAndRestoreFocus(dialog, triggerElementRef); | ||
| setIsCloseHeld(false); | ||
| }, delay); |
There was a problem hiding this comment.
Under a coarse pointer this holds the leaving drawer from the close request until dialog.close(): its content goes inert, a focused control is blurred and a shield swallows every tap. No current claim states this (spec:AST-094 FR35 covers entrances only; component:MobileNav FR4–FR6 say nothing about input while the drawer leaves), so it waits on an engineering owner's decision, which also settles whether architecture:public-component-api INV16 calls for a presentation value.
| type: '(isOpen: boolean) => void', | ||
| description: | ||
| 'Called when the drawer visibility changes (backdrop click, Escape key, or close button). Inside AppShell, this is managed automatically via context.', | ||
| 'Called when the drawer visibility changes (backdrop click, Escape key, or close button). Inside AppShell, this is managed automatically via context. Under a finger, the drawer takes no input while it slides in, and after a close none until its dialog closes; then focus returns to the element that opened it.', |
There was a problem hiding this comment.
The slide-in hold is pointer-only (viewport scope): keys and assistive technology stay live, as spec:AST-094 FR35 requires and the unit test pins. As written, this line (and its variants at 206 and 314) tells builders the drawer takes no input at all while it slides in; "takes no touch while it slides in" matches the changeset.
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| takes no new text selection for that drag only. A claim made after the | ||
| drag has moved clears the page's selection; a press claimed at its own | ||
| pointerdown, before it moves, keeps it. An unclaimed mouse drag keeps its | ||
| released behavior. |
There was a problem hiding this comment.
These sentences add a normative rule to FR41 that the record on main does not state, and today FR41 keeps a mouse on its released behavior. An engineering owner needs to approve or reject them, ideally in the gesture pull request they come from.
- MobileNav: the touch-presentation proofs stop the page's clock and run it on by exact amounts, so each probe and tap lands inside the slide-in or the close, and the close is read exactly (at most 250 ms) (from meta#8374). - Dialog and Drawer: each entrance check plays one drag and proves it arrived (a capture-phase touch probe added before the page's scripts); the page's movement is read from the visual viewport (from meta#8630). - Layer: a hold under a moving touch is released at an exact step of the drag, and the document's log covers the touch from its start to its end.
…av (spec:AST-094 FR35)
Layer/inputHold holds a layer's pointer input while its presentation
enters or leaves: holdInput(layer, {until, scope}) raises a shield (scope
'viewport' while it enters; 'layer' while it leaves, its content inert),
decides by each event's pointer (a finger's or a pen's press is held to
its end; a mouse passes once it moves with no button down, DEC-16), never
cancels a touch's start, and cancels a one-finger move so a pinch still
zooms (FR10). The gesture host asks the hold for each touch (holdsTouch),
so the hold imports nothing from gestures/ and a component in the root
entry may use it statically (architecture:react-component-runtime INV13).
Its first users: Dialog and Drawer hold a finger's tap or drag during
their entrance (holdEntranceInput). MobileNav's drawer takes no finger
input while it slides in (a 'viewport' hold until the slide-in's
transition ends) or out (a 'layer' hold from the close request until the
dialog closes), and AppShell lets the slide-out play to its end.
Every browser proof lands its input at an exact point (an entrance held
where it is, the page's clock, or an exact step of a drag) and plays once.
From meta#8342 and its per-event offer, meta#8630 (Dialog and Drawer) and meta#8374
(MobileNav).
Co-authored-by: Cindy Zhang <cindyxz@meta.com>
|
Folded into #8342 under the maintainers' section plan; the commit credits this PR's authors. Closing; the branch stays. |
Depends on #8342 and #8119. Review only this PR's change.
User impact
What led here: we are bringing Astryx's touch components to iOS quality on the mobile web. On a phone, MobileNav's drawer took taps while it moved: a tap beside the opening drawer closed it before it finished sliding in, and a tap on the leaving drawer still followed its links. UIKit takes no touch until a presentation ends. Inside AppShell, under any pointer, the drawer also vanished 300 ms into its 410 ms slide-out.
Problem and solution fit
spec:AST-094FR35 keeps live.Size (gzip JavaScript per import, base → head): MobileNav 54,340 → 67,601 B, AppShell 61,367 → 74,533 B, SideNav 61,496 → 74,910 B, TopNav 55,172 → 68,487 B.
Expected behavior and authority
spec:AST-094FR35 (#8618); MobileNav FR5 and ORD3; AST-017 FR59.Smallest restoration
One touch branch in MobileNav's open/close effect, a
pointeroption for the tests' media-query stub, AppShell's hide delay, two[fix]changesets.Evidence
A finger closing the drawer inside AppShell (iOS 26.2 simulator) slides out to the end; a mouse, before and after, is unchanged.
Scope
Testing
In Chrome's device toolbar (a phone, touch):
What I ran: the MobileNav and AppShell unit tests and the MobileNav browser specs (Chromium and WebKit, twice) passed. On the base, 5 of 8 browser cases fail; the two newest unit tests fail against the previous timer-based release.
scripts/touch-support-bundle.test.mjsfails its root- and hooks-entry cases because MobileNav now reaches the shared scroll lock; #8671 replaces that check.Recorded on the iOS 26.2 simulator and the Android 15 emulator.