Accessibility Fork - Githubissues

adobe-accessibility / Accessible-Mega-Menu

A demonstration of how to implement a keyboard and screen reader accessible mega menu as a jQuery plugin.

Apache License 2.0

605 stars 199 forks source link

Accessibility Fork #66

Closed maedi closed 3 years ago

maedi commented 4 years ago

A slew of accessibility improvements and bug fixes in one easy to merge pull request.

Handle enter event on open top level link as escape event. #61
Move aria-expanded from panel to top level link #62
On enter activate top level link if it has no panel #63
Fix mistargeting of topnav link when no panel #65

All of these improvements can be tested here: https://www.ag.gov.au/

m80her commented 4 years ago

works for me - nice job @maedi :)

m80her commented 4 years ago

works for me - nice job @maedi :)

@maedi - just noticed a slight quirk when using the up arrow key to close a subnav - the previous top level item (to the left with a subnav) receives the 'open' class and it's subnav is revealed.

maedi commented 4 years ago

I'm not sure I can replicate your issue @m80her, are you able to attach a screenshot?

If you're talking about how pressing the Up/Down key will go to the previous/next top level link and open its subnav, then I think this is desired behaviour. If you want to close the subnav press Escape, or Enter when the top level link is in focus.

The Up/Down keys are basically mimicking Tab/Shift + Tab, cycling through every focusable element. This seems like good behaviour to me.

m80her commented 4 years ago

@maedi makes sense - check out my live implementation and see if it's working as expected for you. Also I've noticed aria-hidden not changing to false when sub-nav is open - still investigating https://cadentgas.com/

maedi commented 4 years ago

@m80her It's correctly toggling [aria-expanded] on the top level link and correctly toggling [aria-hidden] on its sub nav for me. That being said siblings() works for me too. So if your way works for both of us let's use that.

You mentioned changing filter() too but I don't think I have enough to go on. If something else needs to be changed let me know.

maedi commented 4 years ago

Have removed aria-pressed attribute from the mobile menu toggle button. This change is recommended by a recent accessibility audit that says:

The main menu button on the mobile website has both the aria-expanded and aria-pressed attributes. The aria-pressed attribute should not be used for an accordion button. Screen readers will sometimes state "on" or "off" when expanding/collapsing the button, which is less useful than hearing "expanded" or "collapsed".

Would really appreciate if someone could take a look at this pull request. Many hours have been spent testing and improving the megamenu to meet modern accessibility requirements.

It is used in production on a few real world sites:

https://www.ag.gov.au/
https://www.acecqa.gov.au/
https://cadentgas.com/ (nice job @m80her)

With more sites in the pipeline. Well and truly battle tested.

maedi commented 3 years ago

Thanks very much for merging @majornista and thank you for fixing the window.cvox issue in https://github.com/adobe-accessibility/Accessible-Mega-Menu/commit/5c070bc9011f125ae337cbb952749b44e0f86d54