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(NcAppNavigation): add focus trap on mobile and improve a11y #4633

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 11, 2023

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
nav-before nav-after

🚧 Tasks

  • Add focus trap, and activate it on open navigation on mobile, deactivate otherwise
  • Improve a11y a bit:
    • Use <nav> instead of role="navigation" cc @JuliaKirschenheuter to confirm that this is fine
    • Move the toggle button out from the navigation
    • Add ariaLabel and ariaLabelledBy props to be set on navigation
  • Close navigation by ESC on mobile
  • Update tests
    • Test by class instead of internal data property (testing is closed or open state by class also is not great, but we should not test by internal implementation as data property is not a part of the component interface). Alternative: aria-hidden.
    • Add mobile state test
    • Add a11y tests

🏁 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: app-navigation Related to the app-navigation component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Oct 11, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Oct 11, 2023
@ShGKme ShGKme requested a review from szaimen October 11, 2023 14:34
@ShGKme ShGKme self-assigned this Oct 11, 2023
@szaimen
Copy link
Contributor

szaimen commented Oct 11, 2023

node tests seems to be failing

@szaimen szaimen added the 2. developing Work in progress label Oct 11, 2023
@ShGKme ShGKme changed the title feat(NcAppNavigation): add focus trap on mobile feat(NcAppNavigation): add focus trap on mobile and improve a11y Oct 11, 2023
@ShGKme ShGKme changed the title feat(NcAppNavigation): add focus trap on mobile and improve a11y fix(NcAppNavigation): add focus trap on mobile and improve a11y Oct 11, 2023
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Tested and the focus trap seems to work :)

However the ESC key doesnt toggle the navigation currently

Aufzeichnung.2023-10-12.125445.mp4


handleEsc() {
if (this.isMobile) {
this.toggleNavigation(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to work currently in my testing

Copy link
Contributor

Choose a reason for hiding this comment

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

the reason was probably that I tested on a screen with higher res than 1024px (so ismobile was false)

@ShGKme ShGKme force-pushed the fix/app-navigation--focus-trap-on-mobile branch from 1dab23b to 70886ef Compare November 7, 2023 14:38
@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 7, 2023

PR Status update: checking issue with Talk loosing tab navigation with focus trap

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 8, 2023

The Talk issue is related but appears in any focus trap container with input with the trailing button visible only on focus.

What happens:

  1. Tab -> Focus on input.
  2. Now the "Clear" trailing button is visible.
  3. Tab -> Focus the trailing button.
  4. Tab -> issue
    1. Blurring focus from the trailing button hides it, and removes it from the DOM
    2. focus-trap with MutationObserver sees it and thinks that the current focused element is removed
    3. Then focus-trap moves the focus to the first element of the container

focus

Source:
https://github.com/focus-trap/focus-trap/blob/a49e9b247bb4d15e3a16ddcc9e6b847a62725c77/index.js#L862-L875

This behavior is not configurable.

IMO, Possible solutions could be:

  • Fix it on the focus-trap side
  • On Nextcloud/vue: Do not remove the trailing button from the DOM, only hide it
  • On Talk: Hide the trailing button with a delay

@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 8, 2023
@ShGKme ShGKme marked this pull request as ready for review November 8, 2023 09:16
@susnux
Copy link
Contributor

susnux commented Nov 8, 2023

Maybe report this upstream, but

On Talk: Do not remove the trailing button from the DOM, only hide it

Sounds the best for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants