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
3a64377 to
dc0c38b
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 covers CH-01 (the list row kept selected under a pushed NavigationStack screen), holdSelection as a stable @astryxdesign/core/utils export, or the new presentation="adaptive" thresholds, and the draft spec:AST-094 added here (FR1–FR23) is unapproved and does not mention CH-01. To settle it, an engineering owner provides or identifies the governing spec for CH-01 and records a decision on AST-094 by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- Land the predecessor pull requests first, or retarget, so this diff is the 246-line CH-01 commit rather than 30,305 authored lines.
- Release a selection hold when it is replaced, when its push never lands, and when the stack unmounts (
push()and the release loop inNavigationStack.tsx). - On
popToRoot(), deselect the row on the revealed screen with the pop instead of snapping it, and release holds for replaced paths. - Fix the failing checks: list the
contentPopHysteresismeasurement thatuikit.test.tsexpects, then regeneratepackages/cli/assets/discover/official-packages.jsonandinternal/shadcn-registry/routes.lock.json.
[Automated review]
| const key = getKeyRef.current | ||
| ? getKeyRef.current(entry, index) | ||
| : index; | ||
| holdsRef.current.set(`screen-${key}`, hold); |
There was a problem hiding this comment.
This stores the hold before the app has accepted the path. If onPathChange refuses or defers the push, the row stays held with no screen over it. A later push at the same depth then overwrites this entry without end(), and since the press controller never clears held, the first row stays highlighted until it unmounts. Consider ending any hold already stored under the key, keeping the new one pending (as pendingSourceRef is) until the push transition commits, and ending every hold in an unmount cleanup.
| for (const [key, held] of holdsRef.current) { | ||
| if (!present.has(key) && key !== top) { | ||
| held.end(true); | ||
| holdsRef.current.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
On popToRoot() from two levels, this loop ends the hold keyed by the dropped middle screen. That hold belongs to the row on the revealed root, so the row the person sees snaps off, while the deselect below plays on a row of the middle screen that is no longer rendered. Animating the hold of the first removed screen, the one directly above under, and ending the rest would keep the visible row's fade.
| if (transition.kind === 'none') { | ||
| // Settle every screen where it rests. | ||
| view.models.forEach((model, index) => { | ||
| settle(model.key, index === view.models.length - 1 ? 'top' : 'covered'); | ||
| }); | ||
| return; |
There was a problem hiding this comment.
A replaced path (kind: 'none') returns here, before the release loop below, whose comment says replaced paths release their rows at once. The covered row stays held until the next pop.
| * @measure the exact threshold and the faster start: at 1000 pt/s the swipe | ||
| * began on the first move event (~8 pt), so above some speed between 600 | ||
| * and 1000 pt/s it begins sooner; bracket both with finer drags. |
There was a problem hiding this comment.
uikit.test.ts:152 expects @measure M-… with an id listed in motion/MEASUREMENTS.md. This free-text @measure fails test-ui (1/4) at this head.
PR Analysis Report
Modified ComponentsItem (@astryxdesign/core)
TreeList (@astryxdesign/core)
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: 2 accessibility violation(s) found — 2 serious. Radio group pattern - 1 issue(s)
RadioList - 1 issue(s)
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 | View full report |
dc0c38b to
92ce88a
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers CH-01 (the list row kept selected under a pushed NavigationStack screen), holdSelection as a stable @astryxdesign/core/utils export, or the new presentation="adaptive" thresholds, and the draft spec:AST-094 added here (FR1–FR23) is unapproved and does not mention CH-01. An engineering owner records that decision, naming the governing spec for CH-01 and approving or rejecting AST-094, 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:
- Land the predecessor pull requests first, or retarget, so this diff is the 246-line CH-01 commit rather than 31,251 authored lines.
- Release a selection hold when it is replaced, when its push never lands, and when the stack unmounts (
push()and the release loop inNavigationStack.tsx). - On
popToRoot(), fade the row on the revealed screen with the pop instead of clearing it, and release holds for replaced paths. - Add a Changeset naming
@astryxdesign/clifor the regeneratedofficial-packages.json, whichlintnow fails on.
[Automated review]
| }, | ||
| { | ||
| "kind": "component", | ||
| "name": "NavigationScreen" |
There was a problem hiding this comment.
Regenerating this list adds the lab NavigationScreen and NavigationStack to the CLI's discover output, and lint now fails Changeset coverage: "@astryxdesign/cli: this change ships to consumers without a Changeset naming @astryxdesign/cli." Adding a Changeset that names @astryxdesign/cli (pnpm changeset:new --packages @astryxdesign/cli --category <fix|feat>) should clear it.
92ce88a to
04985ca
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers CH-01 (the list row kept selected under a pushed NavigationStack screen), holdSelection as a stable @astryxdesign/core/utils export, or the new presentation="adaptive" thresholds, and the draft spec:AST-094 added here (FR1-FR23) is unapproved and does not mention CH-01. An engineering owner records that decision, naming the governing spec for CH-01 and approving or rejecting AST-094, by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- Land the predecessor pull requests first, or retarget, so this diff is the 246-line CH-01 commit rather than 31,228 authored lines, and get a passing
lintrun. - Release a selection hold when it is replaced, when its push never lands, and when the stack unmounts (
push()inNavigationStack.tsx). - On a multi-level pop such as
popToRoot(), fade the row on the revealed screen with the pop instead of clearing it, and release holds for replaced paths. - Return focus to the control that pushed when it sits inside the held row.
- Add the
## Writing rubric resultsforspec:AST-094to the description, which the review gate reads before a record can be approved.
[Automated review]
| if (hold != null && hold.element.isConnected) { | ||
| hold.focus({preventScroll: true}); | ||
| return; |
There was a problem hiding this comment.
This sends focus to hold.focus(), the row's first focusable descendant (selectionHold.ts:163-172), even when source is another control inside the same row. When a trailing control in an interactive Item pushes, a keyboard or screen-reader user lands on the row's primary button after the pop, while the component doc still says focus returns to the control that pushed. Focusing source when it is connected and inside hold.element, and falling back to hold.focus() otherwise, keeps both cases right.
04985ca to
a72c634
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers CH-01 (the list row kept selected under a pushed NavigationStack screen), holdSelection as a stable @astryxdesign/core/utils export, or the new presentation="adaptive" thresholds, and the draft spec:AST-094 added here (FR1-FR23) is unapproved and does not mention CH-01. An engineering owner records that decision, naming the governing spec for CH-01 and approving or rejecting AST-094, by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- Land the predecessor pull requests first, or retarget, so this diff is the 457-line CH-01 commit rather than 31,549 authored lines.
- Move focus after a pop only once the revealed screen is no longer
inert; today a keyboard Back leaves focus on the page body. - Release a selection hold when its push is refused, when it is overwritten or its path replaced, and when the stack unmounts (
push()and the transition effect inNavigationStack.tsx). - Fix AST-094's citation of a
spec:AST-090that is not in the repository, and add its## Writing rubric resultsto the description.
[Automated review]
| hold?.end(true); | ||
| if (under != null) { | ||
| settle(under, 'top'); | ||
| focusAfter('pop', revealedKey ?? top); |
There was a problem hiding this comment.
This runs right after setView clears leaving, before React re-renders, so the revealed screen still carries inert (isCovered at line 1318) and the focus call does nothing. In Chromium a keyboard Back in the Settings story leaves focus on the page body, while the component doc says focus returns to the control that pushed. Keeping the target in a ref and focusing it in a layout effect once leaving is null (or removing inert from the revealed screen first) would fix it; the jsdom focus tests cannot see this because jsdom ignores inert.
| 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 does not exist in the repository, at this head or on main, so an owner cannot read the FR1 and FR10 this departs from. Cite the record or pull request that holds them, or drop the note; the Current-state impact repeats it.
a72c634 to
c7b3f89
Compare
…xposes no ontouchstart is taken once Chromium fixes ontouchstart when the page is created, so turning touch emulation on later sends Touch Events without it: the host reads Pointer Events for the finger and must not read its Touch Events too. A one-finger long press fails here, the finger tracked twice.
…from their pointer events install() reads a finger from its pointer events when the window has no ontouchstart, but the four touch handlers still took the Touch Events a browser sends anyway (Chromium after touch emulation turns on), tracking the finger twice, so a one-finger long press never recognized. Each touch handler now returns while usesTouchEvents is false, after noting the fingers down, as install()'s comment promised.
…ter events) into meta#8320
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers CH-01 (the list row kept selected under a pushed NavigationStack screen) or what ships with it (divider hiding beside a pressed or held List row, isRtlElement on @astryxdesign/core/hooks, the lab NavigationStack, and the springs and deselect timing it plays outside the spec:AST-094 FR4 motion roles), so an engineering owner needs to provide or identify the governing spec, or record the decision 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:
- Return focus to the row after a finger or mouse pop: a tap on a row's label focuses the label's span, which cannot take focus.
- Read the back swipe's release velocity from the recognizer at the lift (
spec:AST-100FR2/FR3) and cancel any release moving back toward the start (spec:AST-094FR37). - Undo a refused push and a declined pop when the app re-renders with an equal
path. - Land the predecessor pull requests first, or retarget, so the diff is the CH-01 change rather than 34,385 authored lines.
- Keep one implementation of the selection hold, route the animator through
internal/touchasarchitecture:react-component-runtimeINV13 names it, name the external system in the two changed Effects, and correct the[fix]tag of thelist-row-press-compositorChangeset.
[Automated review]
| if ( | ||
| source instanceof HTMLElement && | ||
| source.isConnected && | ||
| hold.element.contains(source) | ||
| ) { | ||
| source.focus(focusOptions); |
There was a problem hiding this comment.
A tap or click on the row's label makes source the label's <span> (rememberActivation stores the deepest target, :1263-1274; Item renders the label in a span inside its button). hold.element.contains(source) is true, so this calls focus() on a span that cannot take focus and focus is lost after the pop. Resolving the activation to its nearest focusable ancestor, or falling back to hold.focus() when source is not focusable, keeps the doc's promise that a pop returns focus to the control that pushed; a test that clicks the label text would catch it.
| win?.requestAnimationFrame(() => { | ||
| if ( | ||
| pendingHoldRef.current === pending && | ||
| pathRef.current === before | ||
| ) { | ||
| pendingHoldRef.current = null; | ||
| pending.hold.end(true); | ||
| } |
There was a problem hiding this comment.
pathRef.current is reassigned on every render (:1158-1159), so an app that refuses the push and re-renders with a new but equal array before the next frame fails this check; with unchanged keys no transition runs (:670-703), and the row stays held under no screen until the next push. The declined-pop spring-back at :1367-1373 has the same check. Comparing whether the pending key became the top screen (or length and keys) avoids the identity dependence.
| // 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.
spec:AST-100 FR2 makes the release velocity 0 after the contact stays still and says no component reads another estimate, FR3 forbids deriving a second velocity, and architecture:interaction-modality INV10 says no Lab component keeps its own sample buffer. This mean of the last three pan velocities ("A pause keeps it") lets a finger that rests and then lifts still fling the pop. Comparing the recognizer's own release velocity at the lift would conform; otherwise the record needs amending first.
| 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 so far." Here shouldFinishPop(0.9, -0.5) is 0.9 − 0.175 = 0.725 > 0.5, so a swipe released moving back from 90 % still pops. Cancelling whenever the velocity points back, with [0.9, -0.5, false] in the test table, would conform.
…Core's internal entries Main's internal/touch, its test, the touch-support bundle test, the internal-entries test, sync-exports and the exports map are kept exactly; this branch adds only what main lacks: internal/motion, the named re-exports Lab builds on (the pan recognizer, the gesture types, the touch presentation queries), their tests and the COVERED list. # Conflicts: # packages/core/package.json # packages/core/src/internal/touch.test.ts # packages/core/src/internal/touch.ts # scripts/core-internal-entries.test.mjs # scripts/sync-exports.js # scripts/touch-support-bundle.test.mjs
…xperimental touch exports The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
…on/README.md) into facebook#8309
…ntries and internal/motion Main's resolveSnapPoint test (meta#8663) keeps the release policy internal: not on the package root, not in a barrel, and no package export naming motion. The last check also caught ./internal/motion, Core's internal entry for Astryx's own packages (architecture:react-component-runtime INV13), which re-exports no release policy. The export check now covers the public keys only, and a new assertion keeps the claim as strong: internal/motion must not re-export resolveSnapPoint either. On this branch with main the test failed on ./internal/motion; it now passes, and it fails when internal/motion re-exports resolveSnapPoint.
…on drives Under load, in a frame that comes a few milliseconds late, WebKit evaluates two layers that animate on one clock about a millisecond apart, their boxes and their computed translates alike, so two layers read in one frame disagree by a pixel or two while the screen shows them together. "a back swipe holds the revealed bar still..." held the top bar to its card within 1 px through the release's spring back as well, and failed there (1.03 px; 2 of 10 runs at 6 workers). traceBars now reads each layer's computed translate (at rest a layer's box is its translate, checked when the trace starts) and records whether an animation drives the frame; the proof keeps 1 px while the finger drives both layers and takes the other bar proofs' 1 % of the width through the release.
…e demoted The story's own first motion leaves its layers promoted for the animator's linger, half a second after that motion ends. The push started about 100 ms after it, so the root bar title's demotion (a style pass and one Layout) came 386-400 ms into the push, at the 380 ms edge of the counted window, and inside it when the push's first frame came late under load (373 ms; 2 of 24 runs at 8 workers). The proof now waits until no layer is promoted before it clicks, so the window holds only the push's own work.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers CH-01 (the list row kept selected under a pushed NavigationStack screen) or what ships with it (divider hiding beside a pressed or held List row, isRtlElement on @astryxdesign/core/hooks, and the stack's touch presentation with the springs and deselect timing it plays outside the spec:AST-094 FR4 motion roles), so an engineering owner needs to provide or identify the governing spec, or record the decision 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:
- Return focus to the row after a finger or mouse pop: a tap or click on a row's label remembers the label's span, which cannot take focus.
- Read the back swipe's release velocity from the recognizer at the lift (
spec:AST-100FR2/FR3) and cancel any release moving back toward the start (spec:AST-094FR37). - Release a refused push's hold when the app re-renders with an equal
pathbefore the next frame. - Land the predecessor pull requests first, or retarget, so the diff is the CH-01 change rather than 34,263 authored lines.
- Keep one implementation of the selection hold, route the animator through
internal/touchasarchitecture:react-component-runtimeINV13 names it, name the external system in the two changed Effects, and correct the[fix]tag of thelist-row-press-compositorChangeset.
[Automated review]
| if ( | ||
| source instanceof HTMLElement && | ||
| source.isConnected && | ||
| hold.element.contains(source) | ||
| ) { | ||
| source.focus(focusOptions); |
There was a problem hiding this comment.
A tap or click on the row's label makes source the label's <span>, which the row contains, so this focuses an element that cannot take focus and focus is left on the page body (measured in Chromium after a tap and after a mouse click). Resolve the activation to its nearest focusable ancestor when it is remembered, or fall back to hold.focus() when source cannot take focus, and add a test that clicks the label text.
| // 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 keeps a Lab sample buffer and releases on the mean of the last three pan velocities, so a finger that rests before lifting still flings the pop. spec:AST-100 FR2/FR3 and architecture:interaction-modality INV10 make the recognizer's release velocity the only one a component reads (0 after a still pause); compare that value instead.
| export function shouldFinishPop( | ||
| progress: number, | ||
| velocityWidthsPerSecond: number, | ||
| ): boolean { | ||
| return ( | ||
| progress + | ||
| navigation.interactivePopFinishProjection * velocityWidthsPerSecond > | ||
| navigation.interactivePopFinishThreshold | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
shouldFinishPop(0.9, -0.5) is 0.725, so a release moving back toward the start from 90 % still pops; spec:AST-094 FR37 says a release whose velocity points back MUST cancel, whatever the travel. Cancel for any velocity pointing back and add [0.9, -0.5, false] to the table.
| win?.requestAnimationFrame(() => { | ||
| if ( | ||
| pendingHoldRef.current === pending && | ||
| pathRef.current === before | ||
| ) { | ||
| pendingHoldRef.current = null; | ||
| pending.hold.end(true); | ||
| } | ||
| }); |
There was a problem hiding this comment.
pathRef.current is reassigned on every render, so an app that refuses the push and re-renders with a new, equal path array before this frame fails the check, and the row stays held with no screen over it (a scratch test with path={[]} and a state update in onPathChange kept it held). Decide from whether the pending key became the top screen instead.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
|
Posted by Cindy's agent on her behalf. Thanks. Cindy decided that keeping a tapped row selected is the product's call through ListItem's existing |
|
Closing per Cindy's decision: a row's selection is the product's call through ListItem's |
Depends on #8108 and #8082. Review only this PR's change.
User need
What led here: we are bringing Astryx's touch components to iOS quality on the mobile web. On iOS, the row you tapped stays selected under the pushed screen and fades as you come back, following a back swipe. That fade shows where you came from; without it, a long list loses your place.
Problem and solution fit
The stack holds the pushed row's selection (Item, ListItem or TreeList) from the tap's own task. A pop deselects it over UIKit's 0.35 s, on the cards' clock; a back swipe scrubs it with the finger and finishes or restores it on release; a pop to root deselects the row in the screen it returns to. Under Reduce Motion it is a 200 ms fade.
Current specification
The hold mirrors #8082's selection hold. Core's internal touch entry does not export it yet, so the stack keeps a copy (
selectionHold.ts) until it does. Proposed, for the owner's review.Public delta and caller burden
No API change: rows that push get this under a finger. Size (gzip JavaScript of
NavigationStack's import, base → head): 59,047 → 60,419 B.Design and alternatives
Evidence
iOS 26.2 simulator: held under a push, deselected on the pop, a back swipe scrubs it, a cancelled swipe selects it again, pop to root, a 600 ms block during the pop. Chromium: Reduce Motion. Android 15 emulator: push and pop, a back swipe.
Scope
Testing
In Chrome's device toolbar (a phone, touch), "Lab/NavigationStack › Settings":
What I ran: the unit tests and the browser proofs (Chromium and WebKit, including
ListSelectionHold) passed. With the stack's hold stubbed out, 4 of 6 unit and 3 of 6 Chromium push and pop cases fail. Atest-uired inCodeBlockHeaderPlugin.test.tsxis a rich-text race that #8569 fixes, not this change.Takes #8108's latest test fix at its next push, or when #8108 merges; it does not merge before that. Recorded on the iOS 26.2 simulator and the Android 15 emulator; a real-device check is the release gate. Includes #8562 (Core's internal entry points); if those land differently, this follows.