Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
20142ee to
3efa2a4
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: against main this diff carries the whole unmerged stack, so it needs a ruling on whether the draft AST-094 record (FR1–FR23) governs the touch foundations, lab NavigationStack and the package-level @astryxdesign/core/motion/experimental and /gestures/experimental subpaths (AST-017 FR21 provides /experimental subpaths only for an owning component), and whether component:Selector DEC-1 ("adaptive keeps its width-and-pointer query") is updated for the new presentation="adaptive" boundary or that change leaves the stack. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. These need fixing whatever that decision:
- Merge the predecessors (or rebase onto them) so this pull request's diff is its own 570-line commit rather than about 27,600 authored lines.
- NavigationStack's content swipe pops the screen when someone swipes a horizontal scroller inside it that can still scroll (
NavigationStack.tsx:1116–1131, against the promise at:280–283). - A push that starts while another push is still sliding shows the new screen with no slide (
NavigationStack.tsx:842–845). motion/uikit/navigation.ts:83needs anM-…measurement id for the UIKit table test,routes.lock.jsonandpackages/cli/assets/discover/official-packages.jsonneed regenerating for NavigationStack, and AST-094 FR7 cites anAST-090that is not in the repository.
[Automated review]
| cardsRef, | ||
| () => | ||
| new PanGestureRecognizer({ | ||
| direction: 'right', |
There was a problem hiding this comment.
This pan claims every rightward drag in the cards, including one that starts on a horizontal scroller that can still scroll: with a scroller at scrollLeft 300 inside the Pushed story's top screen, a 160px rightward touch swipe popped to General instead of scrolling. The card style at lines 280–283 promises the scroller keeps its native sideways scrolling. Failing the pan when touch.scroll?.canScroll(...) says the content can move the finger's way (or using scrollHandoff) would keep that promise.
| releaseRef.current = null; | ||
| let promise: Promise<boolean>; | ||
| if (transition.kind === 'push') { | ||
| if (!wasRunning) { |
There was a problem hiding this comment.
When a push lands while another transition is running, this skips placing the new top card at progress 1, so its animator springs from its resting x = 0 to 0: tapping About while General was still at 131px showed About at 0px on every frame, with no slide. Placing a card that has no presented position off-screen first would keep the slide.
| * @source measured: UIKit lab (injected host app), iOS 26.2 simulator, | ||
| * iPhone 17 Pro, UINavigationController.contentSwipe began after 27–30 pt | ||
| * of travel for drags at or under 600 pt/s, Oct 23 2026 | ||
| * @measure the exact threshold and the faster start: at 1000 pt/s the swipe |
There was a problem hiding this comment.
uikit.test.ts:151–152 needs @measure M-… with a row in motion/MEASUREMENTS.md; this prose @measure fails UIKit constants table: every value is sourced (law P2) > navigation.ts › contentPopHysteresis (also red in CI test-ui (1/4)). Moving the text into a new M- row and referencing its id fixes it.
| // 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 presentation="adaptive" for DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector from (max-width: 768px) and (pointer: coarse) to narrower than 744px or shorter than 500px with a coarse pointer. The current component:Selector DEC-1 says "adaptive keeps its width-and-pointer query", so this needs that record amended by an owner, or it should leave this stack.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or at this head (the same citation recurs at line 235), so a reader cannot see what FR7 departs from. Cite the record where it lives, or drop the note.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
3efa2a4 to
4e1e859
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: against main this diff carries the whole unmerged stack, so it needs a ruling on whether the draft AST-094 record (FR1–FR23, DEC-1, DEC-2) governs the touch foundations, lab NavigationStack and the package-level @astryxdesign/core/motion/experimental and /gestures/experimental subpaths, and whether component:Selector DEC-1 (“adaptive keeps its width-and-pointer query”) is updated for the new presentation="adaptive" boundary or that change leaves the stack. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. These need fixing whatever that decision:
- NavigationStack's content swipe pops the screen instead of scrolling a horizontal scroller inside it, since the pan now claims the touch from 5 px (
NavigationStack.tsx:1116–1131, against the promise at:280–283). - A push that starts while another push is still sliding shows the new screen with no slide (
NavigationStack.tsx:842–845). - The
lintcheck fails becausepackages/cli/assets/discover/official-packages.jsonchanges CLI output without a Changeset naming@astryxdesign/cli. - Land the predecessors (or retarget) so the diff is this pull request's own commit, and point AST-094 FR7 at a record that exists instead of
AST-090.
[Automated review]
| const content = useGestureRecognizer( | ||
| cardsRef, | ||
| () => | ||
| new PanGestureRecognizer({ | ||
| direction: 'right', | ||
| hysteresis: CONTENT_CLAIM_PX, | ||
| maximumNumberOfTouches: 1, | ||
| shouldReceiveTouch: (_recognizer, touch) => | ||
| !( | ||
| browserOwnsEdgeRef.current && | ||
| startsInEdgeBand( | ||
| touch.startX, | ||
| writingDirection(containerRef.current), | ||
| ) | ||
| ), | ||
| }).requireToFail(edge), |
There was a problem hiding this comment.
This pan claims a rightward drag from 5 px (the host's early claim for a one-way pan), before any native scroll, so a horizontal scroller in a pushed screen can no longer scroll back toward its start: in Chromium a 160 px rightward swipe inside one left it at scrollLeft 300 and popped the screen, against the promise at lines 280–283. Let the pan fail when the touch's horizontal scroll chain can still scroll its way.
| if (transition.kind === 'push') { | ||
| if (!wasRunning) { | ||
| setProgress(1, top, under); | ||
| } |
There was a problem hiding this comment.
When a push lands while another transition runs, the new top card is never placed at progress 1, so animateProgress(0, …) starts from its resting x = 0 and nothing slides: pushing General then About 80 ms later leaves About at 0px from its first frame. Place the incoming card at progress 1 unless it is the card the running transition already moves.
| { | ||
| "kind": "component", | ||
| "name": "NavigationScreen" | ||
| }, | ||
| { | ||
| "kind": "component", | ||
| "name": "NavigationStack" | ||
| }, |
There was a problem hiding this comment.
This changes @astryxdesign/cli's shipped discovery data, and the lint check's Changeset coverage step fails for it; add a Changeset naming @astryxdesign/cli.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or in this pull request (the citation recurs at line 235), so an approver cannot see which exit rules FR7 departs from; cite the record that states them, or drop the departure.
4e1e859 to
b9088e1
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: against main this diff carries the whole unmerged stack, so it needs a ruling on whether the draft AST-094 record (FR1–FR23, DEC-1, DEC-2) governs the touch foundations, lab NavigationStack and the package-level @astryxdesign/core/motion/experimental and /gestures/experimental subpaths, and whether component:Selector DEC-1 (“adaptive keeps its width-and-pointer query”) is updated for the new compact-touch boundary or that change leaves the stack. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. These need fixing whatever that decision:
- The content swipe pops a pushed screen when someone drags a horizontal scroller inside it back toward its start (
NavigationStack.tsx:1116–1134, against the promise at:280–283). - A push that starts while another transition runs shows the new screen with no slide (
NavigationStack.tsx:842–845). .changeset/touch-presentation.mddoes not say that everyDropdownMenuSubMenuleft at its default now drills in or flies out at the new boundary.- Land the predecessors (or retarget) so the diff is this pull request's own 570-line commit, and point AST-094 FR7 at a record that exists instead of
AST-090.
[Automated review]
| new PanGestureRecognizer({ | ||
| direction: 'right', | ||
| hysteresis: CONTENT_CLAIM_PX, | ||
| maximumNumberOfTouches: 1, | ||
| shouldReceiveTouch: (_recognizer, touch) => | ||
| !( | ||
| browserOwnsEdgeRef.current && | ||
| startsInEdgeBand( | ||
| touch.startX, | ||
| writingDirection(containerRef.current), | ||
| ) | ||
| ), | ||
| }).requireToFail(edge), |
There was a problem hiding this comment.
This pan claims a rightward drag from 5 px through the host's early claim, before any native scroll, so a horizontal scroller in a pushed screen can no longer scroll back toward its start: in Chromium with touch input, a 160 px rightward swipe inside a scroller at scrollLeft 300 popped the screen and never scrolled it, while a leftward swipe scrolled it to 433. spec:AST-025 FR18 gives that drag to the nearest owner on the axis, and the card style at lines 280–283 promises it. Letting the pan begin only when the scrollers under the touch cannot scroll its way (the recognizer's scrollEdge option) would fix it, and a scroller case in the interaction proof would keep it fixed.
| if (!wasRunning) { | ||
| setProgress(1, top, under); | ||
| } |
There was a problem hiding this comment.
When a push lands while another transition runs, the new top card is never placed off screen, so animateProgress(0, …) starts from its resting x = 0: pushing About 80 ms into General's push shows About at 0 px from its first rendered frame, where a lone push still has it 218 px out after 57 ms. Placing the incoming card at layersAt(1, …).topX whenever it is new to the transition, plus a quick double-push case in the proof, would fix it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
The same constant drives useMenuDrillIn, and DropdownMenuSubMenu defaults to presentation = 'adaptive', so every sub-menu left at its default now opens as a flyout on an iPad mini in portrait and drills in on a phone in landscape, with no opt-in. spec:AST-017 FR7 makes the Changeset body the whole declaration, so this one should name that change too.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or in this pull request (the citation recurs at line 235), so an approver cannot see which exit rules FR7 departs from; cite the record that states them, or drop the departure.
b9088e1 to
ebc4eb2
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: against main this diff carries the whole unmerged stack, so it needs a ruling on whether the draft AST-094 record (FR1–FR23, DEC-1, DEC-2) governs the touch foundations, lab NavigationStack and the package-level @astryxdesign/core/motion/experimental and /gestures/experimental subpaths, and whether component:Selector DEC-1 (“adaptive keeps its width-and-pointer query”) is updated for the new compact-touch boundary or that change leaves the stack. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. These need fixing whatever that decision:
- NavigationStack's content swipe pops a pushed screen when someone drags a horizontal scroller inside it back toward its start (
NavigationStack.tsx:1157–1176, against the promise at:283–286). - Focus drops to the page after every pop in a real browser, because the revealed screen is still
inertwhenfocusAfterruns (NavigationStack.tsx:926–933); the jsdom focus test cannot see this. - A push that starts while another transition runs shows the new screen with no slide (
NavigationStack.tsx:878–881). .changeset/touch-presentation.mddoes not say that everyDropdownMenuSubMenuleft at its default now drills in or flies out at the new boundary.- Land the predecessors (or retarget) so the diff is this pull request's own 632-line change, and point AST-094 FR7 at a record that exists instead of
AST-090.
[Automated review]
| () => | ||
| new PanGestureRecognizer({ | ||
| name: 'NavigationStack.contentBackSwipe', | ||
| direction: 'right', |
There was a problem hiding this comment.
This one-way pan has no scroll check, so the host claims the touch from 5 px its way and a horizontal scroller in a pushed screen can't be dragged back toward its start: in Chromium with touch input, a 160 px rightward swipe inside an overflow-x: auto strip at scrollLeft 300 popped About and never scrolled it (a leftward swipe scrolled it to 459). The card comment at lines 283–285 promises the opposite, and spec:AST-025 FR18 gives that drag to the nearest owner on the axis. Letting the pan begin only when the scrollers under the touch can't move its way (the recognizer's scrollEdge option), plus a scroller case in the proof, would fix it.
| ); | ||
| if (under != null) { | ||
| settle(under, 'top'); | ||
| focusAfter('pop', top); |
There was a problem hiding this comment.
When this runs, the setView above has not committed yet, so view.leaving is still set and every card and bar is inert (lines 1216 and 1228): source.focus() does nothing, and the fallback at line 855 targets the leaving screen's own title. In Chromium, Back after a keyboard push, Back on the deep-linked story and Done all leave focus on body; the jsdom test at NavigationStack.test.tsx:133–149 can't see it because jsdom ignores inert. Focusing after the commit that clears leaving, with the revealed screen's title as the fallback, would fix it.
| releaseRef.current = null; | ||
| let promise: Promise<boolean>; | ||
| if (transition.kind === 'push') { | ||
| if (!wasRunning) { |
There was a problem hiding this comment.
A push that lands while another transition runs skips this, so the new top card starts from its resting x = 0 and never slides: pushing About 81 ms into General's push kept About at 0 px on every frame for 270 ms, where a lone push starts it at 370 px. Placing the incoming card at layersAt(1, …).topX whenever it is new to the transition would keep the slide.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
The same constant drives useMenuDrillIn, and DropdownMenuSubMenu defaults to presentation = 'adaptive', so every sub-menu left at its default now drills in on a phone in landscape and flies out on an iPad mini in portrait, with no opt-in. spec:AST-017 FR7 makes the Changeset body the whole declaration, so this one should name that change too.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 on main or in this pull request (the citation recurs at line 235), so an approver cannot see which exit rules FR7 departs from; cite the record that states them, or drop the departure.
ebc4eb2 to
a808553
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the draft AST-094 record (FR1–FR23, DEC-1, DEC-2), the only spec behind the touch foundations, the lab NavigationStack, the package-level motion/experimental and gestures/experimental subpaths and the new presentation="adaptive" boundary, and on whether its FR4 ('physics are not themable') is amended for the new theme motion roles and platform condition or those come out. 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. Whatever the decision, these need fixing:
- The content swipe pops a pushed screen when someone drags a horizontal scroller inside it back toward its start (
NavigationStack.tsx:1171–1190). - Focus lands on the page body after every pop, because the revealed screen is still
inertwhenfocusAfterruns (NavigationStack.tsx:935–942). - A push that starts during a running transition shows the new screen without sliding (
NavigationStack.tsx:887–890). test-node (1/4)fails because thedocs-topics.jsongolden no longer matches the motion docs topic..changeset/touch-presentation.mddoes not say that a defaultDropdownMenuSubMenunow drills in or flies out at the new boundary, and AST-094 FR7 citesAST-090, which is not in the repository.- Land the predecessors first so this diff carries only its own commit, and record iOS Safari runs of the back swipes.
[Automated review]
| () => | ||
| new PanGestureRecognizer({ | ||
| name: 'NavigationStack.contentBackSwipe', | ||
| direction: 'right', |
There was a problem hiding this comment.
This one-way pan has no scrollEdge, so it claims at 5 px and cancels a horizontal scroller's native scroll: a rightward drag on a carousel inside a pushed screen pops the screen. scrollEdge: true lets it begin only when no scroller under the touch can move that way.
| ); | ||
| if (under != null) { | ||
| settle(under, 'top'); | ||
| focusAfter('pop', top); |
There was a problem hiding this comment.
setView above is only queued, so every card is still inert here and this focus call does nothing; in Chromium focus ends on the body after the back button. Move focus after the commit that clears leaving.
| releaseRef.current = null; | ||
| let promise: Promise<boolean>; | ||
| if (transition.kind === 'push') { | ||
| if (!wasRunning) { |
There was a problem hiding this comment.
When a transition is running this skips placing the new top card off-screen, and a newly mounted card starts at x = 0, so a second quick push shows the new screen without sliding.
| // in that edge band. Server render and first paint assume a tab. | ||
| const [browserOwnsEdge, setBrowserOwnsEdge] = useState(true); | ||
| const browserOwnsEdgeRef = useRef(true); | ||
| useEffect(() => { |
There was a problem hiding this comment.
This effect repairs browserOwnsEdge after the first render; useSyncExternalStore with a server snapshot, as useTouchIdiom does, derives it during render (react-component-runtime INV1/INV3).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
DropdownMenuSubMenu defaults to adaptive and reads the same query, so every default sub-menu now drills in or flies out at the new boundary too; please name it here.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository, so an approver cannot see what FR7 departs from; cite where it lives or drop the clause until it lands.
a808553 to
580bd5a
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the draft AST-094 record (FR1–FR23, DEC-1, DEC-2), the only spec behind the touch foundations, the lab NavigationStack, the package-level motion/experimental and gestures/experimental subpaths and the new presentation="adaptive" boundary (which also needs Selector's DEC-1 updated), and on whether FR4 ("physics are not themable") is amended for the theme motion roles and platform condition or those come out. 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. Whatever the decision, these need fixing:
- The content back swipe pops a pushed screen when someone drags a horizontal scroller inside it back toward its start (
NavigationStack.tsx:1171–1190). - Focus lands on the page body after every pop, because every screen is still
inertwhenfocusAfterruns (NavigationStack.tsx:935–942). - A push that starts during a running transition shows the new screen without sliding (
NavigationStack.tsx:887–890). - The motion docs, theme template and
[feat]changeset say themes retune sheets, alerts, menus and scrolling, but only the lab NavigationStack reads the motion roles today. .changeset/touch-presentation.mdomits the defaultDropdownMenuSubMenudrill-in, AST-094 FR7 citesAST-090, which is not in the repository, and the back swipes and tab edge still need iOS Safari runs.- Land the predecessors first so this diff carries only its own commits.
[Automated review]
| () => | ||
| new PanGestureRecognizer({ | ||
| name: 'NavigationStack.contentBackSwipe', | ||
| direction: 'right', |
There was a problem hiding this comment.
This one-way pan has no scrollEdge, so it claims at 5 px (PanGestureRecognizer.ts:112) and the host cancels the finger's touchmove: a rightward drag on a horizontal scroller inside a pushed screen pops the screen instead of scrolling it back, despite the promise at lines 285–287. scrollEdge: true begins it only when no scroller under the touch can move that way.
| ); | ||
| if (under != null) { | ||
| settle(under, 'top'); | ||
| focusAfter('pop', top); |
There was a problem hiding this comment.
The setView above is only queued, so every card and bar is still inert here and this focus call does nothing; focus ends on the page body after Back. Move focus after the commit that clears leaving, and fall back to the revealed screen's title rather than the leaving one (line 864).
| releaseRef.current = null; | ||
| let promise: Promise<boolean>; | ||
| if (transition.kind === 'push') { | ||
| if (!wasRunning) { |
There was a problem hiding this comment.
When a transition is running this skips placing the new top card off-screen, and a newly mounted card rests at x = 0, so a second quick push shows the new screen without sliding. Place a newly mounted card at progress 1 even when retargeting.
| // in that edge band. Server render and first paint assume a tab. | ||
| const [browserOwnsEdge, setBrowserOwnsEdge] = useState(true); | ||
| const browserOwnsEdgeRef = useRef(true); | ||
| useEffect(() => { |
There was a problem hiding this comment.
This effect repairs browserOwnsEdge after the first render and names no external system (react-component-runtime INV1/INV3); useSyncExternalStore with a server snapshot, as useTouchIdiom does, derives it during render.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
Only the lab NavigationStack reads motion roles or the animator at this head; no released sheet, alert, menu or scroller plays these springs, so a theme that sets springs.sheetPresent (the example here, in the template and in defineTheme's JSDoc) changes nothing a product ships. Describe what reads the roles today.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
DropdownMenuSubMenu defaults to presentation = 'adaptive' and its drill-in reads the same query (useMenuDrillIn.tsx:39), so every default sub-menu now drills in or flies out at the new boundary too; please name it here (AST-017 FR7).
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository (the citation recurs at line 235), so an approver cannot see what FR7 departs from; cite where it lives or drop the clause until it lands.
580bd5a to
bf91c97
Compare
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…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.
…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.
There was a problem hiding this comment.
Approval waits on an engineering owner providing or identifying the governing spec for the new public isRtlElement export from @astryxdesign/core/hooks, or deciding that it stays internal; 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. Whatever that decision, the back swipe, the gesture host and the scope need these changes:
- A release moving back toward the start still pops (
navigationTransition.ts:121–130), wherespec:AST-094FR37 requires a cancel. NavigationStackTouch.tsaverages its own three velocities, so a resting finger still flings;spec:AST-100FR2 and FR3 require the pan's own release velocity, and the case atNavigationStack.interaction.browser.spec.ts:669–687should expect a cancel.spec:AST-094FR19 and FR43 attach gestures for every pointer and keep only the edge swipe off under a mouse, a trackpad or a pen, so the coarse-only gesture host, both swipes and the mouse case at:839–869need to conform, unless an engineering owner first records an exception.- A swipe that catches the return from past the start should resume from the signed position (
NavigationStack.tsx:1166, FR6), and the rubber band (FR3), the swipe direction (spec:AST-100FR5), the iOS-only wording atNavigationStack.doc.mjs:67, real-device evidence for the iOS Safari and Android Chrome claims, and the caller-less internal options need the fixes noted in the review. - At 31,476 authored lines the pull request carries its whole stack, so landing the layers below first, or re-targeting at #8107's branch, keeps this review to its own change.
[Automated review]
| export function shouldFinishPop( | ||
| progress: number, | ||
| velocityWidthsPerSecond: number, | ||
| ): boolean { | ||
| return ( | ||
| progress + | ||
| navigation.interactivePopFinishProjection * velocityWidthsPerSecond > | ||
| navigation.interactivePopFinishThreshold | ||
| ); | ||
| } |
There was a problem hiding this comment.
spec:AST-094 FR37: a release whose velocity points back toward the start must cancel, whatever the travel. From 0.85 of the width, any backward release slower than one width per second still passes this projection and pops. Check the velocity's sign before projecting.
| // UIKit's release velocity: the mean of the pan's last three | ||
| // velocities, the lift's own update included. A pause keeps it. | ||
| let v = 0; | ||
| if (recognizer.state === 'ended') { | ||
| swipe.velocities.push( | ||
| popVelocity(recognizer.velocity().x, width, direction), | ||
| ); | ||
| const recent = swipe.velocities.slice( | ||
| -navigation.interactivePopVelocitySamples, | ||
| ); | ||
| v = recent.reduce((sum, value) => sum + value, 0) / recent.length; | ||
| } |
There was a problem hiding this comment.
This mean of the last three per-move velocities never decays during a rest, so a finger that stops before lifting still flings (the case at NavigationStack.interaction.browser.spec.ts:669–687 asserts it). spec:AST-100 FR2 makes that release 0 and FR3 rules out a second estimate; read the pan's own release velocity at the lift.
| layers?.barDim?.stop(); | ||
| } | ||
| runningRef.current = null; | ||
| return width > 0 ? Math.abs(x) / width : 0; |
There was a problem hiding this comment.
Past the start the card sits at a negative x (layersAt), so Math.abs turns a catch during the return into a jump to the mirrored offset (spec:AST-094 FR6). Return the signed progress: (direction === 'rtl' ? -x : x) / width.
| private isActive(recognizer: GestureRecognizer): boolean { | ||
| return this.isCoarse || this.perEventRecognizers.has(recognizer); | ||
| } |
There was a problem hiding this comment.
spec:AST-094 FR19 and FR43 attach recognizers for every pointer, and architecture:react-component-runtime INV13 rules out a touch-only query; this gate keeps them inactive under a mouse, a trackpad, or a finger or pen on a device whose primary pointer is fine.
| export {useTreeFocus} from './useTreeFocus'; | ||
| export type {UseTreeFocusOptions, UseTreeFocusReturn} from './useTreeFocus'; | ||
|
|
||
| export {isRtlElement} from './isRtlElement'; |
There was a problem hiding this comment.
This makes isRtlElement stable public API; no current record approves it, and its changeset says it is pending owner review. Lab could reach it through an internal entry until an engineering owner decides.
Depends on #8107. Review only this PR's change.
Maintainer impact
What led here: we are bringing Astryx's touch components to iOS quality on the mobile web, matching UIKit's motion. NavigationStack (#8107) reproduces UIKit's navigation, but its unit tests use fake timers and never see what a browser draws: a push off UIKit's curve, a swipe that stops following the finger, or a push that freezes on a busy page would pass them. These tests drive the stack in real Chromium and WebKit with real touch, and fail CI on any of that.
Intended invariant
spec:AST-094FR22).Change
The options were more unit tests (fast, blind to rendering), screenshot comparisons (single frames, brittle) or the repository's interaction harness (real input, every frame, slower). I chose the harness because it checks what a person sees, in both engines. Moving layers are compared by their animations, not computed positions, which WebKit can report late on a busy machine. Added: one test file, two stories (a sideways scrolling strip; an app that refuses a pop) and the harness as a lab dev dependency.
Evidence
Scope
Testing
What I ran and what it showed:
pnpm storybook:build), which the tests load.node node_modules/@playwright/test/cli.js test --config playwright.interaction.config.ts --project=chromium packages/lab/src/NavigationStack) and WebKit (node internal/interaction-harness/scripts/webkit-container.mjs packages/lab/src/NavigationStack): every test passed, twice in each engine; WebKit skips three that need Chromium-only traces.velocity.topX * 0.9inNavigationStack.tsx'sanimateProgress): "A back swipe holds the revealed bar still…" failed every run.Recorded on the iOS 26.2 simulator and the Android 15 emulator. Includes #8562 (Core's internal entry points); if those land differently, this follows.
Local gates are the evidence here; no CI run is expected.