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
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNew ComponentsSearchField (@astryxdesign/lab) · View in Storybook
Modified ComponentsItem (@astryxdesign/core) · View in Storybook
TreeList (@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. 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 | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval has to wait. The new search prop on NavigationScreen and the draft spec:AST-094 requirements it builds on (FR1–FR23, DEC-1, DEC-2) have no current spec authority, so an engineering owner needs to approve or reject them by commenting /approve-spec <full head SHA> on this pull request; the author can record that decision if they are an engineering owner. Whatever that decision is, these changes are needed:
- Land or retarget the predecessor pull requests so this diff is only its own ~1,050-line commit: against
mainit is 32,244 authored lines, including an unannounced change topresentation="adaptive"on five released core components. - Popping a screen while its search is active (the content back swipe stays enabled) leaves its key in
searchingRef, so pushing it again hides its back button and bar background; disable the back swipes during search, and clear or replay the search state when screens leave or reach the top. - Read the active state from the
SearchFieldinstead of a secondisActive, keepsetLiftsout of the search layout effect, run the interaction proofs inside an existing CI owner rather than a newpr-interactionlane (AST-030 FR11), and fix AST-094's citation of the nonexistentspec:AST-090.
[Automated review]
| ), | ||
| }).requireToFail(edge), | ||
| recognizer => onSwipe(recognizer, CONTENT_DEAD_ZONE_PX), | ||
| {isEnabled: canPop && isInteractivePopEnabled}, |
There was a problem hiding this comment.
The content back swipe stays enabled while the top screen's search.isActive is true, and a pop never runs runSearch(key, false). The popped key stays in searchingRef, heldCollapsesRef and searchRestRef, so pushing that screen again leaves its back button and bar items at opacity 0 (still focusable), fades the bar background to 0 and swallows its large-title crossings. Consider disabling both back swipes during search and clearing a key's search records when it leaves view.models.
| } | ||
| return; | ||
| } | ||
| if (previous.key !== topKeyNow) { |
There was a problem hiding this comment.
A screen that becomes top with search.isActive already true is never handed the bar: this branch only fades the background. Its large title and content stay put while the SearchField places itself lifted on its first layout. Running runSearch(topKeyNow, true, true) here when the new top is searching would line them up.
| if (lift == null) { | ||
| lift = measureLift(key) ?? 0; | ||
| storeLift(key, lift); | ||
| } |
There was a problem hiding this comment.
When isActive flips without a focus inside the field (as the Toggle search test does), the lift is measured and stored here, inside the search layout effect. The activating commit renders the field with lift 0, and a second render repairs it, which architecture:react-component-runtime INV1/INV3 rule out unless the effect names what it synchronizes. Could the field measure its own lift as it activates, or the measurement stay in the event path?
| */ | ||
| field: ReactNode; | ||
| /** Whether search is active: the field's controlled `isActive`. */ | ||
| isActive: boolean; |
There was a problem hiding this comment.
isActive repeats the SearchField's own controlled isActive, and nothing checks that the two agree; a mismatch renders a half hand-over (AST-002 FR15/FR16). The stack already clones field to inject lift; could it read or inject the active state instead?
| 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, so this amendment has nothing to amend. Cite the record that owns exit motion today, or drop the amendment.
| '@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.
This changes the released result of presentation="adaptive" on five components (landscape phones get the sheet; 744–768px coarse screens get the popover). AST-017 FR3 asks for the change to be judged under FR1 and for the changeset to name the option that restores the old default. It also looks like it belongs in its own pull request rather than this one.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This adds the interaction proofs as their own self-routed lane, with its own scope pattern. AST-030 FR11 asks new coverage to join an existing surface owner rather than create a separately routed lane, and FR10 admits a new surface only through an owner-approved amendment to AST-030. Could the proofs run inside an existing test owner, or come with that amendment in the harness's own pull request?
| onSubmit, | ||
| handleRef, | ||
| lift = 0, | ||
| cancelLabel = 'Cancel', |
There was a problem hiding this comment.
These defaults become the close and clear buttons' aria-label, so every locale hears English. The internationalization architecture (INV1a) asks for Astryx-owned assistive strings to be en.json keys resolved by the translator, as NavigationStack does for its Back label. The props can stay as overrides.
0597b32 to
db79dd5
Compare
There was a problem hiding this comment.
Approval still has to wait on an engineering owner. No current spec governs the new search prop on NavigationScreen (or NavigationStack and SearchField), so an owner needs to provide or identify that spec, and to approve or reject the draft spec:AST-094 requirements it builds on (FR1–FR23, DEC-1, DEC-2), recording the decision by commenting /approve-spec <full head SHA> on this pull request; the author may record it if they are an engineering owner. Whatever that decision is, these changes are still needed:
- Land or retarget the predecessor pull requests so this diff is only its own commit of about 1,260 lines; against
mainit is about 32,800 authored lines, including an unannounced change topresentation="adaptive"on five released core components. - A screen popped while its search is active keeps its search state, so pushing it again leaves the field in the bar with the large title and rows behind it and no back button (the Search story's Done row reproduces it); clear or replay that state when screens leave or reach the top.
- Read the active state from the
SearchFieldinstead of a secondisActive, keepsetLiftsout of the search layout effect, run the interaction proofs inside an existing CI owner (AST-030 FR11), back SearchField's button labels with catalog strings, and fix AST-094's citation of the nonexistentspec:AST-090.
[Automated review]
| <ListItem label={`${title} has nothing deeper.`} /> | ||
| ) : null} | ||
| {rows.length === 0 ? ( | ||
| <ListItem label="Done" onClick={() => nav.popToRoot()} /> |
There was a problem hiding this comment.
In the Search story this row pops to the root while the screen's search can still be active, and nothing clears that screen's search records on a pop (NavigationStack.tsx:1634-1639). Pushing the screen again brings its field back lifted into the bar while the large title and rows stay below, with no back button or bar background until Cancel. Clearing a key's records when it leaves the stack, or replaying the hand-over for a screen that becomes top already searching, would fix it.
db79dd5 to
333d1d7
Compare
There was a problem hiding this comment.
Approval has to wait on an engineering owner: no current spec governs the NavigationStack search hand-over or themable touch motion, and the draft spec:AST-094 they build on (FR1–FR23, DEC-1, DEC-2) needs approval or rejection, including whether touch motion stays out of themes (its FR4 and DEC-1) or follows the new defineTheme motion roles. 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 changes are still needed:
- Land or retarget the predecessor pull requests so this diff is only its own commit of about 1,300 lines; against
mainit is about 35,500 authored lines, including an unannouncedpresentation="adaptive"change on five released core components and a self-routed CI lane. - A screen popped while its search is active keeps its search state, so pushing it again leaves the field in the bar with the large title and rows behind it and no back button (the Search story's Done row reproduces it).
- Replace the
searchconfiguration object and its secondisActive, whichspec:AST-056DEC-2 and FR6 rule out, and bring the motion roles in line with the theme-token, adaptation-condition and experimental-surface rules, or have those records' owners amend them. - Keep
setLiftsout of the search layout effect, back SearchField's button labels with catalog strings, limit the scroller's overscroll containment to overflowing axes, regenerate the stale CLI golden that failstest-node, and fix AST-094's citation of the nonexistentspec:AST-090.
[Automated review]
| } | ||
| return; | ||
| } | ||
| if (previous.key !== topKeyNow) { |
There was a problem hiding this comment.
When the top screen changes, this branch only fades the bar background. A screen popped mid-search (the Search story's Done row, or a back swipe on iOS) keeps its key in searchingRef, heldCollapsesRef, searchRestRef and lifts, and pushing it again leaves its field lifted into the bar while the large title and rows stay below, with no back button until Cancel. Clearing a key's records when it leaves view.models, and running runSearch(key, true, true) for a screen that becomes top already searching, would cover both directions.
| * the titles and the bar items fade on the field's activation clock, and | ||
| * the content rides up with the field into the bar. | ||
| */ | ||
| search?: NavigationSearch; |
There was a problem hiding this comment.
spec:AST-056 DEC-2 ("No search configuration object") and FR6 rule out an object-valued search prop, and isActive here restates the SearchField's own controlled isActive with nothing reconciling the two (AST-002 FR15/FR16). Could the stack take the field as a slot and read its active state from it?
| : null; | ||
| if (lift == null) { | ||
| lift = measureLift(key) ?? 0; | ||
| storeLift(key, lift); |
There was a problem hiding this comment.
When isActive flips without a focus inside the field, the lift is measured and stored here, inside the search layout effect, so the activating commit renders the field with lift 0 and a second render repairs it (architecture:react-component-runtime INV1/INV3). Measuring where isActive flips, or keeping the lift out of React state, would avoid the repair.
| measures (0.55 for a scroll view and a sheet), and a release MUST pick its | ||
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch |
There was a problem hiding this comment.
FR4 says touch presentations read their motion from the table on every theme, and DEC-1 rejects tuning touch motion per theme, but this pull request adds defineTheme({motion: {springs, durations, curves}}) motion roles that retune exactly that, including the search clock. One of the two needs to change before the record can be approved.
| 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, so this amendment has nothing to amend. Cite the record that owns exit motion today, or drop the amendment (the Current-state impact list repeats it).
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
A platform condition widens the adaptation-condition vocabulary that architecture:theme-authoring-contract keeps fixed, and the motion roles beside it write --motion-* names that do not originate in tokens.stylex.ts (architecture:theme-tokens INV1). Both need those records' owners, or a shape that fits them.
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
This changes the released result of presentation="adaptive" on five components (landscape phones get the sheet; 744–768px coarse screens get the popover), while component:Selector DEC-1 records that adaptive keeps its width-and-pointer query and the changeset names no AST-017 FR3 restoring option. It also looks separable from the search work.
| if git fetch --quiet --no-tags --depth=1 origin "$BASE_SHA" \ | ||
| && git show "$BASE_SHA:.github/scripts/pr-scope-touches.sh" > "$helper"; then | ||
| scope=0 | ||
| bash "$helper" "$BASE_SHA" '^(internal/interaction-harness/|playwright\.interaction\.config\.ts$|packages/[^/]+/src/|apps/storybook/)' || scope=$? |
There was a problem hiding this comment.
This makes the interaction proofs their own self-routed lane over existing surfaces. AST-030 FR11 asks new coverage to join an existing surface owner, and FR10 admits a new surface only through an owner-approved amendment. Could the proofs run inside an existing test owner?
| onSubmit, | ||
| handleRef, | ||
| lift = 0, | ||
| cancelLabel = 'Cancel', |
There was a problem hiding this comment.
These defaults become the close and clear buttons' aria-label, so every locale hears English. architecture:internationalization INV1a asks for Astryx-owned assistive strings to be en.json keys resolved by the translator, as NavigationStack does for its Back label; the props can stay as overrides.
| paddingTop: BAR_HEIGHT, | ||
| scrollPaddingTop: BAR_HEIGHT, | ||
| overflowY: 'auto', | ||
| overscrollBehavior: 'contain', |
There was a problem hiding this comment.
overscrollBehavior: 'contain' is active even when the screen does not overflow, which spec:AST-025 FR14 rules out (containment only on axes that currently overflow); a short screen inside a scrolling page becomes a dead scroll zone.
333d1d7 to
733dc0a
Compare
There was a problem hiding this comment.
Approval has to wait on an engineering owner: no current spec governs the NavigationStack search hand-over or themable touch motion, and draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2) needs approval or rejection, including whether touch motion stays out of themes (its FR4 and DEC-1) or follows the new defineTheme motion roles. 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; whatever the decision, these changes are still needed:
- Land or retarget the predecessor pull requests so this diff is only its own commit of about 1,360 lines; against
mainit is about 39,500 authored lines, including unannounced changes topresentation="adaptive"on five released components and a new keyboard-riding default forChatLayout. - A screen left while its search is active keeps its search state, so pushing it again shows the field in the bar with the large title and rows behind it and no back button (the Search story's Done row reaches it).
- On Chrome for Android the field's keyboard overlay leaves the end of a long screen under the keyboard with no room to scroll it into view, and
search.isActiverestates the field's own state with no check. - Bring the motion roles in line with the theme-token, adaptation-condition and experimental-surface rules, keep state out of the search layout effect, move the interaction proofs into an existing CI owner, back SearchField's button labels with catalog strings, and fix AST-094's citation of a nonexistent
spec:AST-090.
[Automated review]
| if (previous.key !== topKeyNow) { | ||
| barBackground.animator.animate( | ||
| {opacity: searchingRef.current.has(topKeyNow) ? 0 : 1}, | ||
| {spring: springs.navigationBarElements}, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
When the top screen changes, this branch only fades the bar background. A screen left while its search is active (the Search story's Done row, or a back swipe on iOS, both still enabled during search) keeps its key in searchingRef, so pushing it again sends its back button and bar items to opacity 0 while runSearch never lifts its large title and rows; with the story's active entry still true, the field places itself in the bar's row above its own large title until Cancel. Run runSearch(key, true, true) for a screen that becomes top already searching, and clear the records of a key that leaves view.models.
| const finish = () => { | ||
| searchingRef.current.delete(key); | ||
| freshLiftsRef.current.delete(key); | ||
| searchRestRef.current.delete(key); | ||
| releaseCollapses(key); | ||
| }; |
There was a problem hiding this comment.
This is the only place the per-screen search records are cleared, and it runs only at the end of a cancel on the top screen. A pop never reaches it, so searchingRef, heldCollapsesRef, searchRestRef, freshLiftsRef and lifts keep a popped key: if the app ends search on pop, the re-pushed screen's back button stays invisible but focusable and its large-title crossings are swallowed.
| releaseOverlayRef.current = | ||
| editable != null && win != null | ||
| ? getKeyboardLayoutGuide(win).overlayWhileFocused(editable) | ||
| : null; |
There was a problem hiding this comment.
With the overlay set, Chrome on Android stops resizing and panning the page for the keyboard, but nothing in the stack pads the screen's scroller by the keyboard's inset (ChatLayout pairs the same overlay with a spacer of the keyboard's height). The end of a long screen, about the keyboard's 312 px, stays under the keyboard and cannot be scrolled above it until typing ends. Pad the scroller by the guide's inset while it holds the overlay, or leave the overlay out.
| */ | ||
| field: ReactNode; | ||
| /** Whether search is active: the field's controlled `isActive`. */ | ||
| isActive: boolean; |
There was a problem hiding this comment.
isActive restates the SearchField's own controlled isActive, and nothing checks that the two agree: with this false while the field is active, the field rises into a bar that keeps its background and controls and paints over it; with this true while the field is inactive, the bar's controls fade and turn inert while the field stays put. AST-002 FR15 asks for invalid combinations to be unrepresentable or warned about; letting the stack learn the active state from the field would leave field and hidesWhenScrolling here.
| if (lift == null) { | ||
| lift = measureLift(key) ?? 0; | ||
| storeLift(key, lift); | ||
| } |
There was a problem hiding this comment.
When isActive flips without a focus inside the field (a first layout that starts active, or a programmatic toggle), the lift is measured and stored here, inside the search layout effect, so the commit that activates hands the field the wrong lift and a follow-up render repairs it. react-component-runtime INV1 and INV3 ask an effect that sets state to name the external system or DOM measurement it derives from; measuring in the activating event, or naming the measurement, would satisfy it.
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
A platform condition widens the adaptation-condition vocabulary that architecture:theme-authoring-contract keeps fixed, and the motion roles it is meant for write --motion-<role> variables whose names do not originate in tokens.stylex.ts (theme-tokens INV1). Draft AST-094 FR4 and DEC-1 in this same pull request also rule out tuning touch motion per theme, so an owner has to choose one.
| if (registered !== undefined && registered.kind !== kind) { | ||
| throw new Error( | ||
| `${location}: "${id}" is a ${registered.kind}; set it under the matching group.`, | ||
| ); | ||
| } | ||
| const variable = motionRoleVariable(id); |
There was a problem hiding this comment.
A well-formed role id that no motion table registers passes this check and is written as a variable nothing reads, so a typo or a role a table later renames silently does nothing. Reject unregistered ids with the same named error the wrong-kind case gets.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This adds the interaction proofs as their own job with its own scope check. AST-030 FR11 says new coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context; running these proofs inside the existing UI test owner would conform.
| cancelLabel = 'Cancel', | ||
| clearLabel = 'Clear text', |
There was a problem hiding this comment.
These defaults become the close and clear buttons' accessible names, so every locale hears English. internationalization INV1a asks for Astryx-owned visible and assistive strings to be catalog keys resolved by the translator, as @astryx.navigationStack.back already is for the stack's back button.
| - **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 | ||
| (reduced motion closes at once) for touch presentations (owners: ernestt, | ||
| cixzhang). |
There was a problem hiding this comment.
There is no spec:AST-090 on main or at this head, so this proposed amendment has nothing to amend. Cite the record that owns exit motion today, or drop the amendment.
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
733dc0a to
33c0fde
Compare
github-actions
Bot
added
the
needs:design-review
…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.
…st's cadence Under load the screencast delivers frames at its own rate: 13 in the judged window where the 60 Hz count wanted at least 15, while the title changed on every frame delivered. The proof now counts against the cast's measured cadence, the median interval between the frames it delivered in the window (never faster than 60 Hz); a frozen title delivers none there and still fails.
The bases merged with none of this PR's own change, so a compare from here shows only that change. The bases both add to the shared motion notes, the UIKit table's entry header and the en catalog; each keeps both sides' entries. Both re-export useAnimator from Core's internal motion entry; it keeps one, meta#8181's, with its type.
…wn start Under load the trace's round trip could begin well after the rise had started (38 % up at its first frame), so "150 ms into the rise" landed about 190 ms in, at 84 % of the rise, and the bar's carried-on turn passed the 85 % bound (92 %; once in two WebKit runs). The sampler now times the cancel from the rise animation's start; when a busy runner begins the trace too late for three frames before the cancel point, the bar is let back down and the activation tried again, up to three times.
…slates, the Layout/Paint proof waits for demotion, the handover counts at the cast's cadence) into the bar look's bases
…) into the NavigationStack bar look # Conflicts: # packages/core/src/AppShell/AppShell.tsx # packages/core/src/AppShell/AppShellScrollEdge.test.tsx # packages/core/src/hooks/useScrollEdgeEffect.ts # packages/core/src/motion/useScrollEdgeEffect.test.tsx
There was a problem hiding this comment.
Approval has to wait on an engineering owner: no current spec governs the NavigationStack search hand-over, the Lab NavigationStack and SearchField touch presentations it builds on (including the clocks and springs they play as internal constants), or the new isRtlElement export from @astryxdesign/core/hooks. An engineering owner can approve or reject those additions by commenting /approve-spec <full head SHA> on this pull request, and the author may record it as an engineering owner. Whatever that decision, these still need changes:
- The diff carries the whole unmerged stack, about 41,300 authored lines; landing or retargeting the predecessors would leave the hand-over's own commits, the List divider and build-plugin changes belong in their own pull requests, and the selection hold and
isRtlElementeach need one copy instead of two. - A SearchField passed without a controlled
isActiverises into the bar's row under a bar that never steps aside, leaving Cancel out of a finger's reach (NavigationStack.tsx:831-839). - Returning to a screen whose search is still active, after pushing from its results, leaves the covered screen's opaque bar over the field, because nothing hides it again after the transition (
NavigationStack.tsx:2092-2103). - The back swipe averages its own last three velocities, so a finger that rests before lifting can still finish the pop, which AST-100 FR2 rules out (
NavigationStackTouch.ts:171-183); the fine-pointer press-tracker listeners inGestureHost.ts:199-212go beyond what AST-094 FR19 allows, and the layout effect atNavigationStack.tsx:2040-2064should not set state.
[Automated review]
| /** Whether a screen's search field is active: the field's own `isActive`. */ | ||
| function isSearchFieldActive( | ||
| props: NavigationScreenProps | undefined, | ||
| ): boolean { | ||
| const field = props?.search; | ||
| return ( | ||
| isValidElement<{isActive?: boolean}>(field) && field.props.isActive === true | ||
| ); | ||
| } |
There was a problem hiding this comment.
The stack follows only the element's isActive prop, but SearchField activates itself on focus when it is uncontrolled (defaultIsActive), and the focusin handler still stores a lift for it. A <SearchField label="Search" /> therefore rises into the bar's row under a bar that keeps its background, back button and items, so Cancel is out of a finger's reach (AST-002 FR15: do not silently render a broken state). Follow the field's own state (for example, wrap its onActiveChange the way lift is injected), or make isActive required in the search type and inject lift only into a controlled field.
| if (previous.key !== topKeyNow) { | ||
| if (isTopSearching && !searchingRef.current.has(topKeyNow)) { | ||
| // A screen that becomes top already searching (pushed again with | ||
| // its field active) gets the bar at once. | ||
| runSearch(topKeyNow, true, true); | ||
| return; | ||
| } | ||
| barBackground.animator.animate( | ||
| {opacity: searchingRef.current.has(topKeyNow) ? 0 : 1}, | ||
| {spring: springs.navigationBarElements}, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
A screen that becomes the top while still searching only fades the shared bar background here. showCoveredBar() already ran when the transition started and cleared hiddenBarRef, so the covered screen's freshly mounted bar layer (opaque, at rest) stays over the field. In the Search story: General, focus its field, About, then back. Call hideCoveredBar() in this branch when searchingRef holds the new top.
| useLayoutEffect(() => { | ||
| const present = new Set(view.models.map(model => model.key)); | ||
| const forget = (records: Map<string, unknown> | Set<string>) => { | ||
| for (const key of [...records.keys()]) { | ||
| if (!present.has(key)) { | ||
| records.delete(key); | ||
| } | ||
| } | ||
| }; | ||
| forget(searchingRef.current); | ||
| forget(heldCollapsesRef.current); | ||
| forget(searchRestRef.current); | ||
| forget(freshLiftsRef.current); | ||
| setLifts(current => { | ||
| const gone = [...current.keys()].filter(key => !present.has(key)); | ||
| if (gone.length === 0) { | ||
| return current; | ||
| } | ||
| const next = new Map(current); | ||
| for (const key of gone) { | ||
| next.delete(key); | ||
| } | ||
| return next; | ||
| }); | ||
| }, [view.models]); |
There was a problem hiding this comment.
This layout effect sets state (setLifts) to drop the entries of screens gone from view.models, and its comment names no external system or DOM measurement (react-component-runtime INV1, INV3). Read lifts through the present keys during render, or prune where the path change is derived.
| const p = stack.progress(); | ||
| // 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.
The swipe keeps its own sample buffer and releases on the mean of the last three pan velocities, so a finger that rests before lifting still finishes the pop on earlier motion. AST-100 FR2 and FR3 and interaction-modality INV10 give one release velocity from core's tracker, 0 after a still pause, with no Lab buffer or filter. Pass the pan recognizer's release velocity at ended to shouldFinishPop and drop interactivePopVelocitySamples.
| /** | ||
| * The recognizer, or the press tracker, reads each event's pointer itself, | ||
| * so it attaches even where the primary pointer is fine, with no warning. | ||
| * A recognizer gets the host's listeners and its element's style there, as | ||
| * `spec:AST-057`'s row swipe does. A press tracker there gets only passive | ||
| * `touchstart` and `pointerdown` listeners on its own element, and no | ||
| * style; the host's document and window listeners come for a finger's | ||
| * press on it and go when the press ends. It paints that press's feedback | ||
| * only (an FR19 exception for press feedback only, pending in | ||
| * `spec:AST-094`'s amendment): no control event, nothing kept from the | ||
| * browser, which still pans, and no mouse or pen press. Everything else | ||
| * stays inactive there. @default false | ||
| */ | ||
| perEventPointer?: boolean; |
There was a problem hiding this comment.
A perEventPointer press tracker gets passive touchstart and pointerdown listeners where the primary pointer is fine, which AST-094 FR19 ("no touch listener is registered") and FR24 (only module:DropdownMenu/useMenuPress is excepted) rule out, and no component passes it. Remove the press-tracker carve-out until AST-094 is amended; the AST-057 recognizer carve-out FR19 names can stay.
| * @position Lab-internal, not exported from `@astryxdesign/lab`: lab's own | ||
| * direction check, used by SearchField's touch layer. Core's helper of the | ||
| * same name is not among core's published exports, so lab keeps its own |
There was a problem hiding this comment.
Core exports isRtlElement from @astryxdesign/core/hooks in this same diff, and NavigationStack already imports it from there, so this header is stale and Lab carries a second copy; keep one.
|
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. |
Depends on #8185 and #8181. 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, tapping a list's search field lifts it into the navigation bar while the bar steps aside. SearchField (#8181) moves itself; it needs a host that hands over the bar.
Problem and solution fit
NavigationScreengainssearch(a SearchField element) andhidesSearchWhenScrolling(default true). The field sits under the large title and scrolls with the content. The stack follows the field's ownisActive: it measures how far the field must rise (lift), and on the field's clock the back button and bar items fade, the large title rides up fading, and the content rises with it. A change of mind turns from where things are; Cancel brings everything back. While searching, bar controls are inert and back swipes are off. Under a mouse, every part moves at once.Current specification
Proposed, for the owner's review; the seam follows AST-056 DEC-2 (the stack reads the field element's own state).
Public delta and caller burden
searchandhidesSearchWhenScrollingonNavigationScreen, replacing the earliersearch: {field, isActive, hidesWhenScrolling}object and itsNavigationSearchtype. The bar now overlays the screens, which scroll under it; nothing changes at rest.Design and alternatives
Evidence
iOS 26.2 simulator: activate, then Cancel, on a pushed screen, a field that scrolls away, a 600 ms block inside the rise. Chromium: cancel mid-activation, Reduce Motion. Android 15 emulator: activate, then Cancel.
Scope
Testing
In Chrome's device toolbar (a phone, touch), "Lab/NavigationStack › Search":
What I ran: the unit tests, the browser proofs (Chromium and WebKit), the registry contract and the sandbox build passed. Four review findings have tests that failed before their fixes (for example, a screen left mid-search and pushed again starts at rest). The cancel proof times its cancel from the rise's own start and retries only when too few frames were captured before it.
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.