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

Navigation: Improve accessibility (#4651, #4755, #4756) #8684

Merged

Conversation

igotinfected
Copy link
Contributor

@igotinfected igotinfected commented Apr 13, 2024

Description

Implemented accessibility recommendations for MudNavGroup, MudNavMenu, and MudNavLink.

The main goal of this PR is to improve/fix keyboard navigation in navigation blocks and to address the issues linked below.

I introduced a NavigationContext that allows passing state down a navigation group chain which allows us to figure out whether buttons/links at a specific level of the chain should be focusable via keyboard navigation.

Usage of tabindex, aria-label, aria-expanded, aria-hidden, aria-controls attributes should now be better (but not necessarily perfect).

I also had to change the Localizer API to accept arguments which can be quite useful (passing a value to a longer translation to include it in a certain position for example).

Fixes #4651
Resolves #4756 (original issue is fixed/improved but implementation differs from request)
Fixes #4755

How Has This Been Tested?

Visually + unit tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

NavMenu Sub Groups example comparison

chrome_E1b0GaRbDx.mp4

MudBlazor docs navigation comparison

chrome_aVp2Q3o3LD.mp4

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added the bug Something does not work as intended/expected label Apr 13, 2024
Copy link

codecov bot commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (28bc599) to head (183b4a9).
Report is 55 commits behind head on dev.

❗ Current head 183b4a9 differs from pull request most recent head fa22ed0. Consider uploading reports for the commit fa22ed0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8684      +/-   ##
==========================================
+ Coverage   89.82%   90.10%   +0.27%     
==========================================
  Files         412      418       +6     
  Lines       11878    12028     +150     
  Branches     2364     2369       +5     
==========================================
+ Hits        10670    10838     +168     
+ Misses        681      658      -23     
- Partials      527      532       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@igotinfected igotinfected marked this pull request as ready for review April 14, 2024 13:30
@igotinfected
Copy link
Contributor Author

igotinfected commented Apr 14, 2024

FYI @ScarletKuro @henon!

These changes should work with #8672. I made MudNavGroup a nav too because that cleans up the accessibility tree quite a bit:

image

@ScarletKuro I changed the Localizer API without thinking too much about it, let me know if you want it done differently (i.e. by adding an overload instead).

@ScarletKuro
Copy link
Member

@ScarletKuro I changed the Localizer API without thinking too much about it, let me know if you want it done differently (i.e. by adding an overload instead).

Looks ok, as I understand it's optional and it doesn't break existing code and doesn't change how the Localizer is behaving.

@henon
Copy link
Collaborator

henon commented Apr 16, 2024

These changes should work with !8672.

@igotinfected please use # instead of ! to link to issues or PRs. i.e. #8672

@igotinfected
Copy link
Contributor Author

These changes should work with !8672.

@igotinfected please use # instead of ! to link to issues or PRs. i.e. #8672

My bad, thanks for catching that (and in one of my other comments too), I'm on Azure DevOps at work where you have to use ! for PRs so it's just muscle memory 🤪

@henon henon merged commit 22955eb into MudBlazor:dev Apr 18, 2024
2 checks passed
@henon
Copy link
Collaborator

henon commented Apr 18, 2024

@igotinfected Thanks. For this we don't need to add anything to the v7 migration guide right?

@igotinfected
Copy link
Contributor Author

@henon nope should be transparent!

@igotinfected igotinfected deleted the feature/improve-mudnav-accessibility branch April 18, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
3 participants