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 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 | +30.5KB |
+98B |
| core/DateInput | +30.6KB |
+85B |
| core/DateTimeInput | +30.7KB |
+89B |
| core/PowerSearch | +29.8KB |
+101B |
| core/TimeInput | +30.7KB |
+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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an owner decision: the touch open and close springs, and the stacked release, tracking, dim and adaptive-breakpoint changes, rest on the draft spec AST-094 (FR4, FR5, FR7, FR8 and FR13), which no current spec states, so an engineering owner needs to adopt those requirements or identify the governing spec, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may do this if they are an engineering owner). Independently of that decision, three changes are needed:
- Land the predecessor pull requests first and rebase, so this one carries only its own 894-line commit instead of 27,971 authored lines.
- Make
pr-interactionpart of the core test owner (thetestjoin), as AST-030 FR11 requires for new coverage. - Plan the entrance in
present()from the reopened rest instead of the previoussettledOffset, so reopening after a close from a peek detent neither jumps nor keeps the peek's lighter scrim, and add a test for it.
[Automated review]
| } | ||
| // Below the screen: the sheet's visible height at its rest. | ||
| const height = measureHeight(); | ||
| const restOffset = settledOffset; |
There was a problem hiding this comment.
This reads settledOffset from the render that reopened the sheet, while the reopen reset (setSettledOffset(0), line 812) renders after this microtask. After a touch close from a peek detent the entrance is planned for the peek, the reset render then drops the peek's resting transform mid-spring (a visible jump), and endScrimTracking(restingDim) at line 1440 leaves the peek's 0.3 scrim on a sheet resting at full height. Planning from the reopened rest (offset 0 and its dim), plus a close-from-peek-then-reopen test, would cover it.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself and is not in the test join's needs (line 888), so a failing BottomSheet interaction proof never fails test. AST-030 FR11 asks new coverage inside an existing surface to join that surface's owner rather than add a separately routed lane.
There was a problem hiding this comment.
Approval waits on an owner decision: the touch open and close springs, and the stacked release, tracking, dim and adaptive-breakpoint changes, rest on the draft spec AST-094 (FR4, FR5, FR7, FR8 and FR13), which no current spec states, so an engineering owner needs to adopt or reject those requirements, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may do this if they are an engineering owner). Independently of that decision, four changes are needed:
- Land the predecessor pull requests first and rebase, so this one carries its own 986-line change instead of 28,063 authored lines.
- Plan the entrance in
present()from the reopened rest instead of the previoussettledOffset, since after a close from a lower detent (the 96 px peek in the Snap Points With Peek story) the next open keeps a 0.3 scrim behind a full-height sheet, and add a test that closes from a peek and reopens. - Make
pr-interactionpart of the core test owner (thetestjoin), as AST-030 FR11 requires for new coverage. - Name what the changed reopen reset effect in
useSheetGestures.tsnow synchronizes (the sheet'stranslateand the dialog's backdrop), as react-component-runtime INV3 requires.
[Automated review]
| } | ||
| // Below the screen: the sheet's visible height at its rest. | ||
| const height = measureHeight(); | ||
| const restOffset = settledOffset; |
There was a problem hiding this comment.
This reads settledOffset from the render that reopened the sheet, while the reset to the tallest detent (setSettledOffset(0), line 812) renders only afterwards. After a close from a lower detent the entrance is planned for that detent and endScrimTracking(restingDim) (line 1440) leaves its dim on a full-height sheet: in Chromium with touch emulation, "Snap Points With Peek" reopens at backdrop opacity 0.3 instead of 1. Plan from the reopened rest (offset 0 and its dim), and add a test that closes from a peek and reopens.
| setSettlingLayoutOffset(null); | ||
| gestureExitRef.current = null; | ||
| travelHandleRef.current = null; | ||
| animator.set({y: 0}); |
There was a problem hiding this comment.
This reset now also writes the animator, stops the scrim follow and reports scrim tracking off, but its comment names only the detent reset. architecture:react-component-runtime INV3 requires a new or changed Effect to name the external system it synchronizes: name the sheet's compositor translate and the dialog's ::backdrop dim here, or move the reset to where the open is planned.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself and is not in the test join's needs (lines 888–896), so a failing BottomSheet interaction proof never fails test. spec:AST-030 FR11: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context." Run these proofs in the core test owner, or make this an internal job that test needs.
604cf60 to
aa3b7de
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner decision: the touch open and close springs, the stacked release, tracking and dim changes, and the new adaptive size rule rest on the draft spec AST-094 (FR4, FR5, FR7, FR8 and FR13), which no current spec states, while the newest commit's entrance input hold contradicts that draft's FR6 and the release, tracking and dim changes also reach mouse drags, which the draft leaves out of scope. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request once the record matches it, and the author may do this as an engineering owner. Independently of that decision, four changes are needed:
- Land the predecessor pull requests first and rebase, so this one carries its own 1,277-line change instead of 28,571 authored lines.
- Plan the entrance in
present()from the reopened rest instead of the previoussettledOffset, since after a close from the 96 px peek in "Snap Points With Peek" the next open keeps a 0.3 dim behind a full-height sheet, and add a test that closes from a peek and reopens. - Make
pr-interactionpart of the core test owner (thetestjoin), as AST-030 FR11 requires for new coverage. - Name what the changed reopen reset effect in
useSheetGestures.tsnow synchronizes (the sheet'stranslateand the dialog's backdrop dim), as react-component-runtime INV3 requires.
[Automated review]
| } | ||
| // Below the screen: the sheet's visible height at its rest. | ||
| const height = measureHeight(); | ||
| const restOffset = settledOffset; |
There was a problem hiding this comment.
This reads settledOffset from the render that reopened the sheet, while the reset to the tallest detent (setSettledOffset(0), line 839) renders only afterwards. After a close from a lower detent, the entrance is planned for that detent, and endScrimTracking(restingDim) (line 1477) leaves its dim on a full-height sheet. In Chromium with touch emulation, "Snap Points With Peek" reopens at backdrop opacity 0.3 instead of 1. Plan from the reopened rest (offset 0 and its dim), and add a test that closes from a peek and reopens.
| setSettlingLayoutOffset(null); | ||
| gestureExitRef.current = null; | ||
| travelHandleRef.current = null; | ||
| animator.set({y: 0}); |
There was a problem hiding this comment.
This reset effect now also writes the animator, stops the dim's animation and reports scrim tracking off to the host, but its comment (line 833) names only the detent reset. architecture:react-component-runtime INV3 requires a new or changed Effect to name the external system it synchronizes. Name the sheet's compositor translate and the dialog's ::backdrop dim here, or move the reset to where the open is planned.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself through its own scope step and is missing from the test join's needs (lines 888–896), so a failing BottomSheet interaction proof never fails test. spec:AST-030 FR11: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context." Run these proofs in the core test owner, or make this an internal job that test needs.
| // presentation, for its whole life (F4 b59): a hold covers the sheet and | ||
| // the scrim until the sheet lands, so such a touch reaches neither (its | ||
| // later moves and lift stay with the hold, even once it is gone). | ||
| const [isEntranceHeld, setIsEntranceHeld] = useState(false); |
There was a problem hiding this comment.
While the entrance hold is up, a tap on the dim and a grab are both ignored for about 0.6 s. The draft AST-094 states no such behavior, and its FR6 says "A finger MUST be able to catch any animation mid-flight at the value on screen." The cited source, "F4 b59", is not in the repository. The owner question on this pull request asks whether the hold stays, with FR6 amended, or goes.
| const dimFor = (o: number) => | ||
| scrimOpacityForOffset(o, offsets, dismissedOffset, peekOffset); | ||
| lastDimForRef.current = dimFor; | ||
| const release = resolveSheetRelease({ |
There was a problem hiding this comment.
settleFromDrag runs for mouse drags too, so a released mouse drag now takes the UIKit rule. At rest it dismisses only past halfway to gone, instead of 40% of the shortest detent's height below it. The move handler at line 1742 likewise drops the 40 px magnet and the 48 px stretch for every pointer. AST-094 lists pointer presentations as a non-goal, so these mouse changes have no authority even in the draft; they are part of the owner question.
…the full scrim present() planned the entrance from `settledOffset`, which still held where the last close left the sheet: the reopen reset renders after present's microtask. After a close from a peek, the next open planned 96 px of travel to the peek, then dropped the peek's transform mid-spring (a jump) and left its 0.3 scrim behind a full-height sheet (astracat-bot on meta#8203). The entrance now plans from the tallest detent, where every open rests.
aa3b7de to
3398452
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an engineering owner decision: the touch BottomSheet open and close springs, the entrance input hold and the stacked touch release, tracking and dim rest on the draft spec AST-094, which no current spec states, and that record itself conflicts with the hold (FR6), the stacked theme motion roles (FR4) and the current reduced-motion design spec (FR8). An engineering owner, the author included as one, records adoption by commenting /approve-spec <full head SHA> on this pull request once the record matches the decision. Independently of that decision, these changes are needed:
- Land the stacked predecessors and rebase, so this pull request carries only its own eight commits (about 1,700 changed lines instead of 35,605).
- Drop the adaptation
platformcondition, which the closed condition vocabulary of AST-012 FR2 does not admit. - Run the interaction proofs under the existing core test owner instead of the separately routed
pr-interactionlane (AST-030 FR11). - Correct
astryx docs motion, which says alerts, menus, navigation and scrolling play iOS motion, and the adaptive changeset's promise to follow a theme's ownmdpoint.
[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 is a sixth when field. spec:AST-012 FR2: "A when object may contain width, height, pointer, contrast, and motion … Future environment fields require explicit additions to this closed vocabulary." Drop it here, or have the record amended first.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction decides its own scope and is missing from the test join's needs (lines 888–896), so a failing BottomSheet interaction proof never fails test. spec:AST-030 FR11: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context."
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
Only the BottomSheet plays the motion table at this revision; no alert, menu or navigation component imports it, and AST-094 keeps scrolling native. Limit the sentence to what ships.
| export const COMPACT_QUERY = DEFAULT_COMPACT_DEFINITION.query; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = |
There was a problem hiding this comment.
This is the default definition (768 px). spec:AST-012 FR9 makes compact "below the effective md point", and FR1 lets a theme override md, while the changeset promises "the theme's md point". Resolve it from the active theme, or say 768 px in the changeset.
| // with the sheet). The pin is lifted only after a style flush has applied | ||
| // the last value, so that value does not ease in again. | ||
| // Under a finger UIKit drops every touch that begins during a sheet's | ||
| // presentation, for its whole life (F4 b59): a hold covers the sheet and |
There was a problem hiding this comment.
"F4 b59" resolves nowhere in the repository (not in the UIKit table, MEASUREMENTS.md or AST-094), and AST-094 FR6 says "A finger MUST be able to catch any animation mid-flight". The entrance hold needs an owner decision and a recorded source.
| // UIKit's sheet tracks (the release, not the drag, picks the detent). | ||
| // Up past fully open it holds: UIKit's sheet doesn't stretch beyond its | ||
| // largest detent. | ||
| const next = Math.max(0, raw); |
There was a problem hiding this comment.
This move handler runs for every pointer, so mouse and pen drags also lose the 40 px detent magnet and the 48 px stretch above fully open, although AST-094 leaves pointer presentations out of scope and the release keeps today's rule under a mouse.
3398452 to
4ed70c6
Compare
…ter a stalled dispatch, and a proof can check the pacing the page received WebKit's touch command takes no timestamp, so the page times each event by its arrival. Under box contention a dispatch stalls 27 to 31 ms, and the driver, which aimed every event at the gesture's start, then sent the overdue moves together, 1 to 2 ms apart: C17 measured BottomSheet's flick at 2000 px/s reaching the page as 3,627 to 13,200 px/s. playSteps now sends each event at its scripted time and never sooner after the one before it than the script spaces them, so a stall stretches one gap instead of bunching the rest. With `checkPacing`, WebKit's play reads the timeStamps the page received (a passive capture listener on touch events) and throws TouchPacingError, naming both gap lists, when an event is missing or a gap is more than TOUCH_PACING_TOLERANCE_MS off the script. retakeOnTouchPacing(testInfo, attempt) runs a proof's attempt once more on a TouchPacingError only: two attempts in all, a product failure is never re-taken, and each re-take is listed as a `touch-retake` annotation. The tolerance is measured: on one worker of a loaded machine, 440 gaps over 40 WebKit flicks moved at most 4 ms (99th percentile 3 ms; the page's stamps are whole ms); under eight workers 4% of flicks had a gap further off, up to 21 ms. Red first: the stalled-dispatch case fails on the previous scheduling; checkTouchPacing and retakeOnTouchPacing are new, with the three re-take cases (both attempts off pace stays red, a product failure after good input is not re-taken, off pace then good passes and is listed). In the browser a 40 ms driver stall makes WebKit's play throw TouchPacingError, and Chromium, whose page sees the scripted stamps, plays on.
…a click, and SCRIPTED_CLICK_SKIP names the skip where it does not Playwright's WebKit synthesises a click only from its tap gesture (`driver.tap`), never from touches sent one by one, so a proof that needs the click of a scripted press and lift has to know. `driver.synthesizesClicks` says it (Chromium true, WebKit false), and such a proof skips with `test.skip(!driver.synthesizesClicks, SCRIPTED_CLICK_SKIP)`, as PARTIAL_LIFT_SKIP does for a partial lift. The self-test plays a scripted tap and then driver.tap on one button and reads the order of touchstart, touchend and click: driver.tap's click comes last and a scripted tap's click, if any, before driver.tap's touchstart, so it waits on that order rather than on the clock. This came from the web-input drivers helper line, which no longer carries it.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: this pull request adds an Element jobs item for the entrance hold's shield to the current BottomSheet record, and an engineering owner needs to approve it, and settle whether its non-modal sentence should say pointer input as AST-094 FR35 does, by commenting /approve-spec <full head SHA> on this pull request (the author may do this as an engineering owner). Whatever that decision, these need changes:
- Land or retarget the open predecessor pull requests and move the
@astryxdesign/buildtreeshakeCompensationdefault to its own pull request, so this diff carries only its own change (37,948 authored lines now, about 2,556 of them this pull request's), then bring that under 2,000 lines or explain why it cannot be split. - On a non-modal sheet, keep key and assistive-technology activation live during the entrance and stop a held drag's expected click from swallowing the first tap after the sheet lands (
sheetSelfHold.ts), with tests for both. - Play the sheet springs through AST-094 FR4 motion roles, keep the dim on the spring-back when a sheet is reopened during its exit (AST-108 FR4a), and state the touch exit in the BottomSheet record (AST-094 FR7).
- Make the carried
PressTrackera factory, as react-component-runtime INV13 asks.
[Automated review]
| return; | ||
| } | ||
| expireClicks(hold); | ||
| if (hold.holders > 0 || hold.clicks.length > 0) { |
There was a problem hiding this comment.
While the hold is on, this swallows every click on a non-modal sheet, including the one Enter, Space or a screen reader dispatches with no pointer press, so Done does nothing during the entrance. AST-094 FR35: "Only pointer input is held: keys and assistive technology stay live." Swallowing only a click that consumes the expectation of a held pointer press (and not because holders > 0) keeps keys and assistive technology live.
| hold.clicks = hold.clicks.filter(expiry => expiry > now); | ||
| }; | ||
| const lifted = (hold: SelfHold) => { | ||
| hold.clicks.push(performance.now() + CLICK_WINDOW_MS); |
There was a problem hiding this comment.
Every lifted held press queues an expected click, but a drag never clicks, so its expectation outlives the entrance and swallows the next real tap for up to a second after the lift, once the sheet has landed. Queue it only for a press that can still click, or clear it at the next unheld press.
| // start. | ||
| const handle = animator.animate( | ||
| {y: 0}, | ||
| {spring: SHEET_PRESENT_SPRING}, |
There was a problem hiding this comment.
SHEET_PRESENT_SPRING here (and SHEET_DISMISS_SPRING at line 1901, the reopen spring at line 1109) is a table constant, not a motion role: motion/roles.test.ts asserts springs.sheetPresent is no role. AST-094 FR4: "A component MUST read touch motion through these roles and MUST NOT play a literal." springs.viewChange carries the same measured value.
| } | ||
| // And the dialog's ::backdrop dim: its animation stops, and the host | ||
| // gets its own easing back. | ||
| stopScrimFollow(); |
There was a problem hiding this comment.
On a reopen during the touch exit the sheet springs back (lines 1107–1111), but this stops the exit's dim and hands it to the host, and nothing follows the spring-back, so the dim no longer moves with the sheet. AST-108 FR4a asks a sheet and its scrim to start and land together; following the scrim on the spring-back's curve, as present() does, would keep them in step.
| 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.
"holds only input on itself" reads as every input; AST-094 FR35 holds only pointer input and keeps keys and assistive technology live. AST-094 FR7 also asks this record to state the sheet's touch exit, which this edit could add.
| return tap.numberOfTapsRequired === 1 && tap.numberOfTouchesRequired === 1; | ||
| } | ||
|
|
||
| export class PressTracker implements TouchConsumer { |
There was a problem hiding this comment.
react-component-runtime INV13: "The recognizer family (its base, the arena, and the host) is the one place these modules use classes ... Every other module ... is built from create… factories." The press tracker is outside that family, so it would be a createPressTracker factory.
| // compiled (the TypeScript transform drops it as unused), the module is | ||
| // never loaded, and its constants never reach the CSS: a media-query key | ||
| // fails the build and a value ships as an undefined custom property. | ||
| treeshakeCompensation: true, |
There was a problem hiding this comment.
This flips the {stylexOptions} form's treeshakeCompensation default for every caller who leaves it unset. It has no relation to the BottomSheet change, so it would be easier to review and release on its own.
Once a component in the root entry holds its layer's input (a popover's presentation hold, meta#8223), Layer/inputHold's static imports of gestures/GestureHost and gestures/nativeScroll put eight gesture modules (about 47 KB) in the root entry, against architecture:react-component-runtime INV13. Bundle a component that imports the input hold and require no gesture bytes. Fails while the input hold imports from gestures/.
…e host asks it Layer/inputHold registered a touch gate with the gesture host (getGestureHost(doc).addTouchGate(isInHeldLayer)) and walked the flat tree through gestures/nativeScroll, so any component that holds its layer statically carried the gesture system into its bundle and the root entry (architecture:react-component-runtime INV13). Invert it: the host asks isInHeldLayer for every touch itself (gestures may import Layer), the hold registers nothing, and it walks the flat tree through utils/flatTree, where parentAcrossShadow already lives. addTouchGate stays for other gates. A host test holds a held layer's touches with no gate (it fails if the host stops asking the hold).
…d off its pacing, once In WebKit a touch carries its real dispatch time, so a dispatch that stalls under load reaches the page as a burst of moves 1-2 ms apart, which the sheet reads as a much faster flick and correctly dismisses. The three proofs whose outcome follows a velocity (the downward flick, the moderate and fast throws, and Android's flick) now play with `checkPacing`, and only on a proven `TouchPacingError` re-take the gesture once, on a fresh page. A product assertion is never re-taken, and a second pacing error fails with its cause.
…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.
…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.
…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.
Approval waits on an engineering owner's decision on two record edits at this head: the new Element jobs item in component:BottomSheet (the entrance hold's viewport shield, and whether its non-modal sentence should say pointer input) and the spec:AST-094 FR41 clause on claimed mouse and pen drags carried from #8665. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- Land or retarget the predecessors so the diff carries only this pull request's own change (41,248 authored lines today, 2,556 of them its own), and move the
@astryxdesign/builddefault change to its own pull request. - Keep key and assistive-technology activation, and the first tap after a held drag, live on a non-modal sheet during its entrance (
sheetSelfHold.ts,spec:AST-094FR35). - Play the sheet springs through FR4 motion roles, keep the dim on the spring when a sheet is reopened during its exit (AST-108 FR4a), and state the touch exit in the BottomSheet record (FR7).
[Automated review]
| return; | ||
| } | ||
| expireClicks(hold); | ||
| if (hold.holders > 0 || hold.clicks.length > 0) { |
There was a problem hiding this comment.
On a non-modal sheet this swallows every click while the hold is on, including the detail 0 click Enter, Space or a screen reader dispatches, so a keyboard user’s Done does nothing during the entrance. spec:AST-094 FR35: “Only pointer input is held: keys and assistive technology stay live”. Let a detail 0 click through, as utils/clickGuard.ts line 135 does, and swallow a pointer click only when it consumes the expectation of a press the hold took.
| hold.clicks = hold.clicks.filter(expiry => expiry > now); | ||
| }; | ||
| const lifted = (hold: SelfHold) => { | ||
| hold.clicks.push(performance.now() + CLICK_WINDOW_MS); |
There was a problem hiding this comment.
Every held pointerup queues an expected click, but a drag produces none, so this expectation outlives the entrance and swallows the next real tap for up to a second after the lift. Queue it only for a press that can still click, or clear it at the next unheld press, and add a test for a tap after a held drag.
| // start. | ||
| const handle = animator.animate( | ||
| {y: 0}, | ||
| {spring: SHEET_PRESENT_SPRING}, |
There was a problem hiding this comment.
SHEET_PRESENT_SPRING (and SHEET_DISMISS_SPRING at line 1901) is played as a table literal; motion/roles.test.ts lines 169–170 assert it is no role. spec:AST-094 FR4: “A component MUST read touch motion through these roles and MUST NOT play a literal”. springs.viewChange carries the same measured value.
| } | ||
| // And the dialog's ::backdrop dim: its animation stops, and the host | ||
| // gets its own easing back. | ||
| stopScrimFollow(); |
There was a problem hiding this comment.
On a reopen during the touch exit the sheet springs back on SHEET_PRESENT_SPRING, but this stops the exit’s dim and hands it to the host’s stylesheet transition, so the sheet and its scrim run on two clocks (AST-108 FR4a). Follow the scrim on the spring-back’s curve from the dim on screen, as present() does.
| 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.
“holds only input on itself” reads as every input; spec:AST-094 FR35 holds only pointer input and keeps keys and assistive technology live. The record also states no touch exit for the sheet, which FR7 asks each overlay record to state.
Depends on #8128 and #8342. Review only this PR's change: https://github.com/vjeux/astryx/compare/3c55bcb86cfa1934942b035887fa53182f9d7e69..eeef50191a55dc4392ad224e15ac4e68bf297865
Primary intent
On a touch device, a BottomSheet opens and closes on iOS's sheet spring, with the dim following it, and ignores taps while it is still arriving.
User or maintainer impact
On a phone, a sheet opened and closed on the theme's fixed curve, so it looked unlike an iOS sheet and stuttered on a busy page, and unlike iOS it took taps while still arriving. Now, under a finger:
A mouse keeps today's transitions.
Problem and solution fit
spec:AST-094FR7 asks for UIKit's presentation under a finger, and FR35 for the entrance hold. Options:CASpringAnimationmass 3, stiffness 1000, damping 500, critically damped) on the animator the release already uses (chosen): the same machinery as fix(core): BottomSheet releases a drag the way an iOS sheet does #8105's settle, so a release and a presentation look alike. It costs a layer-wide input hold while entering, which comes from feat(Layer): one input hold, first used by Dialog, Drawer and MobileNav (spec:AST-094 FR35) #8342.Authority
spec:AST-094FR6, FR7, FR35;architecture:interaction-modalityINV8;design:reduced-motion; AST-090 FR14;spec:AST-107FR7 with INV13's gaps (#8751).BottomSheet.spec.mdnames the hold's shield.Change and evidence
iOS Safari (simulator), main then this PR:
Through a 700 ms long task: open before, after. Android (emulator):
Scope
Testing
How to check, in the Snap points story on a phone: open the sheet (it rises on a spring, the dim with it); tap the content while it rises (nothing happens); close and reopen it mid-way (it turns back from where it is); with Reduce Motion on, it fades.
What ran: the whole core and lab packages pass. In Chromium and WebKit with real touch, the entrance and exit stay within 0.5 px of UIKit's spring and a tap during the entrance is ignored; on a touchscreen laptop a finger still pulls the sheet.
touch-support-bundlefails here, not on main; #8671 replaces that check.