Menubar examples: Make mouse behaviors and parent menu item appearanc…

[carmacleod]

…e change triggers more consistent with desktop conventions #408

This commit fixes the menubar-2 example.

It ensures the following behavior:

• open menus stay open on mouseout
• remove timeouts that close menu on mouseout
• close menu when user:
  • clicks outside menu
  • clicks again on menubar item
  • clicks on menu item in menu
  • types tab or shift+tab
  • types space or enter on menu item (but not for space on radio/checkbox)
  • hovers over another menubar item (other menu opens)
• menubar item retains its activated appearance while its menu is open

Details of the code changes:

• deleted hasFocus and hasHover state booleans because they are no longer needed
• added popupMenu.isOpen() which checks for aria-expanded=true on the corresponding menubar item's DOM element
• added ESC key handling on menubar items (closes menu if open)
• clicking on menubar item opens menu, clicking again on same menubar item closes it
• make sure browser doesn't scroll when clicking on menubar item (default behavior for clicking on link)
• removed focus, blur, and mouseout event handling and timeouts from menubar items
• removed focus, blur, mouseover, and mouseout event handling and timeouts from popup menu items
• added focusout on menubar item and popup menu item to close menu if focus will be going outside of the menubar
• if a menu is open, then hovering over a different menubar item opens its menu instead
• removed check for anchor element children of menubar items (was probably an artefact from a previous implementation)
• removed mouseover and mouseout events from popup menu
• removed 'force' parameter from popup menu close() function
• deleted some vars that were declared but not used, or were declared twice
• fixed a few comments that didn't quite match the code
• declared some vars as local that were declared as global (i.e. without var keyword) but did not need to be
• removed some redundant checks, i.e. while (e) { if (e ...) }
• changed != to !== and == to === where appropriate
• removed css for hovering over menubar item (not needed because hovering focuses menubar item)

[mcking65]

@annabbott, @sh0ji, @shirsha, @jongund,

You can view/test the result of these changes in the RawGit view of the editor menubar example page in Carolyn's repo.

For your convenience, here is the current published version of the editor menubar on w3/tr.

@carmacleod, thank you very, very much for this contribution. It's a big change. So, I am having several people look closely before commiting to master.

With multiple reviewers, there could be some churn in the comments. Please do not make changes unless we have clear agreement that a change is needed.

[mcking65]

1. Windows Firefox/JAWS:
   • All kb commands work as described
   • All roles, states, and properties being set/changed as described.
2. Windows Chrome/JAWS:
   • All kb commands work as described
   • All roles, states, and properties being set/changed as described.

Note: above tests are of the example code, not the screen reader.

[jongund]

I tested with VoiceOver with Safari and Firefox on OSX and it works

I tested with NVDA with Firefox and Chrome on Windows 10 and it works.

The only question I have is when you select a regular menuitem when a screen reader is running, the popup menu closes and moves focus back to the menubar menuitem. The popup menu does not close when testing keyboard support without a screen reader, the popup menu just stays open.

The updated mouse behaviors are more inline with OS menu mouse behaviors.

[annabbott]

Tested with Win10/FF ESR 52.6.0/JAWS 2018 (build 1712.10 ILM)
All good!

[annabbott]

Tested with Win10/FF ESR 52.6.0/kbd only (no screen reader)
All good!

[annabbott]

Tested with Win10/Chrome Version 64.0.3282.140 (Official Build) (64-bit)/JAWS 2018 (build 1712.10 ILM)
Menubar: expected result - number of menuitems on menubar is announced (x of x) as navigated with arrow keys.
Menubar: actual result - number of menuitems on menubar is NOT announced correctly as navigated with arrow keys (always announces 'one of one' for each of four menuitems.

This is specific to Chrome. It is announced correctly on FF.

All good!

[annabbott]

Tested with Win10/Chrome Version 64.0.3282.140 (Official Build) (64-bit)/kbd only (no screen reader)
All good!

Note: I was unable to replicate Jon's kbd-only test results.

[annabbott]

My testing is complete.

[sh0ji]

Great stuff! Lots of good changes in this PR.

Verified that it works as expected in Chrome and Firefox with NVDA on Windows 10. I'll test on macOS tomorrow, but I don't think that needs to hold up this review.

My only issue is that hovering over a menuitem element on the menubar now also focuses the menuitem element. I've never encountered a widget where hovering causes a change of focus. My expectation is that hovering might cause some visual change to let the user know that the item is interactive, but that any change of context should be initiated by the user more deliberately (click or tab to focus). This is kind of similar to WCAG SC 3.2.1, except hovering is causing the change of context.

I wasn't able to replicate the keyboard issue described by @jongund.

[sh0ji]

As a follow-up, I'd actually argue that the screen reader experience is better than the keyboard or mouse experience on this one now, though not because of changes made in this PR. We may want to review the visual design for that reason. Specifically:

• There is no visual indicator that the submenu items are a checkbox or radio. You just have to test it or guess to know whether clicking one will de-select another.
• Similarly, sub-groups ([role="group"] elements) have a label that makes it clear how the items are related, but that label is inaccessible to everyone except screen reader users. It would be useful for everyone.

[carmacleod]

Thank-you all so much for your comments and testing!

I have a few replies:

1. The issue that @jongund mentioned was there originally, and I agree that it is odd. I believe that the intent was for the menu to stay open if the user selects a radio or checkbox menu item using the space key, and to close if the user selects a push button menu item with space, or any menu item with enter. This is listed as "Optional" behavior in the APG design pattern for menu. It works the same whether the screen reader is running or not.
   To test, use space to select any item in the first 3 menus (these are all radio or checkbox menu items), and then use space to select "Smaller" or "Larger" in the "Size" menu (these are the only push button menu items in this example). Please let me know if you would like this behavior kept or changed.

2. I opened a Chromium bug for the "menu 1 of 1" issue that @annabbott mentioned. It would be great for it to have a few more "Stars" to hopefully raise the priority a bit. :)

3. I agree with @sh0ji that simply hovering over a menubar item - when no menus are already open -
   should not focus the item. I will fix that. Do you prefer that I open a new issue for this, or add another commit to this PR?

4. Good point about no visual indicator when submenu items are checkbox or radio, @sh0ji . I looked at the old Windows Notepad menu, and it has the same behavior, i.e. the user cannot tell, other than perhaps by context, that a menu item is a checkbox. For example, View -> Status Bar, or Format -> Word Wrap. I'm not sure what the best solution to this problem is; does Mac have anything nice? One possibility is to use a red X (or an empty box) when the checkbox is not selected, and a green checkmark (or green checkmark in a box) when it is selected. It would be nice to distinguish between radio and checkbox as well, so, for example, an empty circle (unselected) or blue dot within circle (selected) for radio.

5. Good point also about the grouping labels, however I think we need to be careful not to get too verbose in both sighted and screen reader menus. For example, very few menus in the past ever had group labels. If there truly was a need for a group label, then one might argue that the group would be better implemented as a submenu. In this example, every separator is followed by a group label. If you take a look at the menus in Notepad on Windows (I'm on Windows 7 - I don't know if the Windows 10 Notepad is the same) you can see that there are several separators, but no group labels. For example, the Edit menu has Undo, separator, Cut, Copy, Paste, Delete, separator, Find..., Find Next, Replace..., Go To..., separator, Select All, Time/Date. If we said that separators delineate groups, and groups need labels, then it would be very difficult to come up with a good label for each of those groups, and it would add unnecessary verbosity to the Edit menu.

[mcking65]

@carmacleod, thank you for opening the chromium bug!!

@carmacleod commented:

I agree with @sh0ji that simply hovering over a menubar item - when no menus are already open - should not focus the item. I will fix that. Do you prefer that I open a new issue for this, or add another commit to this PR?

We would like a commit added to this pull request for this focus on hover problem before merging.

Good point about no visual indicator when submenu items are checkbox or radio, @sh0ji.

This would be a separate issue and PR. I opened issue #605.

Good point also about the grouping labels, however I think we need to be careful not to get too verbose in both sighted and screen reader menus. For example, very few menus in the past ever had group labels. If there truly was a need for a group label, then one might argue that the group would be better implemented as a submenu. In this example, every separator is followed by a group label. If you take a look at the menus in Notepad on Windows (I'm on Windows 7 - I don't know if the Windows 10 Notepad is the same) you can see that there are several separators, but no group labels. For example, the Edit menu has Undo, separator, Cut, Copy, Paste, Delete, separator, Find..., Find Next, Replace..., Go To..., separator, Select All, Time/Date. If we said that separators delineate groups, and groups need labels, then it would be very difficult to come up with a good label for each of those groups, and it would add unnecessary verbosity to the Edit menu.

I agree. But, in this particular case, the group labels are particularly useful because they provide a screen reader analog to the visual separation. Remember that separators are not announced. That is generally fine since most of the time separators are there to help with visual scanning as opposed to communicating a specific intent. Here, labels like "none" aren't particularly good unless they are accompanied by the group label. And, btw, NVDA is the only screen reader that reads the group labels when they should be read.

I am guessing that sighted users can easily dicern "none" in the text decoration group means "no text decoration" because of its proximity. If that is not the case, then maybe that label should be changed to "No decoration". On the other hand, the visual separation of colors is surely adequate and there is no need for a visual label of "text color". I didn't open a new issue for this aspect of the visual design; I am not confident there is an issue.

I did note that we didn't document the use of aria-label on the groups so I created issue #606, which I can fix later after we merge.

[mcking65]

One issue to fix before merging raised by @sh0ji: hover should not move focus.

[sh0ji]

I agree with @sh0ji that simply hovering over a menubar item - when no menus are already open -
should not focus the item. I will fix that. Do you prefer that I open a new issue for this, or add another commit to this PR? (@carmacleod)

One issue to fix before merging raised by @sh0ji: hover should not move focus. (@mcking65)

I'm generally okay with the current implementation, which I'll summarize: when not active, hover either does nothing or provides visual feedback (it currently does this by inverting colors and changing to a pointer cursor), but it does not focus anything until the user activates the menubar through either a focus or click action. Once the user activates the menubar, focus follows the hovered item.

This is similar to how operating system menus work--macOS actually does nothing on hover until the user activates the menubar, but otherwise works identically to the current example.

The pattern of a hover event triggering a focus event is nearly unheard of on the web, though. And I argue that it's similar to SC 3.2.1 in WCAG, but with the hover event replacing the focus event: "When any component [is hovered], it does not initiate a change of context." That rule exists because unexpected changes of context can be disorienting for users. It doesn't include hover, though, which is why I would be okay with the current implementation. Feedback from others would be useful here. I know @jongund may have some thoughts about this.

[sh0ji]

@mcking65 commented:

On the other hand, the visual separation of colors is surely adequate and there is no need for a visual label of "text color".

While I agree for our specific case, I don't want to assume this for the general case. There's a cognitive load associated with looking at a grouping of items and determining how they're related. That cognitive load can be mitigated by having an explicit label for the group, regardless of how the user experiences it. On a similar note, if we're okay with the cognitive load required to understand the grouping for sighted users, does it need a label at all?

I don't have answers for this. I just wanted to point out that the screen reader user has access to explicit information for a couple items that are only ever implicit for sighted users.

[carmacleod]

Good news: the chromium "menu 1 of 1" bug is fixed, and will be in Chrome 67.

[carmacleod]

This latest commit does not change any behavior; I only changed css. (and a comment)
Please give it a try. (I'm hoping you are all at CSUN and you can try it together :)

Please don't worry about focus following hover inside an open menu, because when the menu closes, focus goes back to the parent menuitem anyhow. So there's no possibility of loss of context, because the user never left the context of the menubar.

Note again that focus does not follow hover unless the menubar or one of its menus already has focus (which can only happen after the user explicitly clicks on or tabs into the menubar).

We all agree that this is how application menus work on the desktop, and as @katiehockman pointed out in #598, this is also how Google Docs chose to implement their web application menus.

[carmacleod]

Latest fixes now available on rawgit. Please try and let me know.
https://rawgit.com/carmacleod/aria-practices/issue408/examples/menubar/menubar-2/menubar-2.html

[carmacleod]

I don't think I'm explaining this very well, somehow. :)
I think this code is ok and could go in. :)

Please try it again, and do the following test to reassure yourself that all is well:

1. Go to the rawgit application menubar example page.
2. Hover the mouse over one of the menubar items (don't click!)
3. Now type Tab. Focus moves to the first link at the top of the page, which is exactly where you would expect it to go. There is no "focus on hover" problem.
4. Now click the mouse on a menubar item. Focus moves to the menubar item and opens its menu, as expected. If you type any combination of up, down, left, right arrow keys, you will navigate cleanly through the menubar and its menus and their menuitems exactly as specified.
5. Now type Tab. Focus moves to the textarea, which is exactly how any single-tab-stop control works.
6. Now, again with the mouse, click on a menubar item to open its menu. This time, move the mouse down so that it hovers over any of the items in the open menu. Now use the up or down arrows to continue moving through items from where the mouse left off.

It is this last point that has some of you worried because the implementation at that point does involve moving focus on hover. However, I will argue that this implementation detail results in a superior user experience for the following reasons:

1. It creates a seamless experience for users who switch from mouse to keyboard and back again.
2. It creates a consistent experience for sighted screen reader users, because without the focus change on hover, the screen reader remains silent when the mouse is moving through the items, even though they are being highlighted with css.
3. It is harmless, because there is no loss of context. The menubar already has the focus, and all focus changes are contained within the menubar and its children (i.e. focus is not being blasted to some random element on hover - it's just moving within already-focused children). In fact, the focus changes help to maintain context.
4. It is the same behavior as desktop menubars.
5. It is the same behavior as Google docs web app menubars.

[sh0ji]

@carmacleod This looks great, and I'm in agreement about the "move focus on hover after entering the menubar." Thanks for all your patience on this!

@mcking65 This can be merged as far as I'm concerned.