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: Fix accessibility findings with app layout toolbar #3063

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

avinashbot
Copy link
Member

@avinashbot avinashbot commented Nov 26, 2024

Description

Made the following app layout changes:

  • Provided a label to the navigation region around the side navigation trigger button
  • Removed undefined classname and testid findings
  • Provided matching id to bottom split panel to match aria-controls on the split panel trigger
  • Miscellaneous dev page labelling fixes

Unlabeled side navigation trigger

The issue we're facing is one of an unlabeled nav element on the page. We can't just assign the ariaLabels.navigation value to it, because we're already assigning to the open side navigation panel, and we can't have two navigation landmarks with the same labels. The "floating button" design doesn't have this issue because it goes away as soon as the panel is opened. We can do the same thing here, by just removing the navigation role around the button container when the panel is open.

I didn't change the nav to a div out of pure caution - didn't want to give this change the chance to break any consumer tests.

Related links, issue #, if available: AWSUI-60014

How has this been tested?

Passing existing tests, verified locally with VoiceOver. Will definitely need running through the dev pipeline (AwsUi-v3-dwaraa) and a dry-run (7134801478), just to be extra sure.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.34%. Comparing base (da7a8fa) to head (4eb02bf).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3063    +/-   ##
========================================
  Coverage   96.34%   96.34%            
========================================
  Files         779      779            
  Lines       21914    21918     +4     
  Branches     7099     7505   +406     
========================================
+ Hits        21113    21117     +4     
  Misses        794      794            
  Partials        7        7            

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

@avinashbot avinashbot changed the title fix: Provide aria-label to the toolbar button navigation region fix: Fix automated accessibility findings with app layout toolbar Nov 28, 2024
@avinashbot avinashbot changed the title fix: Fix automated accessibility findings with app layout toolbar fix: Fix accessibility findings with app layout toolbar Nov 28, 2024
@avinashbot avinashbot marked this pull request as ready for review December 2, 2024 10:35
@avinashbot avinashbot requested a review from a team as a code owner December 2, 2024 10:35
@avinashbot avinashbot requested review from orangevolon and just-boris and removed request for a team December 2, 2024 10:35
just-boris
just-boris previously approved these changes Dec 2, 2024
Comment on lines 185 to 186
role={navigationOpen ? 'presentation' : 'navigation'}
aria-label={navigationOpen ? undefined : ariaLabels?.navigation}
Copy link
Member

Choose a reason for hiding this comment

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

To make this code more error-proof, it could be a separate object

const navLandmarkAttributes = navigationOpen ? {role: 'presentation'} : {role: 'navigation', 'aria-label': ariaLabels?.navigation};

@avinashbot avinashbot added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit a5be1f7 Dec 4, 2024
38 checks passed
@avinashbot avinashbot deleted the fix-app-layout-nav-label branch December 4, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants