Repository navigation
fix(core): a BottomSheet pulled past its top stretches on every pull, on the iOS rubber band - #8109
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 waits on a spec decision: no current record governs BottomSheet's touch drag and release, so an engineering owner needs to identify the governing spec or approve the draft requirements in spec:AST-094 (FR1-FR23, DEC-1, DEC-2) by commenting /approve-spec 33fd380cea15e0362c892d8ce834eff356d5a342 on this pull request, which the author may do as an engineering owner. Independently of that decision, please:
- land or rebase the stacked predecessors so this pull request holds only the 160-line tracking commit instead of 25,457 authored lines;
- reconcile FR3, which requires a 0.55 rubber band on a sheet, and FR9, which requires the shared recognizers, with the sheet that now holds still above its tallest detent and keeps its own handlers;
- keep a fast upward release inside the 48 px reserve under the sheet (it overshoots 66 px at 3,000 px/s from 5 px below fully open), and give the dismissal-declined effect a named system and a cleanup.
[Automated review]
| millisecond, 0.99 for the fast rate), a drag past an edge MUST resist with | ||
| UIKit's rubber band `(1 − 1 ÷ (x·c ÷ d + 1))·d` with the coefficient its use | ||
| measures (0.55 for a scroll view and a sheet), and a release MUST pick its |
There was a problem hiding this comment.
FR3 requires a 0.55 rubber band for "a scroll view and a sheet", but this pull request makes the BottomSheet hold still above its tallest detent (useSheetGestures.ts:1311). Please state where a sheet bands, if anywhere, and that it holds above its largest detent, so the record matches the sheet it would govern.
| // How far the surface extends below the viewport, so a release that carries | ||
| // the sheet past fully open (the 0.8-damped spring overshoots) never shows a | ||
| // gap under it. | ||
| const OVERSCROLL_PADDING = 48; |
There was a problem hiding this comment.
With springs.sheetReleaseFast (response 0.344 s, damping ratio 0.8) and the finger velocity handed to it, a release 5 px below fully open overshoots 54.7 px at 2,500 px/s and 66.3 px at 3,000 px/s, past this 48 px reserve, so the page shows under the sheet for a moment. Consider capping the release travel above fully open at the reserve, or sizing the reserve from the overshoot at a capped speed.
| // A drag-dismissed sheet whose host keeps it open (a controlled `open` that | ||
| // stays true) springs back to where it rests, as a UIKit sheet does when | ||
| // its dismissal is declined. | ||
| const [dismissRequest, setDismissRequest] = useState(0); | ||
| useLayoutEffect(() => { | ||
| if (dismissRequest === 0 || gestureExitRef.current == null || !isOpen) { | ||
| return; | ||
| } | ||
| gestureExitRef.current = null; | ||
| springTravel( | ||
| settledOffset, | ||
| settledLayoutOffsetRef.current, | ||
| springs.sheetDetent, | ||
| ); | ||
| }, [dismissRequest, isOpen, settledOffset, springTravel]); |
There was a problem hiding this comment.
This effect starts a spring through springTravel without a cleanup, and its comment names a UIKit behavior rather than the system it synchronizes. architecture:react-component-runtime INV3 asks a new effect to name its external system and release what it starts: naming the animator's translate channel and returning a cleanup that stops the travel handle would cover it.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| BottomSheet | 55.6 kB (+580 B) · 7.5 kB CSS (+7 B) | — | ✓ · ✓ · — |
| DateTimeInput | 174.8 kB (+424 B) · 17.2 kB CSS (+5 B) | — | — · — · — |
| TimeInput | 138.9 kB (+417 B) · 14.5 kB CSS (+10 B) | — | — · — · — |
22 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch | 245.7 kB (+409 B) · 22.2 kB CSS (+4 B) | — | — · — · — |
| DateInput | 152.6 kB (+385 B) · 15.5 kB CSS (+10 B) | — | — · — · — |
| lab/CommentThread | 260.5 kB (+147 B) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 265.8 kB (+123 B) · 2.5 kB CSS (0) | — | — · — · — |
| Popover | 52.6 kB (+11 B) · 12.8 kB CSS (+12 B) | — | — · — · — |
| TableHeaderCell | 76.8 kB (+2 B) · 16.1 kB CSS (+6 B) | — | — · — · — |
| Table | 85.4 kB (+1 B) · 16.6 kB CSS (+6 B) | — | — · — · — |
| DropdownMenu | 74.7 kB (−1 B) · 16.0 kB CSS (+7 B) | — | — · — · — |
| MoreMenu | 75.0 kB (−1 B) · 16.0 kB CSS (+7 B) | — | — · — · — |
| MultiSelector | 73.7 kB (−2 B) · 15.4 kB CSS (+8 B) | — | — · — · — |
| Tokenizer | 70.7 kB (0) · 17.2 kB CSS (+5 B) | — | — · — · — |
| Markdown | 146.7 kB (−2 B) · 18.8 kB CSS (+6 B) | — | — · — · — |
| lab/TourStep | 58.6 kB (+3 B) · 590 B CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 17.2 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| ContextMenu | 76.0 kB (−5 B) · 15.9 kB CSS (+5 B) | — | — · — · — |
| TableCell | 77.2 kB (−7 B) · 16.4 kB CSS (+6 B) | — | — · — · — |
| Selector | 79.8 kB (−10 B) · 16.2 kB CSS (+8 B) | — | — · — · — |
| lab/ChatEmojiPicker | 66.4 kB (−8 B) · 773 B CSS (0) | — | — · — · — |
| lab/CommentComposer | 105.4 kB (−9 B) · 1.9 kB CSS (0) | — | — · — · — |
| Pagination | 88.6 kB (−14 B) · 16.5 kB CSS (+4 B) | — | — · — · — |
| lab/ChatReactionBar | 66.9 kB (−10 B) · 1.1 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 229.2 kB (−21 B) · 454 B CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.45 MB packed (+2.5 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 8 rows, core 4 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
33fd380 to
3e0773b
Compare
There was a problem hiding this comment.
Approval waits on a spec decision: no current record governs BottomSheet's touch drag and release, so an engineering owner needs to identify the governing spec or approve the draft requirements in spec:AST-094 (FR1-FR23, DEC-1, DEC-2) by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. Independently of that decision, please:
- land or rebase the stacked predecessors so this pull request holds only the 160-line tracking commit instead of 26,558 authored lines;
- keep a fast upward release inside the 48 px reserve under the sheet: released 5 px below fully open at about 3,000 px/s it overshoots about 61 px, and 13 px of the page shows under it;
- reconcile AST-094 FR3 (a 0.55 rubber band on a sheet) and FR9 (shared recognizers) with the sheet this pull request ships, and retarget the amendment to
spec:AST-090, which has no record onmain; - name the synchronized system and add a cleanup in the effects at
useSheetGestures.ts:1150-1164and:703-728.
[Automated review]
3e0773b to
0a84e33
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the draft spec:AST-094, the only spec behind the new BottomSheet drag, the overshoot cap, the theme motion roles and the two /experimental subpaths. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Before that, FR3, FR4, FR8 and FR9 should describe what the code does, with FR8 deferring to the current reduced-motion record. Whatever the decision, please:
- Land #8105 first, or retarget this branch onto it, so this pull request is reviewed as its own two commits rather than 33,020 lines.
- Pass the theme-resolved spring to
limitOvershoot(useSheetGestures.ts:949), because a retunedsprings.sheetReleaseFastcan overshoot past the 48 px reserve and show the page under the sheet. - Remove the new
platformadaptation condition, or first amend the closedwhenvocabulary inspec:AST-012FR2, which lists onlywidth,height,pointer,contrastandmotion. - Drop the "no component uses it yet" lines from five changesets, and fix the gesture README's 744 px compact width and its
useMotionPresentation()reference.
[Automated review]
| const handed = velocity ?? animator.getVelocity('y'); | ||
| const v = | ||
| motion.kind === 'uikit' && offset === 0 | ||
| ? limitOvershoot( |
There was a problem hiding this comment.
This cap is computed for the table spring, but animator.animate plays resolveMotion(sheet, spring) (motion/animator.ts:1353), the theme's value for that role. With a theme that sets springs.sheetReleaseFast to damping 0.6, a throw released 5 px below this detent at 3,000 px/s is capped for damping 0.8 yet overshoots 57.5 px, past the 48 px reserve. Resolve the spring once and pass the resolved value to both limitOvershoot and animator.animate.
| 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.
"Motion that follows a finger, such as a throw, MUST keep its motion" contradicts the current reduced-motion record. DR1 forbids travel under reduced motion, and DR2 makes a touch drag's release change opacity only, in 200 ms or less. That is what the animator in this pull request already does, so this sentence should defer to DR2.
0a84e33 to
7dbec71
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: no current spec covers the new BottomSheet drag, the overshoot cap, the theme motion roles, the press selection lock or the two /experimental subpaths, so an engineering owner needs to approve the draft spec:AST-094 they are proposed under (or identify another governing spec) and, under spec:AST-017 FR59, decide whether mouse and trackpad drags should change too. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever the decision, please:
- Land #8105 first, or retarget onto it, so this pull request is reviewed as its own two commits rather than 38,068 lines.
- Compute the overshoot cap with the theme-resolved spring, since a retuned
springs.sheetReleaseFastcan still show the page under the sheet. - Keep the release filter's weights, its stillness interval and the flick thresholds out of
@astryxdesign/core/motion/experimental, asspec:AST-100FR2 and FR3 require. - Keep the selection and callout lock off links and text-bearing surfaces, so a long press on a Link still opens the iOS link callout.
- Register the springs table in the themed-spring browser proof (
animator.interaction.browser.spec.ts), which fails in the interaction job on this head. - Show fine-pointer evidence before and after the drag change (or keep today's drag under a mouse), make AST-094 FR8 and FR17 defer to the current reduced-motion record and
spec:AST-100, and correct the six changesets and the gestures README that say no component uses these modules.
[Automated review]
| * tables, each value with its source. | ||
| * @experimental | ||
| */ | ||
| export * as uikit from './uikit'; |
There was a problem hiding this comment.
This re-exports the whole UIKit table, so @astryxdesign/core/motion/experimental exposes the release filter's weights and its 40 ms stillness interval (uikit.UIKIT_GESTURES.velocity) and the sheet's flick threshold (uikit.sheet.release.fastSpeed). spec:AST-100 keeps the stillness interval and every threshold internal (FR2, FR3). Could this export only the springs and timing a theme may retune?
| @@ -283,6 +301,9 @@ function onPointerDown(event: PointerEvent): void { | |||
| gesture: idlePressGesture(), | |||
| pointerId: event.pointerId, | |||
| timer: null, | |||
| // A held finger never starts text selection on the surface (UIKit's | |||
| // controls and cells never do), on any coarse pointer. | |||
| unlock: lockSelection([element], event.target), | |||
There was a problem hiding this comment.
This turns -webkit-touch-callout and text selection off on every pressed usePressFeedback surface for touch and pen, which includes Link's anchor (Link.tsx:424). On iOS Safari a long press on an Astryx Link then no longer opens the link callout (preview, Open in New Tab, Copy Link), and text in ClickableCard or Item rows can't be selected with a long press. Could the lock skip links and text-bearing surfaces, or stay with recognizers that opt in through preventsTextSelection?
| // UIKit's sheet tracks (the release, not the drag, picks the detent). | ||
| // Up past fully open it holds: UIKit's sheet doesn't stretch beyond its | ||
| // largest detent. | ||
| const next = Math.max(0, raw); |
There was a problem hiding this comment.
This clamp has no pointer branch, so mouse and trackpad drags also lose the 40 px magnet and the 0.35 stretch. spec:AST-017 FR59 asks touch work to leave fine-pointer behavior alone unless the pull request names the reason and shows fine-pointer evidence before and after, and a new fine-pointer feel needs its own owner decision. Could the description add mouse before-and-after evidence, or could a fine pointer keep today's drag?
| travel: Math.abs(delta), | ||
| velocity: | ||
| event.timeStamp - state.lastTime < VELOCITY_WINDOW_MS | ||
| ? state.windowVelocity * 1000 |
There was a problem hiding this comment.
The fine-pointer release still compares this 100 ms window speed, next to the shared tracker the touch release reads (line 1472). architecture:interaction-modality INV10 says no Core component keeps its own sample buffer or velocity math, and every flick threshold compares the tracker's release velocity. Could resolveDefaultRelease take the tracker's velocity instead?
| follow UIKit's and come from the table. A pan begins at 10 pt of travel on | ||
| either axis, and its translation excludes that travel; its velocity is | ||
| UIKit's estimate, 0.2 × the newest event's instantaneous velocity + 0.8 × | ||
| the previous event's, never decayed through a rest. |
There was a problem hiding this comment.
"Never decayed through a rest" contradicts current spec:AST-100 FR2 and DEC-1, where a contact still for longer than the stillness interval reads 0, which VelocityTracker in this pull request already does. Could FR17 defer to spec:AST-100 for the release velocity?
|
|
||
| - touch behaviours (this system, the press model, UIKit springs and | ||
| timings) key on a coarse PRIMARY pointer (`TOUCH_QUERY`); | ||
| - layout behaviours key on a compact viewport, narrower than 744 px or |
There was a problem hiding this comment.
The compact width in code is below the theme's md point (768 px by default, spec:AST-012 FR9), not 744 px. Line 445 also points to a useMotionPresentation() that doesn't exist.
7dbec71 to
8480daa
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on spec:AST-094: the 48 px stop in a finger's rubber band and the release velocity scaled to stay inside the sheet's reserve depart from FR3 and FR38 as written, so FR3 and FR38 need to be amended to state that limit, or kept as written with a reserve deep enough for the full band and the throw. 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. Whatever the decision, please:
- Hand
resolveSheetReleasethe unclamped release position (useSheetGestures.ts:1640): a finger that pulls the sheet above fully open and flicks it back down now lands a stop lower instead of cancelling (FR37). - Keep the reduced-motion skip for a mouse's detent tick (
spec:AST-017FR59), correct thesnapPointsdescription, and add the real-device iOS Safari and Android Chrome recordings AST-094's Platform support asks for. - Land the parent pull requests first, or retarget onto #8105, so this pull request carries only its 760-line sheet change.
[Automated review]
| } | ||
| const target = event.currentTarget as HTMLElement; | ||
| const delta = event.clientY - state.startCoord; | ||
| const offset = Math.max(0, state.baseOffset + delta); |
There was a problem hiding this comment.
This clamp reaches resolveSheetRelease as position (line 1276), so a finger that pulls the sheet above fully open and flicks it back down, lifting while still above where it pressed, makes sheetRelease.ts:168 read start - position as 0: no FR37 cancel, and the flick branch sends the sheet to the next stop. In the Snap points story a 100 px pull, an 80 px flick down in 60 ms and a lift 20 px above the press land the sheet on the half stop, where main keeps it fully open. Passing the unclamped state.baseOffset + delta to the release decision, and clamping only where the spring starts, keeps spec:AST-094 FR37.
| ); | ||
| if (target !== baseOffset) { | ||
| // A short tick where the platform vibrates (gestures/haptics.ts). | ||
| playHaptic('impactLight'); |
There was a problem hiding this comment.
This tick now plays for every pointer under reduced motion: main's hapticTick returned under prefers-reduced-motion, while playHaptic never checks it (gestures/haptics.ts:37-38). A mouse release onto another stop with Reduce Motion on calls navigator.vibrate([8]) where it exists, an unnamed fine-pointer change under spec:AST-017 FR59. Skipping it under reduced motion unless releaseMotionRef.current.kind === 'uikit' keeps the mouse path as it was.
| 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.
"Under a finger … elsewhere it settles as before" no longer matches the code: a touchscreen laptop's finger (a fine primary pointer) gets today's rule, and a mouse held still before the lift no longer throws the sheet (spec:AST-100 FR2). Something like "a finger or pen on a touch screen settles as an iOS sheet does; anything else keeps today's rule and the theme's timing, on the release speed every pointer reads" would say what ships.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on spec:AST-094: the 48 px stop in a finger's rubber band and the release velocity scaled to stay inside the sheet's reserve depart from FR3 and FR38 as written, so FR3 and FR38 need to be amended to state that limit, or kept as written with a reserve deep enough for the full band and the throw. 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. Whatever the decision, please:
- Hand
resolveSheetReleasethe unclamped release position (useSheetGestures.ts:1640): a finger that pulls the sheet above fully open and flicks it back down now lands a stop lower instead of cancelling (FR37). - Keep the reduced-motion skip for a mouse's detent tick (
spec:AST-017FR59), correct thesnapPointsdescription, and add the real-device iOS Safari and Android Chrome recordings AST-094's Platform support asks for. - Land the parent pull requests first, or retarget onto #8105, so this pull request carries only its 760-line sheet change.
[Automated review]
| } | ||
| const target = event.currentTarget as HTMLElement; | ||
| const delta = event.clientY - state.startCoord; | ||
| const offset = Math.max(0, state.baseOffset + delta); |
There was a problem hiding this comment.
This clamp reaches resolveSheetRelease as position (line 1276), so for a drag begun at fully open and lifted above it, Math.sign(start - position) is 0 and FR37's cancel never runs (sheetRelease.ts:168); a flick back down at 1,008 px/s or more then goes on to the next stop, and a dismissible sheet with no extra stops closes. spec:AST-094 FR37: "A release whose velocity points back toward the start MUST cancel, whatever the travel so far." Passing the unclamped state.baseOffset + delta as the release position, and clamping only where the spring starts, keeps the cancel.
| ); | ||
| if (target !== baseOffset) { | ||
| // A short tick where the platform vibrates (gestures/haptics.ts). | ||
| playHaptic('impactLight'); |
There was a problem hiding this comment.
This tick now plays for every pointer under reduced motion: main's hapticTick returned under prefers-reduced-motion, while playHaptic never checks it (gestures/haptics.ts:38-39). For a mouse that is a fine-pointer change spec:AST-017 FR59 does not allow; skipping the tick under reduced motion unless releaseMotionRef.current.kind === 'uikit' keeps the touch tick and main's mouse behavior.
| 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.
"Under a finger … elsewhere it settles as before" no longer matches the code: a touchscreen laptop's finger (a fine primary pointer) gets today's rule (releaseMotion.ts:110-113), and a mouse held still before the lift no longer throws (spec:AST-100 FR2). Suggest: a finger or pen on a touch screen settles as an iOS sheet does; anything else keeps today's rule and the theme's timing, on the release velocity every pointer reads.
|
/approve-spec b276ffe |
There was a problem hiding this comment.
Changes requested. An engineering owner approved this head's spec content, so the new BottomSheet FR18 and DEC-3 and the AST-094 BottomSheet line are settled direction; three things still need fixing:
- The long list's content-end hand-off and its hand-back on a reversal (
useSheetGestures.ts, the content-end branch of the body'stouchmovehandler) contradict AST-094 FR14 ("The drag MUST NOT change hands midway, in either direction"), which this pull request leaves unamended, so either amend FR14 here for a sheet's content-end hand-off or decide the list's pull at touch-down and reword FR18. BottomSheet.spec.mdstill says "No new height or gesture precedence rule is introduced." while FR18 introduces one, so remove or reword that line.- AST-094's Platform support asks for real-device evidence for iOS Safari claims, and the body-pull and list-hold claims rest on the iOS Simulator only, so add an iPhone Safari recording of a body pull on the Filters sheet and of a long-list pull past its end.
The rubber band, the deeper reserve on touch devices, the content-fits claim and the unchanged mouse path match AST-094 FR3 and AST-017 FR59, and the new unit tests fail on main and pass here. A new commit voids the spec approval, so the fixed head needs a fresh /approve-spec from an engineering owner, which the author may record as one.
[Automated review]
| // ran out and give the sheet everything past it, so the pull continues | ||
| // into the sheet with no jump and no lost scrolling. | ||
| if (activeOffsetRef.current > 0 && chainAtBottom(armed.scrollers)) { | ||
| if (chainAtBottom(armed.scrollers)) { |
There was a problem hiding this comment.
At the tallest detent this now hands a long list's pull to the sheet midway, and the reversal branch above (:1625-1637) hands it back. AST-094 FR14, unamended here, says: "Who gets a drag that starts inside a scroll area MUST be decided once, when the drag begins … The drag MUST NOT change hands midway, in either direction." Either amend FR14 in this pull request for a sheet's content-end hand-off, or decide at touch-down (a list already at its end goes to the sheet, as fitting content now does) and reword FR18.
There was a problem hiding this comment.
AST-094 FR14 now carries this as its one exception, the spec owners' FR18 rule: at a sheet's tallest detent, an upward pull that scrolls its content to the end goes on to stretch the sheet, and a reversal hands it back to the content. The amended records await the owners' approval at 5ace389.
| | FR15 | Under `prefers-reduced-motion: reduce` the recede, the return from receded, a push's entrance, a pop's exit and the replace handoff — and a `BottomSheetSwitcher` step's, which shares the sheet panel's motion — follow `design:reduced-motion`, keyed on its queries (`prefers-reduced-motion: reduce`, with `pointer: coarse` for the cross-fade): no transform motion runs (DR1) — a covered sheet appears in its receded place and size the frame the push lands and returns the same way, the entering sheet appears in its final place, the retained sheet does not align down; on a coarse pointer the entering sheet fades in and the leaving or retained sheet fades out where it is, opacity only, in 200 ms or less (DR2); a dismissal the release of a touch drag started (a swipe past the dismiss line) fades out in place the same way on every pointer (DR2's drag-release clause); on any other pointer every other change is immediate (DR3); the handoff's steps run in the released order, so the retained sheet is hidden and its dialog closed once its fade ends. The constant page dim through a push, a pop and a handoff (FR6) holds as it does with motion. | DEC-2; `design:reduced-motion` | | ||
| | FR16 | An open modal sheet is a focus region through Core's shared focus-region module (`component:Dialog/DEC-2`): in the top sheet, Tab from the last tabbable control moves focus to the first and Shift+Tab from the first moves focus to the last, so focus never leaves the sheet for the browser's own interface. A covered or closing sheet does not answer Tab, and a non-modal sheet contains nothing (FR11). | `component:Dialog/DEC-2` | | ||
| | 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. A mouse keeps its own resisted stretch, at most 48 px (`spec:AST-017` FR59). | DEC-3; `spec:AST-094` FR3 | |
There was a problem hiding this comment.
"A long list scrolls to its end first, then holds still while the sheet stretches" conflicts with AST-094 FR14, and line 216 of this record still says "No new height or gesture precedence rule is introduced." Reword line 216, and either amend FR14 or this sentence.
There was a problem hiding this comment.
Line 216 now says the record adds this one gesture precedence rule, and AST-094 FR14 names the same exception, so the two records agree (5ace389).
| return chain.every(elementAtBottom); | ||
| } | ||
|
|
||
| /** Some element in the chain has content to scroll. */ |
There was a problem hiding this comment.
Non-blocking (INV29): this JSDoc restates the name chainScrolls; drop it.
| // Only a finger stretches the sheet past MIN_RESERVE, so a device with no | ||
| // touch pointer keeps the smallest reserve. Elsewhere the reserve follows | ||
| // the viewport while the sheet is presented. A closed sheet holds no resize | ||
| // listener. |
There was a problem hiding this comment.
Non-blocking (INV29): keep "Only a finger stretches the sheet past MIN_RESERVE, so a device with no touch pointer keeps the smallest reserve." The last two sentences restate tracksViewport on the lines below.
There was a problem hiding this comment.
Trimmed to its first sentence in 5ace389.
|
/approve-spec 0598907 |
|
/approve-spec 04964dc |
There was a problem hiding this comment.
Changes requested: an engineering owner approved this head's spec content, so the new BottomSheet FR18 and DEC-3 and AST-094's FR14 exception and DEC-26 are settled direction, the new unit tests fail on main and pass here, and the new Chromium tests pass here; two things still need fixing.
- On a device with a touch pointer, a sheet whose caller sets
style.heightnow hides 40% of the viewport below the screen instead of 48 px (a 500 px sheet shows 162 px instead of 452 px on an 844 px phone), so keep the 48 px reserve while the caller owns the height, or add the reserve to that height, with a test. - AST-094's Platform support asks for real-device evidence for iOS Safari claims, and the
overscroll-behavior: nonehold and the iOS results rest on the iOS Simulator only, so please attach an iPhone Safari recording, at the new head, of a body pull on the Filters sheet and of a long list pulled past its end.
Two comment trims are suggested inline and do not block. A new commit voids the spec approval, so the fixed head needs a fresh /approve-spec from an engineering owner, which the author may record as one.
[Automated review]
| paddingBlockEnd: `calc(env(safe-area-inset-bottom, 0px) + var(--_sheet-reserve, ${MIN_RESERVE}px))`, | ||
| marginBlockEnd: `calc(-1 * var(--_sheet-reserve, ${MIN_RESERVE}px))`, |
There was a problem hiding this comment.
A caller's style.height is kept as the element's height, but unlike styles.budget and styles.hugHeight nothing adds this reserve back to it. With the reserve at ceil(0.4 × innerHeight) on a touch device, a 500 px sheet shows 162 px instead of 452 px on an 844 px phone, and a 300 px sheet shows almost nothing. Keep MIN_RESERVE while the caller sets the height (the finger's stretch already stops at the reserve), or add the reserve to the caller's height, and cover it with a touch-pointer test.
There was a problem hiding this comment.
A sheet whose caller sets the height now keeps the 48 px reserve, and the finger's stretch stops there (d7b22cd). A touch-pointer unit renders a 500 px caller height and expects 48px; it was 308px before. FR18 and its verification row say so.
| /** | ||
| * iOS rubber-bands a scroller past its end even under `overscroll-behavior: | ||
| * contain`. While the content sits at its end mid-pull, `none` holds it still, | ||
| * so only the sheet moves. Returns the undo. | ||
| */ |
There was a problem hiding this comment.
This hold exists for an iOS Safari behaviour, and AST-094's Platform support asks for real-device evidence for every iOS Safari claim (spec:AST-013 FR10). The description's iOS results come from the iOS 26.2 Simulator. Please attach an iPhone Safari recording, at the new head, of a long list pulled past its end and of a body pull on the Filters sheet.
There was a problem hiding this comment.
I have no device of my own. The body now cites the owner's phone check (iPhone, Safari) at b276ffe, which covered a body pull on the Filters sheet and a long list pulled past its end, and notes that it did not cover a caller-set height. That case gets its own phone check on this head's preview.
| // Up past fully open, springing back on release. A finger stretches | ||
| // the sheet on UIKit's rubber band over its visible height. A sheet | ||
| // shares a scroll view's coefficient. A touch device's reserve covers | ||
| // the whole stretch, so the clamp binds only where the reserve is short. |
There was a problem hiding this comment.
Non-blocking: the first three sentences restate the call below. Suggest keeping only: "A touch device's reserve covers the whole stretch. The clamp binds only where the reserve is short."
There was a problem hiding this comment.
Trimmed to those two sentences in d7b22cd.
| // armed at `touchstart`. That drag never cancelled the native scroll, so | ||
| // it has to yield again if the finger comes back down. | ||
| promotedAtContentEnd: boolean; | ||
| // Undoes the hold that keeps the content still at its end, while one is set. |
There was a problem hiding this comment.
Non-blocking: this restates releaseContentHold. Drop it, or keep only "Null while no hold is set."
There was a problem hiding this comment.
Now "Null while no hold is set." (d7b22cd).
|
/approve-spec d7b22cd |
There was a problem hiding this comment.
Approval is on hold for one engineering-owner decision, and two fixes are needed either way.
The new content-end hold (overscroll-behavior-y: none while a list sits at its end mid-pull) also runs below the tallest detent, so a list in a sheet resting at a lower detent now holds still at its end while the sheet expands. component:BottomSheet FR18 and the spec:AST-094 FR14 exception (DEC-26) describe the hold at the tallest detent only. An engineering owner needs to decide whether FR18 should state the hold at every detent or the hold should run only at the tallest detent, and records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Blocking fixes:
- A sheet whose height comes from
xstyleorclassNamestill gets the 40%-of-viewport reserve on touch devices, so a 500 px sheet on an 844 px phone shows 162 px instead of 452 px; treat those heights likestyle.height, as FR18 asks for any caller-set height, and cover it with a touch-pointer test. - The changeset says a finger's pull stretches "with no 48 px cap", but a sheet whose caller sets its height deliberately keeps that cap, so the release note should say so.
The rubber band, the content-fits claim, the long-list hand-off at the tallest detent and the unchanged mouse stretch match their records, and the new tests fail on main and pass here.
[Automated review]
| const tracksViewport = | ||
| isPresented && hasTouchPointer && consumerHeight == null; |
There was a problem hiding this comment.
Only style.height counts as a caller height here. A height from xstyle or className (both reach this panel) keeps the viewport reserve, so on an 844 px phone a 500 px sheet shows 162 px instead of 452 px. FR18 asks for 48 px under any caller-set height; please cover those inputs too, with a touch-pointer test.
There was a problem hiding this comment.
A height from xstyle or a class name now counts as a caller size too, along with an inline minHeight, maxHeight or block-size form. StyleX styles reach the panel only as class names (bare hashes in production), so any xstyle or class name counts. Such a sheet keeps 48 px, which is main's reserve. Touch-pointer tests cover each input, and the new "Caller height through xstyle" story shows it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a finger's pull up past the top stretches the sheet on every pull, on iOS's rubber band, with no 48 px cap. The sheet keeps enough of its surface below the screen that the page never shows. A pull on the content stretches it too once the content has no room left to scroll: at once for content that fits, and after a long list reaches its end. A mouse keeps its current stretch. |
There was a problem hiding this comment.
"with no 48 px cap" is not true for a sheet whose caller sets its height: FR18 and the new tests keep the 48 px cap there. Suggest: "…with no 48 px cap, unless the caller sets the sheet's height, which keeps it."
There was a problem hiding this comment.
Reworded: "…with no 48 px cap, unless the caller sets the sheet's height, which keeps it."
| if (chainAtBottom(armed.scrollers)) { | ||
| if (armed.contentEndY == null) { | ||
| armed.contentEndY = t.clientY; | ||
| armed.releaseContentHold ??= holdAtEnd(armed.scrollers); |
There was a problem hiding this comment.
This hold runs at every detent, so a list at a lower detent now holds still at its end while the sheet expands (asserted at the 200 px detent by "lets the list go when the finger comes back to the content end"). FR18 and the AST-094 FR14 exception describe it at the tallest detent only; an engineering owner's decision is needed on which way the record or the code goes.
There was a problem hiding this comment.
The component owner is deciding this one: either the record states the hold at every detent, or the code keeps it at the tallest. This push doesn't change the hold.
There was a problem hiding this comment.
Approval is on hold for one engineering-owner decision, and one fix is needed either way.
The finger stretch, the content-fits pull and the list hand-off at the tallest detent follow the owner-approved records at this head. One behaviour has no governing record yet: the content-end hold (overscroll-behavior-y: none, useSheetGestures.ts:1689-1692) also runs below the tallest detent, so a long list at a lower detent now holds still at its end while the sheet expands, where it bounced before. An engineering owner needs to decide whether component:BottomSheet FR18 states that hold at every detent or the code keeps it at the tallest detent, and records the decision by commenting /approve-spec <full head SHA>; the author may do so if they are an engineering owner.
Blocking:
BottomSheetPanel.tsx:672-675gives anyclassNameorxstyle, even one that sets no size, the 48 px reserve and a stretch that stops at 48 px, while FR18, its verification row and the changeset promise that only to a sheet whose height the caller sets: state that rule in FR18, its verification row and the changeset (for an owner to approve), or count only block sizes the panel can read.
Non-blocking: the new BLOCK_SIZE_PROPERTIES and hasStyles sit between layoutViewportHeight and its comment (BottomSheetPanel.tsx:108-112); moving them above it keeps that comment with its function.
[Automated review]
| const isSizedByCaller = | ||
| BLOCK_SIZE_PROPERTIES.some(property => style?.[property] != null) || | ||
| hasStyles(xstyle) || | ||
| (className != null && className !== ''); |
There was a problem hiding this comment.
Any non-empty className and any xstyle count as a caller height here, so a sheet styled only for colour or theme keeps the 48 px reserve, and a finger's stretch stops at 48 px. FR18 and its verification row exempt only "a sheet whose height the caller sets", and the tests at BottomSheetPanel.test.tsx:164 and :182 assert 48 px for a class name with no rule and a colour-only xstyle. In Chromium phone emulation, a 240 px body pull lifts the Filters sheet 105.4 px, and lifts it 48 px with className="app-sheet". StyleX rules can't be read at runtime, so the smallest fix is to state this rule in FR18, its verification row and the changeset for an owner to approve. The alternative is to count only block sizes the panel can read.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a finger's pull up past the top stretches the sheet on every pull, on iOS's rubber band, with no 48 px cap, unless the caller sets the sheet's height, which keeps it. The sheet keeps enough of its surface below the screen that the page never shows. A pull on the content stretches it too once the content has no room left to scroll: at once for content that fits, and after a long list reaches its end. A mouse keeps its current stretch. |
There was a problem hiding this comment.
"with no 48 px cap, unless the caller sets the sheet's height" overstates the fix: a sheet given any className or xstyle keeps the cap too. Suggest: "…with no 48 px cap, unless the caller sizes the sheet inline or passes any xstyle or className, which keeps it." Or narrow the code to match.
| // the virtual keyboard does not shrink, so `visualViewport` would differ by | ||
| // the keyboard's height. `useMobileKeyboard` handles the keyboard and does | ||
| // not change the detents. | ||
| // The inline style properties that size the sheet. |
There was a problem hiding this comment.
Non-blocking: these declarations now sit between layoutViewportHeight and its comment ("Measure the viewport that the height budgets use…", lines 108-111), so that comment reads as if it describes BLOCK_SIZE_PROPERTIES. Move BLOCK_SIZE_PROPERTIES and hasStyles above line 108. This line restates the constant's name, so it can go.
|
/approve-spec 4fc665c |
1 similar comment
|
/approve-spec 4fc665c |
|
/approve-spec d7b7cea |
There was a problem hiding this comment.
Changes are needed before this can merge; the records themselves are owner-approved at this head.
Blocking:
- Regression:
useSheetGestures.ts:1319-1328gives a finger the 0.55 rubber band but keeps a mouse and a pen on 0.35 of the drag capped at 48 px, whilespec:AST-094FR43 now has a sheet follow a mouse, a pen, a trackpad and a finger alike "with the same rubber band past its ends", so amend FR43 (or AST-094's BottomSheet line) to state that exception for an engineering owner to approve, or have a mouse and a pen follow the finger's band. BottomSheetPanel.tsx:695-698keeps the 48 px reserve and stop for anyclassNameorxstyle, even one that sets no size, while FR18, its verification row and the changeset promise that only to a sheet whose height the caller sets, so state the shipped rule in those records and the changeset (for an owner to approve) or count only block sizes the panel can read.- Miss: the reserve's window
resizelistener (BottomSheetPanel.tsx:133-138) should go throughsubscribeInput(packages/core/src/utils/inputStream.ts), Core's one entry for window listeners, in this pull request rather than a follow-up.
Non-blocking: move BLOCK_SIZE_PROPERTIES and hasStyles above the comment that describes layoutViewportHeight (BottomSheetPanel.tsx:108-127), and consider saying in FR18 that the content-end hold runs at every detent.
[Automated review]
| state.pointerType === 'touch' | ||
| ? Math.max( | ||
| -state.reserve, | ||
| rubberBand( | ||
| raw, | ||
| state.height - state.reserve, | ||
| scroll.rubberBand.coefficient, | ||
| ), | ||
| ) | ||
| : Math.max(-OVERSCROLL_MAX, raw * OVERSCROLL_RESISTANCE); |
There was a problem hiding this comment.
Regression: a finger gets the 0.55 band here and a mouse or a pen keeps 0.35 of the drag capped at 48 px. spec:AST-094 FR43 on main now has a sheet follow "a mouse, a pen, a trackpad and a finger alike, with the same rubber band past its ends", and this pull request leaves FR43 unamended (FR18 keeps the mouse, and does not mention a pen). Either amend FR43 or AST-094's BottomSheet line to state this exception for owner approval, or let a mouse and a pen follow the band.
There was a problem hiding this comment.
AST-094's BottomSheet line now states the exception: past the tallest detent, a mouse and a pen keep FR18's resisted stretch of at most 48 px (spec:AST-017 FR59). A unit pins the pen (076a2a4).
| const isSizedByCaller = | ||
| BLOCK_SIZE_PROPERTIES.some(property => style?.[property] != null) || | ||
| hasStyles(xstyle) || | ||
| (className != null && className !== ''); |
There was a problem hiding this comment.
Any non-empty className and any xstyle count as a caller size here, so a sheet that only sets a colour or a theme class keeps the 48 px reserve, and a finger's stretch stops at 48 px. FR18 exempts only "A sheet whose height the caller sets", and its verification row fails a reserve under 40% "unless the caller sets the height"; BottomSheetPanel.test.tsx:164 and :182 assert the opposite. State this rule in FR18, its verification row, AST-094's BottomSheet line and the changeset, or count only the block sizes the panel can read.
There was a problem hiding this comment.
FR18, its verification row and the changeset now state the shipped rule. An inline block size, any xstyle and any class name count, because the sheet cannot read their rules (076a2a4).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a finger's pull up past the top stretches the sheet on every pull, on iOS's rubber band, with no 48 px cap, unless the caller sets the sheet's height, which keeps it. The sheet keeps enough of its surface below the screen that the page never shows. A pull on the content stretches it too once the content has no room left to scroll: at once for content that fits, and after a long list reaches its end. A mouse keeps its current stretch. |
There was a problem hiding this comment.
"with no 48 px cap, unless the caller sets the sheet's height" overstates the fix: a sheet given any className or xstyle keeps the cap too (BottomSheetPanel.tsx:695-698).
There was a problem hiding this comment.
It now reads "unless the caller sizes or styles the sheet (an inline block size, any xstyle or class name), which keeps it" (076a2a4).
| function subscribeToResize(onChange: () => void): () => void { | ||
| window.addEventListener('resize', onChange); | ||
| return () => { | ||
| window.removeEventListener('resize', onChange); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Miss: this adds a window listener of its own. subscribeInput (packages/core/src/utils/inputStream.ts) is Core's one entry for window listeners, and scripts/lib/pre-review-findings.mjs names it the owner of a new window listener:
const subscribeToResize = (onChange: () => void) =>
subscribeInput(window, 'resize', onChange);There was a problem hiding this comment.
Done in 076a2a4: subscribeToResize returns subscribeInput(window, 'resize', onChange), and review:pre lists no finding.
| // the virtual keyboard does not shrink, so `visualViewport` would differ by | ||
| // the keyboard's height. `useMobileKeyboard` handles the keyboard and does | ||
| // not change the detents. | ||
| // The inline style properties that size the sheet. |
There was a problem hiding this comment.
Non-blocking: this and hasStyles now sit between layoutViewportHeight and its comment ("Measure the viewport that the height budgets use…"), and this line restates the constant's name. Move both declarations above line 108 and drop this line.
There was a problem hiding this comment.
Moved both above the comment, which sits on layoutViewportHeight again, and dropped the line. FR18 now also says the list's hold runs at every detent (076a2a4).
|
/approve-spec 076a2a4 |
There was a problem hiding this comment.
Changes requested: the spec records are owner-approved at this head, and one code fix remains.
- A stylus pulling the sheet's body (an Apple Pencil in iPadOS Safari, which raises touch events with
touchType: 'stylus') gets the finger's uncapped rubber band, because the body path records every touch as'touch'(useSheetGestures.ts:1207,:1319-1327), while FR18, AST-094's BottomSheet line and the changeset say a pen keeps its 48 px stretch; record a stylus as a pen and add a body test for it, or narrow the wording to a pen on the grabber for an owner to approve.
The earlier points (the FR43 exception, the caller-style rule and the resize listener) are resolved at this head. Non-blocking: AST-094's BottomSheet line could add FR18's caller-size exception, and the comment above OVERSCROLL_RESISTANCE could name the pen.
[Automated review]
| // A touch device's reserve covers the whole stretch. The clamp binds | ||
| // only where the reserve is short. | ||
| next = | ||
| state.pointerType === 'touch' |
There was a problem hiding this comment.
Every body touch reaches this branch as 'touch': asPointer (:1569-1578) marks it synthetic and beginDrag records 'touch' (:1207) without reading Touch.touchType. An Apple Pencil in iPadOS Safari raises touch events with touchType: 'stylus', so a stylus pull on the body gets this uncapped band (61.4 px at a 150 px pull on content that fits), while FR18 and the changeset say a pen keeps its 48 px stretch. gestures/GestureHost.ts:356-362 already treats a stylus touch as a pen; recording it as 'pen' here, with a body test, would match.
There was a problem hiding this comment.
Fixed in c58330c: a body pull reads a stylus touch as a pen, as GestureHost does, so an Apple Pencil keeps the 48 px stretch. A body unit pins it: 28 px, then 48.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: a finger's pull up past the top stretches the sheet on every pull, on iOS's rubber band, with no 48 px cap, unless the caller sizes or styles the sheet (an inline block size, any `xstyle` or class name), which keeps it. The sheet keeps enough of its surface below the screen that the page never shows. A pull on the content stretches it too once the content has no room left to scroll: at once for content that fits, and after a long list reaches its end. A mouse or a pen keeps its current stretch. |
There was a problem hiding this comment.
"A mouse or a pen keeps its current stretch" holds on the grabber only: a stylus pull on the sheet's body takes the finger's band (useSheetGestures.ts:1207, :1319-1327), where on the base it moved nothing at the tallest detent. Fix the stylus path, or say "a mouse, or a pen on the grabber".
There was a problem hiding this comment.
The stylus path is fixed in c58330c, so the line now holds on the body too.
… on the iOS rubber band A finger's pull past the tallest detent stretched the sheet by a fixed fraction and held still at 48 px, the depth of the surface the sheet keeps below the screen. A pull on the sheet's body at the tallest detent moved nothing: it went to content that had nothing to scroll. A finger's stretch is now UIKit's rubber band over the sheet's visible height. On a device with a touch pointer the sheet keeps 40% of the viewport of its surface below the screen, more than the whole stretch, so the page under it never shows; the stretch never passes that reserve. A pull on the body counts as a pull on the sheet once the content has no room left to scroll that way. Content that fits is claimed from the first move. A long list hands the rest of the pull to the sheet at its end without claiming the touch, so a reversal still scrolls it back. iOS rubber-bands a scroller past its end even under `overscroll-behavior: contain`, so the list holds still (`none`) while it sits at its end mid-pull. A mouse or a pen keeps its resisted stretch, capped at 48 px. A body pull reads an Apple Pencil's `stylus` touches as a pen, as the grabber does. So does a sheet the caller sizes, through `style`, `xstyle` or a class name: nothing extends its height by the reserve. StyleX styles and class names reach the sheet only as class names, so any of them counts as a size. The BottomSheet record gains FR18 and DEC-3. AST-094 FR14 gains the one exception FR18 makes to deciding a drag once, and AST-094's BottomSheet line says the same. That line also keeps a mouse's and a pen's capped stretch past the tallest detent, an exception to FR43's same rubber band.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready for review, a maintainer adds the ci:run label so CI runs.
The spec records are owner-approved at this head, and the stylus fix holds: a stylus touch on the sheet's body now keeps a pen's 48 px stretch, and its new test fails without the fix.
Non-blocking:
- AST-094's BottomSheet line could add FR18's caller-size exception ("unless the caller sizes or styles it").
- The comment above
OVERSCROLL_RESISTANCEcould name the pen as well as the mouse.
[Automated review]
| moves it. Past its tallest detent, every finger's pull stretches it on FR3's | ||
| rubber band with no hold, over a reserve deep enough for the whole stretch | ||
| and the release. A pull on its content does so once the content cannot |
There was a problem hiding this comment.
Non-blocking: FR18 stops a sheet the caller sizes or styles at 48 px, so "every finger's pull stretches it on FR3's rubber band with no hold" reads wider than the component record. Adding "unless the caller sizes or styles it (FR18)" makes the two records read alike.
There was a problem hiding this comment.
Agreed. This head keeps its approvals, so the line gains "unless the caller sizes or styles it (FR18)" in a small follow-up after this merges.
| // A mouse's stretch past fully open: a resisted fraction of the drag, capped | ||
| // at OVERSCROLL_MAX. |
There was a problem hiding this comment.
Non-blocking (INV29): this branch now serves a pen and an Apple Pencil's stylus touch too. Suggested:
// A mouse's or a pen's stretch past fully open: a resisted fraction of the
// drag, capped at OVERSCROLL_MAX.There was a problem hiding this comment.
Agreed. The comment will name the pen in the same follow-up, so this approved head stays as it is.
|
/approve-spec c58330c |
|
/approve-spec 7c33e08 |
There was a problem hiding this comment.
Approved. The spec records are owner-approved at this head and CI passes.
The change behaves as the records describe: a finger now stretches a fully open sheet from its content and from a list at its end (0 px before, 105.4 px on the Filters sheet for a 240 px pull), while a mouse keeps its 48 px stop.
Non-blocking, as already agreed for a follow-up:
- AST-094's BottomSheet line could add FR18's caller-size exception ("unless the caller sizes or styles it").
- The comment above
OVERSCROLL_RESISTANCEcould name the pen as well as the mouse.
[Automated review]
User impact
On a phone, a pull up on a fully open BottomSheet's content mostly did nothing, and a grabber pull held still at 48 px.
Problem and solution fit
useSheetGesturesgave a content pull to the sheet only below the tallest detent, even with nothing to scroll.The fix stays in the existing path:
overscroll-behavior: none) until the finger lifts or comes back.xstyleor class name) keeps 48 px, and its stretch stops there.spec:AST-017FR59), an exception AST-094's BottomSheet line states to FR43.Expected behavior and authority
component:BottomSheetFR18 and DEC-3,spec:AST-094FR3, FR14's exception, FR43's mouse and pen exception and DEC-28, from the owner's decisions.Smallest restoration
Two conditions and a hold in the body hand-off, one branch in the past-the-top math, and the reserve's depth.
Evidence
mainbounced it 69 px past.Scope
Writing rubric results
R1–R6 pass for FR18, its verification row, DEC-3 and AST-094's BottomSheet line, including its FR43 exception.
Testing
Local, at c58330c on
main298bafb (Linux): the core and Storybook typechecks;CI=true pnpm linton the changed files (0 errors); prettier;check:knowledge; the Changeset coverage check (ships to@astryxdesign/core, named); BottomSheet's unit tests (13 files, 513 tests);pnpm review:pre(0 findings; [b], [d], [f], [g]: the records are owner-gated). At d7b7cea, the same lines:pnpm build;pnpm test:ui --changed 298bafbc10(173 files, 5,444 tests).BottomSheet's a11y contracts and browser specs: Chromium 53 passed; WebKit in CI's profile (4 CPUs, one worker) 39 passed, 2 skipped. With
main's two product files and these tests, the 15 new unit cases fail. On a Storybook built from them, the two new Chromium tests read 0 px. The stylus case fails on 076a2a4's product, in evidence commit e2bc2de (not for landing): 37.2 px, a finger's band.Since 4fc665c: rebased onto
main298bafb; the decision is nowspec:AST-094DEC-28, becausemaintook DEC-26 and DEC-27. Review brought two product changes: the reserve'sresizelistener goes throughsubscribeInput, and a body pull reads an Apple Pencil'sstylustouches as a pen. A finger's pull records the same pointer type as before; only astylustouch differs. So the owner's phone checks no longer carry by the byte-identical rule, and whether a stylus body pull needs a check is the owner's call. The records state the caller-size rule and the mouse and pen exception.iOS 26.2 Simulator (Safari, iPhone 17 Pro),
mainand this head on one lease, as in Evidence.