Repository navigation
fix(core): a BottomSheet touch drag's close keeps its speed, and its scrim fades with it - #8105
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🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TimeInput | 145.3 kB (+1.5 kB) · 15.1 kB CSS (0) | — | — · — · — |
| BottomSheet | 58.5 kB (+1.5 kB) · 7.5 kB CSS (0) | — | ✓ · ✓ · — |
| DateInput | 168.3 kB (+1.5 kB) · 16.1 kB CSS (0) | — | — · — · — |
32 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch | 264.8 kB (+1000 B) · 22.9 kB CSS (0) | — | — · — · — |
| DateTimeInput | 190.3 kB (+708 B) · 17.8 kB CSS (0) | — | — · — · — |
| Markdown | 161.4 kB (+132 B) · 19.5 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 239.6 kB (+123 B) · 454 B CSS (0) | — | — · — · — |
| Selector | 88.8 kB (+120 B) · 16.5 kB CSS (0) | — | — · — · — |
| lab/Comments | 281.6 kB (+116 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/TourStep | 66.9 kB (+114 B) · 590 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 75.2 kB (+113 B) · 1.1 kB CSS (0) | — | — · — · — |
| DropdownMenu | 85.8 kB (+110 B) · 16.6 kB CSS (0) | — | — · — · — |
| Table | 99.6 kB (+110 B) · 17.2 kB CSS (0) | — | — · — · — |
| Popover | 60.8 kB (+109 B) · 13.4 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 275.9 kB (+107 B) · 2.4 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 74.7 kB (+106 B) · 773 B CSS (0) | — | — · — · — |
| MoreMenu | 86.1 kB (+105 B) · 16.6 kB CSS (0) | — | — · — · — |
| Pagination | 98.7 kB (+105 B) · 16.9 kB CSS (0) | — | — · — · — |
| ContextMenu | 90.2 kB (+103 B) · 16.5 kB CSS (0) | — | — · — · — |
| Tokenizer | 80.4 kB (+102 B) · 17.8 kB CSS (0) | — | — · — · — |
| TableCell | 91.4 kB (+101 B) · 16.9 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 91.0 kB (+101 B) · 16.7 kB CSS (0) | — | — · — · — |
| MultiSelector | 88.2 kB (+95 B) · 16.2 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 115.4 kB (+72 B) · 1.9 kB CSS (0) | — | — · — · — |
| SideNav | 81.5 kB (+36 B) · 10.7 kB CSS (0) | — | — · — · — |
| AppShell | 81.5 kB (+34 B) · 12.8 kB CSS (0) | — | — · — · — |
| TreeList | 41.5 kB (+31 B) · 5.4 kB CSS (0) | — | — · — · — |
| Collapsible | 25.3 kB (+29 B) · 4.5 kB CSS (0) | — | — · — · — |
| Slider | 96.8 kB (+28 B) · 8.3 kB CSS (0) | — | — · — · — |
| SegmentedControl | 88.5 kB (+24 B) · 6.8 kB CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 17.4 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| ProgressBar | 11.5 kB (0) · 6.2 kB CSS (0) | — | — · — · — |
| Switch | 94.6 kB (0) · 9.5 kB CSS (0) | — | — · — · — |
| lab/NavigationScreen | 74.9 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
| lab/NavigationStack | 74.9 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.69 MB packed (+6.3 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 21 rows; run pnpm api:report.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | 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.
|
/approve-spec 7aab77b |
There was a problem hiding this comment.
Changes requested: one fix remains before this can land.
- The touch drag exit plays
springs.sheetDismissSwipeas a plain table constant (BottomSheetPanel.tsx:771), butspec:AST-094FR4 onmainstill requires every spring a gesture plays to be a motion role, so land #9915's FR4 change first (this then clears with no change here) or amend FR4 in this pull request with an implementation-gap note.
The touch release rule (FR19, DEC-4, the compatibility note and the FR36 sentence) is approved for this head, and the FR19 mouse note and its Basis cell are fixed. In Chromium a short fast flick now closes the Filters sheet, a flick from the top of Snap Points stops at half, and a quick mouse drag behaves as on main.
[Automated review]
| const exit = animatorFor(element).animate( | ||
| {y: playedDragExit.distance}, | ||
| { | ||
| spring: springs.sheetDismissSwipe, |
There was a problem hiding this comment.
This spring is still not a motion role: SPRING_ROLES has no sheetDismissSwipe row, and spec:AST-094 FR4 on main says every spring a gesture plays "MUST be a motion role" and a component "MUST NOT play a literal". This clears with no change here once #9915's FR4 carve-out is on main; if this pull request lands first, it needs an FR4 implementation-gap note instead.
There was a problem hiding this comment.
Agreed: this pull request waits for #9915, which merges first, so no change here.
resolveSettleMs gives a spring role's settling time in milliseconds at the tempo of the theme an element is in, so a CSS part that plays with the spring starts and lands with it on one clock. A fixed role, or a spring that is not a role, gives its own.
|
/approve-spec 10b3acb |
There was a problem hiding this comment.
Changes requested: one defect remains now that an engineering owner has approved this head's record changes.
Blocking:
- After a touch drag closes a sheet, the scrim's fade runs on the stylesheet's 300 ms (its duration is written in the panel's exit effect after the fade has started,
BottomSheetPanel.tsx:863) while the close spring the dialog waits on lasts about 700 ms, so the page looks clear but swallows taps until about 730 ms, againstspec:AST-108FR4a; decide the fade in the release callback before the scrim's opacity changes, time it from the curve the animator plays rather than the spring's settle from rest (resolveMotion.ts:155), and test the running::backdroptransition rather than the declared duration.
The touch release rule, the FR37 cancel and the spring exit otherwise behave as the approved records describe in Chromium, and mouse and pen drags are unchanged.
[Automated review]
| registerSpringsTable(); | ||
| // The scrim fades for as long as the close spring takes to settle at | ||
| // the theme's tempo, so the two land together. | ||
| onScrimFadeDurationRef.current?.( |
There was a problem hiding this comment.
This runs when the panel enters its exit state, a commit after BottomSheet.tsx:733 set the scrim's opacity to 0. In Chromium the ::backdrop opacity transition has already started on the stylesheet's 300 ms by then, at tempo 1 and 1.5 alike, though transition-duration reads 0.531 s and 0.796 s. The value is also the spring's settle from rest (531 ms), while the exit curve the dialog waits on lasts about 700 ms. Deciding the duration in the release callback (closeSheet in useSheetGestures.ts) from the curve the animator plays would keep the scrim and the close on one clock (spec:AST-108 FR4a).
There was a problem hiding this comment.
Fixed in c3d9af2. The fade is now set as the host takes the close, in a panel layout effect that runs before the host clears the scrim. It is timed from the move the animator plays: resolveSettleMs now takes the move (distance, release velocity, the animator's threshold). The release spec reads the running ::backdrop transition: at tempo 0.75, 1 and 1.5 it lasts 535, 710 and 1,056 ms, the same as the close's curve, and the dialog closes about 45 ms after it ends.
| * How far ahead a release is projected, as seconds of its velocity. | ||
| * UIKit's sheet settles on the position nearest `offset + velocity × | ||
| * this`, and the dismissed position (fully off screen) counts as one of | ||
| * them. So a release dismisses only when it lands nearer gone than any |
There was a problem hiding this comment.
Non-blocking: with the flick rule, a flick from the top dismisses once its projection passes the lowest detent, so "a release dismisses only when it lands nearer gone than any detent" no longer holds. Suggested: "A slow release settles on the position nearest this landing. A flick goes to the first position at or past it."
…d turning back cancels A touch drag's release takes UIKit's measured sheet values. It is projected 0.1 s ahead (measured), with closed counted as one position, and a release slower than 250 px/s is not projected (measured). A release at 1,008 px/s or faster is a flick, as UIKit's sheet counts one. It lands on the first position at or past its projection, and at least on the next one. So a short fast flick at the lowest detent closes the sheet instead of springing back. From a higher detent, a flick that 0.1 s of its speed carries past the next detent goes on: a hard flick closes the sheet, and a softer one stops at the next detent (AST-094 FR36). A faster touch release never travels less than a slower one. A touch release moving back toward where the drag began cancels to that detent (AST-094 FR37). The touch release calls the shared release policy, resolveSnapPoint, whose new flickPastProjection option lands a flick at or past its projection. A slow or medium touch throw now travels less than it did, and a slow touch drag closes the sheet only past halfway from the lowest detent to closed. A sheet a touch drag closes holds where the finger left it, then leaves on its close spring (springs.sheetDismissSwipe, UIKit's measured swipe dismissal, which AST-094 FR4 and FR45 keep for a gesture's motion), played by the shared animator from the release speed. Its scrim's fade is decided as the host takes the close, before the scrim starts to clear. It lasts as long as the animator plays that move at the theme's tempo (resolveSettleMs, which now also times one move), so the two finish together (AST-094 FR45's spring tiers, AST-108 FR4a). A reopen during the exit takes it back. Under reduced motion the stylesheet's exit runs. A mouse or pen drag keeps its release: the scroll projection and the dismiss line, with no flick, no close spring and no paced scrim. The BottomSheet record's FR19 adds these touch values to the release rule, with the mouse and pen differences as implementation gaps, and its verification rows merge. AST-094 FR36's BottomSheet sentence names the touch values. DEC-4 and its compatibility notes say so.
|
/approve-spec c3d9af2 |
There was a problem hiding this comment.
Changes requested: the touch release and its record changes are approved for this head, and one defect remains.
Blocking:
- Miss: after a touch drag closes a
BottomSheetSwitcherflow, its scrim fades on the stylesheet's 300 ms while the close spring plays about 700 ms, so the page looks clear but a tap 450 ms after the release is lost, againstspec:AST-108FR4a, because only the standalone sheet's panel getsonScrimFadeDuration(BottomSheet.tsx:976) and the switcher step's panel (BottomSheet.tsx:1313-1330) gets none; give the switcher step the same fade, or let the panel write it on its own dialog, and add a switcher case to the drag-exit tests.
On a standalone sheet the scrim fix works: in Chromium the running fade lasts exactly as long as the close at tempo 0.75, 1 and 1.5, and the release outcomes match the approved FR19. Optionally, tighten the Chromium scrim case from 100 ms to one frame, as FR4a's verification asks.
[Automated review]
| xstyle={xstyle} | ||
| onDismiss={dismissOnLightInteraction} | ||
| onScrimOpacity={handleScrimOpacity} | ||
| onScrimFadeDuration={handleScrimFadeDuration} |
There was a problem hiding this comment.
Only the standalone host passes this. SwitcherBottomSheetItem's panel (around line 1326) plays the same drag exit with no fade handler, so after a touch drag closes a switcher flow the scrim clears on the stylesheet's 300 ms while the dialog stays up about 730 ms; in Chromium a tap on the page 450 ms after the release is lost (spec:AST-108 FR4a). Passing the same handler for the scrim-owning step, or having the panel write the fade on its own dialog, would cover both hosts.
There was a problem hiding this comment.
Fixed in 7ab8a89. The switcher step passes the same fade handler, and both hosts share one writer. A drag-closed switcher flow's scrim now fades with its 710 ms close instead of 300 ms (a new drag-exit unit case and a Chromium case cover it).
| expect( | ||
| Math.abs(fadeEnd - (atTempo.closeStart + atTempo.close)), | ||
| ).toBeLessThan(100); |
There was a problem hiding this comment.
Non-blocking: spec:AST-108 FR4a's verification fails paired animations that end more than one frame apart. A 100 ms bound would pass a fade that ends six frames early; about 17 ms would guard the claim (they measure one frame apart here).
There was a problem hiding this comment.
Done in 7ab8a89: the bound is one frame (17 ms). The fade now takes the close's start time, so they end together (0.0 ms in 100 runs under a 2-core pin).
|
/approve-spec c3d9af2 |
useSheetGestures.ts: main's refused-dismissal branch (meta#10141) runs in this branch's closeSheet, for touch and for mouse or pen. A refused request counts itself, and the scrim returns to the detent the drag left. A taken close keeps its drag exit.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision, and one defect needs a fix either way.
This head merges main. The proposed component:BottomSheet FR19, DEC-4, its verification row and compatibility note, the spec:AST-094 FR36 BottomSheet sentence and the springs.sheetDismissSwipe motion-role row are unchanged, but the earlier /approve-spec covered the previous head. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this head (the author may, as an engineering owner).
Blocking:
- Miss: after a touch drag closes a
BottomSheetSwitcherflow, its dim clears in about 260 ms while the dialog stays up until about 730 ms, so a tap on the page 450 ms after the release is lost, againstspec:AST-108FR4a, because only the standalone sheet passesonScrimFadeDuration(BottomSheet.tsx:976) and the switcher step's panel (BottomSheet.tsx:1313-1330) does not; give the switcher step the same fade, or let the panel write it on its own dialog, and add a switcher case to the drag-exit tests.
Optional:
- Regression:
springs.sheetDismissSwipeis now the only spring role row not defined withspringRole, so it becomes a role only whenregisterSpringsTable()runs (inline note). - The Chromium scrim case could assert one frame instead of 100 ms, as FR4a's verification asks.
[Automated review]
| menuClose: MENU_CLOSE_SPRING, | ||
| subMenuOpen: SUB_MENU_OPEN_SPRING, | ||
| subMenuBack: SUB_MENU_BACK_SPRING, | ||
| sheetDismissSwipe: SHEET_DISMISS_SWIPE_SPRING, |
There was a problem hiding this comment.
Non-blocking: now that main makes each spring role row register itself on import, this is the one SPRING_ROLES row that is a plain object (SHEET_DISMISS_SWIPE_SPRING, line 246) rather than springRole('sheetDismissSwipe', ...). It becomes a role only when something calls registerSpringsTable(), which BottomSheetPanel.tsx does twice for this reason, and the doc on registerSpringsTable() now says a call "registers nothing new" and counts 18 rows. Defining the row with springRole would let both calls go.
There was a problem hiding this comment.
Done in 7ab8a89: sheetDismissSwipe is a springRole row like the others, and the panel no longer calls registerSpringsTable().
motion/uikit/index.ts: main's header and exports, with this branch's sheet export.
|
/approve-spec 86761b0 |
There was a problem hiding this comment.
Changes requested: one defect remains now that an engineering owner has approved this head's record changes.
The touch release rule, the drag exit and the springs.sheetDismissSwipe role match spec:AST-094 and the approved component:BottomSheet FR19 and DEC-4.
Blocking:
- Miss: after a touch drag closes a
BottomSheetSwitcherflow, its dim clears in about 260 ms while the dialog stays up until about 730 ms, so a tap on the page in between is lost, againstspec:AST-108FR4a, because only the standalone sheet passesonScrimFadeDuration(BottomSheet.tsx:976) and the switcher step's panel (BottomSheet.tsx:1313-1330) does not; give the switcher step the same fade, or let the panel write it on its own dialog, and add a switcher case to the drag-exit tests.
Optional:
- Regression:
springs.sheetDismissSwipeis the only spring role row not defined withspringRole, so it becomes a role only whenregisterSpringsTable()runs. - The Chromium scrim case could assert one frame instead of 100 ms, as FR4a's verification asks.
[Automated review]
|
/approve-spec 7ab8a89 |
There was a problem hiding this comment.
Changes requested: one scope item remains, and the earlier defects are fixed.
The touch release, the drag close and the springs.sheetDismissSwipe role match spec:AST-094 and the owner-approved component:BottomSheet FR19 and DEC-4. A drag-closed BottomSheetSwitcher flow's scrim now ends with its close.
Blocking:
- The pull request now changes 2,073 authored lines, over the 2,000-line limit, in two parts that can land apart (the touch landing rule, about 940 lines, and the touch drag's close with its scrim clock, about 1,130), so move the close into a second pull request on top of this one and declare both with one
Stack:line, or state in the description why the two cannot land apart.
Optional:
- Two new comments in
BottomSheetPanel.tsx(lines 163 and 929-931) could follow INV29's style; suggestions are inline.
[Automated review]
| return ( | ||
| effect?.target === dialog && | ||
| (animation as CSSTransition).transitionProperty === 'opacity' && | ||
| // Not a fade the drag started, which a style update replaces. |
There was a problem hiding this comment.
Non-blocking (INV29 style): this reads as vague. Suggest:
// Skip a fade that the drag started. A style update replaces that fade.| // The scrim's fade follows the close. Once the close has started, the | ||
| // fade takes its start time, so the two end together. A fade that is | ||
| // not styled yet is looked for again on the next few frames. |
There was a problem hiding this comment.
Non-blocking (INV29 style): "is looked for again" is passive. Suggest:
// The scrim's fade follows the close. Once the close starts, the fade takes
// its start time, so the two end together. If the fade has no style yet,
// look for it again on each of the next few frames.
|
On the scope item: the two parts cannot land apart. A flick that now closes the sheet leaves at its speed only on the close spring, so the landing rule alone would close more sheets on the stylesheet's close from rest, the stall this fixes. The approved FR19 and DEC-4 describe both, and the spec owners ruled one pull request (Nov 6). The description now says so, with the test share: about 1,500 of the 2,070 authored lines are tests. |
A touch drag that closes a BottomSheetSwitcher flow now paces the flow's scrim on the close's own clock, as a standalone sheet's does. The step a drag closes is the flow's retained step, whose backdrop is the scrim, and it takes the panel's fade duration. Both hosts share one writer, which leaves a stack's 0s pin alone and clears only its own value. The scrim's fade now follows the close on one clock: once the close has started, the running fade that clears the scrim takes the close's start time. The close itself is unchanged. Under load a CSS transition and the animator's close otherwise start frames apart. WebKit reports the backdrop's fade on the dialog itself, so where no backdrop effect is reported, the fade on the dialog is taken. sheetDismissSwipe registers itself at import, like every other role row, so the panel no longer calls registerSpringsTable().
|
/approve-spec 8bcccb5 |
There was a problem hiding this comment.
Changes requested: one scope item remains.
The touch release, the drag close and the springs.sheetDismissSwipe role match spec:AST-094 and the owner-approved component:BottomSheet FR19 and DEC-4, and the release, drag-close and scrim unit suites pass.
Blocking:
- The pull request changes 2,073 authored lines and no size exemption applies (the behavior-module line names
useSheetGestures.ts, which is not a behavior module, and the panel and host add the close itself), while the close and its scrim clock read onlydragExitand can land before the landing rule, so split them into a two-PR stack, close first and the landing rule on top, declared with oneStack:line, or show why neither order can land alone.
CI has not run on this head yet, because the pull request is a draft.
[Automated review]
…scrim fades with it A sheet a touch drag closes holds where the finger left it, then leaves on its close spring (springs.sheetDismissSwipe, UIKit's measured swipe dismissal), played by the shared animator from the release speed. The spring is a motion role that registers itself, so the theme's tempo scales it. Its scrim fades for as long as that close plays (resolveSettleMs now times one move), and the fade takes the close's start time, so the two end together (AST-094 FR45, AST-108 FR4a). A BottomSheetSwitcher flow's scrim does the same. A reopen during the exit takes the sheet back. Under reduced motion the stylesheet's exit runs. A mouse or pen drag keeps the stylesheet's exit. The touch landing rule (the 0.1 s projection, the flick and the cancel), its tests and its records move to the pull request stacked on this one.
|
/approve-spec 62f82d1 |
|
Split as asked: this pull request is now the touch drag's close and its scrim clock. The touch landing rule moved to #10234, stacked on this one (the close can land alone; the landing rule needs it). |
There was a problem hiding this comment.
Changes requested: two fixes before this can be approved.
The touch drag close on springs.sheetDismissSwipe and its scrim clock match spec:AST-094 FR38 and spec:AST-108 FR4a. The BottomSheet and motion unit suites pass.
Blocking:
- The title and description still describe the touch landing rule this head moved out (the 0.1 s projection, the flick and the moving-back cancel, with its evidence and DEC-4), so retitle the pull request and rewrite the description for the drag close and its scrim only.
- FR19's verification row now fails "a mouse drag that plays the close spring", but the FR19 text no longer calls the mouse and pen stylesheet exit an implementation gap, which contradicts
spec:AST-094FR38 and FR43; put the gap sentence back in FR19.
A new commit needs an engineering owner's /approve-spec for its head, and the author may comment it as an engineering owner. CI has not run on this head while the pull request is a draft.
[Automated review]
| | FR17 | A BottomSheet open on the server, by its own `isOpen` or as a `BottomSheetSwitcher`'s active sheet, renders closed until hydration (no open styles, no `aria-modal`, the markup of a closed sheet) and opens in the render after hydration, the first that can enter its native host; it never claims a modality it does not have (`spec:AST-020` FR1, WCAG 2.2 SC 4.1.2). The server and hydrating renders produce the same markup (`spec:AST-013` FR9), and the open after hydration is settled, with no entry motion; later opens and closes animate as usual (`spec:AST-013` FR13). | `spec:AST-020` FR1 | | ||
| | FR18 | A finger's pull up past the tallest detent stretches the sheet on `spec:AST-094` FR3's rubber band, the same on every pull, with no hold. The sheet's surface reaches below the screen far enough for the whole stretch and the release, so the page under it never shows. A pull on the sheet's body stretches it as a pull on the grabber does, once the content has no room left to scroll that way: content that fits stretches at once, and a long list scrolls to its end first, then holds still while the sheet stretches. At every detent, a list that reaches its end mid-pull holds still while the sheet takes the rest of the pull. A sheet the caller sizes or styles keeps 48 px below the screen, and its stretch stops there. An inline block size, any `xstyle` and any class name count, because the sheet cannot read their rules. A mouse or a pen keeps its own resisted stretch, at most 48 px (`spec:AST-017` FR59). | DEC-3; `spec:AST-094` FR3 | | ||
| | FR19 | A release settles on the detent nearest its projected position (`spec:AST-094` FR36); a release whose velocity points back toward its start returns to its starting detent (`spec:AST-094` FR37). An `info` sheet whose projection passes the dismiss point closes; a `form` or `required` sheet settles on its shortest detent (FR7). Implementation gap: the settle plays `--ease-standard` over the theme's `--duration-medium`, at a speed set by the distance left rather than the finger's (`spec:AST-094` FR38). Implementation gap: in WebKit, the settle stops advancing while the main thread is blocked (`spec:AST-094` FR5). Implementation gap: a release whose velocity points back toward its start still commits when it follows a net downward drag and its projection passes the dismiss point, or when its projection lies nearer the next detent in the drag's direction (`spec:AST-094` FR37). Implementation gap: a release after 48 px of travel or less projects nothing: it is judged where the finger lifted, so a short fast flick closes an `info` sheet only when the finger itself lifts past the dismiss point (`spec:AST-094` FR36). | `spec:AST-094` FR5, FR36, FR37, FR38; FR7 | | ||
| | FR19 | A release settles on the detent nearest its projected position (`spec:AST-094` FR36); a release whose velocity points back toward its start returns to its starting detent (`spec:AST-094` FR37). An `info` sheet whose projection passes the dismiss point closes; a `form` or `required` sheet settles on its shortest detent (FR7). A sheet that a touch release closes leaves on its close spring from the release speed (`spec:AST-094` FR38), UIKit's measured swipe dismissal `springs.sheetDismissSwipe`, a motion role that the theme's `tempo` scales (`spec:AST-094` FR4, FR45). Its scrim fades for as long as that spring takes to settle at the theme's `tempo`, so the two finish together (`spec:AST-094` FR45's spring tiers; `spec:AST-108` FR4a). Under reduced motion its exit follows FR15. Implementation gap: the settle on a detent plays `--ease-standard` over the theme's `--duration-medium`, at a speed set by the distance left rather than the finger's (`spec:AST-094` FR38). Implementation gap: in WebKit, the settle stops advancing while the main thread is blocked (`spec:AST-094` FR5). Implementation gap: a release whose velocity points back toward its start still commits when it follows a net downward drag and its projection passes the dismiss point, or when its projection lies nearer the next detent in the drag's direction (`spec:AST-094` FR37). Implementation gap: a release after 48 px of travel or less projects nothing: it is judged where the finger lifted, so a short fast flick closes an `info` sheet only when the finger itself lifts past the dismiss point (`spec:AST-094` FR36). | `spec:AST-094` FR4, FR5, FR36, FR37, FR38, FR45; `spec:AST-108` FR4a; FR7 | |
There was a problem hiding this comment.
This row no longer says that a mouse or pen close keeps the stylesheet's exit as a gap, while the verification row (line 395) fails "a mouse drag that plays the close spring". Current spec:AST-094 FR38 has the commit start at the release velocity "under every pointer (FR43), on a spring", and FR43 says "a release from any of them settles on FR45's spring from the release velocity". The previous head carried the sentence; putting it back keeps the record and AST-094 consistent:
Implementation gap: a mouse or pen close plays the stylesheet's exit, not the close spring (`spec:AST-094` FR43, FR45).
There was a problem hiding this comment.
Put back at 5518789, verbatim from the previous head, with FR43 added to the basis cell.
| } | ||
| const current = dialog.style.getPropertyValue('--_sheet-scrim-duration'); | ||
| if (ms == null) { | ||
| // Only its own value: a stack's 0s pin in the meantime stays. |
There was a problem hiding this comment.
Non-blocking (INV29 style): this is a fragment. Suggest:
// Remove only the value this host wrote. A stack's 0s pin written since then stays.…s as a sentence FR19 states again that a mouse or pen close plays the stylesheet's exit, not the close spring, as an implementation gap against AST-094 FR43 and FR45. The scrim-fade clear's comment becomes a sentence.
|
On the title and description: that review read them as they were at the 07:23:01Z push. The rename and the new description followed at 07:23:40Z. They describe only the drag close and its scrim; the landing rule's text is in #10234. |
|
/approve-spec 5518789 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because every test, lint and typecheck job skips while the pull request is a draft; it runs once the pull request is marked ready for review.
The touch drag close on springs.sheetDismissSwipe and its scrim clock match spec:AST-094 FR38 and FR4 and spec:AST-108 FR4a, and an engineering owner approved the component:BottomSheet FR19 changes and the motion role row for this head. The earlier requests are addressed: the title and description now cover only the drag close and its scrim, and FR19 again names the mouse and pen exit as an implementation gap. The BottomSheet and motion unit suites, core's typecheck and the new Chromium release specs pass locally on this head.
[Automated review]
Stack: #8105 → #10234 (the touch landing rule, which needs this close spring)
User impact
On a phone, a sheet a touch drag closed ran the stylesheet's close from rest instead of keeping the drag's speed, so the close stalled where the finger left it.
Problem and solution fit
spec:AST-094FR38), and its scrim faded on the stylesheet's own clock.springs.sheetDismissSwipe(UIKit's measured swipe dismissal, now a self-registering motion role thattemposcales) from the release speed, on the shared animator.resolveSettleMs, which now times one move) and takes the close's start time, so the two end together. A BottomSheetSwitcher flow's scrim does too.Expected behavior and authority
component:BottomSheetFR19's close and scrim sentences;spec:AST-094FR4, FR38, FR45 and the motion role table;spec:AST-108FR3 and FR4a. Owner-gated.Smallest restoration
The exit on the shared animator, one motion role, a settle time for one move, and the scrim's fade on the close's clock. No public API.
Evidence
mainplus these tests, not for landing): onmaina 3,000 px/s touch close runs the stylesheet's 300 ms transition from rest at every tempo. The arm fails 6 of the 7 release and scrim-clock cases in Chromium and 15 unit cases.Scope
Writing rubric results
R1–R6 pass for FR19's close and scrim sentences, its verification row and the motion role table row.
Testing
Local (Linux), at 0b72d3b. This head changes only FR19's cells and two comments, re-checked here with
check:knowledge, lint, BottomSheet's units (540) andpnpm review:pre. At 0b72d3b: coretsc; units for BottomSheet, motion, hooks, SegmentedControl and Slider (2,696);CI=true pnpm linton the changed files (0 errors); Prettier;check:knowledge;check:motion-roles(36 roles); the Changeset coverage check;pnpm review:pre(0 findings). On a Storybook built from that code: the release and scrim-clock specs 7 of 7 in Chromium (twice) and the scrim-clock case in WebKit (twice); BottomSheet's a11y specs in Chromium 71 of 71 and in WebKit 51 passed, 2 skipped; its interaction specs in Chromium 15 of 15 and in WebKit 6 passed, 9 skipped. Under a 2-core pin with 4 workers, the five end-together cases pass 100 of 100 (×20), each fade ending 0.0 ms from its close. On the red-first arm the WebKit scrim-clock case passes, because onmainthe close and the fade are both CSS transitions.