Skip to content
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 modal focus trap with additional elements and arrow navigation #4406

Merged
merged 2 commits into from Aug 16, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 24 additions & 13 deletions src/components/NcModal/NcModal.vue
Expand Up @@ -616,18 +616,26 @@ export default {
}
},

// Key Handlers
handleKeydown(e) {
switch (e.keyCode) {
case 37: // left arrow
this.previous(e)
break
case 39: // right arrow
this.next(e)
break
case 27: // escape key
this.close(e)
break
/**
* @param {KeyboardEvent} event - keyboard event
*/
handleKeydown(event) {
if (event.key === 'Escape') {
return this.close(event)
}

const arrowHandlers = {
ArrowLeft: this.previous,
ArrowRight: this.next,
}
if (arrowHandlers[event.key]) {
// Ignore arrow navigation, if there is a current focus outside the modal.
// For example, when the focus is in Sidebar or NcActions's items,
// arrow navigation should not be intercept by modal slider
if (document.activeElement && !this.$el.contains(document.activeElement)) {
return
}
return arrowHandlers[event.key](event)
}
},

Expand Down Expand Up @@ -714,10 +722,13 @@ export default {
allowOutsideClick: true,
fallbackFocus: contentContainer,
trapStack: getTrapStack(),
// Esc can be used without stop in content or additionalTrapElements where it should not deacxtivate modal's focus trap.
// Focus trap is deactivated on modal close anyway.
escapeDeactivates: false,
Comment on lines +725 to +727
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Esc can be used without stop in content or additionalTrapElements where it should not deacxtivate modal's focus trap.
// Focus trap is deactivated on modal close anyway.
escapeDeactivates: false,

Not used anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'm not sure if it is required, I need to re-check.

But disabling escapeDeactivates is definitely safe:

  • If a modal is closed by ESC, trap is deactivated anyway
  • If a modal is not closed by ESC, the focus trap should also not be deactivated.

}

// Init focus trap
this.focusTrap = createFocusTrap(contentContainer, options)
this.focusTrap = createFocusTrap([contentContainer, ...this.additionalTrapElements], options)
Pytal marked this conversation as resolved.
Show resolved Hide resolved
this.focusTrap.activate()
},
clearFocusTrap() {
Expand Down