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 owner decision: the scrim's new behavior (thinning linearly to the dismissed position and staying locked to the sheet through drags and releases) is not covered by any current spec, and the draft spec:AST-094 this pull request adds is not yet approved. An engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or name the record that should govern the scrim's mapping and timing. Two fixes are needed whatever that decision is:
- Closing the sheet while a release is still settling (Escape, back, or
isOpen={false}) skips the close fade: the0sduration pin and the::backdropanimation keep the page fully dimmed until the dialog closes, so tracking should end when the sheet starts closing. - The diff against
maincarries the whole stack (about 26,000 authored lines); landing #8045, #8068, #8105 and #8109 first leaves this pull request's own 611-line commit to review.
[Automated review]
| return; | ||
| } | ||
| if (tracking) { | ||
| dialog.style.setProperty('--_sheet-scrim-duration', '0s'); |
There was a problem hiding this comment.
This pin stays on until the release's handle finishes. If the sheet closes first (Escape, back, isOpen={false}), the close effect writes --_sheet-scrim-opacity: 0 under a 0s duration while the ::backdrop animation still holds the dim, so the close fade never plays. Lift the pin and stop the scrim follow when the sheet starts closing.
| return; | ||
| } | ||
| if (dimFor != null) { | ||
| endScrimTracking(dimFor(offset)); |
There was a problem hiding this comment.
When the sheet has started closing, this writes the release's target dim back onto the closing (or closed) dialog. Consider skipping the opacity write when isOpenRef.current is false, and ending tracking from the close path instead.
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 | +15.7KB |
+16B |
| core/DateInput | +16.0KB |
+16B |
| core/DateTimeInput | +16.0KB |
+16B |
| core/PowerSearch | +15.8KB |
+21B |
| core/TimeInput | +16.2KB |
+19B |
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 | +19B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 504 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
dd6e04f to
b8880d8
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the scrim's new behavior (thinning linearly to clear only at the dismissed position, locked to the sheet with the backdrop's easing switched off) and on the draft spec:AST-094 it rests on; 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.
Needed whatever that decision is:
- Closing the sheet while a drag release is still settling skips the scrim's close fade and later writes the release's dim back onto the closed dialog (
BottomSheet.tsx:367,useSheetGestures.ts:1054), so please hand the easing back on close and add a test that closes before the spring lands. - The diff carries about 27,000 authored lines from the stacked pull requests underneath it, so please land or split those so this one carries only its own commit.
- The changed reopen Effect and the new spring-back layout Effect in
useSheetGestures.tsshould name the external systems they synchronize.
[Automated review]
d384a3c to
0a98094
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the scrim's new behavior (thinning linearly to clear only at the dismissed position, locked to the sheet with the backdrop's easing switched off) and on whether draft spec:AST-094, which does not state that mapping itself, is the record that governs it; 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 is, these need changes:
- Closing the sheet while a drag's release is still settling keeps the page dimmed through the exit and then writes the release's dim back onto the closing dialog (
BottomSheet.tsx:367,useSheetGestures.ts:1054), so please hand the easing back when the sheet starts closing and add a test that closes before the spring lands. - The diff carries about 27,300 authored lines from the stacked pull requests underneath, so please land those first so this one carries only its own commits.
- The changed reopen and spring-back Effects in
useSheetGestures.tsshould name the external systems they synchronize, and the changeset should say the old easing was the theme's medium duration (410 ms by default), not 300 ms.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A BottomSheet's scrim follows the sheet the way an iOS sheet's dim does. Dragged below its shortest detent, the dim thins in a straight line with the sheet's position and clears only when the sheet reaches the bottom of the screen; it used to clear halfway there. It follows the finger at once instead of easing 300 ms behind. When the sheet settles or is flung away, the dim rides the same spring frame for frame, on the compositor. A peek detent's lighter scrim is unchanged. |
There was a problem hiding this comment.
Main's backdrop eases over the theme's --duration-medium, 410 ms by default (BottomSheet.tsx:120), not 300 ms. Also, a BottomSheetSwitcher step gets the new clear point without the pin (BottomSheet.tsx:819 passes no onScrimTracking), so "follows the finger at once" holds for a standalone sheet.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository at this head, so an owner cannot read what FR7 amends. Cite a resolvable record, or drop the amendment until that record lands.
| gestureExitRef.current = null; | ||
| travelHandleRef.current = null; | ||
| animator.set({y: 0}); | ||
| stopScrimFollow(); |
There was a problem hiding this comment.
This changed Effect now also disposes the backdrop animation and calls onScrimTracking(false), and the spring-back layout Effect at line 1322 now starts the dim's animation too. architecture:react-component-runtime INV3 asks each new or changed Effect to name the external system it synchronizes (here the sheet's Web Animations and the dialog's backdrop); neither comment does.
|
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.
|
0a98094 to
8239cc2
Compare
github-actions
Bot
added
the
needs:design-review
…(dim drag), Chromium 158
…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 waits on an engineering owner's decision about the new touch scrim, which thins in a straight line to clear only at the bottom of the screen and follows the sheet with the backdrop's own easing switched off: no current spec covers that mapping or timing, since spec:AST-094 says only how such a motion runs and BottomSheet.spec.md gives the Scrim no position mapping or drag timing. An engineering owner can approve it by commenting /approve-spec <full head SHA> on this pull request (the author can do this as an engineering owner), or name the record that should own the scrim's mapping and timing. Whatever that decision is, these still need changes:
- Flicking the sheet away, or closing it during a touch release, clears the dim in one frame while the sheet is still on screen, because the host writes its exit value under the release's
0spin before the passive close Effect (useSheetGestures.ts:1023-1027) hands the dim back; please hand it back at its on-screen value first and let a flick's dim ride its spring. beginScrimTracking(useSheetGestures.ts:757-760) pins the dim for a mouse wherever the primary pointer is coarse, while the release follows UIKit only for a touch or pen; please pin only for those, so a mouse keeps today's dim asspec:AST-017FR59 requires.- The spring-back Effect's cleanup (
useSheetGestures.ts:1604-1627) stops only the sheet's travel, not the backdrop dim and pin its setup now starts (architecture:react-component-runtimeINV3). - The changeset is tagged
[fix]thoughspec:AST-017FR3 asks for[feat]when a change intentionally changes visual direction, and its flick and mouse sentences do not match the code. - The iOS Safari and Android Chrome claims rest on the simulator and the emulator, while
spec:AST-094asks for real-device evidence, which the description already names as the release gate. - The diff also carries the open stacked pull requests underneath (34,395 authored lines, against 1,230 in this pull request's own commits); please land them first or retarget this pull request onto #8109's branch.
[Automated review]
| useEffect(() => { | ||
| if (!isOpen && scrimOwnerRef.current === 'release') { | ||
| releaseScrimTracking(); | ||
| } | ||
| }, [isOpen, recordSettledLayoutOffset]); | ||
| }, [isOpen, releaseScrimTracking]); |
There was a problem hiding this comment.
This passive Effect runs after the host's layout Effect has already written --_sheet-scrim-opacity: 0 under the release's 0s pin (BottomSheet.tsx:660-675), so cancelling the backdrop animation here lands the dim on 0 in one frame, both for a close during a touch release and for a flick away, whose exit spring is still moving the sheet. Hand the dim back at its on-screen value (what stopScrimFollow() returns) and unpin before the host's exit write, for example from a layout Effect here, which commits before the host's, and leave a drag's own dismissal on its exit spring.
| // by frame; under a mouse the host keeps easing the dim toward each value, | ||
| // as today. | ||
| const beginScrimTracking = useCallback(() => { | ||
| if (!readTouchPresentation().isTouch) { |
There was a problem hiding this comment.
This asks only whether the primary pointer is coarse, and beginDrag calls it before readReleaseMotion reads the pointer type (:1675-1680), so a mouse drag on a tablet is pinned to 0s and its theme-timed release plays the dim on ::backdrop, which spec:AST-017 FR59 and the changeset's mouse sentence rule out. Pin only for a touch or pen drag, as readReleaseMotion decides (releaseMotion.ts:110-115).
| return () => { | ||
| if (travelHandleRef.current === handle) { | ||
| animator.stop(['y']); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The setup now passes lastDimForRef to springTravel, which starts the ::backdrop dim under the pin, but this cleanup stops only the sheet's travel, and a stopped travel resolves unfinished, so nothing unpins (:1290-1292). architecture:react-component-runtime INV3 needs the cleanup to release the backdrop dim and the pin as well, and the comment above to name the backdrop dim.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under a finger, on every touch device, a BottomSheet's scrim follows the sheet the way an iOS sheet's dim does. Dragged below its shortest detent, the dim thins in a straight line with the sheet's position and clears only when the sheet reaches the bottom of the screen; it used to clear 40 % of the shortest detent's height below it. A standalone sheet's dim follows the finger at once instead of easing behind it over the theme's medium duration (410 ms by default). When the sheet settles or is flung away, the dim rides the same spring frame for frame, on the compositor. Under a mouse the dim is unchanged. A peek detent's lighter scrim is unchanged. |
There was a problem hiding this comment.
spec:AST-017 FR3 asks for [feat] when a change intentionally changes visual direction. Also, the dim does not ride the spring when the sheet is flung away (the close cancels it at the lift), and a mouse where the primary pointer is coarse is pinned, so the flick and mouse sentences do not match the code.
…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 waits on an engineering owner's decision about the new touch scrim, which thins in a straight line to clear only at the bottom of the screen and follows the sheet with the backdrop's own easing switched off: no current spec covers that mapping or timing, since spec:AST-094 says only how such a motion runs and BottomSheet.spec.md gives the Scrim no position mapping or drag timing. An engineering owner can approve it by commenting /approve-spec <full head SHA> on this pull request (the author can do this as an engineering owner), or name the record that should own the scrim's mapping and timing. Whatever that decision is, these still need changes:
- Flicking the sheet away, or closing it during a touch release, clears the dim in one frame while the sheet is still on screen (0.62 to 0 at the lift, with the sheet sliding on for 300 ms), because the host writes its exit value under the release's
0spin before the passive close Effect (useSheetGestures.ts:1023-1027) hands the dim back; please hand it back at its on-screen value first and let a flick's dim ride its spring. beginScrimTracking(useSheetGestures.ts:757-760) pins the dim for a mouse wherever the primary pointer is coarse, while the release follows UIKit only for a touch or pen; please pin only for those, so a mouse keeps today's dim asspec:AST-017FR59 requires.- The spring-back Effect's cleanup (
useSheetGestures.ts:1604-1627) stops only the sheet's travel, not the backdrop dim and pin its setup now starts (architecture:react-component-runtimeINV3). - The changeset is tagged
[fix]thoughspec:AST-017FR3 asks for[feat]when a change intentionally changes visual direction, and its flick and mouse sentences do not match the code. - The iOS Safari and Android Chrome claims rest on the simulator and the emulator, while
spec:AST-094asks for real-device evidence. - The diff also carries the open stacked pull requests underneath (36,293 authored lines, against 1,230 in this pull request's own commits); please land them first or retarget this pull request onto #8109's branch.
[Automated review]
| useEffect(() => { | ||
| if (!isOpen && scrimOwnerRef.current === 'release') { | ||
| releaseScrimTracking(); | ||
| } | ||
| }, [isOpen, recordSettledLayoutOffset]); | ||
| }, [isOpen, releaseScrimTracking]); |
There was a problem hiding this comment.
This passive Effect runs after the host's layout Effect has already written --_sheet-scrim-opacity: 0 under the release's 0s pin (BottomSheet.tsx:660-675), so cancelling the backdrop animation here lands the dim on 0 in one frame. In Chromium with real touch on the Snap points story, a flick away takes the backdrop from 0.62 to 0 on the first frame after the lift while the sheet slides on for 300 ms, and an Escape during a release does the same. Hand the dim back at its on-screen value (what stopScrimFollow() returns) and unpin before the host's exit write, for example from a layout Effect here, and leave a drag's own dismissal on its exit spring.
| // by frame; under a mouse the host keeps easing the dim toward each value, | ||
| // as today. | ||
| const beginScrimTracking = useCallback(() => { | ||
| if (!readTouchPresentation().isTouch) { |
There was a problem hiding this comment.
This asks only whether the primary pointer is coarse, and beginDrag calls it before readReleaseMotion reads the pointer type (:1675-1680), so a mouse drag on a tablet is pinned to 0s and its theme-timed release plays the dim on ::backdrop, which spec:AST-017 FR59 and the changeset's mouse sentence rule out. Pin only for a touch or pen drag, as readReleaseMotion decides (releaseMotion.ts:110-115).
| return () => { | ||
| if (travelHandleRef.current === handle) { | ||
| animator.stop(['y']); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The setup now passes lastDimForRef to springTravel, which starts the ::backdrop dim under the pin, but this cleanup stops only the sheet's travel, and a stopped travel resolves unfinished, so nothing unpins (:1290-1292). architecture:react-component-runtime INV3 needs the cleanup to release the backdrop dim and the pin as well, and the comment above to name the backdrop dim.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under a finger, on every touch device, a BottomSheet's scrim follows the sheet the way an iOS sheet's dim does. Dragged below its shortest detent, the dim thins in a straight line with the sheet's position and clears only when the sheet reaches the bottom of the screen; it used to clear 40 % of the shortest detent's height below it. A standalone sheet's dim follows the finger at once instead of easing behind it over the theme's medium duration (410 ms by default). When the sheet settles or is flung away, the dim rides the same spring frame for frame, on the compositor. Under a mouse the dim is unchanged. A peek detent's lighter scrim is unchanged. |
There was a problem hiding this comment.
spec:AST-017 FR3 asks for [feat] when a change intentionally changes visual direction. Also, the dim does not ride the spring when the sheet is flung away (the close cancels it at the lift), and a mouse where the primary pointer is coarse is pinned, so the flick and mouse sentences do not match the code.
Depends on #8109. Review only this PR's change: https://github.com/vjeux/astryx/compare/94b858804cf4e3e45ca279b1fb5e6a969076e32a..d4266b9075ea54800c24699d76be9e7512d97674
Primary intent
Under a finger, a BottomSheet's dim follows the sheet the way an iOS sheet's dim does.
User or maintainer impact
On a phone, the dim behind a dragged sheet cleared 40 % of the way to gone, trailed the finger on the backdrop's 410 ms easing, and eased on its own clock after a release. Now, under a finger:
A mouse keeps today's dim (every pointer: #8386).
Problem and solution fit
On iOS 26.2, UIKit's dim is linear from the shortest detent to gone, locked to the sheet. Options:
::backdropalong the sheet's curve. It costs keeping the open sheet's backdrop on its own layer, so the dim re-rasters nothing.A close during a release hands the easing back, so the close fade runs.
Authority
spec:AST-094FR5, FR13;design:reduced-motion;spec:AST-107FR7 with INV13's gaps (#8751).BottomSheet.spec.mdmakes no claim on the dim's mapping or timing.Change and evidence
iOS simulator (red dot: the finger), a drag 100 pt below half, before and after:
Android (emulator):
Scope
Testing
How to check, in the Snap points story on a phone: drag the sheet slowly below half (the dim thins with it); let go (both settle together).
What ran: the sheet, gesture and motion unit tests pass (2,123); in Chromium (22) and WebKit with real touch, the backdrop's opacity matches the mapping at every move and every sample of a release, and keeps changing through a 700 ms busy main thread, apart from a harness self-test fixed by #8535. Under heavy load that busy proof can fail on WebKit's late first frames; #8906 brings its fix.
touch-support-bundlefails here, as BottomSheet now puts the touch module in the root entry; #8671's per-component check replaces it and passes. Checks ran locally; no CI run is expected.