Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Remove aria-hidden from scrollable navigation #2875

Merged
merged 10 commits into from
Jan 8, 2020

Conversation

DenisHdz
Copy link
Contributor

@DenisHdz DenisHdz commented Jan 7, 2020

Overall change:
Apple has released a new Accessibility feature with macOS Catalina and iOS 13, called Voice Control, that allows you to control your device entirely just with your voice.

Unfortunately, when applying aria-hidden: true to an element, this can't be reached using Voice Control speaking an action, such as "Tap Home".

For this reason, we should remove the aria-hidden from the navigation and also the tab-index:-1 from the links as everything will be visible now to the users.

Code changes:

  • Do not apply aria-hidden:true to the scrollable navigation
  • Do not apply tab-index:-1 to links
  • Merge CanonicalScrollableNavigation and AmpScrollableNavigation into ScrollableNavigation
  • Remove unnecessary tests
  • Remove isScrollable prop
  • Remove hook from stories
  • Remove README references to aria-hidden

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@DenisHdz DenisHdz added the ws-home Tasks for the WS Home Team label Jan 7, 2020
@DenisHdz DenisHdz self-assigned this Jan 7, 2020
@DenisHdz DenisHdz marked this pull request as ready for review January 7, 2020 15:15
</StyledScrollableNav>
);
};
export const CanonicalScrollableNavigation = ({ children, dir }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now exactly the same as the Amp scrollable nav right? And we probably need the spread props for canonical too for event tracking? Should we just merge them?

Copy link
Contributor

@OlgaLyubin OlgaLyubin left a comment

Choose a reason for hiding this comment

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

"When to use this component" and "Amp Menu Button Usage" sections in the Readme still have mentions of the AmpScrollableNavigation

@tochwill
Copy link
Contributor

tochwill commented Jan 7, 2020

Thanks Denis!

Copy link
Contributor

@AlistairGempf AlistairGempf left a comment

Choose a reason for hiding this comment

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

Looks good to me

@PriyaKR PriyaKR self-assigned this Jan 8, 2020
@DenisHdz DenisHdz changed the title Remove aria-hidden from Navigation Remove aria-hidden from scrollable navigation Jan 8, 2020
@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 8, 2020

Looks good to me.Tested the canonical nav with voice over and voice control.

@amywalkerdev amywalkerdev merged commit f6a338a into latest Jan 8, 2020
@amywalkerdev amywalkerdev deleted the navigation-a11y-tweaks branch January 8, 2020 19:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants