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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: the new release behavior rests on draft spec:AST-094 (FR3–FR8, FR16–FR17), and its FR4 narrows the current architecture:theme-tokens INV3 to pointer presentations, so an owner needs to approve or amend that direction, update theme-tokens INV3 if FR4 stands, and say whether mouse drags should also take the new stop rule, which AST-094 leaves out of scope. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision:
- Land the stacked pull requests first and rebase, so this one shows only its BottomSheet change (about 2,000 changed lines instead of 25,000).
BottomSheet.doc.mjsand the changeset describe the new stop rule as touch-only, buthandleReleaseapplies it to mouse drags too; say so, or keep the previous rule for fine pointers.
[Automated review]
| type: 'ReadonlyArray<number | string>', | ||
| description: | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim.", | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim. On a touch screen a released drag settles as an iOS sheet does: on the stop nearest where its speed carries it, dismissal included, with iOS's spring.", |
There was a problem hiding this comment.
This scopes the new stop rule to a touch screen, but handleRelease calls resolveSheetRelease for mouse drags too; only the spring and its velocity are touch-only. A mouse release at 1,000 px/s now projects 114 px instead of about 499 px, a 30 px nudge is no longer exempt, and the at-rest dismissal line moves from 40 % to 50 % of the shortest detent. Say the stop rule applies to every drag, or keep the previous rule for fine pointers.
| ); | ||
| // Gone: the top of the sheet at the bottom of the screen. | ||
| const dismissedOffset = maxOffset + shortestDetentHeight; | ||
| const release = resolveSheetRelease({ |
There was a problem hiding this comment.
resolveSheetRelease runs for every pointer, so mouse drags get the UIKit stop rule as well (0.114 s projection, off screen as a stop, halfway at rest), while draft AST-094 leaves pointer presentations out of scope and the docs and changeset describe the rule as touch-only. If mouse drags should keep the previous rule, choose the rule by releaseMotionRef.current.kind; otherwise update the docs and changeset.
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 | +14.8KB |
0B |
| core/DateInput | +15.4KB |
0B |
| core/DateTimeInput | +15.6KB |
0B |
| core/PowerSearch | +14.6KB |
0B |
| core/TimeInput | +15.1KB |
0B |
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.6KB | 0B |
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
6861a79 to
558b8d6
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: the new release behavior rests on draft spec:AST-094 (FR3–FR8, FR16–FR17), whose FR4 narrows the current architecture:theme-tokens INV3 to pointer presentations, so an owner needs to approve or amend that direction, update theme-tokens INV3 if FR4 stands, and say whether mouse drags should also take the new stop rule and velocity reading, which AST-094 leaves out of scope. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision:
- Land the stacked pull requests first and rebase, so this one shows only its BottomSheet change (about 2,000 changed lines instead of 26,000).
BottomSheet.doc.mjsand the changeset describe the new release rule as touch-only, but mouse drags take it too: a slow drag released at rest between 40 % and 50 % of the sheet no longer dismisses, and a quick pull held still before release now does; say so, or keep the previous rule for fine pointers.- In
useSheetGestures.ts, name the external system the changed reopen effect synchronizes, and stop the animator when the sheet unmounts so its completion timers do not outlive it.
[Automated review]
| if (state.tracker.lastSample?.y !== event.clientY) { | ||
| state.tracker.add(0, event.clientY, event.timeStamp); | ||
| } | ||
| const velocity = state.tracker.velocity().y; |
There was a problem hiding this comment.
This velocity is read for every pointer, and VelocityTracker never decays through a pause, so a mouse drag held still before release keeps its last speed: on the Text only story a quick 140 px pull held 0.42 s now dismisses the sheet, where the 100 ms window read zero and kept it open. Draft AST-094 FR17 scopes this reading to touch. If mouse drags should keep the previous behavior, pick the window velocity by releaseMotionRef.current.kind; otherwise document it beside the stop rule.
| setSettlingLayoutOffset(null); | ||
| gestureExitRef.current = null; | ||
| travelHandleRef.current = null; | ||
| animator.set({y: 0}); |
There was a problem hiding this comment.
This changed Effect now also resets the animator's translate channel, but neither its comment nor its set-state-in-effect reasons name the external system it synchronizes, which architecture:react-component-runtime INV3 asks of a changed Effect. Naming the sheet's animator here, and what the state resets derive from, would cover it.
| [recordSheetHeight], | ||
| [animator, recordSheetHeight], | ||
| ); | ||
| useEffect(() => () => observerRef.current?.disconnect(), []); |
There was a problem hiding this comment.
Only the observer is released on unmount. The animator is released through sheetRef(null) → attach(null), which cancels its Web Animations but not the per-handle completion timer, so a settle cut short by unmount still resolves finished as true and calls completeScrollAreaSettle on the unmounted hook (architecture:react-component-runtime INV5). Calling animator.stop() in this cleanup settles the handles as unfinished and clears their timers.
|
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.
|
558b8d6 to
bd89b28
Compare
github-actions
Bot
added
the
needs:design-review
|
Pushed in bd89b28 (head):
|
…(dim drag), Chromium 158
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: the new touch-release behavior rests on draft spec:AST-094 (FR3, FR5–FR7 and FR16–FR17), and its FR4 says touch physics are not themable while this pull request adds theme motion roles for them, so an owner needs to approve or amend that direction and choose between FR4 and the motion roles. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Land the stacked pull requests and rebase; the BottomSheet change alone is still about 3,200 changed lines, so split it or show why it cannot be split.
- The new
platformadaptation condition is outsidespec:AST-012FR2's closed list of conditions, so remove it or have FR2 amended first. - AST-094 FR8 says a throw keeps its motion under Reduce Motion, which contradicts
design:reduced-motionDR2 and this pull request's own code; align the record. - Under Reduce Motion a finger's release on a fine-pointer screen lands at once instead of cross-fading, because
releaseAnimates()reads the panel'stransition: noneas a zero duration; let touch releases reach the animator and cover it with a test that rendersBottomSheet. - Keep the sheet element in a ref rather than React state in
useSheetGestures.ts, and describe the rule as following the primary pointer, not "under a finger", in the docs and changeset.
[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.
spec:AST-012 FR2 keeps when to a closed vocabulary ("A when object may contain width, height, pointer, contrast, and motion … Future environment fields require explicit additions to this closed vocabulary"), so platform needs that record amended first. No component in this pull request reads it; removing it here, with its template, guide and changeset text, would keep the theme contract as it stands.
| const gestureExitRef = useRef<SheetGestureExit | null>(null); | ||
| // The mounted sheet, as state: the dialog-level touch listeners (see | ||
| // `drawnSheetRect`) attach once it is in the DOM. | ||
| const [sheetNode, setSheetNode] = useState<HTMLElement | null>(null); |
There was a problem hiding this comment.
architecture:react-component-runtime INV1 says DOM nodes must not be stored in React state. The dialog listener effect only needs to know the sheet is mounted: keeping the node in sheetElRef and keying the effect on a boolean (or attaching the dialog listeners from the callback ref with a cleanup) avoids the extra render on each mount and replacement.
| // theme's timing not when it is zero (transitions turned off). | ||
| const releaseAnimates = useCallback(() => { | ||
| const motion = releaseMotionRef.current; | ||
| return motion.kind === 'uikit' || motion.durationMs > 0; |
There was a problem hiding this comment.
Under Reduce Motion the panel renders transition: none, so on a fine-primary-pointer screen readReleaseMotion reads a 0 ms theme duration and this returns false. The settle is then skipped and springTravel never runs, so a finger's release (dragTouchRef) lands at once instead of the 200 ms cross-fade design:reduced-motion DR2 requires for "the release of a touch drag". Returning true for a touch release under Reduce Motion would let the animator's touchRelease fade play; the hook-only test at useSheetGestures.test.ts:2350 cannot see this because its element never gets the panel's inline transition.
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that | ||
| follows a finger, such as a throw, MUST keep its motion. **Proposed; pending |
There was a problem hiding this comment.
This contradicts current design:reduced-motion DR2 (a transition started by "the release of a touch drag … MUST change opacity only and MUST finish in 200 ms or less") and DR4 (no opt-out), and the code in this pull request now cross-fades a touch release. Conforming FR8 to DR2 would keep the record and the code saying the same thing.
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch | ||
| presentation MUST read its motion from the table on every theme. Themes keep |
There was a problem hiding this comment.
The same diff lets themes retune these springs through motion roles (defineTheme({motion: {springs}}), resolved per element by the animator), which this sentence forbids. One of the two has to change once the owners decide whether touch physics are themable.
| type: 'ReadonlyArray<number | string>', | ||
| description: | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim.", | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim. Under a finger, a released drag settles as an iOS sheet does: on the stop nearest where its speed carries it (a quick flick goes at least to the next stop), dismissal included, with iOS's spring; elsewhere it settles as before, on the theme's motion.", |
There was a problem hiding this comment.
The rule is chosen by the device's primary pointer (readTouchPresentation().isTouch), not by the pointer that drags: a finger on a touchscreen laptop keeps the previous rule, and a mouse on a screen whose primary pointer stays coarse takes the iOS one. "On a screen whose primary pointer is coarse (a phone or tablet)" would describe it exactly; the changeset's "on every touch device" has the same gap.
…ings meta#8360's foundation carries the animator's current rules, which reach this PR's release tests (red on this merge); meta#8341 already followed them, and the same test changes now sit here, where the animator arrives: - The end-state hold is 60 s: specConstants.END_HOLD_MS follows it (5000 -> 60_000). Red: its guard, "match the animator's end-state hold", and the reduced-motion cross-fade spec in Chromium and WebKit, which judged each curve by a 5 s hold. - The animator compiles a curve into keyframes on a 30 Hz grid, each chord within a quarter of a pixel of the curve: the spring tests check every keyframe against the closed-form curve at 0.05 px, and that one falls in every 30 Hz interval, instead of reading between two keyframes, which is the browser's interpolation. Red: two tests 0.06 and 0.10 px off. - The inline style holds the value on screen while a curve runs, and the end is written when it retires, so no frame shows where it is going: the release tests read the start right after the release, the end in the last keyframe, and the end written once the motion retires. Red: the drag-dismissal test read 400 px where the element still held 150.
…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.
…erting it The settle test asserted today's settle, the theme's 300 ms --ease-standard transition, frame by frame. That curve starts a release at rest at about 1,400 px/s, where spec:AST-094 FR38 starts a BottomSheet's release at the finger's speed, so the test pinned a curve the spec rules out. It now records the curve, its easing, its duration and its frame-by-frame fit, in the evidence and an annotation, as it already did for the velocity hand-off, and keeps asserting where the settle lands: the half detent.
…against a compositor-only control On a two-core runner the software compositor and the screencast share the core the block spins on, so a compositor animation presents fewer frames there than a display-rate bound allows, though it moves on every frame the compositor presents: pinned to two cores, Lightbox's double-tap zoom proof failed its bound in 7 of 10 runs. startBusyControl shows a small box (a manual popover, in the top layer) whose transform moves it for as long as the page runs; tracked from the same screencast and passed to busyWindowProof as control, it is what the compositor presented in that window. The component passes when it moved on every frame the control moved and stalled no longer than the control; a window where the control moved on fewer than four frames fails. A main-thread animation still fails. No tolerance: both are read from the same screencast frames, and over 140 proofs of a correct compositor animation (Chromium, Chromium pinned to two cores and WebKit, at full and 1-in-6 screencast rates) the component missed none of the control's frames and never stalled longer, while the rAF mutant moved on none of the 39 to 47 frames the control moved in a full window. Red first: five unit cases (a stall the compositor made for both, which the verdict without the control fails; a frozen component; one the control outpaces; one that misses a single control frame; a control that did not move) fail on the old busy.ts.
…ver pooled runs A fade that lasts a fraction of a second leaves one blocked window only a few frames: in WebKit, or on a loaded machine, the control can present fewer than the four one window needs, and the proof fails as too little to judge, saying nothing about the animation. pooledBusyProof(runs) judges several runs of the same short animation as one long window is judged against the control: the control must move on at least four frames over all the runs, the component on every one of them, and in each run its longest stall may be no longer than that run's control's, since a stall belongs to its run. Red first: a 30 ms judged window holds at most three frames and one run fails as too little to judge. Pooled over six runs, a compositor animation passes (no miss in 70 pools, in Chromium, Chromium pinned to two cores and WebKit) and a main-thread one fails (it moved on at most 55% of the control's frames); a pool whose component misses a single control frame fails.
…tep a frame from the change that ends it Against the control, a frame with no new component position was a miss. On an easing tail the component moves under one cast step a frame, which the screencast cannot show on every frame: Lightbox's dismiss-cancel proof in WebKit missed one of the control's 12 to 14 frames in 7 of 10 runs, each time at its spring's last plateau (210, 210, then 208 on WebKit's 2 px grid). Such a run of unchanged frames now counts as moved when the change that ends it is a single cast step. A freeze ends in a catch-up of more than one step and stays a miss, as does a run that no change ends, and the component must still show a new position on at least minJudgedFrames of the control's frames, so a frame-counted animation that stands still through the block and resumes one step a frame fails too. The cast step is read from the tracked value's changes in the judged part rather than from its positions: the photo's position once the block ended (207) sat off WebKit's 2 px grid and made the step read as 1 px. Red first: the tail, pooled tail, off-grid commit and frame-counted freeze cases fail on the previous busy.ts; the single-miss, slow freeze and stop-for-good cases still fail. Lightbox's five busy proofs with this judgment: 30 of 30 in Chromium, 39 of 40 in WebKit (the other presented 3 frames in its judged part, under the 4 a window needs). The harness's own proofs still pass a compositor animation with no miss and fail the rAF one (0 of 39 to 47 control frames) in both engines.
…ame window Both 'a dip held before its first frame lands once the hold is …' proofs waited for no running animation, then one frame and a timeout, assuming the next step starts within a frame. The dip lands on a timer due on the call's own clock, though, and the proofs passed only because the evaluate round trips covered the gap: in WebKit on a loaded box the window closed first and the proof read the element mid-dip (opacity 0, translate none; reproduced 1 in 4 with the round trips removed). They now wait on the call's own finished, then on the browser's own animations still running or waiting for their first frame, and read nothing from the animator.
…ment at once and settles then A Reduce Motion dip whose landing runs more than its whole fade back in late (a hold released long after the dip was due) played a 1 ms opacity animation, which WebKit keeps pending until its next frame, and settled the call on a 1 ms timer: the call's finished could resolve while the element still showed opacity 0. The animator's notes say the element appears at once there. Fails now: the landing plays one opacity animation and the call has not settled.
…element at once and settles A Reduce Motion landing that runs at least the whole fade back in late had nothing left of the cross-fade, but still played a 1 ms opacity animation and settled the call 1 ms later. WebKit keeps that animation pending until its next frame, so the call's finished could resolve with the element still at opacity 0. Such a landing now writes the end opacity at once (handing a borrowed one back to the stylesheet), plays nothing, and settles the call then, as the animator's notes say.
…ng curve only when it is at most a frame longer than the run before it astracat found that the sub-step credit let pooledBusyProof pass a rAF loop that steps a fixed 1 or 2 px a frame: in each 70 ms block the cast still shows the loop's last steps, then it stands still and goes on one step a frame without catching up, so its run ended in a single step. With the previous busy.ts the self-test's frame-counted loop passed the pooled proof in 5 of 5 Chromium runs. A slowing curve lengthens its runs a frame at a time, so a run now counts as moved only when it is at most one frame longer than the component's run before it; a stall is longer than the runs before it. A judged part with no change reads the finest cast step, 0.5 px, so a larger jump never passes for one step. The self-test adds the frame-counted loop, at 1 and 2 px a frame, to the pooled proof's mutants: it fails every pool (Chromium 10 of 10, moved on 2 to 8 of 7 to 12 control frames; WebKit 10 of 10, 0 or 1 of 11 or 12). The timed rAF mutant can read as moved on one control frame, presented as the block starts, so its assertion takes 0 or 1 and also requires its stall to exceed the control's (Chromium showed 1 of 40 once). Red first: the frame-counted stall, the pooled frame-counted loop and the finest-step cases fail on the previous busy.ts. Lightbox's five busy proofs with this judgment: 30 of 30 in Chromium; 33 of 40 in WebKit with no miss, the other 7 presenting too few control frames to judge.
…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 is on hold for an engineering owner's decision. spec:AST-094 governs this release, but three of its deltas fall outside what it states:
- the 1,008 px/s flick floor, which dismisses releases whose 0.1 s projection stops short of FR36's commit point;
- the three sheet springs played as constants, where FR4 and DEC-5 expect motion roles that
component:BottomSheetmeasures; - the drag dismissal's spring exit, which FR7 leaves to that record to state.
An engineering owner settles this in one of three ways: amending those claims, recording the springs, the exit and the flick rule in component:BottomSheet, or asking the pull request to conform. They record the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, please also:
- Let
detail === 0activation through after a finger catches the sheet, and cancel the caught touch'stouchendinstead of using a 700 ms click window (AST-094 FR21). - Time the scrim's fade from the sheet's spring when the primary pointer is coarse (
spec:AST-108FR4a). - Land the stack beneath this pull request and rebase, then split or justify the 5,375-line BottomSheet area.
- Name the new mouse catch of a settling sheet and show mouse before-and-after evidence (
spec:AST-017FR59), and add real-device iOS Safari and Android Chrome evidence for the catch and the selection lock. - Describe the release as applying where the primary pointer is coarse, and reuse
resolveSnapPointand the shared selection lock instead of copying them.
[Automated review]
| const onClick = (event: MouseEvent) => { | ||
| if (performance.now() <= suppressClickUntilRef.current) { | ||
| suppressClickUntilRef.current = -Infinity; | ||
| event.stopPropagation(); | ||
| event.preventDefault(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This guard cancels the first click in the window by time alone. After a catch that drags the sheet, no click of its own comes, so a tap on a control in the sheet within 700 ms of the lift does nothing. Enter or Space while the finger holds the sheet (Infinity from line 1847) is cancelled too. spec:AST-094 FR21 says "Activation with detail === 0 … MUST always pass" and asks for the taken touch's touchend to be cancelled, but that listener is passive (line 1912). Cancelling the caught touch's touchend and skipping detail === 0 clicks would fix both.
| const baseLayoutOffset = settledLayoutOffsetRef.current; | ||
| let baseOffset = settledOffset; | ||
| let baseLayoutOffset = settledLayoutOffsetRef.current; | ||
| if (travelHandleRef.current != null) { |
There was a problem hiding this comment.
This catch runs for every pointer, so a mouse press on a settling sheet now stops it mid-flight; on the base the drag started from the target detent. spec:AST-017 FR59 allows a fine-pointer change from touch work only when the pull request names it and shows mouse before-and-after evidence. Name it with that evidence, or keep the catch to touch.
| : animator.animate( | ||
| {y: to}, | ||
| {spring, velocity: velocity == null ? undefined : {y: velocity}}, | ||
| {crossfadeAs, touchRelease}, | ||
| ); |
There was a problem hiding this comment.
Under a coarse pointer the sheet now settles on this spring, but the scrim's ::backdrop still fades on --duration-medium (BottomSheet.tsx:210-211). spec:AST-108 FR4a wants "BottomSheet's scrim, whose fade lasts the sheet's computed settle time". Setting --_sheet-scrim-duration from the spring's settle time when a release springs would keep them on one clock.
| const isFast = Math.abs(velocity) >= sheet.release.fastSpeed; | ||
| let index = choice.index; | ||
| if (isFast) { | ||
| // A flick goes at least to the next position beyond where it is. | ||
| const tolerance = sheet.release.detentTolerance; | ||
| let next = -1; | ||
| if (velocity > 0) { | ||
| next = positions.findIndex(p => p > position + tolerance); | ||
| } else { | ||
| for (let i = positions.length - 1; i >= 0 && next === -1; i -= 1) { | ||
| if (positions[i] < position - tolerance) { | ||
| next = i; | ||
| } | ||
| } | ||
| } | ||
| if (next !== -1 && (velocity > 0 ? next > index : next < index)) { | ||
| index = next; | ||
| } | ||
| } |
There was a problem hiding this comment.
Awaiting an engineering owner's decision (the open question): this flick branch dismisses a 20 px drag at 1,100 px/s from the 300 px detent (projected 430 px, short of the 470 px halfway line), while a 60 px drag at 900 px/s, projected farther to 450 px, returns. spec:AST-094 FR36 commits only when the projected travel reaches the commit point. Either FR36 is amended to this measured rule, or the dismissal follows FR36.
| /** The deceleration rate whose projection reaches `seconds` of the velocity. */ | ||
| function rateForProjection(seconds: number): number { | ||
| // WWDC18's projection coasts (v / 1000) · r / (1 − r): r / (1 − r) = 1000·τ. | ||
| const ms = seconds * 1000; | ||
| return ms / (1 + ms); | ||
| } |
There was a problem hiding this comment.
This helper, and the flick and cancel branches below, re-implement motion/resolveSnapPoint.ts, whose header says it "is the one 'where does a release land' helper, so a second one should not be added beside it". architecture:react-component-runtime INV13 also has core touch behavior come from one shared set of modules. Calling resolveSnapPoint with projection: {seconds: 0.1, minSpeed: 250}, flickThreshold: 1008, tolerance: 1 and start, and keeping only the sheet's dismissal mapping, would leave one implementation.
| type: 'ReadonlyArray<number | string>', | ||
| description: | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim.", | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim. Under a finger, a released drag settles as an iOS sheet does: on the stop nearest where its speed carries it (a quick flick goes at least to the next stop), dismissal included, with iOS's spring; elsewhere it settles as before, on the theme's motion.", |
There was a problem hiding this comment.
The code keys on the primary pointer (releaseMotion.ts:104, spec:AST-094 FR13), not on the pointer that drags: a touchscreen laptop's finger gets the theme rule, and a mouse on a coarse-primary tablet gets the iOS one. "Where the primary pointer is coarse" would describe it accurately, here and in .changeset/bottom-sheet-uikit-release.md.
…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.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: spec:AST-094 governs this release, but three deltas fall outside what it states, namely the 1,008 px/s flick floor that dismisses releases whose 0.1 s projection stops short of FR36's commit point, the three sheet springs played as constants where FR4 and DEC-5 expect motion roles that component:BottomSheet measures, and the drag dismissal's spring exit that FR7 leaves to that record to state. An engineering owner settles them by amending those claims, by recording the springs, the exit and the flick rule in component:BottomSheet, or by asking the pull request to conform, and records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever the decision, please also:
- Land the stack beneath this pull request (#8068, #8360, #8423, #8906 and #8910) and rebase, then split or justify the 5,406-line BottomSheet area.
- Arm the shared
armClickGuardat a caught touch's lift instead of the 700 ms click window, and cancel that touch'stouchend, sodetail === 0activation always passes (AST-094 FR21). - Time the scrim's fade from the release spring's settle time when the primary pointer is coarse (
spec:AST-108FR4a). - Name the new mouse catch of a settling sheet with mouse before-and-after evidence (
spec:AST-017FR59), and add real-device iOS Safari and Android Chrome evidence for the catch and the selection lock. - Describe the release as applying where the primary pointer is coarse, and reuse
resolveSnapPointandlockSelectioninstead of copying them.
[Automated review]
| const onClick = (event: MouseEvent) => { | ||
| if (performance.now() <= suppressClickUntilRef.current) { | ||
| suppressClickUntilRef.current = -Infinity; | ||
| event.stopPropagation(); | ||
| event.preventDefault(); | ||
| } |
There was a problem hiding this comment.
This guard cancels the first click in the window by time alone. After a catch that drags the sheet no click of its own comes, so a tap on a control in the sheet within 700 ms of the lift does nothing, and Enter or Space while the finger holds the sheet (Infinity from line 1847) is cancelled too. spec:AST-094 FR21 says "Activation with detail === 0 … MUST always pass" and asks for the taken touch's touchend to be cancelled, but that listener is passive (line 1912). The shared armClickGuard in utils/clickGuard.ts already lets detail === 0 through and disarms at the next pointerdown; arming it at the caught touch's lift, and cancelling that touch's touchend, would meet FR21.
| const baseLayoutOffset = settledLayoutOffsetRef.current; | ||
| let baseOffset = settledOffset; | ||
| let baseLayoutOffset = settledLayoutOffsetRef.current; | ||
| if (travelHandleRef.current != null) { |
There was a problem hiding this comment.
This catch runs for every pointer, so a mouse press on a settling sheet now stops it mid-flight; on the base the drag started from the target detent. spec:AST-017 FR59 allows a fine-pointer change from touch work only when the pull request names it and shows mouse before-and-after evidence. Name it with that evidence, or keep the catch to touch.
| releaseAnimates(), | ||
| ); | ||
| if (settlingLayoutOffsetRef.current != null) { | ||
| springTravel(target, layoutOffset, release.spring, velocity); |
There was a problem hiding this comment.
Under a coarse pointer the sheet now settles on this spring, but the scrim's ::backdrop still fades on --duration-medium (BottomSheet.tsx:210-211). spec:AST-108 FR4a wants "BottomSheet's scrim, whose fade lasts the sheet's computed settle time". Setting --_sheet-scrim-duration from the spring's settle time whenever a release springs would keep them on one clock, the drag dismissal included.
| if (next !== -1 && (velocity > 0 ? next > index : next < index)) { | ||
| index = next; | ||
| } |
There was a problem hiding this comment.
Awaiting an engineering owner's decision: this flick branch dismisses a 20 px drag at 1,100 px/s from the 300 px detent (projected 430 px, short of the 470 px halfway line), while a 60 px drag at 900 px/s, projected farther to 450 px, returns. spec:AST-094 FR36 commits only when the projected travel reaches the commit point. Either FR36 is amended to this measured rule, or the dismissal follows FR36.
| /** The deceleration rate whose projection reaches `seconds` of the velocity. */ | ||
| function rateForProjection(seconds: number): number { | ||
| // WWDC18's projection coasts (v / 1000) · r / (1 − r): r / (1 − r) = 1000·τ. | ||
| const ms = seconds * 1000; | ||
| return ms / (1 + ms); | ||
| } |
There was a problem hiding this comment.
This helper, and the flick and cancel branches below, re-implement motion/resolveSnapPoint.ts, whose header says it "is the one 'where does a release land' helper, so a second one should not be added beside it". Calling resolveSnapPoint with projection: {seconds: 0.1, minSpeed: 250}, flickThreshold: 1008, tolerance: 1 and start, and keeping only the sheet's dismissal mapping, would leave one release policy in core (architecture:react-component-runtime INV13).
| type: 'ReadonlyArray<number | string>', | ||
| description: | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim.", | ||
| "Extra heights the sheet can rest at when dragged; its own height is always the tallest stop, and omitting this gives a sheet that only opens and closes. Each stop is the sheet's visible height: a number is a viewport fraction (0.5 is half the screen), '50%' the same in CSS, '320px' an absolute length. A stop of a quarter of the sheet or less is a peek: it slides away instead of reflowing, and thins the scrim. Under a finger, a released drag settles as an iOS sheet does: on the stop nearest where its speed carries it (a quick flick goes at least to the next stop), dismissal included, with iOS's spring; elsewhere it settles as before, on the theme's motion.", |
There was a problem hiding this comment.
The code keys on the primary pointer (releaseMotion.ts:104, spec:AST-094 FR13), not on the pointer that drags: a touchscreen laptop's finger gets the theme rule, and a mouse on a coarse-primary tablet gets the iOS one. "Where the primary pointer is coarse" would describe it accurately, here and in .changeset/bottom-sheet-uikit-release.md.
Depends on #8068, #8360 and #8423. Review only this PR's change: https://github.com/vjeux/astryx/compare/4166bc935bae01ab785dd366c083ebb7b73287f9..0ca61da23fe827e1358eb0178d50f1f0736e5094
Primary intent
A BottomSheet a finger lets go of settles on iOS's release spring at the finger's speed, and a finger can catch it mid-settle.
User or maintainer impact
On a phone, a released sheet started from a standstill on the theme's curve, so a quick flick lost its speed, and a busy page made the settle stutter. Now, under a finger:
A mouse keeps its release, except that a throw held still before letting go no longer flicks.
Problem and solution fit
#8068 decides where a release lands; this PR decides how the sheet gets there (
spec:AST-094FR36–FR40). Options:Authority
spec:AST-094FR5, FR6, FR24, FR36–FR40;spec:AST-100FR2, FR4;design:reduced-motion;spec:AST-107FR7 with INV13's gaps (#8751).Change and evidence
iOS simulator (red dot: the finger), then Android emulator:
The WebKit fix, tapping a settling sheet's drawn top (before, after):
Scope
Testing
How to check, in the Snap points story on a phone: drag and let go slowly, then flick (the sheet keeps your speed and springs to a detent); touch it while it settles (it stops); turn on Reduce Motion and release (it fades in place).
What ran: the whole core package passes. In Chromium and WebKit with real touch, the settle stays within 0.5 px of UIKit's spring at three speeds, even through a 700 ms busy main thread. On a touchscreen laptop a finger still pulls the sheet. Mouse throws land where they did on main, except the two held still before the lift. Two failures also fail on main: one CI shard (#8671) and one accessibility check (#8647).