Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
![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 an engineering owner: the 10 px drag threshold (a mouse's included), a second finger no longer cancelling a begun drag, and the 5 px edge claim rest on spec:AST-094 r2 DEC-2, FR10, FR14 and FR17, which are proposed in #8378 and not yet current, so an engineering owner needs to approve those claims (by landing #8378, or by commenting /approve-spec <full head SHA> here, which the author may do if they are an engineering owner) or the sheet keeps today's behaviour for them. Independently of that decision, three changes are still needed:
- Land or rebase onto the stacked pull requests so this diff is only this PR's commit; today it is 46,769 changed lines, and the commit alone is 3,193.
- Limit the
BottomSheet.catchwatcher to touch and pen (or name the change), because a mouse press anywhere on a settling sheet now stops it, whichspec:AST-017FR59 does not allow as a side effect. - Add mouse before-and-after evidence for the threshold and release-velocity changes, as FR59 requires for touch work that changes fine-pointer behaviour.
[Automated review]
| sheetElRef, | ||
| () => | ||
| new LongPressGestureRecognizer({ | ||
| name: 'BottomSheet.catch', |
There was a problem hiding this comment.
This watcher takes every pointer, so a mouse press anywhere on a settling sheet now stops it until the button comes up (measured: held at 186.4 px for 500 ms after a press on the list), which main never did. spec:AST-017 FR59 keeps fine-pointer behaviour unchanged unless the PR names a defect or deliberate consistency and shows mouse evidence before and after. allowedPointerTypes: ['touch', 'pen'] here, with a mouse catch kept on the handle, would restore that.
| handleElRef, | ||
| () => | ||
| new PanGestureRecognizer({ | ||
| name: 'BottomSheet.handle', |
There was a problem hiding this comment.
With UIKit's 10 px hysteresis this pan also delays a mouse: 8 px of handle travel no longer moves the sheet and 30 px moves it 20 px (16.9 px on main). If that is deliberate cross-device consistency, spec:AST-017 FR59 asks the PR to say so and show mouse evidence before and after; the threshold itself cites spec:AST-094 FR17, which is not current yet.
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Modified ComponentsMultiSelector (@astryxdesign/core)
Popover (@astryxdesign/core)
MobilePowerSearch (@astryxdesign/lab)
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 | +35.6KB |
+98B |
| core/DateInput | +35.5KB |
+85B |
| core/DateTimeInput | +35.3KB |
+89B |
| core/PowerSearch | +35.2KB |
+101B |
| core/TimeInput | +35.6KB |
+92B |
| lab/MobilePowerSearch | +35.4KB |
0B |
| lab/MobileTokenizer | +35.5KB |
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 |
| @astryxdesign/lab | 4.6KB | 1.9KB | 1.9KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.7KB | +94B |
| @astryxdesign/lab | 11.1KB | 11.1KB | 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
d877965 to
71afa62
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner. The 10 px drag threshold (a mouse's included), a second finger no longer cancelling a drag that has begun, and the 5 px claim at a scroll edge rest on spec:AST-094 r2 FR17, FR10 and FR14, which are proposed in #8378 and not yet on main. An engineering owner needs to approve those claims, by landing #8378 or by commenting /approve-spec <full head SHA> here (the author may do this if they are an engineering owner), or the sheet keeps today's behaviour for them; note that FR14 as proposed also rules out the mid-gesture scroll hand-off this change keeps. Whatever that decision, three changes are still needed:
- Limit the
BottomSheet.catchwatcher to touch and pen. A mouse press anywhere on a settling sheet now stops it until the button lifts, whichspec:AST-017FR59 does not allow as a side effect of touch work. - Add mouse before-and-after evidence for the threshold, or keep a mouse drag moving from its first pixel. FR59 requires this whenever touch work changes fine-pointer behaviour.
- Land or rebase onto the stacked pull requests so this diff is only this PR's commit. The diff is 46,966 changed lines today, and the commit alone is 3,193.
[Automated review]
Brings the wave (FR37, FR3, FR59, the base fold) onto the migration, on the gesture line's new head. Resolved: - useSheetGestures.ts: the one moveDrag tracks with UIKit's rubber band under UIKit's presentation and today's magnet and stretch otherwise; the sheet's own selection lock now takes the gesture host's counted lock (gestures/selectionLock.ts), so the two give the sheet its values back in whichever order they let go; an unused spring import goes. - useSheetGestures.test.ts: the migration's helpers for the merged tests (a finger's rubber band, the exact mouse magnet); the tests that touch a sheet right after a release let its settle report its end (a promise) first, so the touch finds it at rest instead of catching it. - The Detent spec: the base's version plus the migration's pan hysteresis, which the rubber-band spec now also reads. - motion/README.md: both sides' rows.
# Conflicts: # packages/core/src/gestures/GestureHost.ts
# Conflicts: # packages/core/src/MultiSelector/MultiSelector.test.tsx # packages/core/src/Selector/Selector.test.tsx # packages/core/src/Selector/__tests__/Selector.combobox.a11y.test.tsx
…ta#8386 # Conflicts: # packages/core/src/BottomSheet/useSheetGestures.test.ts
# Conflicts: # packages/core/src/BottomSheet/useSheetGestures.ts
…n lock under UIKit's presentation) into meta#8386 # Conflicts: # packages/core/src/BottomSheet/useSheetGestures.ts
…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.
…this head The headers of morphPath.ts and its test named rectMorph.ts and rectMorph.test.ts, which arrive with the pull request stacked on this one, and cited 'law M1', which no document defines. They now describe the player without naming its file and cite spec:AST-094 FR5, which keeps touch motion to translate, scale and opacity on the compositor.
The header cited 'law M1' and 'laws M2, M3', which no document defines. It now cites spec:AST-094 FR5 for compositor-only motion and FR6 for an interruption that starts from the presented rect with its velocity, and its paragraph wraps within the line width.
…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.
Takes meta#8360's head: the held-dip proofs wait on the call's finished and a late landing shows the element at once (88a1866), and the spring-before-a-block proof waits for the motion (d0f5fd5). In the animator spec the proof keeps meta#8360's waits and bounds and runs in WebKit too, as this pull request's start-after-block makes it pass there. # Conflicts: # packages/core/src/motion/__tests__/animator.interaction.browser.spec.ts
…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.
…hold asks the layer, not the gestures) into meta#8203
|
Thanks. On each point:
|
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: this head adds to spec:AST-094 FR41 and its verification row that a mouse or pen drag a recognizer claims takes no new text selection and clears the page's selection once it has moved, which current FR41 does not say, so an engineering owner needs to record approval by commenting /approve-spec 26f2ae686a0db229233d5262229d77e3cb5c2d07 on this pull request (the author may do so if they are an engineering owner), or the drag keeps today's mouse and pen selection behavior. Moving BottomSheet's drag onto the gesture recognizers otherwise follows the current AST-094 drag, release and entrance rules. Whatever the decision, these still need to change before this can land:
- Land the stacked #8341 and #8665 first (or rebase once they land) and move the
@astryxdesign/buildStyleX fix into its own pull request; the diff is 45,495 authored lines today. - Add real-device iPhone Safari and Android Chrome recordings of the slow pull from a list's top and a handle drag, as AST-094's Platform support asks.
- Drop the detent haptic tick in
useSheetGestures.tsuntil a UIKit measurement shows a sheet plays one there (FR23), or cite that measurement with a test. - Add a mouse run before and after for a press that stops a settling sheet (
spec:AST-017FR59).
[Automated review]
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| takes no new text selection for that drag only. A claim made after the | ||
| drag has moved clears the page's selection; a press claimed at its own | ||
| pointerdown, before it moves, keeps it. An unclaimed mouse drag keeps its | ||
| released behavior. |
There was a problem hiding this comment.
These sentences, with the FR41 verification row at line 663, add a selection rule for claimed mouse and pen drags that current FR41 does not state: on main a mouse “otherwise keeps its released behavior”. GestureHost.ts (lockClaimedDrags) already implements it. An engineering owner records approval by commenting /approve-spec with this head's full SHA; without it, the drag keeps current FR41's mouse and pen selection behavior.
| onScrimOpacityRef.current?.(dimFor(target)); | ||
| if (target !== baseOffset) { | ||
| // A short tick where the platform vibrates (gestures/haptics.ts). | ||
| playHaptic('impactLight'); |
There was a problem hiding this comment.
This vibrates 8 ms wherever navigator.vibrate exists, on every release that changes stop and under every pointer. spec:AST-094 FR23 allows a haptic only at a moment UIKit plays one, and the sheet's haptics log is still measurement task M-SH16 in motion/MEASUREMENTS.md, so nothing yet shows a UIKit sheet ticks here. Drop it until that measurement records the moment, or cite it and add FR23's haptics test.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: this head carries, from #8665 and #8341, an amendment to spec:AST-094 FR41 (a mouse or pen drag a recognizer claims takes no new text selection, and clears the page's selection once it has moved) and a new shield entry in BottomSheet's element jobs, which the current records do not state, so an engineering owner needs to record approval by commenting /approve-spec 8265e441a5fa4687559c44960b125a45fc90585e on this pull request (the author may do so if they are an engineering owner), or the drag keeps today's mouse and pen selection behavior. Moving BottomSheet's drag onto the gesture recognizers otherwise follows the current AST-094 drag and release rules, and no change is needed for the detent haptic, which main already plays at the same moment. Whatever the decision, these still need to change before this can land:
- Land #8341, #8360 and #8665 first (or rebase once they land), then split or justify the remaining 4,513-line change; the diff is 47,954 authored lines today.
- Add real-device iPhone Safari and Android Chrome recordings of the slow pull from a list's top and of a handle drag, as AST-094's Platform support asks.
[Automated review]
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| takes no new text selection for that drag only. A claim made after the | ||
| drag has moved clears the page's selection; a press claimed at its own | ||
| pointerdown, before it moves, keeps it. An unclaimed mouse drag keeps its | ||
| released behavior. |
There was a problem hiding this comment.
These sentences, with the FR41 verification row at line 663, add a selection rule for mouse and pen drags a recognizer claims that current FR41 does not state: on main a mouse “otherwise keeps its released behavior (spec:AST-017 FR59; DEC-9)”. They arrive from #8665, and GestureHost.ts lockClaimedDrags implements them. Approval waits on an engineering owner: comment /approve-spec with this head's full SHA, or keep current FR41's mouse and pen selection behavior until the amendment lands on main.
| - while a modal sheet enters under a finger, the layer's input hold adds a | ||
| shield inside the dialog: a transparent cover of the viewport that takes | ||
| a tap or a drag landing during the entrance (`spec:AST-094` FR35). It is | ||
| removed when the sheet lands. A non-modal sheet adds no shield: it holds | ||
| only input on itself, and the page beside it stays live. |
There was a problem hiding this comment.
This adds the input hold's shield to an open sheet's element jobs, which the current record does not list and the element-count test checks. It is a requirement change, so it waits on the same engineering-owner approval as the FR41 amendment.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one input hold for every layer (`Layer/inputHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act, and a pinch still zooms the page); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates, and `isInputShield` lets a hit test see past a hold's shield. Not exported from the package entry point. |
There was a problem hiding this comment.
At this head Layer/inputHold.ts registers no touch gate: the gesture host asks isInHeldLayer itself for each touch (GestureHost.ts:1561-1567). “The gesture host holds the layer's touches through its touch gates” misdescribes that; the sentence needs updating where this changeset is authored.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A BottomSheet's drag runs on the shared gesture recognizers under every pointer, a finger, a pen and a mouse: it begins after 10 px of travel and moves the sheet by the travel past that, as UIKit's does; a slow pull down from the top of the sheet's content moves the sheet instead of rubber-banding the content; and a second finger that lands during a drag no longer sends the sheet back to its detent. A mouse, like a finger, pulls the sheet down from the top of its content and up from its end while a taller detent exists, and its release dismisses the sheet or picks a detent by the same rule as a finger's. A mouse, and a pen or finger where the primary pointer is fine, keep the pointer's tracking and the theme's release timing. |
There was a problem hiding this comment.
The iOS claim here (a slow pull from the top of the content moves the sheet) is shown only on the iOS 26.2 simulator. spec:AST-094 Platform support asks for real-device evidence for every claim about iOS Safari and Android Chrome behavior; a recording on an iPhone in Safari and an Android phone in Chrome would cover it.
Depends on #8341, #8360 and #8665. Review only this PR's change: https://github.com/vjeux/astryx/compare/09ed788d392fb1bedaef1d136ce94cea891d8262..26f2ae686a0db229233d5262229d77e3cb5c2d07
Primary intent
Every BottomSheet drag, under a finger, a pen or a mouse, runs on the shared gesture recognizers.
User or maintainer impact
The sheet had two drag implementations that disagreed: a mouse flick closed the sheet where a finger's stops at the next detent, and a mouse dragging up from the content's end selected text. Now every pointer gets one release rule, a drag that begins after 10 px (one begun in the content stays the content's), a catch during a settle, the pull up from the content's end, and a dim that clears only once the sheet is gone. A mouse keeps the theme's timing.
Problem and solution fit
spec:AST-094attaches gestures for every pointer. Options:Authority
spec:AST-094FR13, FR14, FR17, FR19, FR36, FR43;spec:AST-017FR59;spec:AST-107FR7 with INV13's gaps (#8751).Change and evidence
Finger (iOS simulator, Android emulator):
Mouse on main and on this PR (desktop Chrome, 1280 × 800, timed input, two runs each):
Scope
Testing
With a mouse in the Snap points story: flick the handle down (stops at half); at half, scroll to the content's end and drag up (the sheet rises).
Core, lab, the sheet's Chromium and WebKit tests and the touchscreen-laptop test pass; 53 new tests fail on the old code. Lab's MobilePowerSearch swipe test now sends a finger's touch and pointer events on a sheet-sized box; its expectation is unchanged.
touch-support-bundlefails here, not on main; #8671 replaces that check.