Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval 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
Modified ComponentsMultiSelector (@astryxdesign/core)
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/BottomSheet | +15.1KB |
0B |
| core/DateInput | +15.3KB |
0B |
| core/DateTimeInput | +15.4KB |
0B |
| core/PowerSearch | +15.1KB |
0B |
| core/TimeInput | +15.4KB |
0B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | 0B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | 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
…tep a frame from the change that ends it Against the control, a frame with no new component position was a miss. On an easing tail the component moves under one cast step a frame, which the screencast cannot show on every frame: Lightbox's dismiss-cancel proof in WebKit missed one of the control's 12 to 14 frames in 7 of 10 runs, each time at its spring's last plateau (210, 210, then 208 on WebKit's 2 px grid). Such a run of unchanged frames now counts as moved when the change that ends it is a single cast step. A freeze ends in a catch-up of more than one step and stays a miss, as does a run that no change ends, and the component must still show a new position on at least minJudgedFrames of the control's frames, so a frame-counted animation that stands still through the block and resumes one step a frame fails too. The cast step is read from the tracked value's changes in the judged part rather than from its positions: the photo's position once the block ended (207) sat off WebKit's 2 px grid and made the step read as 1 px. Red first: the tail, pooled tail, off-grid commit and frame-counted freeze cases fail on the previous busy.ts; the single-miss, slow freeze and stop-for-good cases still fail. Lightbox's five busy proofs with this judgment: 30 of 30 in Chromium, 39 of 40 in WebKit (the other presented 3 frames in its judged part, under the 4 a window needs). The harness's own proofs still pass a compositor animation with no miss and fail the rAF one (0 of 39 to 47 control frames) in both engines.
…ame window Both 'a dip held before its first frame lands once the hold is …' proofs waited for no running animation, then one frame and a timeout, assuming the next step starts within a frame. The dip lands on a timer due on the call's own clock, though, and the proofs passed only because the evaluate round trips covered the gap: in WebKit on a loaded box the window closed first and the proof read the element mid-dip (opacity 0, translate none; reproduced 1 in 4 with the round trips removed). They now wait on the call's own finished, then on the browser's own animations still running or waiting for their first frame, and read nothing from the animator.
…ment at once and settles then A Reduce Motion dip whose landing runs more than its whole fade back in late (a hold released long after the dip was due) played a 1 ms opacity animation, which WebKit keeps pending until its next frame, and settled the call on a 1 ms timer: the call's finished could resolve while the element still showed opacity 0. The animator's notes say the element appears at once there. Fails now: the landing plays one opacity animation and the call has not settled.
…element at once and settles A Reduce Motion landing that runs at least the whole fade back in late had nothing left of the cross-fade, but still played a 1 ms opacity animation and settled the call 1 ms later. WebKit keeps that animation pending until its next frame, so the call's finished could resolve with the element still at opacity 0. Such a landing now writes the end opacity at once (handing a borrowed one back to the stylesheet), plays nothing, and settles the call then, as the animator's notes say.
…ng curve only when it is at most a frame longer than the run before it astracat found that the sub-step credit let pooledBusyProof pass a rAF loop that steps a fixed 1 or 2 px a frame: in each 70 ms block the cast still shows the loop's last steps, then it stands still and goes on one step a frame without catching up, so its run ended in a single step. With the previous busy.ts the self-test's frame-counted loop passed the pooled proof in 5 of 5 Chromium runs. A slowing curve lengthens its runs a frame at a time, so a run now counts as moved only when it is at most one frame longer than the component's run before it; a stall is longer than the runs before it. A judged part with no change reads the finest cast step, 0.5 px, so a larger jump never passes for one step. The self-test adds the frame-counted loop, at 1 and 2 px a frame, to the pooled proof's mutants: it fails every pool (Chromium 10 of 10, moved on 2 to 8 of 7 to 12 control frames; WebKit 10 of 10, 0 or 1 of 11 or 12). The timed rAF mutant can read as moved on one control frame, presented as the block starts, so its assertion takes 0 or 1 and also requires its stall to exceed the control's (Chromium showed 1 of 40 once). Red first: the frame-counted stall, the pooled frame-counted loop and the finest-step cases fail on the previous busy.ts. Lightbox's five busy proofs with this judgment: 30 of 30 in Chromium; 33 of 40 in WebKit with no miss, the other 7 presenting too few control frames to judge.
…ter a stalled dispatch, and a proof can check the pacing the page received WebKit's touch command takes no timestamp, so the page times each event by its arrival. Under box contention a dispatch stalls 27 to 31 ms, and the driver, which aimed every event at the gesture's start, then sent the overdue moves together, 1 to 2 ms apart: C17 measured BottomSheet's flick at 2000 px/s reaching the page as 3,627 to 13,200 px/s. playSteps now sends each event at its scripted time and never sooner after the one before it than the script spaces them, so a stall stretches one gap instead of bunching the rest. With `checkPacing`, WebKit's play reads the timeStamps the page received (a passive capture listener on touch events) and throws TouchPacingError, naming both gap lists, when an event is missing or a gap is more than TOUCH_PACING_TOLERANCE_MS off the script. retakeOnTouchPacing(testInfo, attempt) runs a proof's attempt once more on a TouchPacingError only: two attempts in all, a product failure is never re-taken, and each re-take is listed as a `touch-retake` annotation. The tolerance is measured: on one worker of a loaded machine, 440 gaps over 40 WebKit flicks moved at most 4 ms (99th percentile 3 ms; the page's stamps are whole ms); under eight workers 4% of flicks had a gap further off, up to 21 ms. Red first: the stalled-dispatch case fails on the previous scheduling; checkTouchPacing and retakeOnTouchPacing are new, with the three re-take cases (both attempts off pace stays red, a product failure after good input is not re-taken, off pace then good passes and is listed). In the browser a 40 ms driver stall makes WebKit's play throw TouchPacingError, and Chromium, whose page sees the scripted stamps, plays on.
…a click, and SCRIPTED_CLICK_SKIP names the skip where it does not Playwright's WebKit synthesises a click only from its tap gesture (`driver.tap`), never from touches sent one by one, so a proof that needs the click of a scripted press and lift has to know. `driver.synthesizesClicks` says it (Chromium true, WebKit false), and such a proof skips with `test.skip(!driver.synthesizesClicks, SCRIPTED_CLICK_SKIP)`, as PARTIAL_LIFT_SKIP does for a partial lift. The self-test plays a scripted tap and then driver.tap on one button and reads the order of touchstart, touchend and click: driver.tap's click comes last and a scripted tap's click, if any, before driver.tap's touchstart, so it waits on that order rather than on the clock. This came from the web-input drivers helper line, which no longer carries it.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision 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]
| @@ -1286,13 +1638,26 @@ export function useSheetGestures({ | |||
| 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.
…d off its pacing, once In WebKit a touch carries its real dispatch time, so a dispatch that stalls under load reaches the page as a burst of moves 1-2 ms apart, which the sheet reads as a much faster flick and correctly dismisses. The three proofs whose outcome follows a velocity (the downward flick, the moderate and fast throws, and Android's flick) now play with `checkPacing`, and only on a proven `TouchPacingError` re-take the gesture once, on a fresh page. A product assertion is never re-taken, and a second pacing error fails with its cause.
…se reads it, its last two move intervals and its lift C17 adopted checkPacing on BottomSheet's flick detent proofs and every WebKit attempt failed, even on an idle worker: WebKit delivers a gesture's first moves with the page's own touchstart handling time (gaps of 4, 11, 3 ms against 8, 8, 8) and its lift about a frame late (8 to 11 ms against 2.1). Neither feeds a release velocity: Core's tracker reads the last three moved samples, two intervals, and a release reads as still only 40 ms after the last move. Before the check the same flicks reached the page at 2,000 px/s. checkPacing now judges what decides a release: an event missing, either of the last two move intervals more than TOUCH_PACING_TOLERANCE_MS (4 ms) off the script, or the lift TOUCH_LIFT_STILLNESS_MS (40 ms, UIKit's measured stillness, spec:AST-100 FR2) or more after the last move. checkTouchPacing takes the played steps, so it knows which events are moves. Red first: C17's received gaps fail the previous check and pass this one; a stall and its burst in the last two intervals still fails, and so does a lift 45 ms late, while the same stall earlier in the gesture passes. In the browser a 40 ms driver stall before the last move throws in WebKit, and Chromium plays on.
…il cancelled, and its compositor run is blocked only once on the compositor C23 saw the control self-test fail under load at eight workers: "660 ms judged at 16.6 ms/frame: 28 frames presented, 0/28 moved". Its animations ran once, for 1,600 ms from before the screencast started, so on a loaded runner, where starting the cast takes long, the compositor's animation had ended before the block and nothing moved in the judged part. Both animations now go back and forth until cancelled, as the pooled self-test's do, and the compositor's block uses `during`, so it starts only once that animation runs on the compositor. Red first: with the setup stretched to 1.8 s, as a loaded runner takes, the previous test fails with exactly that read (0/39 moved) and this one passes. Normal runs at eight workers: 10 of 10 in Chromium and 10 of 10 in WebKit.
… judges its first frame on that frame's time 'a spring started right before a block plays in full after it' waited a fixed 300 ms of wall clock after the block, then read the screencast. In the full interaction run on a loaded box no moving frame had reached the screencast by then, and the proof failed on an empty read. It now waits until the page shows the box a quarter of the way along and three more frames reach the screencast. Its bound changes form, not strength: the curve's own start (the handle's started) must come after the block, and the first moving frame may be no further along than a curve started a frame before the block ended, at that frame's own time, which is the old 30 px for a frame right after the block and still holds when a loaded machine sends frames sparsely. A curve dated at the call is 107 px in.
…itor run against the control, since a loaded runner's compositor skips frames At 16 workers on a loaded box (load 92), the self-test failed 6 of 32 Chromium runs on its compositor animation. Every presented frame moved (32/32 to 38/38), but the compositor presented none for 63 to 109 ms inside the block, past the display-rate bound. Its compositor run is now judged against startBusyControl's control in the same window, with the bound's own verdict recorded beside it. The rAF mutant stays the negative and fails both: it moved on 0 or 1 of the control's frames with a stall past the control's, and the bound alone fails its longest stall. The run follows the README's order: the cast, then the animation, then the block once the animation runs on the compositor. At 16 workers (load 115 to 164) it passed 48 of 48 in Chromium, where the control's own longest stall in the rAF runs reached 33.5 ms against the rAF's 660 ms. WebKit at 8 workers: 8 of 8.
… animations wrap instead of turning
astracat found that the control self-test's back-and-forth animation turns at 1,600 ms. When a loaded runner starts the cast late enough that the block holds the turnaround, two cast frames straddling it evenly read one position, and the control verdict fails the correct compositor animation: "moved on 6 of the 7 frames the control moved". Both self-tests' compositor animations and their timed rAF loops now wrap from 560 px back to 0, a change on every frame. The pooled case's counted mutants still bounce: they are negatives, and a turn changes their position by a full step.
BusyProof.stepPx's note now describes the step it reads from the judged part's changes, and the file header names pooledBusyProof.
Red first: with the setup stretched so the block holds the turnaround, the previous test fails 7 of 12 Chromium runs ("moved on 5 of the 6" and "6 of the 7 frames the control moved"), and this one passes 12 of 12.
…rol inside an open dialog, leaving its backdrop on the compositor
C17 found that in WebKit the control's top-layer popover, shown above an open modal dialog, takes the dialog's `::backdrop` animation off the compositor. BottomSheet's dim held its value for 155 to 225 ms into the block, then jumped to rest, while the control moved on every frame. `startBusyControl(page, {within})` now mounts the box inside the given element as a plain fixed box, with no popover and no top-layer entry. Its frames still count presented frames, and `raise()` leaves such a box where it is.
The self-test opens a modal dialog whose backdrop dims through a block, and judges the dim's brightness at a point against the control.
Red first: in WebKit at 8 workers, with the top-layer control the dim stalled in 5 of 24 runs, from 43 to 78 ms at the block's start, while the control moved ("17/20 moved, longest stall 76.5 ms at +20 ms"); with `within` it passed 24 of 24. Chromium passed either way.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision 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]
| @@ -1286,13 +1638,26 @@ export function useSheetGestures({ | |||
| 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.
Depends on #8105. Review only this PR's change: https://github.com/vjeux/astryx/compare/7b0b523557f664eefac067e1b5bf4b953d3b50af..94b858804cf4e3e45ca279b1fb5e6a969076e32a
Primary intent
Under a finger, a BottomSheet stays exactly under the finger between detents and stretches past its tallest detent the way an iOS sheet does.
User or maintainer impact
On a phone the sheet felt sticky near a detent: within 40 px of one, a 20 px drag moved it 2.6 px, and above the tallest detent it stretched a fixed 35 % to a wall. Now, under a finger:
A mouse keeps today's magnet and stretch.
Problem and solution fit
Measured on 43 drags, UIKit puts the sheet at the detent offset plus the finger's translation on every update. Options:
Authority
spec:AST-094FR3 (the sheet's rubber band), FR9, FR12, FR13;spec:AST-017FR59 (a mouse unchanged);spec:AST-107FR7 with INV13's gaps (#8751).Change and evidence
iOS simulator (red dot: the finger), a pull past the tallest detent, before and after:
Into the dismiss zone and back, and Android (emulator):
Scope
Testing
How to check, in the Snap points story on a phone: drag the handle slowly through the half detent (the sheet stays under your finger, with no snap); pull above the tallest detent (it stretches less and less, then settles back).
What ran: the sheet, gesture and motion unit tests pass (2,111), each new one failing first on the old code; the sheet's Chromium (18) and WebKit browser tests pass, apart from a harness self-test fixed by #8535; a touchscreen laptop's finger still pulls the sheet.
touch-support-bundlefails here, as BottomSheet now puts the touch module in the root entry; #8671's per-component check replaces it and passes. Checks ran locally; no CI run is expected.