[drawer] Fix popup flashing fully open for a frame on swipe re-grab - #5112
Conversation
atomiks
added
type: regression
commit: |
Bundle size
PerformanceTotal duration: 1,105.77 ms -8.00 ms(-0.7%) | Renders: 78 (+0) No significant changes — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
6a117d8 to
bcc1edb
Compare
Two independent writers reset the popup's swipe transform/movement for a single frame when a swipe gesture is interrupted and re-grabbed: - Popup re-grab: getDragStyles gated transform/transition on the lagging `isSwiping` state instead of `isSwipingRef`, so a render committing before `setSwiping(true)` flushed stripped the inline transform that syncDragStyles had just written, dropping the popup to its resting transform for a frame. - SwipeArea re-grab during close: the popup's --drawer-swipe-movement-* are written imperatively by the swipe area (trackDrag: false), but the viewport also writes them on render (getDragStyles) and on its open-reset effect (resetSwipe -> syncDragStyles), both resetting them to 0px while the viewport itself isn't swiping. A shared swipeAreaActiveRef lets the swipe area re-assert its movement in a layout effect (pre-paint) and the viewport skip resetSwipe while the swipe area drives the open (post-paint).
bcc1edb to
70fa4cd
Compare
github-actions
Bot
added
the
PR: out-of-date
…poitras-8f7472 # Conflicts: # packages/react/src/drawer/swipe-area/DrawerSwipeArea.tsx
github-actions
Bot
removed
the
PR: out-of-date
The open-reset effect skipped both `resetSwipe` and `clearSwipeRelease` while `Drawer.SwipeArea` drove the open. Only `resetSwipe` zeroes the popup's `--drawer-swipe-movement-*` (the flash); `clearSwipeRelease` doesn't touch them, so skipping it leaked stale release state (`swipeStrength` / `data-ending-style`) from a prior dismiss into a swipe-area re-open when the popup is kept mounted. Always run `clearSwipeRelease`; gate only `resetSwipe`. Add a Chromium test that re-grabs the swipe area mid-exit-animation (so the popup is still mounted as the open commit lands) and asserts the popup keeps its swipe movement instead of flashing to `0px`.
There was a problem hiding this comment.
Pull request overview
This PR fixes a one-frame visual glitch in the Drawer swipe interactions where the popup can momentarily snap to its fully-open/resting position when a swipe is interrupted and quickly re-grabbed. It aligns render-time drag styling with the imperative drag writer and coordinates the viewport’s swipe reset behavior with Drawer.SwipeArea when the swipe area is driving the open.
Changes:
- Update
useSwipeDismiss.getDragStyles()to readisSwipingRef.current(instead of laggingisSwipingstate) so React renders don’t clobber imperative drag transforms mid-gesture. - Introduce a shared
swipeAreaActiveRefin drawer root context to prevent the viewport from resetting swipe movement vars during swipe-area-driven open commits. - Add regression tests covering the lagging-render transform case and the swipe-area re-grab during close case.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/utils/useSwipeDismiss.ts | Uses isSwipingRef.current in getDragStyles to avoid a render-path transform reset during swipes. |
| packages/react/src/utils/useSwipeDismiss.test.tsx | Adds a regression test ensuring lagging renders still preserve the active drag transform/transition. |
| packages/react/src/drawer/viewport/DrawerViewport.tsx | Skips resetSwipe() on open while the swipe area is actively driving the open gesture. |
| packages/react/src/drawer/swipe-area/DrawerSwipeArea.tsx | Re-asserts swipe movement styles in a layout effect and tracks swipe-area-driven opens via swipeAreaActiveRef. |
| packages/react/src/drawer/swipe-area/DrawerSwipeArea.test.tsx | Adds a browser-only regression test for swipe-area re-grab during close without movement var clobbering. |
| packages/react/src/drawer/root/DrawerRootContext.ts | Extends drawer root context with swipeAreaActiveRef. |
| packages/react/src/drawer/root/DrawerRoot.tsx | Creates and provides swipeAreaActiveRef through context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The commit that opens the drawer re-renders the popup, resetting `--swipe-movement-*` to `0px` | ||
| // (the viewport isn't swiping). Re-assert after the DOM mutation but before paint. No deps: must | ||
| // run on every commit the swipe area participates in. | ||
| useIsoLayoutEffect(() => { | ||
| if (swipeActive && appliedSwipeStylesRef.current) { | ||
| applySwipeMovement(); | ||
| } | ||
| }); |
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
A focused, well-scoped fix for a one-frame flash where the Drawer popup snaps to its fully-open position when a swipe gesture is re-grabbed. The two mechanisms (SwipeArea re-grab via the shared swipeAreaActiveRef, and the getDragStyles ref-vs-state divergence) are both real and correctly addressed. Nothing is merge-blocking. I verified both new tests pass in Chromium, confirmed the useSwipeDismiss test genuinely fails when the isSwipingRef read is reverted, and confirmed ESLint passes on all changed files.
Bugs (0)
No findings.
I traced the lifetime of swipeAreaActiveRef for the regression I was most worried about — the viewport skipping resetSwipe() leaving stale viewport swipe state. The ref is true only during an active swipe-area gesture (set in applySwipeMovement, cleared in clearSwipeStyles, which runs on both finishSwipeInteraction and the !enabled effect), and the viewport's open-reset effect reads it at the instant open flips true — exactly when the ref is legitimately true. After release the ref is already false, so a subsequent non-swipe open still runs resetSwipe() normally. No stuck-true path leaks.
One residual edge (benign, not actionable): in DrawerPopup, getDragStyles() now reads isSwipingRef while the adjacent snap-point branch (if (swiping && …)) still reads the lagging swiping state. On a snap-points-down drawer during a lagging frame, the snap-point offset is computed from the raw freeze movement rather than getSnapPointSwipeMovement. The transform is stripped by design in both branches and the movement var is preserved, so there's no visible flash — but a maintainer extending the snap-point path may want the two reads to agree.
Tests (0)
No findings.
Both added tests are strong regression guards. The useSwipeDismiss test captures getDragStyles from a render where swiping state is false and asserts the closure still emits the freeze transform — I confirmed it fails (transform undefined) when the fix is reverted. The SwipeArea test exercises a real exit-animation re-grab and pins the exact --drawer-swipe-movement-y: 120px value (200px popup − 80px displacement).
Simplifications (0)
No findings.
Docs (0)
No findings. The new swipeAreaActiveRef JSDoc and the inline comments on the viewport effect, the useIsoLayoutEffect, and the getDragStyles ref read accurately describe the mechanism.
Verdict
Approve — correct, minimal, and well-covered fix; no blocking or non-blocking issues survived verification (tests pass, fix proven necessary by reverting it).
🤖 Review generated with Claude Code
Summary
Fixes a one-frame glitch where the Drawer popup flashes to its fully-open position when a swipe gesture is interrupted and quickly re-grabbed. Two independent code paths caused it.
SwipeArea re-grab during close
Drawer.SwipeAreawrites the popup's--drawer-swipe-movement-*imperatively, but the viewport also resets them to0px(on render viagetDragStyles, and post-paint via the open-reset effect'sresetSwipe) whenever it isn't swiping — clobbering the swipe area's values for a frame. A sharedswipeAreaActiveReflets the swipe area re-assert its movement in a layout effect and the viewport skipresetSwipewhile the swipe area drives the open.Popup re-grab
This one is mostly impossible to encounter in real usage
Regression from #4867, which moved the per-move drag transform onto imperative inline writes (
syncDragStyles). The render-path writer (getDragStyles) still gated the transform on the laggingisSwipingstate, so a render committing beforesetSwiping(true)flushed would strip the transformsyncDragStyleshad just written, dropping the popup to its resting position for a frame. It now readsisSwipingRef.currentto match the imperative writer.