-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
fix(VDialog): fix focus trap when tabbing forward #22101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(VDialog): fix focus trap when tabbing forward #22101
Conversation
|
||
onBeforeUnmount(() => { | ||
document.removeEventListener('focusin', onFocusin) | ||
document.removeEventListener('keydown', onKeydown) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new handler replaces Yes, to capture the first focus.onFocusin
. Is there any reason left to keep both?
Lines 69-79 could be replaced with focusable[0]?.focus()
like in VMenu.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two questions.
-
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.
-
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?
screen-capture.24.webm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
- 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.
There was a problem hiding this comment.
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.
Description
fixes #21945
Markup: