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

Menubar Navigation Example: Make single page and improve implementation of APG coding practices (try 2) #1741

Merged
merged 19 commits into from
Feb 9, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Jan 28, 2021

Updated pull request fix merge problems with pull #1612:

Preview Link

Changes:

  • Added landmark regions to mimic a website look and feel
  • When links selected the content area is updated and the focus moves to the H1 element, previous version went to a dummy link
  • Updated code to use coding practices

Review checklist

@jongund
Copy link
Contributor Author

jongund commented Jan 28, 2021

@mcking65
This is the new pull request for the update to the menubar navigation example. Something happened to pull 1612, and my attempts to fix it only made more conflicts. Please make your editorial changes to this branch as soon as possible before other changes in master cause problems for this pull request.

@jongund jongund requested a review from mcking65 January 28, 2021 23:12
@mcking65 mcking65 changed the title attempt to fix merge issues with update-menubar-nav pull request Menubar Navigation Example: Make single page and improve implementation of APG coding practices (try 2) Feb 2, 2021
@jongund
Copy link
Contributor Author

jongund commented Feb 3, 2021

@mcking65
I removed aria-owns from the example.
I also updated the border documentation to say from 0 to 2 pixels for the border, since that is what this example does.
We do not use a default 1 pixel border, but maybe we should not sure. What do you think?

@mcking65
Copy link
Contributor

mcking65 commented Feb 3, 2021

@jongund, thank you for removing the aria-owns. I have no comment on the 1 pixel border. That is related to visual design and accessibility reviews. So, will leave that up to @a11ydoer and @shirsha. Of course, I lean toward minimizing unnecessary changes.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

This is an approving editorial review! Thank you @jongund! This is a really big step forward.

@carmacleod
Copy link
Contributor

@mcking65 I love the Caution box! Quick question, though - it talks about menu navigation, but the rest of the example page talks about menubar navigation. Should we change menu to menubar in the Cautionary note?

Also, the first bullet in the note had so many words that I had to read it twice to parse what it was saying. Does it make sense to split it up with a comma followed by a noun phrase, as follows?

Correct implementation of the menubar role requires implementation of complex functionality that is not needed for typical site navigation, even when that site navigation is styled to look like a menubar with expandable sections or fly outs.

@jongund
Copy link
Contributor Author

jongund commented Feb 3, 2021

@mcking65
I updated the border styling to use the 1 pixel by default and the 3 pixel when it has focus to be more like other examples and for the target area for a menu item to be outlined in high contrast mode for people with low vision. I updated the documentation to reflect the changes.

@jongund
Copy link
Contributor Author

jongund commented Feb 3, 2021

@mcking65
+1 on Carolyn's comment on changiing "menu" to "menubar".

Suggested edit:
The complexity of the menubar pattern is not needed for typical site navigation that is styled to look like a menu with expandable sections or "fly outs".

@carmacleod
Copy link
Contributor

@mcking65 I like Jon's edit to the first bullet in the cautionary note. (fewer words - yay!)
@jongund I would only make one more change, which is to use "menubar" for the 2nd instance of "menu" as well, i.e.

The complexity of the menubar pattern is not needed for typical site navigation that is styled to look like a menubar with expandable sections or "fly outs".

@jongund
Copy link
Contributor Author

jongund commented Feb 3, 2021

I suggest adding an additional statement to Carolyn's edit:
The complexity of the menubar pattern is not needed for typical site navigation that is styled to look like a menubar with expandable sections or "fly outs". It is important to note, especially in the case of site navigation, that ARIA roles indicate the behavior used to navigate the site links, not how they visually appear.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

+1
This is an approving functional review!
Nice work, @jongund !

@shirsha
Copy link

shirsha commented Feb 4, 2021

  1. Visible focus indicator is confusing, especially when the user tabs to the menubar that has a selected menuitem.
  2. Focus order question
    Steps followed:
  • User navigates to the Admissions - Tuition- undergraduate.

  • Select the Undergraduate menuitem

  • Navigate to the Academics using arrow key and tab out of the menu bar.

  • The tabindex="0" is placed on the selected submenu items i.e. Tutition , Undergraduate. Where as the "Admissions" have a tabindex="-1". The "Academics" menuitem has a tabindex="0"

  • Therefore when the user navigates to the menubar (using shift+tab) the "Academics" will receive the focus instead of "Admissions". Is this the intended way for navigation

Attached the screenshots showing 1 and 2 scenarios:
Visual focus indicator.docx

@carmacleod
Copy link
Contributor

@shirsha

  1. The focus indicator is the border box. The underline plus background color change are intended to show which page is currently selected, because we were trying to visually show the aria-current page. However, I think you are right that it is confusing. I think it might be clearer if the focus indicator stood out more than the "current page" indicator.
    @jongund How about switching the background color change to :focus instead of having it on the current page indicator, and making the underline a bit thinner? Something like the following screen snapshot. Siri, would that be clearer? Visually showing the current page is a new concept that we are trying to get right.
    image

  2. If we can make it clearer (from point 1, above) that the underlined menu item is the "current page" and the box shows the focus, then the navigation sequence you described will make more sense. The tabindex="0" is always following the focus, and it has nothing to do with the current page.

@jongund
Copy link
Contributor Author

jongund commented Feb 4, 2021

@shirsha
@carmacleod
I made some updates to focus and aria-current styling, based on Carolyn's suggestions.
Please review again.
Just a reminder to clear your browser cache before trying the githack link.
NOTE: It seems githack is a little slow updating the example code too.

@jongund
Copy link
Contributor Author

jongund commented Feb 4, 2021

2. Focus order question
   Steps followed:


* User navigates to the Admissions - Tuition- undergraduate.

* Select the Undergraduate menuitem

* Navigate to the Academics using arrow key and tab out of the menu bar.

* The tabindex="0" is placed on the selected submenu items  i.e. Tutition , Undergraduate. Where as the  "Admissions" have a tabindex="-1". The "Academics" menuitem has a tabindex="0"

* Therefore when the user navigates to the menubar (using shift+tab) the "Academics" will receive the focus instead of "Admissions". Is this the intended way for navigation

@shirsha
@carmacleod
@mcking65
The default keyboard behavior for the menu pattern is to have tabindex=0 on the last top level menubar menu item the user interacted with with, even if the menu item is not in the path for the current page link. It sounds like you were expecting tabindex="0" to be associated with the path to the aria-current menu item. Interesting idea, what do other people think?

@carmacleod
Copy link
Contributor

@jongund I really like the visual changes you made - I think it makes it much clearer which item is focused vs. current. I'm interested to hear what @shirsha thinks.

Regarding your question about maybe having the current page menu item in the tab order instead of the last focused top level menubar item, I can't decide. Here are some random thoughts (in no particular order, and probably not very helpful because I didn't reach any conclusion... 😂 ):

  • we need to make sure that focused and current are not confused, and focusing the current first might create confusion?
  • although... most of the time, the current page will actually be the first one focused when you shift+tab back to the menubar (because the current item is usually the last menu item the user interacted with)
  • perhaps the current page menu item is less likely to be where the user wanted to go next, because they just went there?
  • navigation "menu bars" that are implemented as a set of disclosures on the web always focus the first item (or last item depending on which direction you are tabbing from) because all menu items are in the tab order
  • there aren't that many actual application menubars on the web to compare against. Google docs has one, and fyi it just focuses the first item every time you tab to the menubar (from either direction).

Sorry to be undecided... but I guess if I had to choose, I am leaning towards keeping the behavior as it currently is (i.e. the last item that was focused keeps the tabindex="0").

@shirsha
Copy link

shirsha commented Feb 4, 2021

The visual focus looks good. Thanks @jongund and @carmacleod

With respect to the tab order I agree with what @carmacleod mentioned.
I have one scenario that is lingering in my mind:

  • The screen reader user navigates through all available menu items in the menubar.
  • Then navigates to the main page content.
  • Then selects shift+tab, the focus is placed on the last menuitem in the menubar.

Will this confuse the user?

@a11ydoer
Copy link
Contributor

a11ydoer commented Feb 5, 2021

Thanks so much, @carmacleod @jongund @shirsha @mcking65
I am done with the review. In regards to the focus order, current example makes sense to me.

@JAWS-test
Copy link

I think the current implementation is the best:

  • the menuitem which had the focus last is focused
  • when the page is opened for the first time or reloaded, the menuitem with aria-current should get the focus

@jongund
Copy link
Contributor Author

jongund commented Feb 5, 2021

@JAWS-test
I agree that we should stick with the current behavior behavior for tabindex="0" management, without some definitive examples from current practice, I think we should stick to the current menubar pattern recommendations.

@mcking65
Copy link
Contributor

mcking65 commented Feb 9, 2021

@jongund
After commit 722a429, is the documentation in accessibility features still accurate? I'm not sure I understand wha changed about focus other than going back to using 0 and 2 pixels border to distinguish.

@mcking65
Copy link
Contributor

mcking65 commented Feb 9, 2021

It is interesting that both section 4.3 of the ARIA spec and section 6.6 of APG - Keyboard Navigation Inside Components say that it is typical for the first item in menubars and toolbars to receive focus rather than most recent. I'm unsure of the origin of that suggestion. We are certainly not following it. And, perhaps they should both be updated?

@mcking65
Copy link
Contributor

mcking65 commented Feb 9, 2021

@jongund commented:

I suggest adding an additional statement to Carolyn's edit:
The complexity of the menubar pattern is not needed for typical site navigation that is styled to look like a menubar with expandable sections or "fly outs". It is important to note, especially in the case of site navigation, that ARIA roles indicate the behavior used to navigate the site links, not how they visually appear.

I don't think we need the additional words of the second sentence because that it is rather obvious when comparing menubar and disclosure examples. Also, it is somewhat more evident from the wording I just committed. This wording emphasizes that it is the required functionality of menubar that is unnecessary:

The menubar pattern requires complex functionality that is unnecessary for typical site navigation that is styled to look like a menubar with expandable sections or “ fly outs ” .

@mcking65
Copy link
Contributor

mcking65 commented Feb 9, 2021

If accessibility features documentation is accurate and reviewers are OK with my revision of caution box and intro, I will merge.

@JAWS-test
Copy link

JAWS-test commented Feb 9, 2021

It is interesting that both section 4.3 of the ARIA spec and section 6.6 of APG - Keyboard Navigation Inside Components say that it is typical for the first item in menubars and toolbars to receive focus rather than most recent. I'm unsure of the origin of that suggestion.

I suspect the cause is that in desktop applications, it is often the first menu item that gets focus, not the last focused one. However, it should be noted that in desktop applications the menu is often not reached with Tab, but only with Alt Key. However, the standard may also be changing in desktop applications: In Microsoft Office products such as Word and Excel, pressing Alt now focuses on the last focused menu item and not the first. Before anything is adjusted in the ARIA specification and APG, however, more menus should be examined to find out what is currently standard and good usability. Then this menu should be adjusted or the specification so that it is consistent in the end.

Possibly, no recommendation for menus should be given at all, because the best implementation depends on the context of the menu. In some cases it may make sense to focus on the first element, in others on the last focused one, depending on whether or not the work or navigation can be continued meaningfully with the last one. In a navigation menu, for example, I would expect the last focused item to receive focus again, so that I can quickly perceive what the active menu item is. In an action menu, I would only expect the active menu item to receive focus if it is expected that, for example, after assigning a particular formatting, I will want to perform another action that is in the same submenu.

In submenus, on the other hand, the first menu item should probably always get the focus.

Therefore, I suggest the following:

  • menu: always focus on the first entry
  • menubar: rather select the selected or last focused entry. In rare cases also the first

@mcking65
Copy link
Contributor

mcking65 commented Feb 9, 2021

Merging based on final discussion in today's meeting. Thank you @jongund for this awesome update!

@mcking65 mcking65 merged commit 990fc58 into master Feb 9, 2021
@mcking65 mcking65 deleted the update-menubar-nav-2 branch February 9, 2021 20:21
@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern labels Feb 9, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Feb 9, 2021
@jongund
Copy link
Contributor Author

jongund commented Feb 9, 2021

@mcking65
Yeah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Development

Successfully merging this pull request may close these issues.

6 participants