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
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: 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 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: the lab SearchField and the draft spec:AST-094 it relies on have no approved spec yet, so an owner needs to approve that draft as SearchField's governing spec (including whether FR8's Reduce Motion cross-fade or SearchField's current kept travel applies), and the owners of the records it proposes to amend (theme tokens, user states, the buttons family, AST-025) need to update them. An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request; the author may do so if they are an engineering owner. Independently of that decision, these changes are needed:
- Land the stacked predecessors first or retarget: the diff carries 28,312 authored lines, about 26,000 of them from #8052–#8180.
- Keep the released
presentation="adaptive"query here, or move the resolver change to its own pull request with the Selector spec's DEC-1 amended; today it moves landscape phones to the bottom sheet and 744–768 px touch tablets to the popover. - The inactive close button sits 28 px past the field's trailing edge, so phone pages pan sideways and
overflow: autohosts gain a horizontal scrollbar; clip the root's inline overflow or take the button out of layout once it has slid out. - Resolve the default
cancelLabelandclearLabelthrough the translator withen.jsonkeys, as other lab components do.
[Automated review]
| cancelLabel = 'Cancel', | ||
| clearLabel = 'Clear text', |
There was a problem hiding this comment.
These defaults become the close and clear buttons' aria-labels in every locale. Internationalization INV1a asks for an en.json key resolved by the translator; other lab components default through useTranslator() (for example t('@astryx.commentComposer.cancel')). Suggest @astryx.searchField.cancel and @astryx.searchField.clear, keeping the props as overrides.
There was a problem hiding this comment.
Fixed in 8f0233d: the default names are @astryx.searchField.cancel and @astryx.searchField.clear, resolved through the translator; the props still override.
| y: 0, | ||
| middleScale: 1, | ||
| endX: 0, | ||
| closeX: direction * search.stacked.closeTravel, |
There was a problem hiding this comment.
While inactive the close button rests 44 px past its place, which ends 28 px past the root's trailing edge. Transforms count toward scrollable overflow, so a 402 px phone page becomes 414 px wide and pans sideways, and a 200 px overflow: auto host gains a 28 px horizontal scroll. Clipping the root's inline overflow (overflowX: 'clip') or taking the button out of layout once it has slid out would fix it.
There was a problem hiding this comment.
Fixed in 8f0233d: the root clips its inline overflow. Proof: a full-width field leaves a phone page its width (442 px before, 414 after).
| // 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: landscape phones now get the bottom sheet and 744–768 px touch screens the popover. The Selector record's DEC-1 says adaptive keeps its width-and-pointer query; could this land with the resolver's own pull request, with that record amended, and keep the released query here?
There was a problem hiding this comment.
From #8062, stacked below; at this head its resolver takes compact from the theme's definition.
9400573 to
3fccb23
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: lab SearchField and the draft spec:AST-094 it relies on have no approved spec yet, so an owner needs to approve that draft as SearchField's governing spec (including whether FR8's Reduce Motion cross-fade or SearchField's kept travel applies), and the owners of the current records it proposes to amend (theme tokens, interaction modality, user states, the buttons family, AST-025) need to update them. An engineering owner records that decision by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. Independently of that decision, these changes are needed:
- Land the stacked predecessors first or retarget: the diff carries 28,312 authored lines, about 26,000 of them from #8043–#8180.
- Keep the released
presentation="adaptive"query here, or move the resolver change to its own pull request with the Selector spec's DEC-1 amended; today it moves landscape phones to the bottom sheet and 744–768 px touch tablets to the popover. - When a keyboard user cancels from the close button, focus stays on that button after it turns transparent and
aria-hidden; move focus to a visible control. - The inactive close button sits 28 px past the field's trailing edge, so phone pages pan sideways; clip the root's inline overflow or take the button out of layout once it has slid out.
- Resolve the default
cancelLabelandclearLabelthrough the translator, and run the interaction proofs in the existing UI test owner instead of the separately routedpr-interactionlane.
[Automated review]
| const cancel = useCallback(() => { | ||
| setValue(''); | ||
| inputRef.current?.blur(); | ||
| setActive(false); | ||
| }, [setValue, setActive]); |
There was a problem hiding this comment.
When the close button itself has keyboard focus, this blurs only the input: focus stays on the button while it turns aria-hidden, tabindex="-1" and opacity 0, so the focus ring is drawn on nothing (Tab to Cancel, then Enter, in the Default story). Moving focus to a visible control here (the searchbox without re-activating it, or the landmark) would keep keyboard and screen-reader users in place.
There was a problem hiding this comment.
Fixed in 8f0233d: a cancel from the focused close button, or Escape, leaves focus on the search landmark, which takes it only for that and lets it go once focus moves on.
|
|
||
| # The helper comes from the base, since an older PR head lacks it. | ||
| # Exit 1 means out of scope; 0 in scope; 2 unknown, so run. | ||
| - name: Check interaction scope |
There was a problem hiding this comment.
This step makes pr-interaction decide its own scope for component coverage, which spec:AST-030 FR11 rules out ("MUST NOT create a separately routed lane"). Could the interaction proofs run inside the existing UI test owner, or stay with the harness's own pull requests for its owners to decide?
There was a problem hiding this comment.
The pr-interaction lane comes from the interaction harness PRs stacked below, not this PR's commits.
3fccb23 to
19e87c4
Compare
19e87c4 to
7fcda17
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's spec decision before it can be approved: lab SearchField and the draft spec:AST-094 it builds on have no approved spec yet, so an engineering owner needs to approve or amend that draft as SearchField's governing spec, including whether FR8's Reduce Motion cross-fade or SearchField's kept travel applies. 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. Independently of that decision, these changes are needed:
- Land the stacked predecessors first or retarget, since the diff carries 28,316 authored lines, about 26,000 of them from #8043–#8180.
- Keep the released
presentation="adaptive"query here, or move the resolver change to its own pull request with the Selector spec's DEC-1 amended, because it moves landscape phones to the bottom sheet and 744–768 px touch screens to the popover. - When a keyboard user cancels from the close button, move focus to a visible control instead of leaving it on the button after it turns transparent and
aria-hidden. - Keep the inactive close button from sitting 28 px past the field, which makes phone pages pan sideways.
- Resolve the default
cancelLabelandclearLabelthrough the translator, and run the interaction proofs inside the existing UI test owner rather than a separately routed lane. - Get the
lintjob to finish: ESLint runs out of memory on every head of this branch while it passes onmain.
[Automated review]
There was a problem hiding this comment.
This pull request needs an engineering owner's spec decision before it can be approved: lab SearchField and the draft spec:AST-094 it builds on have no approved spec yet, so an engineering owner needs to approve or amend that draft as SearchField's governing spec, including whether FR8's Reduce Motion cross-fade or SearchField's kept travel applies. 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. The latest commit fixes the close button that a tap during a cancel used to lose; independently of the spec decision, these changes are still needed:
- Land the stacked predecessors first or retarget, since the diff carries 28,388 authored lines, about 26,000 of them from #8043–#8180.
- Keep the released
presentation="adaptive"query here, or move the resolver change to its own pull request with the Selector spec's DEC-1 amended, because it moves landscape phones to the bottom sheet, 744–768 px touch screens to the popover, and default sub-menus with them. - When a keyboard user cancels from the close button, move focus to a visible control instead of leaving it on the button after it turns transparent and
aria-hidden. - Keep the inactive close button from sitting 28 px past the field, which makes phone pages pan sideways.
- Resolve the default
cancelLabelandclearLabelthrough the translator, run the interaction proofs inside the existing UI test owner rather than a separately routed lane, and cite the record FR7 means to amend instead of the missingspec:AST-090.
[Automated review]
7c8afd9 to
5590da5
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the draft touch-presentations spec (AST-094): whether themes may retune touch motion, whether adaptations gain a platform condition, and how Reduce Motion behaves for SearchField. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner). Independent fixes are needed either way:
- Cancel leaves focus on the hidden close button; the resting close button widens a 402 px page by 12 px; the Cancel and Clear text labels need catalog keys.
- The 744 px compact edge contradicts current AST-012 FR9 (width below md, 768, or height below 500), so 744–767 px touch screens now get the popover instead of the sheet.
- The new interaction CI job is separately routed and not joined to
test, the spec cites an AST-090 that does not exist, and the 31,607-line change should be split into its stacked parts.
[Automated review]
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.
…task ended at once, however late the first read WebKit under load from other tasks: the call's task ends at once, another task holds the main thread, and the browser commits the animation after it, dated at the call. Nothing in the call's task delayed it, but the first read still moved its start to the read, about 95 ms late on a loaded box, so whatever chains on its end (the second half of a Reduce Motion cross-fade, a cancel fading on the previous fade's end) ran that much late and the cross-fade missed design:reduced-motion DR2's 200 ms. Fails now: the start moves to 1100.
e7b0676 moved a call's start from rest to its first read whenever WebKit had dated it more than two frames before that read. Under load from other tasks the first read comes late with nothing in the call's task delaying it, so the start moved about 95 ms on a loaded box, and whatever chained on the call's end ran that late: the second half of a Reduce Motion cross-fade, so SearchField's cancel missed DR2's 200 ms. The call's task now records when it ended, at its first microtask (or a read in those microtasks is that time), and only a call whose own task ran past the call by more than two frames, the block e7b0676 fixed, moves its start; a read other tasks delay keeps the browser's start.
…s its start past the block Core Animation dates an animation at the commit, when the calling task ends, however short the block before it. A 20 ms block in the call's task, with WebKit's start dated at the call, left the start at the call: the two-frame bound skipped it. Fails now.
… task, with no threshold Core Animation dates an animation at the commit, when the calling task ends, whatever the block's length. The call's task already records when it ended, at its first microtask, so a start the browser dated before that (WebKit's last frame time before the task) now moves to it: a task that ended at once leaves the start at the call, a blocked one moves it past the block, however short, and a read other tasks delay changes nothing. The two-frame bound, which told a read's own frame from a block before the task's end was known, goes, and the start no longer moves to the read.
…few ms, and one task's calls start together Browser start times are frame times, while a task's end is a live clock, so on every call they differ by up to a frame plus the task's own length. Moving every start from rest to its task's end shifted each one by that length, about 5 ms in a component's effect after a tap, and the shift added up along a chained sequence: SearchField's Reduce Motion cancel measured 204-207 ms against design:reduced-motion DR2's 200. The first case fails now (the start moves to 1010). The second keeps one motion's parts together when they are separate calls in one task: a block measured per call would split them by a frame.
…n on more than two frames, and all the task's calls move together fc42d64 moved every start from rest to the end of the call's task. Browser start times are frame times while that end is a live clock, so on every call they differ by up to a frame plus the task's own length: each start slid by that length, and a chained sequence added it up, about 5 ms per link in SearchField. The rule from e6876ca is back, with its bound now named for that quantization: only a task that ran on more than LATE_START_MS (two frames) past its first call blocked anything, and its calls' starts move to their first read. The block is now measured per task, not per call, and every call of a blocked task takes the same read, so one motion's parts that are separate calls in one task (a component's effect) start together instead of splitting by a frame. The 20 ms case keeps the browser's start, as no clock here tells it from quantization.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec:AST-094 FR4: SearchField plays its 383.3 ms activation from the search.activationMs table constant, while FR4 requires every touch duration to be a named motion role, so FR4 needs that role (the row proposed in the conversation) or a ruling that the constant meets it. An engineering owner records the decision by amending FR4 on main or by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, these still need changes:
- Land #8076, #8180, #8340, #8360 and #8562 through their own reviews and rebase, so this pull request carries only SearchField's own 3,596 lines (31,859 authored lines today), then split or justify that increment.
- Re-place the field's parts when a move that a resize interrupted finishes (
SearchFieldTouch.ts:228-232), with a test that resizes mid-activation. - Drop the story's import of the unexported
@astryxdesign/core/utils/touchPresentation(SearchField.stories.tsx:6). - Attach real-device recordings for the iOS Safari and Android Chrome claims, or label them as simulator and emulator results.
[Automated review]
| return; | ||
| } | ||
| const observer = new Observer(() => { | ||
| if (!isAnimating.current && settledRef.current !== null) { |
There was a problem hiding this comment.
A resize that lands while isAnimating is set is dropped here, and the finish handler at lines 197–202 only clears the flag, so the parts keep the pose computed from the width measured when the move started (line 134). From poseFor, a field that is 326 px at the tap and 268 px when the move ends keeps the middle at 0.805 instead of 0.754, about 11 px into the trailing cap, until the next resize. Placing poseFor(settledRef.current, geometry()) when the handles finish, or remembering the skipped resize, would fix it.
| import {useLayoutEffect, useRef, useState} from 'react'; | ||
| import type {Meta, StoryObj} from '@storybook/react'; | ||
| import {SearchField} from '@astryxdesign/lab'; | ||
| import {useTouchPresentation} from '@astryxdesign/core/utils/touchPresentation'; |
There was a problem hiding this comment.
Core does not export ./utils/touchPresentation, so this resolves only through Storybook's source alias, and architecture:react-component-runtime INV13 keeps the touch presentation query to Core and Lab through internal/touch. Reading matchMedia('(pointer: coarse)') with a change listener keeps the story on public ground, as it already restates the clock and curve.
| export const activationEase = cubicBezier(timing.curves.move); | ||
|
|
||
| /** The activation's length, s. */ | ||
| export const ACTIVATION_S = search.activationMs / 1000; |
There was a problem hiding this comment.
This duration is the search.activationMs table constant, not a motion role, which current spec:AST-094 FR4 requires for every duration a touch presentation plays; it waits on the engineering owners' decision about the FR4 row proposed in the conversation.
Depends on #8180, #8340 and #8562. Review only this PR's change: vjeux/astryx@1fb1000...ee835dc (includes #8340's animator files)
User need
Astryx has no phone search field that behaves like iOS 26's, where a tap narrows the field and slides a close button in on a 383 ms clock. Lab gets one.
Problem and solution fit
What led to it: the UIKit-parity program rebuilds iOS controls for Astryx's touch presentation; #8180 recorded the search bar's measurements, and this field plays them.
lift, rising too), on UIKit's curve.searchlandmark, a namedsearchbox, translated button names.Current specification
spec:AST-094FR8 and FR13/FR25, andspec:AST-017FR21/DEC-27.Public delta and caller burden
New lab
SearchField, listed byastryx discover.Size (gzip, base → head):
SearchFieldJS none → 41.70 KB, CSS none → 1.47 KB.Design and alternatives
architecture:component-style-authoringINV7); the middle scales and the ends move, on the compositor. Tradeoff: three layers to keep aligned.Not here: the bottom and tab placements (#8216, #8224).
Evidence
Scope
Testing
What I ran and what it showed (Lab/SearchField stories):
pnpm exec vitest run packages/lab/src: server rendering, translated names and focus after a cancel.Local runs are the evidence; no CI run is expected.