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

1028 disclosure menu example #1036

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Jun 3, 2019

Resolves #1028, adds a site navigation example using the disclosure pattern similar to the main menu of https://www.usa.gov/.

Could use some help reviewing the documentation in particular. Thanks! :)


Preview | Diff

@carmacleod
Copy link
Contributor

Hi, @smhigley ! Here's the preview link I used: https://raw.githack.com/smhigley/aria-practices/1028-disclosure-menu-example/examples/disclosure/disclosure-navigation.html

I only have one editorial comment:

  • "using the disclosure design pattern used" (delete "used")

I also have one visual design consideration/discussion point:

Even though I know this pattern does not support arrow keys within the dropdown lists, I somehow expected arrow keys to navigate within the dropdown, so I was momentarily confused; whereas for some reason, I didn't try using arrow keys inside the dropdowns on usa.gov. There must be subtle visual cues that can cause a user to lean towards tabbing or trying arrow keys. Looking at the visual cues on both sites:

  • usa.gov only outlines expanded disclosure buttons and focused links - this example colors the background
  • usa.gov disclosure buttons have little down-pointing V affordance to show that something will open
  • usa.gov dropdowns are laid out in a grid - this example has a vertical list
  • usa.gov keeps link underlines on hovered and focused links, including a wide blue underline on focused or hovered disclosure buttons - this example removes underlines

I'm not sure which of those gives the strongest cue, or maybe it's a combination, but I think the example maybe looks a bit too much like a menubar? It might be helpful to make it look more "link-ish" so that sighted keyboard users think "tab key"?

Do you agree?

It would be useful to know if anyone else felt this way, or if it was just me.

@smhigley
Copy link
Contributor Author

smhigley commented Jun 4, 2019

@carmacleod I actually felt the same way! I didn't implement arrow keys within the dropdowns because the usa.gov reference didn't use them (and because it's not a real role="menu"). I also tried to pull some styles to be visually consistent with the existing APG menubar/menubar-1 example (e.g. the background grey, brown highlights, hover state).

I think maybe to give the best visual cue to tab into dropdown menus, I could:

  1. Add the downward-pointing "v" to top-level buttons
  2. Underline the links in the dropdown
  3. Change the hover/focus state of links in the dropdown to something else -- maybe an outline?
  4. Add an underline to the hover/focus state of the top-level buttons

Do you think implementing the above would fix the arrow key confusion? I'm also not opposed to adding arrow key functionality (in addition to tabs) to the dropdowns if desired.

@carmacleod
Copy link
Contributor

carmacleod commented Jun 4, 2019

I actually felt the same way!

Phew! I hoped it wasn't just me. :)

Do you think implementing the above would fix the arrow key confusion?

I think those changes would help a lot. :)

I'm also not opposed to adding arrow key functionality (in addition to tabs) to the dropdowns if desired.

While adding arrow key behavior would help sighted keyboard users (because they could choose to use arrows or tabs), it wouldn't make any difference to AT users because arrow keys are used for AT navigation.

This seems like something that should be discussed with the group, i.e. do we want to promote improved arrow key behavior for sighted keyboarders but not AT users? Maybe that's ok?

What is interesting is that arrow key and home/end behavior are listed as optional in the accordion pattern, so I guess this type of thing has been discussed and allowed before. It would be great to have a link to that discussion.

I quickly tested the accordion example and this disclosure menu example in NVDA/Chrome, and I can confirm that arrow keys work very differently depending on whether or not NVDA is running.

@aardrian
Copy link
Contributor

aardrian commented Jun 5, 2019

@carmacleod:

This seems like something that should be discussed with the group, i.e. do we want to promote improved arrow key behavior for sighted keyboarders but not AT users? Maybe that's ok?

I think "improved" is a relative term. Arrow keys work to scroll the viewport (or container if it is a scrolling container). Adding support for arrow keys to the menu removes native support for scrolling when in the navigation menu.

Since there is no obvious indication (instructions or otherwise) that native features are removed, I suggest against adding keyboard support for this example.

Testing with users can provide more insight, though as a consultant I would caution clients against implementing them without testing with their users. And exposing that change to users in text.

@carmacleod
Copy link
Contributor

I think "improved" is a relative term. ...
I would caution clients against implementing them without testing with their users.

Excellent points, @aardrian.

If we do discuss it and decide to go ahead, we should make sure the behavior is listed as "(Optional)".
Which makes me wonder if perhaps each instance of "(Optional)" in the APG should link to a warning note about user testing. (That last sentence sounds like I'm kidding or teasing, but I'm serious that it could be helpful.) The warning note could even mention typical browser default behavior for certain keys.

@aardrian
Copy link
Contributor

aardrian commented Jun 5, 2019

@carmacleod

perhaps each instance of "(Optional)" in the APG should link to a warning note about user testing. […] The warning note could even mention typical browser default behavior for certain keys.

Yes and yes.

Providing a qualifier is good, and a general note about default keys for browsers and maybe AT would be useful. It may be unnecessary to list them, but at least acknowledge that there are default behaviors in different contexts and programmatically assigning keys can blow it up for users.

Good idea.

Now I am wondering if it is worth mentioning_events_ as well. For example an event triggered by Space fires onkeyup, where Enter is onkeydown, which has a potential to affect expectations as well. But now I am rambling. I think you get where my head is.

@smhigley
Copy link
Contributor Author

smhigley commented Jun 5, 2019

@aardrian / @carmacleod good points, thanks for the feedback!

@aardrian you aren't the only person to have mentioned that hijacking the default scroll functionality of arrow keys might cause confusion, so it's definitely on my mind :).

I know this isn't common for aria practices examples, but what if I included something like a checkbox for "include arrow key navigation" to the example, to display both possible behaviors? Then the "Accessibility Features" section can include a mention of the potential benefits and drawbacks of adding arrow key navigation, and it can be listed as optional in the "Keyboard Support" table.

Right now, the notes I'd like to see are:

  • Arrow key support prevents the default page scroll behavior when focus is on the disclosure menu
  • Arrow keys and home/end provide an alternative interaction pattern for users that may prefer it, but do not replace the default tab navigation.
  • Arrow key navigation is primarily for the benefit of keyboard users and not screen reader users
  • More complex examples of dropdown menus may need to revisit arrow key behavior if they collapse into a different orientation at certain breakpoints

What do you think, and is there anything else to add? Right now I'm leaning towards implementing arrow keys + home/end for both the top-level buttons and dropdown links with the added caveats. I'm even more in favor of adding the "yes/no arrow keys" switch to demonstrate both options.

@aardrian
Copy link
Contributor

aardrian commented Jun 5, 2019

@smhigley

I know this isn't common for aria practices examples, but what if I included something like a checkbox for "include arrow key navigation" to the example, to display both possible behaviors?

Not common is no reason not to, IMO. If anything you have made the case to make it more common.

I like your notes. To me they are clear with one tweak:

  • Arrow key navigation is primarily for the benefit of keyboard users and not screen reader users
  • Arrow key navigation is primarily for the benefit of keyboard users who are not running a screen reader; screen readers intercept the arrow keys and will not be passed to the page.

Yeah? Decent? Over-simplified I know.

I'm even more in favor of adding the "yes/no arrow keys" switch to demonstrate both options.

I like that as it does not split the pattern.

BTW, what do you think of the pattern where the top-level item is a link, with the disclosure control as a peer? I ask because I have built a similar nav menu that does it. I have had clients ask for top-level-as-link and top-level-as-control-only for a couple decades (roughly), so I have a pattern I can share (in a different thread to not pollute this PR).

@carmacleod
Copy link
Contributor

@aardrian

an event triggered by Space fires onkeyup, where Enter is onkeydown

Right - I hadn't thought of that. Your knowledge of "user gotchas" is appreciated.
Can probably explain it to mouse users by asking if button "click" on mousedown would be startling... ;)

@smhigley I like the checkbox idea. You could try it as long as you don't mind ripping the code out if the group prefers to keep it simple. You might want to commit the other stuff first (or make a new branch) so that you can back things out easily... :)

@carmacleod
Copy link
Contributor

@aardrian - just saw your latest comment

what do you think of the pattern where the top-level item is a link, with the disclosure control as a peer?

Sounds interesting. Please do share, so we can compare how it "feels". (Perhaps it would even warrant another checkbox 😄 ).

@aardrian
Copy link
Contributor

aardrian commented Jun 5, 2019

@carmacleod @smhigley,
For your consideration: https://codepen.io/aardrian/pen/JaadKE

@carmacleod
Copy link
Contributor

Oh wow, I had the same feeling of "I should be able to use arrow keys here" that I had with this issue's example. I guess I expect links to look... "linkier", with some kind of underline affordance - even if it's only on focus or hover. It makes me feel like I'm tabbing twice for every menuitem... even though I know that it's a link and a button, and not a menuitem. I must have a "menubar mindset" (carried over from the desktop days).
Other than that feeling, it's quite beautiful! :)
(PS: Are the buttons 44 x 44? The hit area feels a tiny bit small compared to the size of the link?)
Thanks for sharing!

@aardrian
Copy link
Contributor

aardrian commented Jun 5, 2019

@carmacleod

(PS: Are the buttons 44 x 44? The hit area feels a tiny bit small compared to the size of the link?)

No. Most of my clients were not interested in 2.5.5 as it is AAA and they felt made it look to blocky. A larger hit area outside of the border is just a Fitts failure.

For an example here, that is an easy change.

@carmacleod
Copy link
Contributor

@aardrian

Most of my clients were not interested in 2.5.5 as it is AAA and they felt made it look to blocky.

Thanks for the reminder - I was thinking it was AA.

A larger hit area outside of the border is just a Fitts failure.

I had to look up Fitts Law - I haven't thought about it in a long time. I wonder if there's a bit of wiggle room - I'd have to do the math.

For an example here, that is an easy change.

Absolutely!

@scottaohara
Copy link
Member

scottaohara commented Jun 6, 2019

@smhigley, if you're interested, I remembered I had a pagination demo page I was doing some testing on (but never finished), and one of the features as a control to turn on arrow key navigation:

I like your idea of a checkbox (switch?) better though :)

@smhigley
Copy link
Contributor Author

@carmacleod / @aardrian / @scottaohara sorry this took a while to update, but the arrow key checkbox and extra wording has been added. Let me know what you think! (link: https://raw.githack.com/smhigley/aria-practices/1028-disclosure-menu-example/examples/disclosure/disclosure-navigation.html)

@carmacleod
Copy link
Contributor

@smhigley That plays very well, and it's clear now that the menuitems are links - nice!

Teeny-tiny bug in the arrow key version:

  • space or enter to dropdown menu
  • down arrow key to go into the menu
  • page scrolls (i.e. need preventDefault for that first down arrow into the menu. After that, it's fine).

@carmacleod
Copy link
Contributor

@smhigley Wow, you even added tests - very cool!

@scottaohara
Copy link
Member

Only thing i noticed was the weird bit of page scroll as @carmacleod mentioned when testing this with macOS safari/chrome/firefox

Space/enter both worked for me, and i noticed that down arrow went into the sub-list when the button was in the expanded state. that makes sense to me cause these aren't menus so I wouldn't expect them to open with down arrow.

@smhigley
Copy link
Contributor Author

@carmacleod thanks, I missed that one! Pushing up the fix now :).

@scottaohara yup, I coded it to have down arrow move into the menu from the button only after it was already opened. Definitely open to suggestion though :)

@smhigley
Copy link
Contributor Author

Based on feedback in the ARIA APG meeting, I'm going to implement a small fake page content section below the menu, and point each link to its idref, then dynamically change the title of the fake page content based on the activated link.

I'll also add a bullet under "accessibility features" that specifies that escape is necessary because of the 2.1 content on hover or focus guideline that specifies popup content should be dismissable.

@a11ydoer
Copy link
Contributor

a11ydoer commented Jun 11, 2019

@smhigley Thanks for all your work. This is great. I also love the option of the arrow key navigation!

For the sake of "consistent" APG example documentation - APG group spent quite some time to figure out this format of APG example template - the heading for the example page can be "Disclosure Navigation Menu Example" or something rather than "Example Disclosure (Show/Hide) for Navigation Menu" Please refer other APG example headings if needed. APG is looking forward to pushing your example to the issue branch!

@smhigley
Copy link
Contributor Author

@a11ydoer in this case, would it be better to follow the format of headings from other examples like menubar or grid, or from the other disclosure examples? I ran into this when initially creating the page, since the other two disclosure examples seem a little different from the rest.

@aardrian
Copy link
Contributor

@smhigley , this is great. Do not apologize for what you think is a delay as this is moving at a comparatively quick pace.

I see <li role="separator"></li> but it is not visible. Did you retain it intentionally to show how it might be coded (in which case I think it needs a visual style), or is it a hold-over from a different pattern?

In a vacuum, I would be inclined to exclude it just to keep the scope of this menu pattern as tight as possible.

@carmacleod
Copy link
Contributor

I agree with Adrian re excluding the separator. There are example separators in the menubar pattern example, so not needed here.

@smhigley
Copy link
Contributor Author

@aardrian / @carmacleod it was 100% a holdover from a different pattern. I kept it originally for parity, then forgot about it when I re-styled 😆

@smhigley smhigley force-pushed the 1028-disclosure-menu-example branch from e1618af to 9e68018 Compare June 12, 2019 19:09
@smhigley
Copy link
Contributor Author

smhigley commented Jun 12, 2019

Changes are up, ready to be re-reviewed! They include:

  • Down arrow scroll from button to open menu is prevented
  • Sample page content region added, links updated to point to the region's id
  • Documentation updated with escape key explanation
  • Documentation updated to mention arrow keys in open dropdowns
  • Separator <li>s removed
  • Page title updated
  • Tests updated

@carmacleod
Copy link
Contributor

Very nice! It feels intuitive and "linky", and still works well with mouse.

A few editorial comments:

  • The following example demonstrates using the disclosure design pattern used to create a dropdown navigation menu.

    The "using"/"used" feels redundant. Maybe delete "used"?

  • The visual indication of expanded and collapsed states is synchronized with the value of aria-expanded using a CSS attribute selector and :before pseudo element that generates an image with the content property.
    ...
    The visual indicator of the show/hide state is created using CSS :before pseudo element and content property so the image is reliably rendered in high contrast mode of operating systems and browsers. (2 instances of this in the table for aria-expanded="false" and "true")

    Did you mean ::after? And is the arrow image supposed to change with the value of aria-expanded? If so, then that doesn't seem to be working.

  • 3. Arrow keys supplement, but do not replace, tabbing between the top-level disclosure buttons. Home and End likewise allow jumping to the first and last button.
    ...
    6. Arrow keys and home/end provide an alternative interaction pattern for users that may prefer it, but do not replace the default tab navigation.

    Points 3 and 6 seem redundant.

  • 8. More complex examples of dropdown menus may need to revisit arrow key behavior if the menus collapse into a different orientation at certain breakpoints.

    I don't really understand point 8. Revisit how? (i.e. remove arrow key support? Or go full-on aria menus?) Also not sure exactly what "if the menus collapse into a different orientation at certain breakpoints" means. Could you elaborate?

  • Tab | Moves keyboard focus between top-level buttons, and into any open menu.

    Tab also moves through the open menu, so maybe: "...and into and then through any open menu."?

@smhigley
Copy link
Contributor Author

@carmacleod thanks for reviewing the documentation! I removed the wording around the down icon changing with aria-expanded, the redundant arrow key bullet, and the 8th bullet about revisiting arrow key behavior based on orientation. The original thinking for the last one was that if dropdowns opened to one side, or changed link orientation, the arrow keys used may need to change. However, since both left/right and up/down were implemented, it seems unnecessary.

I also made the wording changes you recommended for using/used and tabbing, and updated the wording about the down icon and high contrast mode to reflect the actual implementation :).

Thanks again!

@carmacleod
Copy link
Contributor

One more suggestion from @mcking65 to remove references to the word "menu".
For example, the title could be:
"Example Disclosure (Show/Hide) for Navigation Links"

@carmacleod
Copy link
Contributor

Just a quick note that there's still one remaining instance of the :before text icon that needs updating.

@mcking65 mcking65 changed the base branch from master to issue1028-disclosure-menu-example July 6, 2019 04:35
@mcking65
Copy link
Contributor

mcking65 commented Jul 6, 2019

@smhigley, I couldn't push to your fork so merging to a feature branch. Whenever you get a chance, please create a fresh PR from the issue1028-disclosure-menu-example branch.

I'll just be making a few minor editorial changes. The only thing I see in this PR that I don't understand are the changes to package.json. They don't look like changes generated by NPM.

@smhigley
Copy link
Contributor Author

smhigley commented Jul 7, 2019

PR is continued here: #1070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants