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

remove menubar role from EuiSideNav #2429

Merged
merged 3 commits into from
Oct 16, 2019

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Oct 15, 2019

Summary

EuiSideNav was making a promise that it couldn't keep. (Metaphor explainer: a custom role is a promise that developer is making to the assistive tech on how something will work.) Better to ship a simpler structure that's not broken but could be better rather than ship broken semantics.

EuiSideNav can still be improved but this is a small, quick PR to make it just a bit better.

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation examples

  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@ryankeairns
Copy link
Contributor

For the sake of my own edification, menubar is typically used for horizontal menus with submenus as you'd see in Google Docs, for example. In our case, it would be wha we refer to as a 'File menu'.

Is that statement generally correct @myasonik ? Just trying to better understand the 'broken semantics' reasoning. Thanks!

@chandlerprall
Copy link
Contributor

I believe this is because we didn't implement the other roles & labels for menus, yes? https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html

@thompsongl
Copy link
Contributor

Looks like the correct keyboard controls aren't implemented, either (e.g., arrow key movement between items)

@myasonik
Copy link
Contributor Author

Great question and thank you for asking! Strap in for a lot of words!

Generally, I'll say that the recommendation is to avoid role=menu and friends (e.g., menubar, menuitem). They are spec'ed out roles and are quite old (so support is decent) but they've been fraught with issues on the web so users generally aren't fans.

I highlighted "on the web" because these are desktop paradigms brought to the web, not "web native" ideas. These roles were created to recreate OS system menus (the ones that have "File", "Edit", etc.) and if that's not what you're recreating, they're probably not the right role to use. Looking at the OS menus, you'll see that they're predominantly filled with actions (not links) and there's generally a level of complexity to them (several layers of nesting, mixing actions and links next to each other, sometimes an inline action like the Help menu on Macs, etc).

Because of all of those reasons, these roles have a lot of very prescriptive interaction patterns (take a look at all of the keyboard interactions required by the spec let alone what might actually be a good experience). But developers haven't been so great at implementing these to spec so screen readers and users of assistive tech have had to try to figure out what to do with these half-way implemented menus.

So now we're in the kind of unfortunate state that role=menu doesn't mean much anymore. Developers often don't implement it to spec and, even if it is, users assume it hasn't been (or, are no longer are used to using one that is) so it's become this complicated pattern that no one is sure how to implement or use.

OK, so we've got all of that from an industry perspective. But also looking specifically at our implementation, we have some confusion too. It's called EuiSideNav but you also called it a "file menu" which are semantically different things even if people often use the word "nav" and "menu" interchangeably.

And then looking at Kibana, I didn't look at every use of it but what I'd guess is the most common EuiSideNav users will interact with in the one on the management screen which is quite literally acting as a list of links to different management pages and doesn't have any of the normal hallmarks of role=menu.

I'm rambling a bit now and this is getting really long so I want to close this out with two things:

  • Generally, less complicated is better. If a nav element surrounding a list of links communicates about the same as a menu filled with links (which also requires lots more aria-* attributes and custom keyboard interactions) relying on browser and assistive tech built-in functionality is the way to go.
  • Unfortunately, Kibana is complicated so we will have to sometimes build the complicated thing (see the DataGrid a11y spec). When we do that though, we have to take a lot of care to get the semantics just right and test the bejesus out of the thing to make sure we didn't mess it up and that there aren't any browser bugs/inconsistencies that ruin the whole experience.

I mostly haven't cited anything I've been saying here so if you want to read more about menu or anything else accessibility related, let me know and I can round up a collection of links specific to that topic.

@ryankeairns
Copy link
Contributor

Thanks for the explanation, that makes sense and confirmed my (very basic) assumptions.

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM based upon Michail's explanation.

@myasonik myasonik merged commit 5f1a4be into elastic:master Oct 16, 2019
@myasonik myasonik deleted the side-nav-a11y-reset branch October 16, 2019 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants