Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions packages/vuetify/src/components/VDialog/VDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,39 @@ export const VDialog = genericComponent<OverlaySlots>()({
}
}

function onKeydown (e: KeyboardEvent) {
if (e.key !== 'Tab' || !overlay.value?.contentEl) return

const focusable = focusableChildren(overlay.value.contentEl)
if (!focusable.length) return

const firstElement = focusable[0]
const lastElement = focusable[focusable.length - 1]
const active = document.activeElement as HTMLElement | null

if (e.shiftKey && active === firstElement) {
e.preventDefault()
lastElement.focus()
} else if (!e.shiftKey && active === lastElement) {
e.preventDefault()
firstElement.focus()
}
}

onBeforeUnmount(() => {
document.removeEventListener('focusin', onFocusin)
document.removeEventListener('keydown', onKeydown)
Copy link
Contributor

@J-Sek J-Sek Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new handler replaces onFocusin. Is there any reason left to keep both? Yes, to capture the first focus.

Lines 69-79 could be replaced with focusable[0]?.focus() like in VMenu.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see.
I edited it according to the comment before editing, so I'll correct it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two questions.

  1. Did you mean that the first focus was on the dialog itself (v-overlay__content)? I assumed it was on the first focusable element in the dialog (the "Disagree" button in the video below), so I just wanted to confirm.

  2. I was checking the focus behavior of VMenu and noticed that when you press tab on Vmenu, the behavior is correct the first time you press tab, but the second time, Vmenu seems to close. Should I fix this?

demo in play.vuetify

screen-capture.24.webm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. VMenu and VDialog, both try to "capture" the focus when an interaction occurs and the focus is set outside. I don't know the origin of this implementation, but it feels slightly better than auto-focus within (done by native <dialog>) as it would be reasonable to load the dialog content progressively, show spinner, load stuff.

That said, the code between those is not the same because VDialog applied (bugged) focus trap. My suggestion would be to have keydown handler focus on the trap, while focusin handler can do the same thing as VMenu, so as a result, the code should look the same. No need to refactor/abstract a new composable. It is going to happen eventually (#22106). For now, we should just avoid 2 pieces of code having the same responsibility.

  1. Better to keep the PR focused on one thing. It might feel a bit annoying that user tab-out of the menu and is supposed to use up/down arrows for a VList within.. Nobody rised this issue until now, so I guess it is not a bug. It might be equally annoying to be stuck within a menu/popover because developer forgot to put :retain-focus="false", right? Either way, 3.11.0 will hopefully give us an option to set focus trap VMenu and VNavigationDrawer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanation and sharing the current refactor PR 22106!
I've pushed a fix that brings the code as close as possible to Vmenu onFocus. I'd appreciate it if you could review it.

})

if (IN_BROWSER) {
watch(() => isActive.value && props.retainFocus, val => {
val
? document.addEventListener('focusin', onFocusin)
: document.removeEventListener('focusin', onFocusin)
if (val) {
document.addEventListener('focusin', onFocusin)
document.addEventListener('keydown', onKeydown)
} else {
document.removeEventListener('focusin', onFocusin)
document.removeEventListener('keydown', onKeydown)
}
}, { immediate: true })
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,48 @@ describe('VDialog', () => {
await userEvent.click(element)
await expect.poll(() => onAfterLeave).toHaveBeenCalledTimes(1)
})

it('should focus on the last element when shift + tab key is pressed on the first element', async () => {
const model = ref(true)
render(() => (
<div>
<VDialog v-model={ model.value } persistent>
<div>
<button data-testid="first">First</button>
<button data-testid="last">Last</button>
</div>
</VDialog>
</div>
))
const first = screen.getByCSS('button[data-testid="first"]')
const last = screen.getByCSS('button[data-testid="last"]')

first.focus()
await expect.poll(() => document.activeElement).toBe(first)

await userEvent.tab({ shift: true })
await expect.poll(() => document.activeElement).toBe(last)
})

it('should focus on the first element when Tab key is pressed on the last element', async () => {
const model = ref(true)
render(() => (
<div>
<VDialog v-model={ model.value }>
<div>
<button data-testid="first">First</button>
<button data-testid="last">Last</button>
</div>
</VDialog>
</div>
))
const first = screen.getByCSS('button[data-testid="first"]')
const last = screen.getByCSS('button[data-testid="last"]')

last.focus()
await expect.poll(() => document.activeElement).toBe(last)

await userEvent.tab()
await expect.poll(() => document.activeElement).toBe(first)
})
})