-
Notifications
You must be signed in to change notification settings - Fork 355
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 Menubar Example: Improve high contrast support and refactor javascript #1359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps one issue - the down arrow graphics don't seem to shift color and I'm not sure it'd be high contrast enough. And one note - there is no background to the menubar with HCM enabled and focus outline only appears once a menu item is focused w/ the keyboard. I'm thinking that the fact there's no background is as designed though.
Approved pending similar changes to #1356.
Thanks @jongund!
Windows 10.0.17763 Build 17763; Firefox 75, Chrome 81.0.4044.113, Edge 81.0.416.58
Can you re-review the navigation menubar example for high contrast, I added a 1 pixel border to the menubar for this example too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @jongund, thank you!
@carmacleod @zcorpan @mcking65 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 4 commits to address some editorial issues. All copy is good now.
…-practices into issue1357-navigation-menubar-js
@carmacleod |
<li>To support operating system high contrast settings: | ||
<ul> | ||
<li>Focus is highlighted by adding and removing a border around the menu item with focus.</li> | ||
<li>The arrow icons used to indicate the current state of pull expandable menus use inline SVG images.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should also mention currentColor? It's an important part of the support.
(also, probably just say "expandable" and not "pull expandable")
<li>The arrow icons used to indicate the current state of pull expandable menus use inline SVG images.</li> | |
<li>The arrow icons used to indicate the current state of expandable menus use inline SVG images with CSS currentColor.</li> |
Looks great, @jongund ! The triangle icons use the bright yellow link color - very easy to see. I guess currentColor is link color because the menuitems are links. Here's a screen snap of Navigation Menubar Example in Edge High Contrast Black: And here's a screen snap of Navigation Menubar Example in Edge High Contrast White: Now just need to fix Editor Menubar Example the same way, but that can be done in a separate PR. |
@carmacleod |
@jongund Thanks for updating the Accessibility Features documentation. Please see my suggestion re talking about currentColor. |
@carmacleod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jongund Sorry to take so long to finish this up. Thanks for your patience.
I wanted to finish the screen reader testing in case anything in the example needed changing.
I didn't find anything that would hold up merging this. Details below.
I did find a small CSS issue in the Navigation menubar example that doesn't need to hold up merging. Let me know if you'd like me to open a new issue. Steps:
- Focus the link right before the menubar (i.e. the Example Disclosure for Navigation Menus link. This will ensure that you can see the menubar, but it does not have focus)
- Hover the mouse over the About menuitem or the Admissions menuitem, and then move the mouse out again. Repeat. Notice that there's a 1 or 2 pixel left-right "jiggle" as the hover style is drawn and removed.
- Now type tab so that focus moves into the menubar. Shift+tab back to the link. Repeat. Notice the same jiggle as the focus style is drawn and removed.
The jiggle doesn't happen in the Editor menubar except in Firefox on mouse hover only (not focus).
Screen reader testing results
Tested both menubar examples in the following pairings:
-
NVDA in Chrome, Firefox and Edge
- works very well in all 3 browsers
- impressed how well NVDA works in Edge (anybody know if it is using UIA?)
-
JAWS in Chrome and Firefox
- works ok, but doesn't say if a menuitem has a submenu, doesn't read group names
-
JAWS in Edge
- does not work well: doesn't say if a menuitem has a submenu, forms mode issues in Editor menubar
-
Narrator in Edge - works great
-
VoiceOver in Safari and Chrome
- prepends the number 1 to checked menuitems in Editor menubar (I will open a radar for this)
- doesn't always read the first menu item after opening
- doesn't always say item count (and if it does, count may be wrong, and/or may include separators)
- doesn't always say group name when entering group
- doesn't always say if a menuitem has a submenu (Safari only)
Other notes:
- NVDA and Narrator distinguish between checkbox and radio menuitems, but JAWS and VO don't
- Firefox uses separator as a grouping divider (which affects counts), Edge and Chrome only group if there's a group
Both of these examples are working really well! Thanks so much, @jongund !
@jongund Just realized that the 2 links in aria-practices.html Menubar pattern (lines 1938 and 1939 in the code) need to be updated. |
@jongund Here's some code that works (this can probably be written more concisely... I'm not very good at css). ... starting line 113 in menubar-navigation.css border-top: 4px solid #034575;
border-bottom: 4px solid #034575;
border-left: 3px solid #034575;
border-right: 3px solid #034575; |
@carmacleod |
@jongund thank you for all your work on these menubar examples ... awesome improvements!! And thank you to everyone else who has contributed with reviews and testing. Fabulous teamwork! Here goes the merge! |
Navigation Menubar Example: Improve high contrast support and refactor javascript (pull #1359) closes issue #1357 by making the following changes: * Updated javascript to use a single object. * Updated CSS and JS to improve high contrast support. Co-authored-by: Matt King <[email protected]>
Yes!! It's fixed beautifully! I tested the merged Navigation Menubar Example in Chrome, Edge, and Firefox. Thanks again so much @jongund for all of your work on this! |
closes #1357
Preview Link
View navigation menubar in the compare branch for this PR
Review Checklist
Preview | Diff