Skip to content

Review 5375

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 4 revisions

PR

#5375 feat(Toast): add scroll-safe swipe dismissal by rubyycheung (bucket: internal teammate — DESIGNOWNERS)

HEAD REVIEWED

06067bcba4a595e5ea2466f1bd47b16ae6959bc6 <- every claim below was verified at this commit

PROBLEM

Touch and pen users have no direct-manipulation way to dismiss a transient toast, while the first gesture shape for this area (touch-action: none) made normal page scrolling unavailable whenever the gesture started over the toast. The public dismissal reason should also stay stable unless a consumer needs a separate value; this PR shows no consumer needing swipe separate from manual dismissal.

VERDICT: clear

SOLUTION

When a finger or pen starts on the notice, the notice waits until the movement is clearly aimed toward the edge it came from. Movement in any other direction stays with the page or with the control the person touched; accepted movement pauses the timer, follows the finger, and closes only after enough distance or speed. The placement owner decides which edge counts, and the notice itself owns the visual feedback and close, so apps get the new gesture without a new setting or a new dismissal reason.

SOLUTION (5 decisions · ~334 runtime lines of 1346)

  1. touch + pen can swipe a toast toward its top/bottom placement edge [direct dismissal]
  2. the gesture is accepted only after dominant edge-directed travel; opposite, horizontal, and interactive-descendant starts stay native [scroll/control preservation]
  3. auto-hide pauses during an in-flight gesture and resumes on cancel [user is manipulating it]
  4. swipe still reports the existing manual dismissal reason [no stated consumer need for a public union expansion]
  5. placement stays owned by the viewport; the toast reads a private edge direction from that placement owner [avoid Toast.position public surface]

This is over the “2 decisions” smell, but these do not split cleanly: the gesture is not safe without scroll arbitration, timer pause, input-type choice, and the API decision. Each traces to a stated problem in the body.

VERDICT: clear

ARCHITECTURE

OWNER: Toast's stable card/transport owns gesture lifecycle, timer pause/resume, visual swipe feedback, and the dismiss request. ToastViewport owns placement and supplies the dismiss-edge direction. TIER 1: Accessibility primitives (INTERACTIVE_SELECTORS, existing close Button and translated label), plus public DOM/event composition. TIER 2: Status/feedback (info/error) and semantic close icon reuse existing Button/Icon contracts; no size cascade change. SEAMS: Built-in content, custom renderContent, portal/top layer, and consumer interactive children. BEHAVIOR UNIT: inline → useToastGesture — arbitration, thresholds, timing, listeners, cancellation, timer handoff, and transient swipe state form one independently testable state machine.

Driven seam table:

Seam Result
built-in Toast PASS — upward edge swipe dismissed
custom renderContent PASS — custom layout still dismissed
portal/top layer PASS — swipe dismissed while the manual popover was open
custom interactive child PASS — dragging Undo did not dismiss; dragging the surface did

The custom-content seam therefore does not require moving gesture ownership out of Toast: renderContent replaces the card's inner layout, not the stable card root. The architecture block is the implementation unit. Roughly 330 lines of gesture state, listeners, direction arbitration, thresholds, timing, and cancel/dismiss decisions are embedded in the rendering component and can only be exercised through the full viewport/card integration. Extract useToastGesture, pass the resolved direction as React data, and keep the hook invoked by Toast's stable root.

VERDICT: BLOCKS — the gesture protocol needs a named, focused test boundary; lifecycle ownership stays on Toast

IMPACT

Every app using Toast gets an additional touch/pen dismissal path. A touch user can swipe a toast away when the gesture is clearly toward the toast’s edge; sideways, opposite-direction, and control-origin gestures keep their existing behavior. Mouse users see no drag behavior change and still use the close control.

VERDICT: clear

API

No public prop, export, default, or union member is added. ToastDismissReason remains 'auto' | 'manual', ToastViewport.position remains the placement API, and ToastProps still has no position. The new --_toast-* vars are documented as private lifecycle values and listed in the derived-var exception test.

Changeset is present and valid: [feat] patch entry for @astryxdesign/core with @rubyycheung. pnpm check:changesets passed.

VERDICT: clear

THEMING

No new theme target and no public var. Existing astryx-toast target stays on the painting element and still carries type; new private vars are explicitly not authorable theme surface. Styling remains tokenized: motion uses duration/ease tokens and transform/opacity, no raw color/spacing/radius/shadow in the changed runtime styling.

VERDICT: clear

BREAKING

API: no existing call site stops compiling or changes callback reason shape; swipe calls onHide('manual'). Visual: default rendered toast before a gesture is unchanged except the transform expression now includes defaulted swipe vars; before/after Chromium frame shows the same visible toast before the swipe and no toast after accepted swipe. Theme: existing target and type state still resolve; private vars are additive.

Behavior changed intentionally for touch/pen users only: accepted edge swipes dismiss. Mouse drag, close button, F6, endContent controls, opposite/horizontal touches, reduced motion, top/bottom placement direction, RTL start/end placement, and timer resume after cancellation were covered by tests or browser probes.

VERDICT: clear

PERFORMANCE & RESOURCES

Effects/listeners/layout reads:

  • Existing effects kept: auto-hide timer, window blur/focus pause.
  • New effect: 4 element touch listeners per visible non-exiting toast (touchstart, non-passive touchmove, touchend, touchcancel), removed on exit/unmount. Instrumented in Chromium: 4 adds when the toast appears, 4 removes after dismiss.
  • No React state/render pass is added for gesture movement; refs and inline private vars hold transient state.
  • Gesture start reads layout/style once: 1 getBoundingClientRect() and 2 getComputedStyle() reads. Per-move work after acceptance writes private CSS vars only; no per-move layout read.
  • No global listener, observer, dependency, or bundle-sized package addition.

This is new feature cost, not a measured degradation from an existing path. Worst realistic N is bounded by Toast’s visible max (default 5), so the listener count is bounded (20 touch listeners at default max, with 5 non-passive touchmove).

VERDICT: clear

VISUAL EVIDENCE

  • accepted top-edge swipe, before/after, real Chromium: shots/swipe-before.png · shots/swipe-after.png
  • Visual difference: intentional — the toast disappears after an accepted dismissing gesture, matching the stated problem and dismissal behavior. No layout/CSS matrix is owed beyond the interaction states because this PR does not change resting layout, dimensions, colors, target names, or typography.

VERDICT: clear

A11Y & I18N

Auto-covered/static: no new strings in runtime; existing close button still uses @astryx.toast.dismiss; pr-a11y did not appear in the low-count stacked checks, so I did not treat CI as a full accessibility run. Manual Chromium checks: close button keeps accessible name and dismisses; F6 moves focus into the toast onto the dismiss button; interactive descendants do not start swipe; reduced motion collapses transition duration to 0.00001s; RTL still dismisses toward the block edge. Unit tests cover touch cancellation, timer resume, top/bottom RTL, no mouse drag, endContent descendants, and manual reason.

No forced-colors-specific painted state was introduced by the diff; swipe feedback is transform/opacity and the visible close remains the existing Button.

VERDICT: clear

JUDGEMENT

Verdict lines:

  • PROBLEM clear
  • SOLUTION clear
  • ARCHITECTURE BLOCKS — complex gesture protocol has no named testable unit
  • IMPACT clear
  • API clear
  • THEMING clear
  • BREAKING clear
  • PERFORMANCE clear
  • VISUAL EVIDENCE clear
  • A11Y & I18N clear

GOAL: met — real Chromium proves edge-directed touch/pen swipes dismiss while opposite/sideways gestures and interactive children retain their behavior.

DISPOSITION:

  • inline gesture state machine → blocks now — extract useToastGesture, invoked from the stable Toast root
  • placement direction recovered from private CSS → blocks with the same extraction — pass resolved direction into the hook as React data
  • custom renderContent seam → accepted (driven) — default and custom content both preserve swipe and interactive-child arbitration

WORST OUTCOME: “arbitration, thresholds, timer behavior, and cancel/dismiss decisions are only testable through the full viewport/card stack” → request changes under audit C22 / critic R35.

JUDGEMENT NEEDED: none — the capability and lifecycle owner are settled; this is an implementation/test boundary.

request changes

  1. [BLOCKS] The full swipe protocol is embedded in Toast.tsx → one rendering component owns structure plus ~330 lines of gesture state, listeners, thresholds, timing, and timer handoff; focused behavior tests require the whole viewport/card stack
  2. [covered by the same ask] Toast reads placement direction back from private CSS → useToastGesture should take the resolved edge direction as React data from the placement owner

MERGE: hold(request-changes — extract the swipe protocol into a focused hook while keeping lifecycle ownership on Toast)

REVIEW

Thanks, this is good, and I checked it against the custom renderContent branch: the card still owns the gesture, so custom layouts keep swipe dismissal.

One architecture thing I'd like before merge: pull the swipe behavior into a named useToastGesture-style unit, still invoked by the Toast card root. The hook would take the resolved edge direction, pause/resume hooks, dismiss callback, enabled/exiting state, and interactive-descendant policy; it would return the root binding plus transient swipe updates and cleanup. That keeps custom content safely inside the card while making direction arbitration, thresholds, timer pause, and cancel/dismiss decisions independently testable instead of buried in the render file.

Could you make that split while keeping the hook attached to the Toast card, not the custom content?

[Reviewed by Robohands and Cindy]

INLINE

0

EVIDENCE I DID NOT SPEND

  • Open #5428 adds renderContent; its patch does not apply cleanly over this stacked head, so I inspected the seam but did not execute the combined branch. Current endContent composition was driven in Chromium.
  • PR checks listed only Meta CLA, Vercel, and Vercel Preview Comments because this is stacked on #5353; I ran focused local checks instead.
  • Prior GitHub review already called the shape right and asked only rationale nits; this draft does not depend on that for the verdict.

WHAT CINDY CHANGED BEFORE POSTING

  • Added the architecture routing slot and required every composition seam to be driven rather than inferred from source.
  • Distinguished behavior isolation from lifecycle ownership: extract the gesture state machine into a focused useToastGesture-style hook, but keep it invoked by the stable Toast root.
  • Kept the review focused on that architecture request after real Chromium proved built-in content, custom renderContent, the top layer, and interactive custom children all preserve swipe behavior.

Clone this wiki locally