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

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Aug 9, 2023

☑️ Resolves

Two problems with focus trap break navigation with modal.

Problem 1: When Viewer is opened with sidebar, focus on the sidebar is not possible
Source: additionalTrapElements are not used on init but only on change.

Problem 2: NcModal intercept arrow navigation for slider (next/prev)

🖼️ Screenshots

🏚️ Before

before-1.mp4
before-2.mp4

🏡 After

after.mp4

🚧 Tasks

  • Also use additionalTrapElements on init
  • On keyboard navigation handler check if there is an active element.
    • If there is an element and it's outside the modal, then it is
      • either element from additionalTrapElements
      • or some pop over element like emoji picker or nc actions that also have its own navigation
    • Alternative solution: always disable arrow navigation if there is an active element

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added bug Something isn't working feature: modal Related to the modal component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Aug 9, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Aug 9, 2023
@ShGKme ShGKme self-assigned this Aug 9, 2023
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 9, 2023

There is one more problem: when the next or prev slide is a video, then tab navigation goes to visually hidden video controls. But this should be fixed in Viewer (I'll make a PR later)

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 9, 2023

PR is updated.

Only arrow navigation ignores elements outside (which is the problem in mentioned issue), while ESC still closes the modal as it was before, regardless of focus.

It is a problem for elements that uses ESC itself. For example, closing NcSelect by ESC. But I think, it's better to fix it on such components sides (with stop propagation of ESC keydown). Otherwise even focus on sidebar element after click prevents closing Viewer by ESC.

Comment on lines +725 to +727
// 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,
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.

src/components/NcModal/NcModal.vue Show resolved Hide resolved
@susnux susnux merged commit 1079040 into master Aug 16, 2023
16 checks passed
@susnux susnux deleted the fix/a11y-modal-focus-trap branch August 16, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: modal Related to the modal component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Not possible to navigate the tabs in sidebar
4 participants