Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
There was a problem hiding this comment.
Approval is on hold until an engineering owner approves the draft spec:AST-094 requirements this table rests on (FR1 and FR2, and the amendments it proposes to current records) by commenting /approve-spec d3942fae7b785ce0bd61baa7ec5e523647a93b7a on this pull request; the author may do so as an engineering owner. Whatever that decision, these need changes:
- Against
mainthe diff is 26,347 authored lines, of whichuikit.searchis 257; land the stacked pull requests first so this one shows only its own commit. search.bottom.keyboardCenterOffsetis 36, but the frames it cites (centre y 520, keyboard at 539) give 19 pt and no gap; fix whichever is wrong and derive the 17 pt test from those frames.- The stacked
touch-presentationchangeset says an iPad mini in portrait keeps the popover, but at 744 px it moves from the bottom sheet to the popover.
[Automated review]
| * iPhone 17 Pro, the field pinned to keyboardLayoutGuide (centre y 520 | ||
| * with the keyboard at 539), Oct 24 2026 | ||
| */ | ||
| keyboardCenterOffset: 36, |
There was a problem hiding this comment.
The cited frames (centre y 520, keyboard at 539) put the field's centre 19 pt above the keyboard, and with the 38 pt field that leaves no gap, not 36 pt and a 17 pt gap. Could you re-read the frames and fix whichever of the value or the citation is wrong?
There was a problem hiding this comment.
The citation at this head reads centre y 503 with the keyboard's top at 539 (search.ts lines 174-175): 539 - 503 = 36 pt, and with the 38 pt field that leaves 539 - (503 + 19) = 17 pt, the gap the comment states. The 520 figure isn't in this file at 8a593cc; it may come from an earlier revision. The test that derives the gap from the table's own values is being changed to compute it from these cited coordinates, as you suggest in the other comment.
|
|
||
| it('rests the bottom field 17 pt above the keyboard', () => { | ||
| const {fieldHeight, keyboardCenterOffset} = search.bottom; | ||
| expect(keyboardCenterOffset - fieldHeight / 2).toBe(17); |
There was a problem hiding this comment.
This derives the gap from the table's own values, so it passes for any keyboardCenterOffset; computing it from the cited coordinates (keyboard top minus centre minus half the height) would check the measurement.
There was a problem hiding this comment.
Fixed in 8445d60: the gap is now computed from the cited coordinates (centre 503, keyboard top 539).
| * iPhone 17 Pro, Reduce Motion on: the same frames and alphas as without | ||
| * it, Oct 24 2026 | ||
| */ | ||
| reducedMotion: 'keep', |
There was a problem hiding this comment.
The source records Reduce Motion only; draft AST-094 FR8 takes Reduce Motion with Prefer Cross-Fade Transitions on as the reference, so it would help to note that setting for this capture.
There was a problem hiding this comment.
Resolved in 8831bec: the table no longer carries a Reduce Motion setting, since design:reduced-motion decides through the animator. The comment that replaces it records that UIKit keeps the travel with Prefer Cross-Fade off and on (both measured).
| '@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.
An iPad mini in portrait is 744 CSS px wide, so the old (max-width: 768px) and (pointer: coarse) gave it the bottom sheet; with the new query it moves to the popover, so 'keeps the anchored popover' misdescribes the change.
d3942fa to
5c3f25f
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner approves the draft spec:AST-094 requirements this table rests on (FR1 and FR2, and the amendments it proposes to current records) by commenting /approve-spec 5c3f25f5f004580bbac2f1ccc6c346abec4789f2 on this pull request; the author may do so as an engineering owner. The keyboard citation for search.bottom.keyboardCenterOffset now matches its value. Whatever the owner decides, two things still need changes:
- Against
mainthe diff is 26,347 authored lines, of whichuikit.searchis 257; land the stacked pull requests first so this one shows only its own commit. - The stacked
touch-presentationchangeset says an iPad mini in portrait keeps the popover, but at 744 px it moves there from the bottom sheet, and default sub-menus switch between drill-in and flyout on the same screens; please say so and name the options that pin a surface.
[Automated review]
| '@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.
An iPad mini in portrait is 744 CSS px wide: the old (max-width: 768px) and (pointer: coarse) gave it the bottom sheet, and the new rule gives it the popover, so it does not keep the popover. Default DropdownMenuSubMenu rows (presentation = 'adaptive') also switch between drill-in and flyout on the same screens through useMenuDrillIn. Could the note say both, and name the options that pin a surface (presentation="bottom-sheet" or "popover" on the menu, "drill-in" or "flyout" on a sub-menu)?
There was a problem hiding this comment.
Same as above: #8062's changeset, stacked below.
| /** | ||
| * The bottom (toolbar) field's height at rest, pt; it is 336 wide at | ||
| * the bottom of a 402 pt screen. (A tab bar's search tab keeps its own | ||
| * glass geometry in `tabBar`.) |
There was a problem hiding this comment.
There is no tabBar domain under motion/uikit/ at this head, so this pointer leads nowhere; could it say the tab bar's geometry will get its own table, or wait until that table lands?
There was a problem hiding this comment.
Fixed in 8445d60: the note no longer points at a tab bar table.
|
|
||
| /** | ||
| * The UIKit constants table, read-only: `uikit.springs`, `uikit.scroll`, | ||
| * `uikit.timing`, `uikit.navigation`, `uikit.UIKIT_GESTURES` and the haptic |
There was a problem hiding this comment.
This list of the uikit namespace leaves out uikit.search, which uikit/index.ts now re-exports.
There was a problem hiding this comment.
Fixed in 8445d60: the list names uikit.search.
5c3f25f to
8a593cc
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether themes may retune touch motion: the draft spec:AST-094 this table rests on says they may not (FR4, DEC-1), while this pull request registers the search clock, curve and scroll-away snap as theme roles. Once the record and the code agree, an engineering owner records the decision by commenting /approve-spec 8a593cc361864f20d218db27d636ae4d3d60aabb on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- Against
mainthe diff is 28,770 authored lines, of which this pull request's own commit is 326; land the stacked pull requests first so this one shows only its own change. - The stacked
platformadaptation condition adds a field to the vocabularyspec:AST-012FR2 keeps closed; remove it, or amend FR2 first. defineThemerefuses a motion role set under the wrong group only when the motion tables happen to be imported first, and accepts any unknown role id; please validate against a role list the theme module owns.- The stacked
touch-presentationchangeset says an iPad mini in portrait keeps the popover, but at 744 px it moves there from the bottom sheet, and default sub-menus switch with it; please say so and name the options that pin a surface.
[Automated review]
| // (motion/roles.ts). The thresholds (`scrollAway.hysteresisPt`, | ||
| // `scrollAway.collapseThresholdPt`) and the geometry are not durations, | ||
| // springs or curves, so they never become roles. | ||
| registerMotionTable('search', search); |
There was a problem hiding this comment.
This makes the clock, curve and scroll-away snap theme-retunable roles, but draft spec:AST-094 FR4 says a touch presentation reads its motion from the table on every theme, and DEC-1 rejects tuning touch motion per theme. Could the record and this registration be brought into line, in whichever direction an engineering owner approves?
There was a problem hiding this comment.
spec:AST-094, now current through #8378, settles it in FR4 and DEC-3: touch motion is themeable, so the search table's clock, curve and scroll-away snap are theme motion roles, registered by registerSearchTable(), which SearchField's touch module calls once before its first animation. Adding these three to FR4's role list is the record's to do; this table supplies their measured, sourced values (FR1, FR2).
| * 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.
platform is a fifth when field, but spec:AST-012 FR2 limits the vocabulary to width, pointer, contrast and motion and asks for an explicit addition to that closed vocabulary first. Could this wait for an approved FR2 amendment, or leave the stack?
| `${location}: a motion role is named by its table path, such as "sheet.present" or "springs.navigationPush".`, | ||
| ); | ||
| } | ||
| const registered = motionRole(id); |
There was a problem hiding this comment.
motionRole(id) only knows tables some module has already imported, and this module imports roles.ts alone, so the same defineTheme input throws here in one process and passes in another; the test hides it with import '../motion/uikit'. Could the theme validate against a role list it owns, and refuse unknown role ids?
| 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.
No AST-090 record exists at this head or on main, so this amendment names nothing an owner can rule on. Which record should FR7 point at?
There was a problem hiding this comment.
This is the AST-094 spec (now current on main through #8378), not this PR's commits.
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether themes may retune touch motion: the draft spec:AST-094 this table rests on says they may not (FR4, DEC-1), while this pull request registers the search clock, curve and scroll-away snap as theme roles. Once the record and the code agree, an engineering owner records the decision by commenting /approve-spec a06235c742beb6df559e7ea086d05e465ed925a7 on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- Against
mainthe diff is 28,775 authored lines, of which this pull request's own commits are 335; land the stacked pull requests first so this one shows only its own change. search.reducedMotion: 'keep'keeps the activation's travel under Reduce Motion, and the stack's reduced-motion cross-fade runs 500 ms by default and is a theme role (300 ms in thedefineThemeexample), while the currentdesign:reduced-motionasks for an opacity-only fade of 200 ms or less that no theme value can lengthen.- The stack's compact-touch resolver keeps its own 744 px line, while the current
spec:AST-012FR9 defines compact as below the theme'smdpoint (768) or under 500 px tall, and itsplatformadaptation condition is outside FR2's closed vocabulary. defineThemerefuses a misfiled motion role only when the motion tables happen to be imported first and accepts unknown role ids; please validate against a role list the theme module owns.
[Automated review]
| * Reduce Motion keeps the activation's travel and adds no cross-fade: | ||
| * `'keep'` in the animator's terms. | ||
| * @source measured: UIKit lab (injected host app), iOS 26.2 simulator, | ||
| * iPhone 17 Pro, Reduce Motion on with Prefer Cross-Fade Transitions off | ||
| * (M-16): the same frames and alphas as without it, Oct 24 2026 | ||
| */ | ||
| reducedMotion: 'keep', |
There was a problem hiding this comment.
'keep' plays the activation's travel unchanged under Reduce Motion. The current design:reduced-motion record asks a touch transition to change opacity only, in 200 ms or less, with no core value able to keep travel (DR1, DR2, DR4), and draft AST-094 FR8 also takes Prefer Cross-Fade Transitions on as the reference. Could this leaf record the cross-fade form, or leave the table until a cross-fade capture of the search bar exists? Separately, M-16 names no row in motion/MEASUREMENTS.md (its reduced-motion capture is M-RM1), and the table check only resolves @measure IDs, so nothing catches it.
There was a problem hiding this comment.
Fixed in 8831bec: reducedMotion: 'keep' is gone, so the animator applies design:reduced-motion. The comment that replaces it cites the measurement without the lab id.
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; | ||
|
|
||
| /** 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.
spec:AST-012 FR9, current on main, defines compact as width below the theme's effective md point (768 by default) or height below 500, and asks the web touch presentation resolver to use that one definition with no size numbers of its own. With 744 here a 744–767 px touch screen gets the popover and flyout where FR9 gives the sheet and drill-in, and an overridden md changes nothing. Could this follow FR9?
| * // A softer sheet and a slower reduced-motion fade: | ||
| * motion: { | ||
| * springs: { 'springs.sheetPresent': { duration: 0.6, bounce: 0.1 } }, | ||
| * durations: { 'timing.reducedMotionCrossFadeMs': 300 }, |
There was a problem hiding this comment.
This documents a theme setting the reduced-motion cross-fade to 300 ms and calls it slower, though the table's default is 500 ms. The current design:reduced-motion DR2 caps a touch cross-fade at 200 ms and DR4 says theme values offer no way to keep a longer animation under reduced motion. Could timing.reducedMotionCrossFadeMs stay out of the theme roles (as press.scrollBrakeMs is excluded), the default come within 200 ms, and this example go?
|
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.
|
a06235c to
f1b2b56
Compare
meta#8076's new head carries main 70ea859 with meta#8073's writer of the press attribute on main's press controller, and its README names both writers. utils/pressFeedback.ts keeps that controller with this line's own changes on it: the pointer, drag, scroll and visibility listeners through the shared input stream (meta#8356) and touchcancel only while the primary pointer is coarse (meta#8270); the key path stays at the window's bubble phase, so a Space press the page cancels paints nothing. usePressTracker stays in its own module, one tracker for a component's life (meta#8315), so meta#8073's rebuilt-tracker hook and its test are not taken. The harness files are meta#8076's.
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
There was a problem hiding this comment.
Requesting changes: the search table itself is well sourced and fits spec:AST-094 FR1 and FR2, but this diff still carries the whole open #8360 chain, 27,234 of its 27,499 authored lines against this pull request's 265. Please land #8360 and the pull requests beneath it (#8076 first), or rebase onto main once they land, so this pull request is just its six search-table files.
- Merged as it stands, it would also ship that chain's released changes: a 13 px DropdownMenu trigger drift allowance, press feedback on a shared window-capture input stream, and a
treeshakeCompensation: truedefault forastryxStylex({stylexOptions})in@astryxdesign/build.
[Automated review]
Depends on #8360. Review only this PR's change: vjeux/astryx@045f666...1fb1000
User need
On a phone, Astryx's search field should move like iOS 26's: a tap narrows the field, slides in a close button and lifts the large title away on one 383 ms clock. Nothing records those values yet, so SearchField and its host bars would each guess and drift apart.
Problem and solution fit
What led to it: the UIKit-parity program measures each behavior once, in a UIKit app on the iOS 26.2 simulator, into the internal UIKit table components read. This PR adds the search bar's entry,
motion/uikit/search.ts:timing.curves.moverole).MEASUREMENTS.md).Current specification
spec:AST-094FR1, FR2 and FR13, withspec:AST-012FR9/DEC-6's compact screen.Public delta and caller burden
None: internal table entries (
motion/uikit, not exported), with a[chore]changeset.Design and alternatives
spec:AST-094FR4's roles.timing.curves.moverole, which themes can change.A tab bar's search tab stays with the tab bar's own values.
Evidence
search.test.tschecks each value against its frames (for example, the 17 pt gap from the measured coordinates) and the curve against UIKit's ease-in-ease-out.uikit.test.tschecks that every value names its source.Scope
Merge after #8360, which carries the motion stack through #8076.
Testing
What I ran and what it showed:
pnpm exec vitest run packages/core/src/motion/uikit: the derived values agree with the measured frames, and every value names its source.