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
695fcc0 to
d239c85
Compare
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 needs an engineering owner's decision before it can be approved, and the branch needs to shrink to its own change. No current spec governs SearchField's new placement="tab" or SearchFieldHandle.cancel(), and the draft spec:AST-094 on this branch never mentions them while amending current claims (spec:AST-025 FR14, design:user-states DR1 and DR3) and adding new requirements. An engineering owner needs to identify or provide the governing spec for the tab placement and cancel(), update those existing claims or reject the amendments, and record the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever that decision is, these changes are still needed:
- Land or rebase past the predecessor PRs so this diff is the 285-line tab-placement commit rather than 32,035 authored lines across 173 files.
- Fix
spec:AST-094FR7's citation ofspec:AST-090, which is not in the repository. - Add a browser proof that a tap in the tab placement focuses the field without Safari scrolling the page.
[Automated review]
| 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 at this head or on main (it is proposed in #8003), so this departure has no text an owner can check it against. Consider stating the exit and Reduce Motion rule on its own here, or adding the citation once AST-090 lands.
There was a problem hiding this comment.
This record is #8076's (spec:AST-094, the foundation under this stack); the AST-090 citation is its owner's to settle. Nothing in this PR's own diff touches it.
| // Types | ||
| // ============================================================================= | ||
|
|
||
| /** What `handleRef` exposes: focus and blur the field's input. */ |
There was a problem hiding this comment.
This comment, and the handleRef prop's comment at line 177, still describe the handle as focus and blur only. Mentioning cancel() would keep IDE hovers in line with SearchField.doc.mjs.
There was a problem hiding this comment.
Fixed in dafec0d: SearchFieldHandle's comment reads "focus, blur and cancel the field", the cancel() member has its own doc, and the handleRef prop's comment reads "Focus, blur and cancel, for a host that activates the field from its own tap or draws its own cancel button", matching SearchField.doc.mjs.
| * tab bar gives it; the tab bar's own cancel button calls `cancel()`. TabBar's | ||
| * search story shows the real thing. |
There was a problem hiding this comment.
No TabBar search story exists in the repository yet (it arrives with #8192), so this pointer leads nowhere for now. Consider rewording until that story lands.
There was a problem hiding this comment.
Resolved at dafec0d: the InTabBar story's comment no longer points to a TabBar story; it describes its stand-in bar and the handle's cancel().
d239c85 to
9dd3f31
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and the branch needs to shrink to its own change. No current spec governs SearchField's placement="tab" or SearchFieldHandle.cancel(), and the draft spec:AST-094 on this branch never mentions them while adding FR1–FR23 and proposing amendments to current records (spec:AST-025 FR14, design:user-states DR1 and DR3); an engineering owner needs to identify or provide the governing spec for the tab placement and cancel(), approve or reject those draft requirements, and record the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. Whatever that decision is, these changes are still needed:
- Land or rebase past the predecessor PRs so this diff is the 285-line tab-placement commit rather than 32,161 authored lines across 173 files.
- Fix
spec:AST-094FR7's citation ofspec:AST-090, which is not in the repository. - Keep the caret under the finger when SearchField or ChatLayout claims a tap on a text control; today the refocus leaves it at the start of an existing query or draft, or at its last position.
[Automated review]
|
|
||
| /** Focuses without Safari's reveal and puts the caret under the finger. */ | ||
| function focusAtPoint(editable: HTMLElement, x: number, y: number) { | ||
| editable.focus({preventScroll: true}); |
There was a problem hiding this comment.
For a textarea or input composer this claimed tap never places the caret: the gesture host cancels the tap's touchend, and the selection below is set only for contenteditable. In Chromium with touch emulation, focus({preventScroll: true}) leaves the caret at offset 0 of a never-focused draft, or at its last position, where a native tap put it under the finger, so tapping into a draft to change a word lands elsewhere, despite "puts the caret where the finger lifted" above. Consider setting the selection from the tap point for text controls too, or leaving taps on a non-empty text control to the browser, with a browser test that taps mid-text.
| }), | ||
| tap => { | ||
| if (tap.state === 'ended') { | ||
| inputRef.current?.focus({preventScroll: true}); |
There was a problem hiding this comment.
The same caret loss happens here, now for tab as well as bottom: the claimed tap cancels the browser's own focus, and this refocus leaves the caret where script focus puts it (the start, or the last caret position, in Chromium) rather than where the finger lifted in an existing query.
There was a problem hiding this comment.
Fixed in e4f2ea4 (#8216, under this PR): the claimed tap puts the caret where the finger lifted (caretOffsetAt from the tap's location, then setSelectionRange). The bottom interaction spec's "a tap into an existing query puts the caret where the finger lifted" covers it in Chromium and WebKit, and the tab placement takes the same path.
9dd3f31 to
f8792ae
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and the branch needs to shrink to its own change. No current spec governs SearchField's placement="tab" or SearchFieldHandle.cancel(), and the draft spec:AST-094 on this branch never mentions them while adding FR1–FR23 and proposing amendments to current records; an engineering owner needs to identify or provide the governing spec for the tab placement and cancel(), approve or reject those draft requirements, and record the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. Whatever that decision is, these changes are still needed:
- Land or rebase past the predecessor PRs so this diff is the 309-line tab-placement commit rather than 36,898 authored lines across 199 files.
- Take the theme's
platformadaptation condition and--motion-<role>tokens out of this stack, or have their owners amendspec:AST-012FR2,architecture:theme-tokensINV1 andspec:AST-094FR4 and DEC-1 first, since the code contradicts them. - Keep the caret under the finger when SearchField or ChatLayout claims a tap on a text control, and pass
preventScrollon the clear button's refocus intabandbottom. - Fix
spec:AST-094FR7's citation ofspec:AST-090, which is not in the repository.
[Automated review]
| // keyboard rises, as UIKit's clear button makes its field first responder. | ||
| const handleClear = useCallback(() => { | ||
| setValue(''); | ||
| inputRef.current?.focus(); |
There was a problem hiding this comment.
In tab and bottom this is the one refocus left without {preventScroll: true}: the clear button stays visible while the field is active but not editing, and its taps are left to the browser, so tapping it after a tap outside lets Safari reveal the field, the page pan these placements avoid (see the handle comment at line 149). Consider passing {preventScroll: true} here when isBottom || isTab.
There was a problem hiding this comment.
Fixed in e4f2ea4 (#8216, under this PR): the clear button refocuses with {preventScroll: true} in the bottom and tab placements (the field's focusOptions), as do the claimed tap and the handle's focus(). The jsdom case "the clear button refocuses the field without the browser scrolling it into view" and the interaction case "the clear button refocuses the field without Safari's reveal" cover 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.
Current spec:AST-012 FR2 keeps the when vocabulary closed (width, pointer, contrast, motion; "Future environment fields require explicit additions to this closed vocabulary"), and the theme authoring contract says an adaptation change preserves it. platform needs that amendment from the record's owners first, or it should leave this stack with the rest of the motion-role work.
| if (scale) { | ||
| Object.assign(tokens, expandMotionScale(scale)); | ||
| } | ||
| Object.assign(tokens, expandMotionRoles(input.motion, 'motion')); |
There was a problem hiding this comment.
This writes --motion-<role> into the theme's portable tokens (as expandMotionRoles.test.ts asserts), names and defaults that originate in the motion tables rather than tokens.stylex.ts, which current architecture:theme-tokens INV1 requires. It also contradicts this stack's own spec:AST-094 FR4 ("A touch presentation MUST read its motion from the table on every theme") and DEC-1.
f8792ae to
dafec0d
Compare
github-actions
Bot
added
the
needs:design-review
dafec0d to
161147a
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and the branch needs fixes whatever that decision is. No current spec on main governs SearchField's placement="tab", SearchFieldHandle.cancel(), the tab field's focus without Safari's scroll-into-view, or the isRtlElement export from @astryxdesign/core/hooks (spec:AST-094 and spec:AST-099 are still open pull requests), so an engineering owner needs to identify or provide the governing spec for them and record the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. These changes are still needed:
- Make Lab's SearchField import only what Core publishes:
@astryxdesign/core/motion/uikit,/motion/animator,/motion/useAnimator,/gesturesand/utils/touchPresentationare not in Core'sexports, which is whybuild-sandboxfails. - Land or rebase past the predecessor pull requests so this diff is the 483-line tab-placement commit rather than 36,785 authored lines.
- Take the
defineThememotion roles out of this stack, or havearchitecture:theme-tokensINV1 amended first, since the--motion-<role>variables are token namestokens.stylex.tsdoes not declare.
[Automated review]
| type MotionCurve, | ||
| type MotionHandle, | ||
| } from '@astryxdesign/core/motion/animator'; | ||
| import {registerSearchTable, search} from '@astryxdesign/core/motion/uikit'; |
There was a problem hiding this comment.
Core's exports map has no ./motion/uikit (nor ./motion/animator, ./motion/useAnimator, ./gestures or ./utils/touchPresentation), so this import only resolves through the workspace aliases: CI's build-sandbox stops here with "./motion/uikit" is not exported … from package @astryxdesign/core, and any app bundling Lab's SearchField would too. Publish the subpaths Lab needs from Core (as experimental subpaths) or stop importing unpublished Core modules.
There was a problem hiding this comment.
Waits on an engineering owner's decision on #8076: which route Lab touch components take (into core, one marked public entry, or contained Lab subpaths that the registry leaves out). SearchField's imports follow the route chosen.
| } | ||
| const variable = motionRoleVariable(id); | ||
| try { | ||
| tokens[variable] = formatMotionRoleValue(kind, value); |
There was a problem hiding this comment.
This writes --motion-<role> names taken from the motion tables into the theme's tokens. architecture:theme-tokens INV1 says a core token name and its default originate in tokens.stylex.ts and projections do not introduce names, so the roles need to leave this stack or INV1 needs amending first.
…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.
…'s click guard has taken the click The claimed tap swallows its own late mouse events until their click. With meta#8360, the gesture host arms its click guard at the claimed lift, and the guard takes that click first, on the window's capture phase, so the swallow never saw it and stayed armed: the next press's mousedown, mouseup and click never reached the page. The swallow now notes the guard while it is armed and lets go once the guard has (its click, the next press, or its window); with no guard it ends at the click, as before. `internal/touch` re-exports `isClickGuardArmed` by name. Red-first: "a claimed tap swallows its own late mouse events, until the next touch" fails on the previous head (the second press's events are swallowed) and passes in Chromium and WebKit with this change.
…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: no current spec governs SearchField's placement="tab" or SearchFieldHandle.cancel(), so an engineering owner needs to approve or reject both and identify the record that states them, recording the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). Whatever that decision, these changes are still needed:
- Shrink the branch to its own six-file change by landing or retargeting onto its predecessors (#8076, #8340, #8360, #8180, #8181, #8216); it carries 35,115 authored lines today.
- Drop the layout effect that only copies
cancelinto a ref (SearchField.tsx:588-590) by building the handle withcancelin its dependencies (architecture:react-component-runtimeINV3). - Make the tab tap's no-pan promise provable:
SearchFieldTab.interaction.browser.spec.ts:111passes with the tap claim off, so spy on the input's focus calls as the bottom spec does, and add Chrome on Android evidence for the tab tap thatSearchField.doc.mjssays never scrolls the page. - In the predecessors, the Lab keyboard listener's
overlaysContentwrite and geometry-report height contradictspec:AST-099FR8 and DEC-4 (theInTabBarstand-in repeats that rule), and the gesture host's coarse-pointer-only install contradictsarchitecture:react-component-runtimeINV13.
[Automated review]
| useLayoutEffect(() => { | ||
| cancelRef.current = cancel; | ||
| }, [cancel]); |
There was a problem hiding this comment.
This layout effect only copies cancel into a ref, so it names no external system, which architecture:react-component-runtime INV3 requires of a new Effect ("A new or changed Effect MUST name the external system it synchronizes."). Defining cancel before useImperativeHandle and listing it in the handle's dependencies gives the handle the latest cancel without the ref or the effect.
| await (await touchDriver(page)).tap(await centerOf(field)); | ||
| await expect(field).toBeFocused(); | ||
| // No page move: the tap focuses without Safari's reveal. | ||
| expect(await page.evaluate(() => window.scrollY)).toBe(0); |
There was a problem hiding this comment.
This passes with the tap claim off: on the InTabBar story in Chromium an unclaimed tap (the VirtualKeyboard API left in place) and a plain focus() both leave scrollY at 0 with a 32 px scroll range, and it runs before keyboard(page, COVER) raises the emulated keyboard. A spy on the input's focus calls expecting [{preventScroll: true}], as the bottom spec's "a tap focuses the field without Safari's reveal" does, would prove spec:AST-099 FR4's tap route for the tab placement.
| const hold = () => { | ||
| if (appOverlay == null) { | ||
| appOverlay = keyboard.overlaysContent; | ||
| keyboard.overlaysContent = true; | ||
| } | ||
| commit(keyboard.boundingRect.height); | ||
| }; |
There was a problem hiding this comment.
In Chrome on Android this switches the page's overlaysContent on and takes boundingRect.height as the keyboard height, while spec:AST-099 FR8 and DEC-4 take the height from FR2's visual-viewport measurement there too and say Astryx never overrides the app's own viewport setting.
| * - Recognizers attach, and the host registers listeners, only while the | ||
| * primary pointer is coarse (`spec:AST-094` FR19, FR24; coarsePointer.ts). | ||
| * Where it is fine, an attach is recorded but nothing is installed: no | ||
| * document, window or element listener, no `touch-action` or other style, | ||
| * nothing delivered, so a finger on a touchscreen laptop gets the | ||
| * browser's defaults. The host follows the query: turning coarse installs | ||
| * it and styles every registration; turning fine cancels every touch in | ||
| * flight and removes it all. A development warning names the first attach |
There was a problem hiding this comment.
CURRENT architecture:react-component-runtime INV13 says "The gesture host attaches for every pointer, with no touch-only query", and spec:AST-094 FR43 says gestures attach for a mouse, a pen and a finger, not only a coarse primary pointer; this host installs nothing while the primary pointer is fine.
Depends on #8216 and its predecessors. Review only this PR's change: vjeux/astryx@fc37107...15991be
User need
iOS 26's tab bar has a search tab whose field the tab bar itself draws and moves. Astryx's TabBar search tab (#8192) needs just the field's content, and a way to cancel from its own button.
Problem and solution fit
What led to it: the UIKit-parity program; #8192 builds the tab bar's search tab on this placement.
placement="tab": only the field's content (magnifier, query, clear, the Search key, Escape to cancel), sized to the box the tab bar gives it; no background, close button, motion or keyboard reading.handleRef.cancel(): the close button's path (clear, end editing,onActiveChange(false)), for the tab bar's own cancel button.focus()both focus without Safari's reveal (spec:AST-099FR4); Chrome on Android keeps its tap.Current specification
spec:AST-094FR13;spec:AST-099FR4. Pending owner review: thetabplacement andcancel().Public delta and caller burden
placementgains'tab';SearchFieldHandlegainscancel(). Lab only, so no changeset.Size (gzip, base → head):
SearchFieldJS 61.20 → 61.27 KB, CSS 1.54 → 1.54 KB.Design and alternatives
cancel(); the tab bar owns the glass, every motion and the cancel button (feat(lab): TabBar — a search tab #8192). Tradeoff: in this placement the field looks plain on its own.Evidence
In the clip, the gray glass, the × and the bar's place above the keyboard are the story's stand-ins for the tab bar's.
Scope
Testing
What I ran and what it showed (the "In Tab Bar" story):
cancel().pnpm exec vitest run packages/lab/src: the tab placement draws no background or close button;cancel()clears, blurs and reports; the handle'sfocus()doesn't scroll.Local runs are the evidence; no CI run is expected.