Repository navigation
fix(Toast): a quick flick dismisses a toast of any height - #8047
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Toast · Storybook | 🔴 84.1 kB (+23.4 kB) · 9.0 kB CSS (0) | 1,612 (+69) | ✓ · ✓ · ✓ |
| TimeInput | 140.9 kB (+84 B) · 14.5 kB CSS (0) | — | — · — · — |
| Slider | 92.5 kB (+83 B) · 8.3 kB CSS (0) | — | — · — · — |
4 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateTimeInput | 186.7 kB (+75 B) · 17.2 kB CSS (0) | — | — · — · — |
| PowerSearch | 260.5 kB (+75 B) · 22.6 kB CSS (0) | — | — · — · — |
| DateInput | 164.0 kB (+73 B) · 15.5 kB CSS (0) | — | — · — · — |
| SegmentedControl | 84.5 kB (+73 B) · 6.7 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.58 MB packed (+2.5 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 8 rows; run pnpm api:report.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change works as described: a quick flick now dismisses a one-line toast, slow drags behave as before, and the new tests fail on main and pass here. Approval waits on one decision, because no current record settles Toast's swipe thresholds (spec:AST-003 leaves them to the component): should a swipe toward the edge dismiss as a flick at any travel past the 8 px slop, timed by the input events' timestamps, instead of also needing 48 px of travel? Any engineering owner, the author included, records that decision by commenting /approve-spec 6c39f1425a58b4693bf944d170732c5271e00017 on this pull request, or can point to the record that already governs it.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
6c39f14 to
32035ec
Compare
|
Current records now cover the threshold: |
There was a problem hiding this comment.
Approval recommended: a quick flick now dismisses a one-line toast, slow drags behave as before, and the flick's speed is read from the input events' own timestamps. This follows the Toast component record's release rule (FR4, through spec:AST-094 FR36, which sets no travel floor for a flick) and spec:AST-100 FR4, and the new tests fail on main and pass here. The other gaps FR4 lists, the shared tracker's release velocity and cancelling a release that moves back, stay for the follow-up the description names.
[Automated review]
32035ec to
fec4a15
Compare
There was a problem hiding this comment.
Changes requested for one small fix: the flick change itself follows the Toast record's release rule and the event-timestamp rule, and the new tests fail on main and pass here.
- The touch-listener Effect in
useToastGesture.ts(the one whosepointhelper now takes theTouchEvent) needs a one-line note naming what it synchronizes, as the React runtime rules require of every changed Effect; for example: "Native touch listeners on the card root.touchmoveis not passive, so a claimed swipe can stop page scrolling."
One optional comment tidy-up is suggested inline.
[Automated review]
| return; | ||
| } | ||
| const point = (touch: Touch): GesturePoint => ({ | ||
| const point = (touch: Touch, event: TouchEvent): GesturePoint => ({ |
There was a problem hiding this comment.
This edit changes the touch-listener Effect, and a changed Effect needs a note naming the external system it synchronizes (the React runtime rules). Nothing says why these are native listeners rather than React's onTouchMove, which is passive and cannot stop page scrolling. Suggested note above useEffect:
// Native touch listeners on the card root. `touchmove` is not passive,
// so a claimed swipe can stop page scrolling.There was a problem hiding this comment.
Fixed in d7ffd48: the touch-listener Effect now has your note, and the flick comment is shortened as you suggested, in the active voice.
| // the slop toward the edge when it began. On a one-line toast, 40% of the | ||
| // height is below the 48 px floor, so only a flick dismisses a short swipe. |
There was a problem hiding this comment.
Non-blocking: this sentence restates SWIPE_DISMISS_RATIO and SWIPE_DISMISS_MIN_DISTANCE as numbers and goes stale if either is tuned. Shorter:
// A flick needs no travel floor. The swipe passed the slop toward the edge
// before it was claimed.
fec4a15 to
d7ffd48
Compare
6ea3749 to
ef05a13
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, and two fixes are needed either way. The flick fix follows the Toast release rule, its new unit cases fail on main and pass here, and last round's Drawer import guard now passes.
Decision needed: this head changes component:Toast (FR4's first-move sentence, FR6's gutter, a new toolbar sentence and two verification rows), spec:AST-094 FR19, component:LayerProvider FR1 and architecture:layer-runtime INV10, and adds a box-less empty viewport that no clause states; no /approve-spec covers this head. An engineering owner records the decision by commenting /approve-spec ef05a138435133b9f6c8ab443a997ea58164b775, and the author may do so as an engineering owner.
docs/specs/components/Toast/Toast.spec.mdFR3 (line 193) and its Current-state bullet (lines 115-118) still say a dismissal before the fallback viewport mounts does nothing, although this head's shared toast queue fixes that, so delete that gap sentence in both places.- Miss: the four touch listeners in
useToastGesture.ts:376-452are now passive, so React'sonTouchStart,onTouchMove,onTouchEndandonTouchCancelin the returnedbindingscan carry them, and the Effect that attaches them natively should go.
The record fix changes record text, so the approval would be recorded on that head.
[Automated review]
| | ---- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | A card carries `toast` with `type` and shows `body` and, on its first line, `endContent` then a dismiss Button named by `@astryx.toast.dismiss`; long body text wraps below. `renderContent` replaces that layout, receives `body`, `endContent`, `type`, timing, and a `dismiss` function, and gets no injected dismiss control. | Current source and tests | | ||
| | FR2 | With auto-hide on (the default for `info`, off for `error`), the card dismisses with reason `auto` after `autoHideDuration` ms. The timer pauses while the pointer is over the card, focus is inside it, a touch or pen gesture is active, or the window is blurred, and resumes with the remaining time (at least 1 s). Each pause condition holds the timer on its own: one ending resumes it only when no other still holds, and focus moving between the card's own controls keeps it held. A toast raised while the window is already blurred starts with its timer held. | Current source and tests | | ||
| | FR3 | The dismiss button, a swipe, a `dismiss` call, or the returned dismiss function dismiss with reason `manual`; `onHide` fires at most once, when the toast is dismissed, even when dismissed twice during its exit. Implementation gap: with no provider above the caller, a dismissal called before the fallback viewport mounts does nothing, so the toast shows and stays unless it auto-hides (Current-state impact). A toast replaced through `uniqueID` is not dismissed and reports no `onHide`. | Current source and tests | |
There was a problem hiding this comment.
This head's shared toast queue (packages/core/src/Layer/toastHost.tsx:110-123) drops a toast dismissed before the fallback viewport mounts and reports onHide('manual') once, so FR3's implementation-gap sentence, and its copy in the Current-state bullet at lines 115-118, no longer describe what ships; delete both.
|
|
||
| root.addEventListener('touchstart', handleTouchStart, {passive: true}); | ||
| root.addEventListener('touchmove', handleTouchMove, {passive: false}); | ||
| root.addEventListener('touchmove', handleTouchMove, {passive: true}); |
There was a problem hiding this comment.
With all four listeners passive, React's onTouchStart, onTouchMove, onTouchEnd and onTouchCancel can carry these handlers in the returned bindings, beside the pen's pointer handlers (React registers touchstart and touchmove passively), so the Effect at lines 376-452 can go.
ef05a13 to
6b51481
Compare
|
/approve-spec 6b51481 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested for two fixes. The flick fix follows the Toast release rule, its new unit cases fail on main and pass here, the new browser specs pass in Chromium, last round's two fixes are in, and the record changes carry an engineering owner's approval for this head.
packages/core/src/Layer/toastHost.tsxlines 108–121 copy Toast's uniqueID rule for waiting toasts instead of sharing it withToastViewport, and the copy already differs (two toasts raised withuniqueID: ''before the viewport connects show as one, after it as two), so move that decision into one helper both call and delete the copy.- The diff from
mainis 2,207 authored lines, over the 2,000-line limit, and 920 of them are #10012's commit, so add the identical lineStack: #10012 → #8047to both pull request bodies, or land #10012 first.
The fix needs a new commit, so the spec approval will need to be recorded again on that head.
[Automated review]
| const {uniqueID, collisionBehavior = 'overwrite'} = entry.options; | ||
| const namesake = | ||
| uniqueID == null | ||
| ? -1 | ||
| : waiting.findIndex( | ||
| candidate => candidate.options.uniqueID === uniqueID, | ||
| ); | ||
| if (namesake === -1) { | ||
| waiting.push(entry); | ||
| } else if (collisionBehavior === 'overwrite') { | ||
| waiting[namesake] = entry; | ||
| } |
There was a problem hiding this comment.
This repeats ToastViewport's uniqueID rule (ToastViewport.tsx:466-472, :485) for waiting toasts, and the copies already differ: the viewport treats uniqueID: '' as no ID (uniqueID && …), this as an ID (uniqueID == null). Two toasts raised with uniqueID: '' before the viewport connects show as one; after it connects, as two. Suggest one helper that finds an entry's namesake and says whether it is dropped, called here and in ToastViewport's addToast, so the rule has one owner.
LayerProvider rendered ToastViewport itself, so an app that renders a provider loaded Toast's code with it, about 42 KB (gzip) from the Layer entry. Layer now owns the toast context and an outlet. ToastViewport registers itself when its module loads, and the outlet then renders it after the app, on the client only. A toast added before the viewport mounts waits in a queue that the outlet and useToast's fallback root share, and a toast dismissed while it waits reports its onHide and never shows.
A release toward the edge dismissed past max(40% of the card's height, 48 px), or as a flick only when it had also travelled 48 px. On any toast up to 120 px tall the two floors are the same distance, so a flick could never dismiss a one-line toast. The flick now needs no travel of its own beyond the slop the swipe already passed. The flick's speed was also timed with Date.now() in the handlers. Browsers deliver touch and pointer moves on the next frame, so the handlers trail the finger by up to two frames (measured in Chromium: a 33 ms flick handled over 55 ms), which halved the speed of a short flick. It is now timed by the input events' own timeStamp. A bottom toast keeps its 16 px gap above the bottom safe-area inset instead of sitting on the inset's edge. Under a coarse pointer it clears at least 34 px plus that gap: Safari reports the inset only with `viewport-fit=cover`, so a page without it read 0 and left the toast in the home indicator's area. Toast's record (FR6) states that gutter. A touch on the card is decided by its first move: toward the edge, the swipe owns the touch; any other first move leaves the page its scroll (Toast FR4). The gesture host makes that claim: the card attaches a one-way pan whose new `firstMoveDecides` option holds a touch from a first move its own way and fails on any other. Toast's own listeners only draw the swipe, and end it once the pan leaves the touch to the page. Importing Toast now brings in the shared gesture host: about 24 KB gzip on its own, and nothing more where other touch components already load it. LayerProvider does not import Toast, so a provider alone does not bring it in.
iOS Safari colors its bottom toolbar from a fixed element at the bottom edge. A swiped toast leaves through that edge inside the toast viewport, and Safari kept the toast's color while the empty viewport still had a box. The viewport now has no box while it holds no toast, and the toolbar returns to the page color once the last toast leaves. The Toast record lists the limit that remains: while other toasts remain, a swiped toast's exit can tint the toolbar until the last toast leaves.
…'s color In the iOS Simulator, the toast viewport loses its box one frame after the last toast's exit transitions end, and Safari restores its bottom toolbar's color about 0.25 s after that. The Toast record's toolbar gap now says so.
The @output lines of ToastEmptyViewport and ToastSwipeScroll each held one long sentence, of 30 and 45 words. They now say the same in sentences of at most 22 words.
Toast's four touch listeners became passive once the gesture host took over the claim, so they no longer need a native attach: React registers touchstart and touchmove passively. They now come with the card's other bindings, and the Effect that attached them is gone. The cleanup test now checks that unmounting a toast leaves no touch listener on its card, whoever added it.
6b51481 to
3086621
Compare
|
/approve-spec 3086621 |
There was a problem hiding this comment.
Approved pending CI: CI is still running on both heads, so neither pull request is ready to merge until its required checks pass.
#10012 — approve, with no blocking findings. LayerProvider renders the toast viewport only once Toast is imported, and the waiting queue now decides uniqueID namesakes through the same helper as the viewport.
#8047 — approve, with no blocking findings. The flick, the first-move claim, the bottom gutter and the empty viewport follow the Toast and touch-presentation records as approved for this head, and the new flick, first-move and empty-viewport tests fail on main and pass here.
Whole stack — pass. A pull request lands only when it and every pull request below it are approved.
[Automated review]
cixzhang
added
the
ci:run
…nchanged; touch-support list keeps both entries)
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec fb7eadb |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval recommended. CI is green on this head, and an engineering owner approved the Toast and AST-094 record changes on this exact head. #10012 has already merged into main, so this pull request was reviewed on its own against main; no blocking findings remain and nothing else is needed from you.
[Automated review]
|
/approve-spec fb7eadb |
Stack: #10012 → #8047
#10012 (
c6337eb803) keeps Toast out of LayerProvider; this pull request is the five commits above it.User impact
A quick flick snaps back on any toast up to 120 px tall. On an iPhone, a bottom toast's swipe could scroll the page, trigger Reachability, or leave Safari's toolbar tinted.
Problem and solution fit
viewport-fit=cover.Expected behavior and authority
Toast.spec.mdFR4 andspec:AST-094FR36: a release decides on projected travel, with no floor for a flick; on a touch screen the first move decides (an FR19 exception).Toast.spec.mdFR6 states the gutter, FR11 the empty viewport; its Current-state impact lists the accepted toolbar limit.Smallest restoration
timeStamp.firstMoveDecides); otherwise the page scrolls.display: none).Evidence
iOS 26.2 simulator:
main): a 46 px flick on a 56 px toast snaps back.Owner's real-iPhone recording on main: a downward swipe on a toast scrolls the page and triggers Reachability.
Scope
Testing
Local gates at
3086621499, five commits on #10012'sc6337eb803(one commit onmainbfc1dccfc0):main's), Prettier,pnpm check:repo(withcheck:knowledgeandcheck:changesets) andpnpm review:pre --base c6337eb803(0 findings): clean.pnpm test:ui: 840 files, 21,121 tests, 0 failed.main's guarduseScrollLockImports.test.ts("Drawer/Drawer.tsx reaches neither the gesture host nor its arena") passes unchanged: with perf(Layer): LayerProvider loads no Toast; Toast registers its viewport #10012, Drawer no longer reaches Toast.pnpm test:node: 907 of 908 files pass. The one failure isimport-hint-correctness.test.mjs"builds its fixture", a 30 s timeout under load: a known load red onmain.ToastViewport.test.tsx's cleanup case now checks that unmounting a toast leaves no touch listener on its card: with React's touch events, the only one there is the gesture host's, and it is removed.ToastViewport.test.tsxcase "has no box once its last toast has left, and a box for the next toast" fails without the empty-viewport fix (flex, expectednone) and passes here.ToastEmptyViewport.interaction.browser.spec.ts(a swipe exit, then a close-button exit) failed without it in Chromium and WebKit (box count 1, expected 0).ToastSwipeScroll.interaction.browser.spec.tspasses; its page-scroll case runs in Chromium only.ToastSwipeScrollandToastEmptyViewportpass in Chromium (4 of 4, twice) and in WebKit in the Playwright container (1 worker, ×3: 9 pass; the page-scroll case skips there by design). The accessibility contracts, withToastSafeAreaand the toast outlets, 151 pass.main, after a swiped lone toast was gone, the bottom toolbar stayed red through 5 s and a scroll; on this change's build it keeps the page color after a swipe, close-button and auto-hide exit. With two toasts, swiping one away tints it until the last toast leaves.0b44c11fcc, whose flick and first-move code this change keeps byte for byte: the gesture host and Toast units (fivefirstMoveDecidescases fail without it, and Chromium's pointer-before-touch order is covered);ToastSwipeScrollin WebKit ×10 (the CI container profile);ToastSafeArea.a11y.chromium.spec.ts, with a 16 px gap onmainand 50 px here under a finger with no inset; the flick's tests, which fail onmain; and the first-move simulator run in the Evidence section.gzip -9), against perf(Layer): LayerProvider loads no Toast; Toast registers its viewport #10012's head: all of/Toast74,387 → 98,742 B;LayerProvider+useToast181,218 → 204,695 B;LayerProviderfrom/Layer20,865 → 20,865 B.Writing rubric results
spec:AST-094FR19's toast exception state what the system does, in the present tense.#numbers,/pull/or/issues/in the changed lines).