Skip to content

Commit 63ea0fb

Browse files
zernoniaclaude
andcommitted
fix(Drawer): address CodeRabbit review on #2591
Critical: - DrawerContent.vue: non-modal close-auto-focus and interact-outside handlers were reading/writing bare ref variables in template inline handlers. Move them to named functions in <script setup> so the hasInteractedOutside / hasPointerDownOutside refs are accessed via .value explicitly. Fixes broken focus restoration in non-modal mode. - useSwipeDismiss.ts: preventDefault was guarded on isSwiping.value BEFORE processMove, so the very move that crosses the swipe threshold (and sets isSwiping=true) was not prevented, causing a one-frame scroll hitch on mobile. Capture wasSwiping before processMove and prevent on either side of the transition. Major: - useSwipeDismiss.ts: the pendingSwipe block was promoting to an active swipe even when dir === undefined (user dragging against the dismiss direction), which could steal scroll/drag on one-direction drawers. Bail out early when no allowed direction is identified. - DrawerOverlayImpl.vue: useBodyScrollLock was locking unconditionally, which could keep page scroll blocked while forceMount kept the overlay mounted with the drawer closed. Use the returned `locked` ref and sync it with rootContext.open.value via a watcher. - DrawerRoot.vue: update:openComplete was firing via queueMicrotask, which completes almost immediately after the open state changes -- well before any CSS transition finishes. Move the emit to DrawerContentImpl which attaches a one-shot transitionend/animationend listener on the popup element and calls rootContext.notifyOpenComplete only when the transition actually finishes. Guards against child element transitions via event.target === el. - DrawerSwipeArea.vue: directions array captured the initial value only. Wrap in computed so prop/state changes propagate to useSwipeDismiss. - DrawerViewport.vue: only :as and :as-child were forwarded to Primitive, dropping class/style/id/listeners/data-attrs. Use v-bind="props" to match sibling components. Minor: - DrawerIndent.vue: the data-inactive attribute was always set when providerContext was missing. Only set when active is explicitly false. - DrawerTrigger.vue: triggerElement was assigned in onMounted only. Replace with watch on currentElement so v-if / conditional rendering scenarios keep the trigger reference in sync. Clear on unmount. - docs/superpowers/specs/2026-04-04-drawer-design.md: add language tags to fenced blocks (text) to satisfy markdownlint MD040. Not applied: - story --drawer-snap-height -> --drawer-height: CodeRabbit claimed the snap-height var is not produced by the primitive, but it is written by DrawerContentImpl:77 when a snap point is active (and cleared on 81). Changing the story would break snap-point height behavior. Tests: 32/32 still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 3606026 commit 63ea0fb

File tree

10 files changed

+116
-39
lines changed

10 files changed

+116
-39
lines changed

docs/superpowers/specs/2026-04-04-drawer-design.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ Port the [BaseUI Drawer](https://base-ui.com/react/components/drawer) component
1616

1717
## Component Structure
1818

19-
```
19+
```text
2020
packages/core/src/Drawer/
2121
├── DrawerRoot.vue # Context provider + open/modal/swipeDirection state
2222
├── DrawerTrigger.vue # Primitive button that opens drawer
@@ -209,7 +209,7 @@ When dragging past the fully-open position (overshoot in wrong direction), appli
209209

210210
### Returns
211211

212-
```
212+
```text
213213
{
214214
swipePointerProps: { onPointerDown, onPointerMove, onPointerUp }
215215
swipeTouchProps: { onTouchStart, onTouchMove, onTouchEnd }

packages/core/src/Drawer/DrawerContent.vue

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,31 @@ const isTrapFocusOnly = computed(() => rootContext.modal.value === 'trap-focus')
3232
const shouldTrapFocus = computed(() => (isFullModal.value || isTrapFocusOnly.value) && rootContext.open.value)
3333
const shouldHideOthers = computed(() => isFullModal.value ? currentElement.value : undefined)
3434
useHideOthers(shouldHideOthers)
35+
36+
// Non-modal handlers defined as named functions so the refs are accessed via
37+
// `.value` explicitly and the reads/writes are unambiguous.
38+
function onCloseAutoFocusNonModal(e: Event) {
39+
if (!e.defaultPrevented) {
40+
if (!hasInteractedOutside.value)
41+
rootContext.triggerElement.value?.focus()
42+
e.preventDefault()
43+
}
44+
hasInteractedOutside.value = false
45+
hasPointerDownOutside.value = false
46+
}
47+
48+
function onInteractOutsideNonModal(e: any) {
49+
if (!e.defaultPrevented) {
50+
hasInteractedOutside.value = true
51+
if (e.detail.originalEvent.type === 'pointerdown')
52+
hasPointerDownOutside.value = true
53+
}
54+
const target = e.target as HTMLElement
55+
if (rootContext.triggerElement.value?.contains(target))
56+
e.preventDefault()
57+
if (e.detail.originalEvent.type === 'focusin' && hasPointerDownOutside.value)
58+
e.preventDefault()
59+
}
3560
</script>
3661

3762
<template>
@@ -64,24 +89,8 @@ useHideOthers(shouldHideOthers)
6489
v-bind="{ ...props, ...emitsAsProps, ...$attrs }"
6590
:trap-focus="shouldTrapFocus"
6691
:disable-outside-pointer-events="false"
67-
@close-auto-focus="(e: Event) => {
68-
if (!e.defaultPrevented) {
69-
if (!hasInteractedOutside) rootContext.triggerElement.value?.focus()
70-
e.preventDefault()
71-
}
72-
hasInteractedOutside = false
73-
hasPointerDownOutside = false
74-
}"
75-
@interact-outside="(e: any) => {
76-
if (!e.defaultPrevented) {
77-
hasInteractedOutside = true
78-
if (e.detail.originalEvent.type === 'pointerdown')
79-
hasPointerDownOutside = true
80-
}
81-
const target = e.target as HTMLElement
82-
if (rootContext.triggerElement.value?.contains(target)) e.preventDefault()
83-
if (e.detail.originalEvent.type === 'focusin' && hasPointerDownOutside) e.preventDefault()
84-
}"
92+
@close-auto-focus="onCloseAutoFocusNonModal"
93+
@interact-outside="onInteractOutsideNonModal"
8594
>
8695
<slot />
8796
</DrawerContentImpl>

packages/core/src/Drawer/DrawerContentImpl.vue

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,42 @@ function onInteractOutside(event: any) {
219219
emits('interactOutside', event)
220220
}
221221
222+
// --- update:openComplete wiring ---
223+
// Fire `update:openComplete` on the popup's own transitionend/animationend,
224+
// not on a microtask — consumers rely on this marker to know the enter/exit
225+
// transition has actually finished. We track the current listener so a rapid
226+
// open→close doesn't leak or double-fire, and guard with `event.target === el`
227+
// so child element transitions don't spuriously resolve the drawer lifecycle.
228+
let openCompleteCleanup: (() => void) | undefined
229+
230+
function clearOpenCompleteListener() {
231+
openCompleteCleanup?.()
232+
openCompleteCleanup = undefined
233+
}
234+
235+
watch(() => rootContext.open.value, (isOpen) => {
236+
clearOpenCompleteListener()
237+
const el = currentElement.value
238+
if (!el) {
239+
// If there's no element (e.g. closed without ever mounting), fire
240+
// synchronously so consumers still get the contract.
241+
rootContext.notifyOpenComplete(isOpen)
242+
return
243+
}
244+
const handler = (event: Event) => {
245+
if (event.target !== el)
246+
return
247+
clearOpenCompleteListener()
248+
rootContext.notifyOpenComplete(isOpen)
249+
}
250+
el.addEventListener('transitionend', handler)
251+
el.addEventListener('animationend', handler)
252+
openCompleteCleanup = () => {
253+
el.removeEventListener('transitionend', handler)
254+
el.removeEventListener('animationend', handler)
255+
}
256+
})
257+
222258
// Data attributes
223259
const dataAttributes = computed(() => {
224260
const attrs: Record<string, string | undefined> = {
@@ -264,6 +300,7 @@ onMounted(() => {
264300
onUnmounted(() => {
265301
rootContext.notifyParentHasNestedDrawer?.(false)
266302
unsubscribeNestedProgress?.()
303+
clearOpenCompleteListener()
267304
})
268305
269306
// Dev warning for missing DrawerTitle

packages/core/src/Drawer/DrawerIndent.vue

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ onUnmounted(() => {
5353
v-bind="props"
5454
:ref="forwardRef"
5555
:data-active="providerContext?.active.value ? '' : undefined"
56-
:data-inactive="!providerContext?.active.value ? '' : undefined"
56+
:data-inactive="providerContext?.active.value === false ? '' : undefined"
5757
:style="{ [DRAWER_CSS_VARS.swipeProgress]: '0' }"
5858
>
5959
<slot />

packages/core/src/Drawer/DrawerOverlayImpl.vue

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ export interface DrawerOverlayImplProps extends PrimitiveProps {}
55
</script>
66

77
<script setup lang="ts">
8-
import { computed } from 'vue'
8+
import { computed, watch } from 'vue'
99
import { Primitive } from '@/Primitive'
1010
import { useBodyScrollLock, useForwardExpose } from '@/shared'
1111
import { injectDrawerRootContext } from './DrawerRoot.vue'
@@ -14,7 +14,14 @@ import { DRAWER_CSS_VARS } from './utils'
1414
defineProps<DrawerOverlayImplProps>()
1515
const rootContext = injectDrawerRootContext()
1616
17-
useBodyScrollLock(true)
17+
// Only lock body scroll while the drawer is actually open. With `forceMount`,
18+
// the overlay may stay mounted while closed — unconditional lock would keep
19+
// the page scroll blocked.
20+
const locked = useBodyScrollLock(rootContext.open.value)
21+
watch(() => rootContext.open.value, (open) => {
22+
locked.value = open
23+
}, { immediate: true })
24+
1825
useForwardExpose()
1926
2027
// BaseUI parity: the backdrop carries data-swiping / data-swipe-direction so

packages/core/src/Drawer/DrawerRoot.vue

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ export interface DrawerRootContext {
6969
isSwiping: Ref<boolean>
7070
nestedSwipeProgressStore: NestedSwipeProgressStore
7171
onOpenChange: (value: boolean, reason?: DrawerOpenChangeReason) => void
72+
notifyOpenComplete: (value: boolean) => void
7273
setActiveSnapPoint: (point: DrawerSnapPoint | null) => void
7374
onPopupHeightChange: (height: number) => void
7475
onNestedFrontmostHeightChange: (height: number) => void
@@ -169,6 +170,9 @@ provideDrawerRootContext({
169170
uncontrolledOpen.value = value
170171
emit('update:open', value, details)
171172
},
173+
notifyOpenComplete(value) {
174+
emit('update:openComplete', value)
175+
},
172176
setActiveSnapPoint(point) { activeSnapPoint.value = point },
173177
onPopupHeightChange(h) {
174178
popupHeight.value = h
@@ -223,11 +227,9 @@ watch(open, (isOpen) => {
223227
}
224228
}, { immediate: true })
225229
226-
// Emit openComplete after next tick (approximates Base UI's onOpenChangeComplete
227-
// which fires after CSS transitions. Consumers can refine via onTransitionEnd.)
228-
watch(open, (isOpen) => {
229-
queueMicrotask(() => emit('update:openComplete', isOpen))
230-
})
230+
// `update:openComplete` is emitted by DrawerContentImpl after the popup's
231+
// enter/exit transition actually finishes (via transitionend/animationend on
232+
// the popup element). DrawerRoot just exposes the notify hook through context.
231233
232234
onUnmounted(() => {
233235
providerContext?.removeDrawer(contentId)

packages/core/src/Drawer/DrawerSwipeArea.vue

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,14 @@ const openDirection = computed<SwipeDirection>(() => {
4040
4141
const enabled = computed(() => !props.disabled && !rootContext.open.value)
4242
43+
// Keep the directions array reactive — `[openDirection.value]` would capture
44+
// the initial value only and ignore prop/state updates.
45+
const directions = computed<SwipeDirection[]>(() => [openDirection.value])
46+
4347
useSwipeDismiss({
4448
enabled,
4549
elementRef: currentElement,
46-
directions: [openDirection.value],
50+
directions,
4751
movementCssVars: {
4852
x: DRAWER_CSS_VARS.swipeMovementX,
4953
y: DRAWER_CSS_VARS.swipeMovementY,

packages/core/src/Drawer/DrawerTrigger.vue

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ export interface DrawerTriggerProps extends PrimitiveProps {}
55
</script>
66

77
<script setup lang="ts">
8-
import { onMounted } from 'vue'
8+
import { onUnmounted, watch } from 'vue'
99
import { Primitive } from '@/Primitive'
1010
import { useForwardExpose } from '@/shared'
1111
import { injectDrawerRootContext } from './DrawerRoot.vue'
@@ -14,8 +14,17 @@ const props = withDefaults(defineProps<DrawerTriggerProps>(), { as: 'button' })
1414
const rootContext = injectDrawerRootContext()
1515
const { forwardRef, currentElement } = useForwardExpose()
1616
17-
onMounted(() => {
18-
rootContext.triggerElement.value = currentElement.value
17+
// Keep triggerElement in sync with the rendered element — useForwardExpose's
18+
// currentElement is a reactive computed ref, and v-if / conditional rendering
19+
// can swap the underlying DOM node, so a one-shot onMounted assignment would
20+
// go stale and break finalFocus restoration in DrawerContent.
21+
watch(currentElement, (el) => {
22+
rootContext.triggerElement.value = el
23+
}, { immediate: true })
24+
25+
onUnmounted(() => {
26+
if (rootContext.triggerElement.value === currentElement.value)
27+
rootContext.triggerElement.value = undefined
1928
})
2029
</script>
2130

packages/core/src/Drawer/DrawerViewport.vue

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,15 @@ import { injectDrawerRootContext } from './DrawerRoot.vue'
1717
* this component is currently a passthrough that carries the `data-drawer-viewport`
1818
* attribute for downstream selectors. It exists primarily for API parity.
1919
*/
20-
withDefaults(defineProps<DrawerViewportProps>(), { as: 'div' })
20+
const props = withDefaults(defineProps<DrawerViewportProps>(), { as: 'div' })
2121
const { forwardRef } = useForwardExpose()
2222
const rootContext = injectDrawerRootContext()
2323
</script>
2424

2525
<template>
2626
<Primitive
27+
v-bind="props"
2728
:ref="forwardRef"
28-
:as="as"
29-
:as-child="asChild"
3029
data-drawer-viewport=""
3130
:data-state="rootContext.open.value ? 'open' : 'closed'"
3231
>

packages/core/src/Drawer/composables/useSwipeDismiss.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -263,8 +263,14 @@ export function useSwipeDismiss(options: UseSwipeDismissOptions) {
263263
const dir: SwipeDirection | undefined = toValue(directions).find(d => getDisplacement(d, dx, dy) > 0)
264264

265265
if (pendingSwipe && pendingSwipeStartPos) {
266+
// Only promote to an active swipe when we've identified an allowed
267+
// direction. If the user is dragging against the dismiss direction
268+
// (dir === undefined), bail out so we don't steal scroll/drag from
269+
// one-direction drawers.
270+
if (!dir)
271+
return
266272
const pending = getDisplacement(
267-
dir ?? toValue(directions)[0],
273+
dir,
268274
pos.x - pendingSwipeStartPos.x,
269275
pos.y - pendingSwipeStartPos.y,
270276
)
@@ -481,10 +487,14 @@ export function useSwipeDismiss(options: UseSwipeDismissOptions) {
481487
// will lock the axis and retry the decision on the next frame.
482488
}
483489

484-
if (isSwiping.value)
485-
e.preventDefault()
486-
490+
// Capture swiping state BEFORE processMove — that function is what sets
491+
// isSwiping.value = true on the move that crosses the drag threshold, so
492+
// we need to preventDefault on that same event (not the next one) to avoid
493+
// a visible scroll hitch on the first frame of the gesture on mobile.
494+
const wasSwiping = isSwiping.value
487495
processMove(el, pos, e.timeStamp)
496+
if (wasSwiping || isSwiping.value)
497+
e.preventDefault()
488498
}
489499

490500
function onTouchEnd() {

0 commit comments

Comments
 (0)