Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
![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. The interactive-transition driver and BottomSheet's touch moves rest on spec:AST-094, which this pull request adds as a draft, so an engineering owner needs to approve or amend its requirements by commenting /approve-spec <full head SHA>; the author may do this as an engineering owner. The decision to settle is mainly FR4, which says touch motion is not themable while the code lets themes retune touch springs. Whatever that decision is, these changes are still needed:
- Add the
## Writing rubric resultsforspec:AST-094to the description, which the repository asks for before a record is approved. - Retarget or rebase so the pull request carries only its own four commits; against
mainit is 221 files and about 40,000 lines from the stack below. - Remove the
platformadaptation condition, or amendspec:AST-012FR2 first, since FR2 closes thewhenvocabulary. - Bring
spec:AST-094FR8's cross-fade durations within the 200 ms cap ofdesign:reduced-motionDR2, and fix its citation ofspec:AST-090, which does not exist. - Correct the theme-roles changeset, which names a refused role and says no component reads the roles, and make the storyboard test fail when its fixture is missing.
[Automated review]
| * 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 adds a sixth when field. spec:AST-012 FR2 limits when to width, height, pointer, contrast and motion and says new environment fields need an explicit addition to that closed vocabulary. Drop it here, or amend FR2 first.
| }); | ||
|
|
||
| it('matches the storyboards recorded before the driver (within 0.5 px and 0.005 opacity)', () => { | ||
| if (!existsSync(FIXTURE)) { |
There was a problem hiding this comment.
With the fixture missing, this test returns early and afterAll records a new fixture from the code under test, so the "within 0.5 px" proof cannot fail. Fail here unless UPDATE_STORYBOARDS=1.
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen |
There was a problem hiding this comment.
design:reduced-motion DR2 (current) says a touch cross-fade must finish in 200 ms or less, and the animator already uses 200 ms. A 0.5 s push/pop fade and a sheet fade "on its own presentation spring" go past that cap.
There was a problem hiding this comment.
That text is spec:AST-094's, from #8316 below this PR, which doesn't change it.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMultiSelector (@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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/BottomSheet | +32.6KB |
+98B |
| core/DateInput | +32.7KB |
+85B |
| core/DateTimeInput | +32.8KB |
+89B |
| core/PowerSearch | +31.9KB |
+101B |
| core/TimeInput | +32.8KB |
+92B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.7KB | +94B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
|
Posted by Cindy's agent on her behalf. Decision on the shared release rule this PR implements.
|
|
Posted by Cindy's agent on her behalf. Following the decision on #8076: there are no module-level |
e39f9e8 to
f70618a
Compare
|
Aligned with the release-rule decision, so the driver adds no public surface. In f70618a the driver is internal (no |
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved. The interactive-transition driver and BottomSheet's touch moves rest on spec:AST-094, which this pull request carries as a draft, and on theme motion roles that no current record admits, so an engineering owner needs to approve themeable touch motion together with FR3, FR5 and FR6, or rule that touch motion stays fixed to the UIKit table, by commenting /approve-spec <full head SHA> on this pull request; the author may do this as an engineering owner. Whatever that decision is, these changes are still needed:
- Retarget or rebase so the compare carries only this pull request's own commits; against
mainit is 232 files and about 43,000 lines from the stack below. - Under a mouse, give
sheetReleaseRulethe tracker's release velocity instead of BottomSheet's own 100 ms window, asspec:AST-100FR2 and FR4 require. - Bring
spec:AST-094FR8's fades within the 200 ms cap ofdesign:reduced-motionDR2, fix FR7's citation ofspec:AST-090, which does not exist, and add the record's## Writing rubric resultsto the description that carries it. - Correct the theme-roles changeset, which says no shipped component reads the roles, and mark the animator changeset internal like its siblings.
[Automated review]
| shortestDetentHeight: context.shortestDetentHeight, | ||
| dismissible: context.dismissedOffset != null, | ||
| position: input.position, | ||
| velocity: context.today.velocity, |
There was a problem hiding this comment.
Under a mouse this decides on context.today.velocity, BottomSheet's own 100 ms window, not the tracker velocity the rule receives as input.velocity. spec:AST-100 FR2 and FR4 and architecture:interaction-modality INV10, current on main, allow one release velocity for every pointer. Passing input.velocity here, and dropping the sample window in useSheetGestures.ts, keeps today's thresholds on the shared velocity.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `navigation.transitionMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
"No shipped component reads the roles yet" no longer holds: BottomSheet registers the springs table (useSheetGestures.ts:1120) and the driver resolves every spring under the theme (motion/interactive.ts:365), so springs.sheetPresent, springs.sheetDismiss and springs.sheetDetent retune its touch motion.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] The experimental touch animator follows design:reduced-motion. Under Reduce Motion every `animate` and `play` (travel or opacity, on a spring or any curve) becomes an opacity-only cross-fade of 200 ms on a coarse primary pointer, or for a call marked `touchRelease`, and completes at once on any other pointer; nothing moves, scales or springs, and travel lands with a plain style write while the element is transparent, never as an animated transform. `timing.reducedMotionCrossFadeMs` is now 200 ms and is no longer a theme role. The `reducedMotion` and `crossfade` call options are removed; `crossfadeAs` stays. `animator.reducedMotion()` reports the mode for feedback with no fade of its own, `reducedMotionFade()` gives the same fade to code beside the animator, and `reducedMotion.stylex.ts` holds its constants for StyleX transitions. |
There was a problem hiding this comment.
This still calls the animator experimental and ships as [fix], while the animator is internal at this head and the sibling changesets now say "Internal".
c14709d to
9cded4a
Compare
…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.
…se reads it, its last two move intervals and its lift C17 adopted checkPacing on BottomSheet's flick detent proofs and every WebKit attempt failed, even on an idle worker: WebKit delivers a gesture's first moves with the page's own touchstart handling time (gaps of 4, 11, 3 ms against 8, 8, 8) and its lift about a frame late (8 to 11 ms against 2.1). Neither feeds a release velocity: Core's tracker reads the last three moved samples, two intervals, and a release reads as still only 40 ms after the last move. Before the check the same flicks reached the page at 2,000 px/s. checkPacing now judges what decides a release: an event missing, either of the last two move intervals more than TOUCH_PACING_TOLERANCE_MS (4 ms) off the script, or the lift TOUCH_LIFT_STILLNESS_MS (40 ms, UIKit's measured stillness, spec:AST-100 FR2) or more after the last move. checkTouchPacing takes the played steps, so it knows which events are moves. Red first: C17's received gaps fail the previous check and pass this one; a stall and its burst in the last two intervals still fails, and so does a lift 45 ms late, while the same stall earlier in the gesture passes. In the browser a 40 ms driver stall before the last move throws in WebKit, and Chromium plays on.
…this head The headers of morphPath.ts and its test named rectMorph.ts and rectMorph.test.ts, which arrive with the pull request stacked on this one, and cited 'law M1', which no document defines. They now describe the player without naming its file and cite spec:AST-094 FR5, which keeps touch motion to translate, scale and opacity on the compositor.
The header cited 'law M1' and 'laws M2, M3', which no document defines. It now cites spec:AST-094 FR5 for compositor-only motion and FR6 for an interruption that starts from the presented rect with its velocity, and its paragraph wraps within the line width.
…il cancelled, and its compositor run is blocked only once on the compositor C23 saw the control self-test fail under load at eight workers: "660 ms judged at 16.6 ms/frame: 28 frames presented, 0/28 moved". Its animations ran once, for 1,600 ms from before the screencast started, so on a loaded runner, where starting the cast takes long, the compositor's animation had ended before the block and nothing moved in the judged part. Both animations now go back and forth until cancelled, as the pooled self-test's do, and the compositor's block uses `during`, so it starts only once that animation runs on the compositor. Red first: with the setup stretched to 1.8 s, as a loaded runner takes, the previous test fails with exactly that read (0/39 moved) and this one passes. Normal runs at eight workers: 10 of 10 in Chromium and 10 of 10 in WebKit.
… judges its first frame on that frame's time 'a spring started right before a block plays in full after it' waited a fixed 300 ms of wall clock after the block, then read the screencast. In the full interaction run on a loaded box no moving frame had reached the screencast by then, and the proof failed on an empty read. It now waits until the page shows the box a quarter of the way along and three more frames reach the screencast. Its bound changes form, not strength: the curve's own start (the handle's started) must come after the block, and the first moving frame may be no further along than a curve started a frame before the block ended, at that frame's own time, which is the old 30 px for a frame right after the block and still holds when a loaded machine sends frames sparsely. A curve dated at the call is 107 px in.
Takes meta#8360's head: the held-dip proofs wait on the call's finished and a late landing shows the element at once (88a1866), and the spring-before-a-block proof waits for the motion (d0f5fd5). In the animator spec the proof keeps meta#8360's waits and bounds and runs in WebKit too, as this pull request's start-after-block makes it pass there. # Conflicts: # packages/core/src/motion/__tests__/animator.interaction.browser.spec.ts
…itor run against the control, since a loaded runner's compositor skips frames At 16 workers on a loaded box (load 92), the self-test failed 6 of 32 Chromium runs on its compositor animation. Every presented frame moved (32/32 to 38/38), but the compositor presented none for 63 to 109 ms inside the block, past the display-rate bound. Its compositor run is now judged against startBusyControl's control in the same window, with the bound's own verdict recorded beside it. The rAF mutant stays the negative and fails both: it moved on 0 or 1 of the control's frames with a stall past the control's, and the bound alone fails its longest stall. The run follows the README's order: the cast, then the animation, then the block once the animation runs on the compositor. At 16 workers (load 115 to 164) it passed 48 of 48 in Chromium, where the control's own longest stall in the rAF runs reached 33.5 ms against the rAF's 660 ms. WebKit at 8 workers: 8 of 8.
… animations wrap instead of turning
astracat found that the control self-test's back-and-forth animation turns at 1,600 ms. When a loaded runner starts the cast late enough that the block holds the turnaround, two cast frames straddling it evenly read one position, and the control verdict fails the correct compositor animation: "moved on 6 of the 7 frames the control moved". Both self-tests' compositor animations and their timed rAF loops now wrap from 560 px back to 0, a change on every frame. The pooled case's counted mutants still bounce: they are negatives, and a turn changes their position by a full step.
BusyProof.stepPx's note now describes the step it reads from the judged part's changes, and the file header names pooledBusyProof.
Red first: with the setup stretched so the block holds the turnaround, the previous test fails 7 of 12 Chromium runs ("moved on 5 of the 6" and "6 of the 7 frames the control moved"), and this one passes 12 of 12.
…rol inside an open dialog, leaving its backdrop on the compositor
C17 found that in WebKit the control's top-layer popover, shown above an open modal dialog, takes the dialog's `::backdrop` animation off the compositor. BottomSheet's dim held its value for 155 to 225 ms into the block, then jumped to rest, while the control moved on every frame. `startBusyControl(page, {within})` now mounts the box inside the given element as a plain fixed box, with no popover and no top-layer entry. Its frames still count presented frames, and `raise()` leaves such a box where it is.
The self-test opens a modal dialog whose backdrop dims through a block, and judges the dim's brightness at a point against the control.
Red first: in WebKit at 8 workers, with the top-layer control the dim stalled in 5 of 24 runs, from 43 to 78 ms at the block's start, while the control moved ("17/20 moved, longest stall 76.5 ms at +20 ms"); with `within` it passed 24 of 24. Chromium passed either way.
…hold asks the layer, not the gestures) into meta#8203
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: through its stack, the head adds a new element job to BottomSheet's record (BottomSheet.spec.md:233-237, the transparent shield a modal sheet adds while it enters under a finger), and no current spec names that element yet. An engineering owner can record the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or say what must change. Independently of that decision, two changes are still needed:
- Land #8203, #8329 and #8340 first and rebase or retarget this branch, so the compare shows only the driver's own change instead of 41,932 authored lines.
- Add a browser test that scrolls the page beside an entering non-modal sheet, which the FR35 verification in
spec:AST-094lists and no test covers yet.
[Automated review]
| - while a modal sheet enters under a finger, the layer's input hold adds a | ||
| shield inside the dialog: a transparent cover of the viewport that takes | ||
| a tap or a drag landing during the entrance (`spec:AST-094` FR35). It is | ||
| removed when the sheet lands. A non-modal sheet adds no shield: it holds | ||
| only input on itself, and the page beside it stays live. |
There was a problem hiding this comment.
This adds a tested element job to component:BottomSheet's current record: the Element jobs row fails "a named job with no element". No current record names this shield yet (spec:AST-017 FR60 and spec:AST-094 FR35 ask only for a named job and the hold), so it needs an engineering owner's decision on this head, /approve-spec <full head SHA>, or the change they name.
| await expect(dialog).not.toHaveAttribute('open'); | ||
| }); | ||
|
|
||
| test('a non-modal sheet holds only itself while it enters: a tap on the page beside it lands (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
spec:AST-094's FR34–FR35 verification row also lists "a scroll on the page beside an entering non-modal sheet"; this test covers only the tap. A one-finger drag on the page during the entrance that asserts the page scrolled would catch a hold that leaks onto the page.
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: through its stack, the head adds two requirements no current spec states, a rule in spec:AST-094 FR41 that a mouse or pen drag a gesture claims takes no new text selection (clearing the page's selection when the claim comes after the drag moved), and the entrance-shield element job in BottomSheet's record. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or names what must change. The interactive-transition driver itself matches current spec, and two changes remain needed either way:
- Land #8203, #8329 and #8340 first, then rebase or retarget, so the compare shows only this pull request's own 11 files and fixture instead of about 45,600 authored lines.
- Add a browser test that scrolls the page beside an entering non-modal sheet, which
spec:AST-094FR35's verification row requires, in #8203 where the hold lives.
[Automated review]
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| takes no new text selection for that drag only. A claim made after the | ||
| drag has moved clears the page's selection; a press claimed at its own | ||
| pointerdown, before it moves, keeps it. An unclaimed mouse drag keeps its | ||
| released behavior. |
There was a problem hiding this comment.
This adds a new rule to current spec:AST-094 FR41: a mouse or pen drag a recognizer claims takes no new text selection, and a claim made after the drag moved clears the page's selection. Current FR41 says a mouse otherwise keeps its released behavior (spec:AST-017 FR59), so the line needs an engineering owner's approval: comment /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner), or name what must change.
| - while a modal sheet enters under a finger, the layer's input hold adds a | ||
| shield inside the dialog: a transparent cover of the viewport that takes | ||
| a tap or a drag landing during the entrance (`spec:AST-094` FR35). It is | ||
| removed when the sheet lands. A non-modal sheet adds no shield: it holds | ||
| only input on itself, and the page beside it stays live. |
There was a problem hiding this comment.
This names a new element in an open sheet's frame, the entrance shield, which no current spec names (spec:AST-017 FR60 asks for a named job and spec:AST-094 FR35 for the hold). It needs an engineering owner's approval with /approve-spec <full head SHA> on this pull request, or a note of what must change.
| await expect(dialog).not.toHaveAttribute('open'); | ||
| }); | ||
|
|
||
| test('a non-modal sheet holds only itself while it enters: a tap on the page beside it lands (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
spec:AST-094's FR34–FR35 verification row lists both a tap and a scroll on the page beside an entering non-modal sheet. This test covers the tap; a sibling that drags one finger on the page while the hasScrim={false} sheet enters, and asserts the page scrolled, would cover the scroll.
Depends on #8203, #8329 and #8340. Review only this PR's change: https://github.com/vjeux/astryx/compare/5c0fc553e2bc5b204296db7ac57d8b5275b1c169..fd3554c5f96587a81ae5a0cf07bf91e395b179e3
Primary intent
One internal driver for every transition a finger scrubs and a release settles (
createInteractiveTransition), with BottomSheet moved onto it and no change in behavior.User or maintainer impact
No visible change. For maintainers: BottomSheet, Item's swipe actions, Toast, NavigationStack's pop and Lightbox's drag to close all do the same thing (follow a finger, then settle where a rule says), and each was growing its own animation plumbing. With one driver, a fix to grabbing, releasing or reduced motion lands once; Item's swipe moves onto it next (#8344).
Problem and solution fit
The sheet's release code mixed three jobs: where a release lands (a rule), how the sheet and its dim get there (curves), and what a grab mid-settle does. Options:
Authority
spec:AST-094FR36–FR38 (one decision at the release, by the component's rule);spec:AST-100FR2, FR4 (the velocity, passed in);design:reduced-motion;spec:AST-107FR7 with INV13's gaps (#8751). The driver is internal.Change and evidence
Seven sheet release recordings from before the move (a slow settle, a flick to the next detent, a spring back, a flick dismissal, a throw up, a mouse release and a reduced-motion release) hold within 0.5 px and 0.005 opacity after it; the comparison fails if any curve changes. The sheet's rule on the driver matches the old rule on all 134 measured UIKit releases.
Scope
Testing
How to check: in the Snap points story on a phone, drag, flick, catch and release the sheet; it behaves exactly as on the base, as do the dim and Reduce Motion.
What ran: the whole core and lab packages pass, including the recording comparison and the driver's unit tests (its clock, a grab, a coalesced follow, a state layer); the sheet's browser tests pass in Chromium and WebKit.
touch-support-bundlefails here, not on main; #8671 replaces that check.