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

Review revised Navigation Menu Button Example #384

Closed
4 tasks done
mcking65 opened this issue Apr 26, 2017 · 27 comments
Closed
4 tasks done

Review revised Navigation Menu Button Example #384

mcking65 opened this issue Apr 26, 2017 · 27 comments
Assignees
Labels
Example Page Related to a page containing an example implementation of a pattern

Comments

@mcking65
Copy link
Contributor

mcking65 commented Apr 26, 2017

The navigation menu button example has been significantly revised and needs additional task force review

Reviews requested as of April 26, 2017

@mcking65 mcking65 added Example Page Related to a page containing an example implementation of a pattern Needs Review labels Apr 26, 2017
@mcking65 mcking65 added this to the Jan 2017 Clean Up milestone Apr 26, 2017
@a11ydoer
Copy link
Contributor

a11ydoer commented May 1, 2017

@mcking65 @jongund
The last bottom link in the example, "Menu Button Design Pattern in WAI-ARIA Authoring Practices 1.1 " should be http://w3c.github.io/aria-practices/#menubutton, NOT http://w3c.github.io/#menubutton

Other that it looks good. I am so glad to see the closure of the menu button examples!

@annabbott
Copy link

annabbott commented May 1, 2017

Keyboard support > Menu button is missing the following Key and Function:
Key = Escape
Function = Closes open menu and focus remains on the Menu button
Note that the Menu button does work like this and calling out where focus remains preserves the user's point of regard.

Tested using Win10/FF 53.0

@annabbott
Copy link

annabbott commented May 1, 2017

Keyboard support > Menu > Space performs none of the specified functions:

  • Closes the menu.
    
  • Sets focus on the menu button
    
  • Activates the menu item, which is equivalent to activating the link element from which the menu item is made.
    

Keyboard support > Menu > Enter performs only one specified function of activating the menu item (link). Because the linked page is opened and replaces the page with the Menu, unable to tell if Enter closes the menu or sets focus on the menu button.

Tested using Win10/FF 53.0

@annabbott
Copy link

annabbott commented May 1, 2017

Keyboard support > Menu > A-Z and a-z does not perform as expected and there is a typo in the first bullet:

  • Moves focus to the next menu item with a lable (typo: should be label) that starts with the typed character if such an menu item exists.
  • Otherwise, focus does not move.
    

Pressing A-Z and/or z-a does move focus to the typed character, but closes the menu. Same issue as in #382

Tested using Win10/FF 53.0

mcking65 added a commit that referenced this issue May 1, 2017
For issue #384:

1. Per feedback from @a11ydoer, fixed broken link in footer nav.
2. Per feedback from @annabbott, changed spelling of "lable" to "label".
mcking65 added a commit that referenced this issue May 1, 2017
Per feedback from @annabbott in issue #384,
the optional behavior for the space bar of activating a menu item is not implemented.
Removed this key from the table of keys supported in the menu.
@mcking65
Copy link
Contributor Author

mcking65 commented May 1, 2017

@a11ydoer commented

The last bottom link in the example, "Menu Button Design Pattern in WAI-ARIA Authoring Practices 1.1 " should be http://w3c.github.io/aria-practices/#menubutton, NOT http://w3c.github.io/#menubutton

Fixed in e3e2bae.

@annabbott commented:

Keyboard support > Menu button is missing the following Key and Function:
Key = Escape
Function = Closes open menu and focus remains on the Menu button

Ann, Escape is described in the subsequent table of keys for the menu.

@annabbott commented:

Keyboard support > Menu > Space performs none of the specified functions:

  • Closes the menu.
  • Sets focus on the menu button
  • Activates the menu item, which is equivalent to activating the link element from which the menu item is made.

These are optional behaviors, so removed Space from the supported keys in the keyboard table in commit 9c51c87.

@annabbott commented:

Keyboard support > Menu > A-Z and a-z does not perform as expected and there is a typo in the first bullet:

  • Moves focus to the next menu item with a lable (typo: should be label) that starts with the typed character if such an menu item exists.
  • Otherwise, focus does not move.

Pressing A-Z and/or z-a does move focus to the typed character, but closes the menu.

Fixed the typo in e3e2bae.

I cannot reproduce the menu closing on typeahead behavior in FF 53 on Win 10.

@annabbott
Copy link

Verified target of Menu Button Design Pattern in WAI-ARIA Authoring Practices 1.1 link corrected.

@mcking65
Copy link
Contributor Author

mcking65 commented May 1, 2017

@jnurthen, @a11ydoer, @MichielBijl,

Can anyone else reproduce the behavior Ann described where typing a character when the menu open closes the menu?

@annabbott
Copy link

annabbott commented:
Keyboard support > Menu button is missing the following Key and Function:
Key = Escape
Function = Closes open menu and focus remains on the Menu button

Ann, Escape is described in the subsequent table of keys for the menu.

You're correct, Matt - THANKS!

@annabbott
Copy link

annabbott commented:
Keyboard support > Menu > Space performs none of the specified functions:
Closes the menu.
Sets focus on the menu button
Activates the menu item, which is equivalent to activating the link element from which the menu item is made.

These are optional behaviors, so removed Space from the supported keys in the keyboard table in commit 9c51c87.

Verified that Space was removed.

@annabbott
Copy link

annabbott commented:
Keyboard support > Menu > A-Z and a-z does not perform as expected and there is a typo in the first bullet:
Moves focus to the next menu item with a lable (typo: should be label) that starts with the typed character if such an menu item exists.
Otherwise, focus does not move.

Pressing A-Z and/or z-a does move focus to the typed character, but closes the menu.

Fixed the typo in e3e2bae.

Verified that typo is fixed.

@racer2207
Copy link

Hello, I am currently implementing your Navigational menu button example here and have found that the javascript throws an error when you press either the left or right arrow keys.

Uncaught TypeError: Cannot read property 'setFocusToNextItem' of undefined
    at PopupMenuLinks.setFocusToController (PopupMenuLinks.js:144)
    at MenuItemLinks.handleKeydown (MenuItemLinks.js:111)
PopupMenuLinks.setFocusToController @ PopupMenuLinks.js:144
MenuItemLinks.handleKeydown @ MenuItemLinks.js:111
PopupMenuLinks.js:140 Uncaught TypeError: Cannot read property 'setFocusToPreviousItem' of undefined
    at PopupMenuLinks.setFocusToController (PopupMenuLinks.js:140)
    at MenuItemLinks.handleKeydown (MenuItemLinks.js:105)
PopupMenuLinks.setFocusToController @ PopupMenuLinks.js:140
MenuItemLinks.handleKeydown @ MenuItemLinks.js:105

These appears to be caused by the MenuItemLinks function

MenuItemLinks.prototype.handleKeydown = function(event) {
....
   case this.keyCode.LEFT:
      this.menu.setFocusToController('previous');
      this.menu.close(true);
      flag = true;
      break;

    case this.keyCode.RIGHT:
      this.menu.setFocusToController('next');
      this.menu.close(true);
      flag = true;
      break;
....
}

Since this is a menu button and not a menubar, this.controller.menubar is undefined in the PopupMenu object. If I come up with a viable solution, I will let you know.

@jongund
Copy link
Contributor

jongund commented May 2, 2017

I will remove the LEFT and RIGHT conditions since they are not needed in this example, since it is a vertical menu.

@jongund
Copy link
Contributor

jongund commented May 2, 2017

I just made pull request that fixed the left and right arrow issues and fixed the spelling of "label"

mcking65 pushed a commit that referenced this issue May 3, 2017
…abel" (pull #392)

1. To fix bug raised by @racer2207 in issue #384, removed event support for LEFT and RIGHT arrow keys.

2. Changed "lable" to "label" in keyboard table.
@racer2207
Copy link

racer2207 commented May 4, 2017

@jongund I tested the changes you made and the errors have stopped being thrown.

Reading the Keyboard Support section, I think the Menu Button table should read,
"Opens menu and moves focus to first menuitem**.**" instead of "Open menu and move focus to first menuitem" The same for the Up Arrow function text. This would match the Popup Menu function description's grammar.

Also, the second bullet for the Enter key function description is missing the period punctuation mark.

Thanks for the effort.

@DavidMacDonald
Copy link

DavidMacDonald commented May 17, 2017

You can prevent the page from jiggling as the user tabs by using outline rather than border for the focus ring

@ZoeBijl
Copy link
Contributor

ZoeBijl commented May 18, 2017

During a conversation with @Heydon a couple weeks ago we discussed menu buttons as they were working on one for their Inclusive Components project. One question that came up during the conversation was “Isn’t it unintuitive that the links in the menu are announced as menu items?” There’s something to be said for this as the user isn’t informed about what the menu item will do, will it open a new page? Or will it perform an action on the current page? Or will it print out a thousand copies of the current page?

Would it make sense to announce them as links? Or perhaps as menu item links (new role, I know…)? We have menuitemradio and menuitemcheckbox, so it would make sense. The flip side to exposing them as links (not menuitemlink) is that it might not be clear that the user is indeed in a menu.

I’m not sure if this is indeed an issue, but the community is asking questions :) Perhaps you can shed some light on this @mcking65?

Related Twitter conversation with Jared Smith.

@jared-w-smith
Copy link

@MichielBijl Thank you for addressing this. Another concern/question is the example's implementation of Up arrow key to open the menu and move to the last item. This interaction is not defined in the ARIA best practices document. This disconnect could cause author confusion as to whether this is required or not.

@Heydon
Copy link

Heydon commented May 18, 2017

@MichielBijl Thanks for opening this up. I actually talked to @stevefaulkner about the possibility of a menuitemlink role. It seems to me that, for keyboard behavior to work as designed, menu items should not contain child interactive elements. In fact, that's non-conforming (can't find the spec I read recently to cite this, though shakes fist).

With this role, we can expect the announcement of "menuitem link" as one role, needing only one element. What do you and @jared-w-smith think?

@jared-w-smith
Copy link

@Heydon You may be referring to item #4 at http://w3c.github.io/aria-practices/#h-note15 or the final column at https://w3c.github.io/html-aria/#index-aria-menuitem

In general, I think the W3C examples should model minimally viable mechanisms that follow the specification. It's easy to misinterpret additional enhancements as being required.

@mcking65
Copy link
Contributor Author

MichielBijl commented:

During a conversation with @Heydon a couple weeks ago we discussed menu buttons as they were working on one for their Inclusive Components project.
One question that came up during the conversation was “Isn’t it unintuitive that the links in the menu are announced as menu items?”
There’s something to be said for this as the user isn’t informed about what the menu item will do, will it open a new page? Or will it perform an action on the current page?
Or will it print out a thousand copies of the current page?

Would it make sense to announce them as links? Or perhaps as menu item links (new role, I know…)?

We have menuitemradio and menuitemcheckbox, so it would make sense.
The flip side to exposing them as links (not menuitemlink) is that it might not be clear that the user is indeed in a menu.

I’m not sure if this is indeed an issue, but the community is asking questions :) Perhaps you can shed some light on this @mcking65?

I am doubtful this is an issue.

My inclination: We need to be careful not to try to solve UI design problems with accessibility attributes. If the combination of the name of the menu and the name of the menuitem do not communicate the purpose of the menuitem, then the UI has serious deficiencies for all users.

There is a completely different reason for menuitemcheckbox and menuitemradio: they have interactions and states that need to be communicated with ARIA attributes. The interaction model for a menuitem that prints the page is the same as the model for a menuitem that displays new content.

@jnurthen
Copy link
Member

+1 to @mcking65
How does a visual user know what these menu items are going to do? Why would we be seeking to provide more information to a screen reader user than a visual user gets.

mcking65 added a commit that referenced this issue May 19, 2017
Modified aria-practices.html per feedback from @jared-w-smith in issue #384 to add optional up arrow key behavior for menubutton:
(Optional) <kbd>Up Arrow</kbd>: opens the menu and moves focus to the last menu item.
@mcking65
Copy link
Contributor Author

@Heydon commented:

It seems to me that, for keyboard behavior to work as designed, menu items should not contain child interactive elements.
In fact, that's non-conforming (can't find the spec I read recently to cite this, though shakes fist).

That is basically correct; since a menuitem is a widget and not a subclass of composite, it cannot contain interactive elements.
As far as I am aware, the spec does not concisely state this in one place; it is one of those facts you have to assemble from all the various pieces.
And, that is part of why it is important to have a document like the Authoring Practices; it is not feasible for the spec to explicitly state every implication of its implementation.

This is part of the reason that the menuitem role is on the <a> element rather than the <li> element in this example.

One of the side effects of this is that screen readers announce only the widget name on focus.
They do not "dig inside" the focused widget element to see if there are any semantic elements.
In effect, screen readers treat non-composite widgets as if they have children presentational true.
Most non-composites do have children presentational true, but menuitem does not.

There has ben a lot of argument over whether this is ideal. That is, there have been debates over whether menuitem and treeitem should be composite or non-composite, argument over whether all non-composites should have children presentational true, and discussion of how screen readers should parse and render the accessibility tree for non-composite widget elements.
ARIA 1.1 is slightly more clear than ARIA 1.0, but there is definitely room for improvement in this space in the future.

@mcking65
Copy link
Contributor Author

@jared-w-smith commented:

Another concern/question is the example's implementation of Up arrow key to open the menu and move to the last item.
This interaction is not defined in the ARIA best practices document.

Jared, thank you for this catch! I fixed it in the
editor's draft of the menu button pattern
with commit 343c691.

@jared-w-smith commented:

In general, I think the W3C examples should model minimally viable mechanisms that follow the specification. It's easy to misinterpret additional enhancements as being required.

In the patterns, we flag features that that the task force agrees are optional.
In the examples, we often demonstrate optional features, especially if they are high value.
Our thinking is that including the high value options in implementations is best practice and demonstrating best practice will generally be the most informative and helpful approach.
Doing so also helps provide a richer set of test conditions.

If this approach is causing meaningful difficulties, we certainly want to address it.

One other thing that is important to note: the Authoring Practices are not a specification.
They are a "W3C Note".
That is, the Authoring Practices are considered informative, not normative.
This is why you do not see the words "MUST", "SHOULD", or "MAY" and their normative variants.
We similarly work to avoid words like "need" and "required" to the extent reasonable.

Nonetheless, it is intended that the practices are authoratative as a result of being based on consensus, expert interpretation of the ARIA specification. So, confusion of the kind you are describing is something we want to minimize.

@mcking65
Copy link
Contributor Author

Link to CSS file is broken.

@mcking65
Copy link
Contributor Author

mcking65 commented May 22, 2017

@DavidMacDonald, were you seeing the jiggling issue in the menu button examples? The examples do use border, but using border does not seem to have any negative effects in this case.

@DavidMacDonald
Copy link

DavidMacDonald commented May 22, 2017

it was giggling for me as I tabbed ... Chrome a few weeks ago, I recommend the use of outline which won't break author's visual UI.
border takes up UI space, so when focus lands on an element, it adds that to the UI, if its 1px, the border on the focus indicator UI will shift 1px.

mcking65 added a commit that referenced this issue Jun 19, 2017
… grammar

Made the following changes to examples/menu-button/menu-button-links.html for issue #384.

1. Made grammar change in keyboard table as suggested by @racer2207.
2. Fixed link to css.
mcking65 added a commit that referenced this issue Jun 19, 2017
…orce review process

Removed link to issue #384 from examples/menu-button/menu-button-links.html.
The internal task force review is complete for this example.
@mcking65
Copy link
Contributor Author

All issues except for the focus ring, which will be tracked by issue 402, have been addressed. Closing this review issue.

mcking65 added a commit that referenced this issue Jun 19, 2017
…key documentation

To resolve feedback in issue #382 about examples/menu-button/menu-button-actions.html,
made the following changes.

1. Per feedback from @annabbott, Removed documentation of space key behavior. It is optional. It can be restored if appropriate after issue #403 is resolved.
2. Adjusted grammar in keyboard table to be consistent with similar changes made for issue #384 in commit c4d70e8.
3. Per feedback from @annabbott, Fixed href of pattern link in footer.
mcking65 added a commit that referenced this issue Jun 19, 2017
… remove space key documentation

To resolve feedback in issue #383 about examples/menu-button/menu-button-actions-active-descendant.html,
made the following changes.

1. Per feedback from @annabbott, Removed documentation of space key behavior. It is optional. It can be restored if appropriate after issue #403 is resolved.
2. Adjusted grammar in keyboard table to be consistent with similar changes made for issue #384 in commit c4d70e8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
Development

No branches or pull requests

10 participants