Skip to content

Fix modal focus trap with additional elements and arrow navigation - #4406

Merged
susnux merged 2 commits into
masterfrom
fix/a11y-modal-focus-trap
Aug 16, 2023
Merged

Fix modal focus trap with additional elements and arrow navigation#4406
susnux merged 2 commits into
masterfrom
fix/a11y-modal-focus-trap

Conversation

@ShGKme

@ShGKme ShGKme commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

☑️ 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 requested review from AndyScherzinger and Pytal August 9, 2023 14:56
@ShGKme ShGKme self-assigned this Aug 9, 2023
@ShGKme

ShGKme commented Aug 9, 2023

Copy link
Copy Markdown
Contributor Author

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>
@ShGKme
ShGKme force-pushed the fix/a11y-modal-focus-trap branch from ca40299 to 5d809f6 Compare August 9, 2023 15:11
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
ShGKme force-pushed the fix/a11y-modal-focus-trap branch from 5d809f6 to d7410b7 Compare August 9, 2023 18:06
@ShGKme

ShGKme commented Aug 9, 2023

Copy link
Copy Markdown
Contributor Author

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.

@ShGKme
ShGKme requested a review from AndyScherzinger August 9, 2023 18:21
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/components/NcModal/NcModal.vue
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