[MenuList] Fix keyboard a11y when no item is focused when opening #16323

eps1lon · 2019-06-21T14:03:44Z

The actual issue is dispatching keydown events to custom targets. It should always be dispatched on document.activeElement because that's how the spec defines it.

Close #16294, #15973

@ryancogswell Could you take a look at the new tests?

If it happens in a passive effect it happend after a commit. A render is only a call to the render function

eps1lon · 2019-06-21T14:04:58Z

packages/material-ui/test/integration/MenuList.test.js

    });
-  });
-
-  describe('MenuList with focusable divider', () => {


While the implementation does allow this I don't think we should test those. After all this does seem invalid. See https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex#Accessibility_concerns

My intent wasn't really to support focusable dividers, but rather to demonstrate focus management working for something that isn't a MenuItem but still meets the rules for recognizing it as a non-disabled, focusable child of the list. I still think there is value in the intent of this test, though it might be better to use something other than Divider for demonstrating it.

I'm a bit cautious about opening this up to other components as well before we have a concrete use case. Especially since there is quite some movement in the react core with Flare and I would hope we'll be able to fully utilize their new event system without having to support other APIs.

mui-pr-bot · 2019-06-21T14:10:25Z

Details of bundle changes.

Comparing: 6a1bf86...dc8a4b9

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.01%	-0.02%	320,513	320,481	88,461	88,441
@material-ui/core/Paper	0.00%	0.00%	68,278	68,278	20,363	20,363
@material-ui/core/Paper.esm	0.00%	0.00%	61,573	61,573	19,161	19,161
@material-ui/core/Popper	0.00%	0.00%	28,945	28,945	10,391	10,391
@material-ui/core/Textarea	0.00%	0.00%	5,513	5,513	2,376	2,376
@material-ui/core/TrapFocus	0.00%	0.00%	3,753	3,753	1,578	1,578
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,009	16,009	5,791	5,791
@material-ui/core/useMediaQuery	0.00%	0.00%	2,597	2,597	1,102	1,102
@material-ui/lab	0.00%	0.00%	140,293	140,293	43,470	43,470
@material-ui/styles	0.00%	0.00%	51,698	51,698	15,348	15,348
@material-ui/system	0.00%	0.00%	15,420	15,420	4,391	4,391
Button	0.00%	0.00%	84,430	84,430	25,758	25,758
Modal	0.00%	0.00%	14,427	14,427	5,086	5,086
Portal	0.00%	0.00%	3,473	3,473	1,571	1,571
Slider	0.00%	0.00%	74,701	74,701	23,243	23,243
colorManipulator	0.00%	0.00%	3,904	3,904	1,544	1,544
docs.landing	0.00%	0.00%	55,119	55,119	13,940	13,940
docs.main	-0.00%	-0.01%	649,040	649,010	204,806	204,784
packages/material-ui/build/umd/material-ui.production.min.js	-0.01%	-0.04%	293,675	293,643	84,370	84,340

Generated by 🚫 dangerJS against dc8a4b9

eps1lon · 2019-06-21T15:05:36Z

Couldn't verify it manually in browserstack. Will investigate what the issue is.

ryancogswell · 2019-06-21T15:19:32Z

Could you take a look at the new tests?

I've looked through the implementation changes and they make sense to me. I've taken a brief look at the tests. I'm a big fan of switching to using react-testing-library and dispatching the events in a more appropriate way. I'll find some time later today to look through the test changes in more detail.

packages/material-ui/test/integration/MenuList.test.js

ryancogswell · 2019-06-21T19:04:54Z

packages/material-ui/test/integration/MenuList.test.js

    });
-  });
-
-  describe('MenuList with focusable divider', () => {


My intent wasn't really to support focusable dividers, but rather to demonstrate focus management working for something that isn't a MenuItem but still meets the rules for recognizing it as a non-disabled, focusable child of the list. I still think there is value in the intent of this test, though it might be better to use something other than Divider for demonstrating it.

eps1lon · 2019-06-21T19:30:01Z

packages/material-ui/test/integration/MenuList.test.js

@@ -15,210 +16,170 @@ function FocusOnMountMenuItem(props) {
  return <MenuItem {...props} ref={listItemRef} tabIndex={0} />;
 }

-function TrackRenderCountMenuItem({ actions, ...other }) {
-  const renderCountRef = React.useRef(0);
+function TrackCommitCountMenuItem({ actions, ...other }) {


So excited about a stable Profiler component where we can get rid of these workarounds.

Get rid of it?!! I'm rather emotionally attached to that bit of code after all the time I invested into figuring out how to verify that it wasn't re-rendering on focus changes. :)

Well this particular code is already nice because you went with commit tracking instead of the render tracking. That's already future proof when we run fuzzy tests in concurrent mode. Most of the tests around this issue go with render counting which will become unstable.

oliviertassinari · 2019-06-23T17:41:41Z

The keyboard interaction doesn't seem to work on https://material-ui.com/components/menus/#simple-menu. Is this expected?

eps1lon · 2019-06-23T18:32:56Z

Yes. This only fixed the integration with Select. The menu needs an a11y overhaul but I want to draft a general a11y proposal since the increasing discussions around a11y slows down work to the point where it's no longer viable. We need to find a general consensus how important a11y is for us first.

oliviertassinari · 2019-06-23T19:08:17Z

We need to find a general consensus on how important a11y is for us first.

@eps1lon I think that It largely depends on what aspect of a11y and how we want to grow the library.

The majority of our current users don't truly care about it. It's what we have learned from the user survey. What people value is: features. The majority of our users use Material-UI to go fast. a11y is structurally in opposition with this objective, it's a lot of work. It's not necessary to validate business assumptions when building a prototype. It has a negative ROI on small projects.
Only considering our current users, we would be better off deprioritizing a11y because it slows us down. The opportunity cost moves us away from making the components more feature rich. See how Ant Design is popular in China, and yet, their components are not famously known to be accessible, not a coincidence. We should be a projection of what our users value. If they don't value a11y, neither should we. If we do, we take the risk to be replaced by somebody that doesn't and is able to move faster. It's still important for the minority, we could improve it in a pro-bono manner.

However, and that the interesting part, I think that there is an opportunity to grow the project from the (indie hacker land, backend dev, entrepreneurs, full stack devs, people learning to code) in the enterprise land (to target front end developers, Fortune 500 companies). In order to execute, we need to have outstanding accessibility, bundle size & performance (without to forget the features of course).

Today, +50% of the team starts their React design system do it from scratch. People going with the "from scratch approach" have different motives. I believe it's the fear of dependencies. I wish I could dive more in the hidden fear behind it, maybe I will learn more about it. I will keep digging.

Starting from scratch shouldn't be the industry standard. It's wasteful. It should only be an option when you have +5 full-time developers team and a year or two ahead of you. People underestimate the time it takes and the number of iterations necessary to get the components right. We have 350k developers continuously giving us feedback on what we do wrong and reacting on it.

I believe that Material-UI could be a one-stop components store. We can enable teams to reach a quality of components that they would never be able to reach without Material-UI in the same period of time. To do that, I think that we should prioritize the following aspect of a11y in this order:

keyboard & focus, it's the first thing people need and notice, even none impaired one.
aria-x, they are the workers in the shadow, they shine when your screen reader is turned on.
colors, it's the least important dimensions as the most team will override all our default colors, what we implement should only be a guideline, but ultimately, it's the UI / UX designer of people's team that decides. So while this dimension is important, we should also make tradeoffs to resonate with the "go fast" users.

What do you think?

ianschmitz · 2019-06-23T21:49:16Z

As a long term user of MUI that has promoted it heavily internally at my company to the point where we use it on 100% of our next-gen products used by thousands of fortune-500/governmental customers, i can speak to the immense value proposition that out-of-the-box accessibility support increasingly brings us.

The majority of our current users don't truly care about it. It's what we have learned from the user survey. What people value is: features. The majority of our users use Material-UI to go fast. a11y is structurally in opposition with this objective, it's a lot of work. It's not necessary to validate business assumptions when building a prototype. It has a negative ROI on small projects.

I think this boils down to the fact that accessibility is hard! I strongly believe as developers we should be doing more to cater to users that benefit from accessibility. There is a huge knowledge gap for what is required to make an application accessible. MUI is in a unique position due to its market share to promote and bring awareness to accessibility like you are already doing in your components and in the docs!

However, and that the interesting part, I think that there is an opportunity to grow the project from the (indie hacker land, backend dev, entrepreneurs, full stack devs, people learning to code) in the enterprise land (to target front end developers, Fortune 500 companies). In order to execute, we need to have outstanding accessibility, bundle size & performance (without to forget the features of course).

I think this is key. There is an ever increasing demand in North America (and I would hope this also extends to Europe/Global) that web sites/applications be accessible. Government and private organizations are increasingly requiring that the applications they procure meet accessibility requirements. For example my company does significant business with the Canadian/US Government that often require by mandate that our products meet WCAG AA specifications or they will close the door on us even if our product is superior to competitors.

I believe that Material-UI could be a one-stop components store. We can enable teams to reach a quality of components that they would never be able to reach without Material-UI in the same period of time.

💯. The built-in styling that MUI provides is nice, but what I've found even more valuable is the amazing, intuitive, consistent API, including the out-of-the-box accessibility support. We've been able to transform the styling using Theme overrides, all while keeping the wonderful API that you provide and staying accessible. It has significantly reduced our R&D. We've been able to develope our component set in a fraction of the time compared to if we had rolled our own (we tried it... it was a source of immense pain). We simply bring in your components, tweak the styling to suit our brand, and ship it to all of our products.

adamscybot · 2019-06-25T15:58:32Z

Accessibility in some countries is the law. In the UK you could be in trouble if your service is not accessible: https://www.gov.uk/service-manual/helping-people-to-use-your-service/making-your-service-accessible-an-introduction.

For us this makes adopting something like material ui across the company a real challenge. Ignoring it because the majority of users don't care could get you sued.

oliviertassinari · 2019-06-25T17:06:00Z

@adamscybot Same in the US: #14187.

eps1lon · 2019-06-26T07:39:09Z

I wouldn't even go as far as public facing websites. I don't think we're that far ahead yet. It's more of a problem if companies build internal tools (which we already know some). Companies are required to hire a certain quote of people with disabilities (germany) at a certain size. They can't do that if their internal tools aren't accessible.

ianschmitz · 2019-07-09T00:22:31Z

@oliviertassinari @eps1lon sorry to ping, but i didn't want to create a new issue and drive you guys nuts 😛. Do you guys have a plan to address the outstanding a11y issues with Menu/MenuList? Should we open a new issue to track?

eps1lon · 2019-07-09T05:50:17Z

I have it written down for this week.

ryancogswell · 2019-07-09T15:23:08Z

@eps1lon @ianschmitz I'll get the rest of my thoughts about this documented within the next couple days as well. The biggest Menu/MenuList issue was taken care of by #16450, but there are one or two minor issues outstanding.

eps1lon added 3 commits June 21, 2019 09:45

renderCount -> commitCount

2eb1517

If it happens in a passive effect it happend after a commit. A render is only a call to the render function

[test] Rework MenuList integration tests

d4f3b4e

Cleanup lint config [skip ci]

e1374d9

eps1lon added bug 🐛 Something doesn't work accessibility a11y component: menu This is the name of the generic UI component, not the React module! labels Jun 21, 2019

f

b798a50

eps1lon commented Jun 21, 2019

View reviewed changes

eps1lon added 5 commits June 21, 2019 16:10

Reduce number of goto statements

868e4d2

[MenuList] Fix keyboard navigation when no item focused

5801f2f

[docs] Adjust one demo to showcase behavior with no initial selection

50e0f99

Format

646fe75

Better expect focused diff

6d636d7

ryancogswell reviewed Jun 21, 2019

View reviewed changes

eps1lon commented Jun 21, 2019

View reviewed changes

eps1lon added 3 commits June 21, 2019 21:34

Address ryan's review

d00b4a4

expectSingleMenuItemFocusable _> expectSingleMenuItemTabFocusable

e38013b

Skip failing karma test

662d100

eps1lon force-pushed the test/menulist-integration-rtl branch from 68736da to 662d100 Compare June 21, 2019 20:29

eps1lon added 2 commits June 21, 2019 22:47

test karma

00dd104

Skip whole MenuList integration in chrome 49

26da9b3

eps1lon force-pushed the test/menulist-integration-rtl branch from 3ac82c4 to 916aa7d Compare June 21, 2019 21:13

Remove unused mount

dc8a4b9

eps1lon force-pushed the test/menulist-integration-rtl branch from 916aa7d to dc8a4b9 Compare June 21, 2019 21:21

eps1lon mentioned this pull request Jun 21, 2019

[test] Remove karma #16263

Closed

1 task

eps1lon marked this pull request as ready for review June 21, 2019 21:37

eps1lon merged commit 6539a04 into mui:master Jun 22, 2019

eps1lon deleted the test/menulist-integration-rtl branch June 22, 2019 07:56

eps1lon mentioned this pull request Jun 22, 2019

Select Accessibility #15973

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MenuList] Fix keyboard a11y when no item is focused when opening #16323

[MenuList] Fix keyboard a11y when no item is focused when opening #16323

eps1lon commented Jun 21, 2019 •

edited

Loading

eps1lon Jun 21, 2019

ryancogswell Jun 21, 2019

eps1lon Jun 21, 2019

mui-pr-bot commented Jun 21, 2019 •

edited

Loading

eps1lon commented Jun 21, 2019

ryancogswell commented Jun 21, 2019

ryancogswell Jun 21, 2019

eps1lon Jun 21, 2019

ryancogswell Jun 21, 2019

eps1lon Jun 21, 2019

oliviertassinari commented Jun 23, 2019

eps1lon commented Jun 23, 2019

oliviertassinari commented Jun 23, 2019 •

edited

Loading

ianschmitz commented Jun 23, 2019

adamscybot commented Jun 25, 2019 •

edited

Loading

oliviertassinari commented Jun 25, 2019

eps1lon commented Jun 26, 2019

ianschmitz commented Jul 9, 2019

eps1lon commented Jul 9, 2019 •

edited

Loading

ryancogswell commented Jul 9, 2019

[MenuList] Fix keyboard a11y when no item is focused when opening #16323

[MenuList] Fix keyboard a11y when no item is focused when opening #16323

Conversation

eps1lon commented Jun 21, 2019 • edited Loading

eps1lon Jun 21, 2019

Choose a reason for hiding this comment

ryancogswell Jun 21, 2019

Choose a reason for hiding this comment

eps1lon Jun 21, 2019

Choose a reason for hiding this comment

mui-pr-bot commented Jun 21, 2019 • edited Loading

eps1lon commented Jun 21, 2019

ryancogswell commented Jun 21, 2019

ryancogswell Jun 21, 2019

Choose a reason for hiding this comment

eps1lon Jun 21, 2019

Choose a reason for hiding this comment

ryancogswell Jun 21, 2019

Choose a reason for hiding this comment

eps1lon Jun 21, 2019

Choose a reason for hiding this comment

oliviertassinari commented Jun 23, 2019

eps1lon commented Jun 23, 2019

oliviertassinari commented Jun 23, 2019 • edited Loading

ianschmitz commented Jun 23, 2019

adamscybot commented Jun 25, 2019 • edited Loading

oliviertassinari commented Jun 25, 2019

eps1lon commented Jun 26, 2019

ianschmitz commented Jul 9, 2019

eps1lon commented Jul 9, 2019 • edited Loading

ryancogswell commented Jul 9, 2019

eps1lon commented Jun 21, 2019 •

edited

Loading

mui-pr-bot commented Jun 21, 2019 •

edited

Loading

oliviertassinari commented Jun 23, 2019 •

edited

Loading

adamscybot commented Jun 25, 2019 •

edited

Loading

eps1lon commented Jul 9, 2019 •

edited

Loading