[ButtonBase] Use fork of focus-visible polyfill #15484

eps1lon · 2019-04-25T15:20:22Z

~~This is a draft. It doesn't use github drafts to get a netlify deploy.~~
Prerequisite for #15398

Uses a fork of https://github.com/WICG/focus-visible instead of our own implementation which is quite unresponsive.

Playground:

TODO:

prior art? related issues? Edit: Didn't find prior attempts to improve this.
Use polyfill as a fallback only
cleanup (fork and ButtonBase)
probably need to fix tests
green build

@ryancogswell You might be interested in this one.

mui-pr-bot · 2019-04-25T15:27:57Z

@material-ui/core: parsed: -0.12% 😍, gzip: -0.03% 😍
@material-ui/lab: parsed: -0.39% 😍, gzip: -0.10% 😍

Details of bundle changes.

Comparing: 8c5a1ca...7758875

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.12%	-0.03%	311,394	311,022	84,601	84,579
@material-ui/core/Paper	0.00%	-0.01%	67,623	67,623	20,124	20,121
@material-ui/core/Paper.esm	0.00%	-0.01%	60,988	60,988	19,020	19,018
@material-ui/core/Popper	0.00%	+0.02% 🔺	31,114	31,114	10,803	10,805
@material-ui/core/Textarea	0.00%	-0.04%	5,468	5,468	2,365	2,364
@material-ui/core/TrapFocus	0.00%	0.00%	3,731	3,731	1,565	1,565
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,943	15,943	5,777	5,777
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	-0.39%	-0.10%	140,997	140,444	42,651	42,608
@material-ui/styles	0.00%	+0.01% 🔺	51,151	51,151	15,148	15,149
@material-ui/system	0.00%	0.00%	11,765	11,765	3,923	3,923
Button	-0.74%	-0.21%	85,901	85,266	25,732	25,677
Modal	0.00%	-0.03%	20,575	20,575	6,603	6,601
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	51,486	51,486	11,342	11,342
docs.main	-0.02%	-0.02%	648,595	648,437	202,362	202,322
packages/material-ui/build/umd/material-ui.production.min.js	-0.11%	+0.02% 🔺	293,109	292,788	82,525	82,538

Generated by 🚫 dangerJS against 7758875

packages/material-ui/src/ButtonBase/ButtonBase.js

packages/material-ui/src/ButtonBase/focusVisible.js

ryancogswell · 2019-04-25T16:03:36Z

@eps1lon I verified in this netlify deploy that the Menu focus styling is much more immediate and no longer has the slightly-long-keypress issue I mention as issue 5 here.

packages/material-ui/src/ButtonBase/focusVisible.js

packages/material-ui/src/ButtonBase/ButtonBase.js

packages/material-ui/src/ButtonBase/focusVisible.js

oliviertassinari · 2019-04-25T22:10:03Z

packages/material-ui/src/ButtonBase/focusVisible.js

+export function teardown() {
+  document.removeEventListener('keydown', handleKeyDown, true);
+  document.removeEventListener('mousedown', handlePointerDown, true);
+  document.removeEventListener('pointerdown', handlePointerDown, true);


Is the pointerdown event redundant with mousedown and touchstart?

I haven't really looked into those. Not sure what other pointer types e.g. a pen would trigger in browsers that implement PointerEvents.

Keeping it for now until we can verify that a pointerdown from a pen triggers either keydown or mousedown.

packages/material-ui/src/ButtonBase/focusVisible.js

ryancogswell · 2019-04-28T22:49:47Z

packages/material-ui/src/ButtonBase/ButtonBase.js

-  handleBlur = createRippleHandler(this, 'Blur', 'stop', () => {
-    clearTimeout(this.focusVisibleTimeout);
+  handleBlur = createRippleHandler(this, 'Blur', 'stop', event => {
+    handleBlur(event);


Looking at the WIGC focus-visible.js, I think this handleBlur call should either be moved inside the following if (this.state.focusVisible) { or pass this.state.focusVisible as an additional argument to handleBlur. I don't think hadFocusVisibleRecently = true; should occur unless this element had this.state.focusVisible prior to the blur.

Good call. I think we should rename the handleBlur from focusVisible.js to handleBlurVisible. Removing styling etc is the responsibility of the caller but focusVisible.js should manage the global state.

packages/material-ui/src/ButtonBase/ButtonBase.js

Make it clearer when handleBlur should be called

Forgot they won't work that well across windows

enzyme find cant pierce shadow dom

oliviertassinari · 2019-04-29T13:06:59Z

We add x listeners for each button base instance. It might be surprising, but I think that it's fine, it probably has no performance implication.
If would be great to rebase on the next branch so we can try the menu key feature directly on the preview link.
Looks great! I love when a bug fix reduces bundle size. We might want to add a regression test for the menu keyboard feature (I don't know if it's possible) in case we have to change the logic somewhere don't the line.
~~Assuming the focus visible logic will never be used on an element that shows a keyboard, I believe we don't need inputTypesWhitelist.~~ A tooltip can be used to wrap an input ([Tooltip] Display only on keyboard focus #15398). I remember one issue about this use case.

oliviertassinari · 2019-04-29T13:11:21Z

packages/material-ui/src/ButtonBase/ButtonBase.test.js

-      assert.strictEqual(getState().focusVisible, true);
-    });
-
-    it('should ignore the keyboard after 1s', () => {


We changed the 1s duration at some point, entropy 👋

There's alot of behavior that isn't tested right now in our codebase. But this is a fork so it is actually tested somewhere. Just need to make sure we watch for releases of the base.

eps1lon · 2019-04-29T13:14:53Z

Assuming the focus visible logic will never be used on an element that shows a keyboard, I believe we don't need inputTypesWhitelist.

Let's keep it for now and see if the focus visible is useful elsewhere. It's a common a11y helper so I don't want it floating around in the code base under a name that doesn't fully apply.

We add x listeners for each button base instance. It might be surprising, but I think that it's fine, it probably has no performance implication.

It's one listener per document. The only issue is that we don't have automatic cleanup. Unless someone is re-creating documents during app lifetime it should be fine.

oliviertassinari · 2019-04-29T13:16:51Z

Let's keep it for now and see if the focus visible is useful elsewhere.

I have edited my comment. It's useful. I remember somebody using a tooltip wrapping an input.

eps1lon · 2019-04-29T13:44:27Z

@ryancogswell Oh my god it's so beautiful (needs 60fps video):

oliviertassinari · 2019-04-29T13:48:10Z

😍

ryancogswell · 2019-04-29T14:11:52Z

packages/material-ui/src/ButtonBase/ButtonBase.js

      this.setState({ focusVisible: false });
    }
  });

  componentDidMount() {
-    listenForFocusKeys(ownerWindow(this.getButtonNode()));
-
+    prepareFocusVisible(this.getButtonNode().ownerDocument);


@eps1lon It does look like this is going to add a set of document listeners for each ButtonBase mount. Should be able to short-circuit within focusVisible.js prepare function by tracking in a variable whether or not the listeners have already been added to the document.

Should we handle it?

It does look like this is going to add a set of document listeners for each ButtonBase mount.

addEventListener does not add duplicate listeners. Unless the focusVisible.js module is recreated for every ButtonBase instance they shouldn't be duplicated.

Thanks for the education.

ryancogswell · 2019-04-29T14:23:32Z

We might want to add a regression test for the menu keyboard feature (I don't know if it's possible) in case we have to change the logic somewhere don't the line.

@oliviertassinari I'll take care of this later this week after this branch is merged. I tried to do this before -- intending to successfully verify it for the arrow keys and then have a failing test for the text navigation, but with the old focusVisible logic I was going to have to do some rather convoluted things to get a successful test for the arrow keys. It should be more straightforward after this branch is merged.

I also noticed an issue with the text navigation for Select when playing with the netlify deploy from this branch. SelectInput has disableListWrap: true. Currently I'm honoring disableListWrap for the text navigation, but I don't think I should. The native select behavior appears to disable wrapping for arrow keys, but wraps for text navigation. I'll address this at the same time as verifying the focusVisible logic in the MenuList tests.

oliviertassinari · 2019-04-29T21:07:02Z

@ryancogswell Thanks for the follow-up.

oliviertassinari · 2019-04-29T21:10:16Z

@eps1lon People report that Material-UI is slow, this change will greatly help. Really well done 🔥. Thank you for giving a fresh touch at this old logic!

[ButtonBase] Use fork of focus-visible polyfill

2df809e

eps1lon added accessibility a11y component: ButtonBase The React component. labels Apr 25, 2019

eps1lon commented Apr 25, 2019

View reviewed changes

packages/material-ui/src/ButtonBase/ButtonBase.js Show resolved Hide resolved

eps1lon commented Apr 25, 2019

View reviewed changes

packages/material-ui/src/ButtonBase/ButtonBase.js Outdated Show resolved Hide resolved

eps1lon commented Apr 25, 2019

View reviewed changes

packages/material-ui/src/ButtonBase/focusVisible.js Show resolved Hide resolved

ryancogswell mentioned this pull request Apr 25, 2019

Select menu does not fully implement keyboard controls #8191

Closed

1 task

oliviertassinari reviewed Apr 25, 2019

View reviewed changes

ryancogswell mentioned this pull request Apr 26, 2019

[MenuList] Add text keyboard focus navigation #15495

Merged

1 task

oliviertassinari force-pushed the next branch from ad52812 to 5a768d7 Compare April 28, 2019 10:54

ryancogswell reviewed Apr 28, 2019

View reviewed changes

eps1lon added 6 commits April 29, 2019 13:14

Add listeners to owner document

7e14cd4

leave imperative focus visible untouched

a19306f

handleBlur -> handleBlurVisible

7898a80

Make it clearer when handleBlur should be called

Use :focus-visible selector if available

6199ab1

Remove instanceof error checks

e4a1928

Forgot they won't work that well across windows

Fix tests

33e07e2

eps1lon force-pushed the fix/focus-visible branch from 70da954 to 33e07e2 Compare April 29, 2019 12:16

Fix shadowRoot test

6037fe3

enzyme find cant pierce shadow dom

oliviertassinari reviewed Apr 29, 2019

View reviewed changes

oliviertassinari approved these changes Apr 29, 2019

View reviewed changes

Merge branch 'next' into fix/focus-visible

7758875

oliviertassinari added the PR: good for merge label Apr 29, 2019

ryancogswell reviewed Apr 29, 2019

View reviewed changes

oliviertassinari merged commit 6ef44f1 into mui:next Apr 29, 2019

eps1lon deleted the fix/focus-visible branch April 30, 2019 18:57

ryancogswell mentioned this pull request May 2, 2019

[MenuList] Ignore disableListWrap for text focus navigation #15555

Merged

1 task

eps1lon mentioned this pull request May 6, 2019

ButtonBase ownerdocument error #15598

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ButtonBase] Use fork of focus-visible polyfill #15484

[ButtonBase] Use fork of focus-visible polyfill #15484

eps1lon commented Apr 25, 2019 •

edited

Loading

mui-pr-bot commented Apr 25, 2019 •

edited

Loading

ryancogswell commented Apr 25, 2019

oliviertassinari Apr 25, 2019

eps1lon Apr 25, 2019

eps1lon Apr 29, 2019

ryancogswell Apr 28, 2019

eps1lon Apr 29, 2019

oliviertassinari commented Apr 29, 2019 •

edited

Loading

oliviertassinari Apr 29, 2019 •

edited

Loading

eps1lon Apr 29, 2019

eps1lon commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

eps1lon commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

ryancogswell Apr 29, 2019

oliviertassinari Apr 29, 2019

eps1lon Apr 29, 2019

ryancogswell Apr 29, 2019

ryancogswell commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

[ButtonBase] Use fork of focus-visible polyfill #15484

[ButtonBase] Use fork of focus-visible polyfill #15484

Conversation

eps1lon commented Apr 25, 2019 • edited Loading

mui-pr-bot commented Apr 25, 2019 • edited Loading

ryancogswell commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 29, 2019 • edited Loading

oliviertassinari Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

eps1lon commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryancogswell commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

oliviertassinari commented Apr 29, 2019

eps1lon commented Apr 25, 2019 •

edited

Loading

mui-pr-bot commented Apr 25, 2019 •

edited

Loading

oliviertassinari commented Apr 29, 2019 •

edited

Loading

oliviertassinari Apr 29, 2019 •

edited

Loading