Material-ui: Selected button breathing effect on Chrome

[iaarnio]

• [X] The issue is present in the latest release.
• [X] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Selected button has weird breathing effect. This happens on Chrome (on MacOS).
It will becomes visible when browser is switched to background application and then back again (i.e. when it gets focus again).

Expected Behavior 🤔

Button should be visible without breathing effect. The same way it is when Chrome has focus all the time and is not switched to background.

Steps to Reproduce 🕹

Steps:

1. Open webpage like https://material-ui.com/components/buttons/ using Chrome (and MacOS)
2. Press e.g. one of the example contained buttons (e.g. 'Primary')
3. Switch focus away from Chrome (typically cmd-tab)
4. Switch focus back to Chrome (typically cmd-tab)

The lighter color bubble also expands and shrinks like inhale and outhale of breathing:

Your Environment 🌎

MacOS Mojave 10.14.6 (18G87)
Chrome Version 76.0.3809.100 (Official Build) (64-bit)

I cannot reproduce the same using Safari or Firefox so could be also browser issue.

[mbrookes]

In Chrome, the last clicked button gets keyboard focus (you can tab to the previous or next button), but the keyboard focus indicator isn't shown until you leave and return to the page.

In Firefox, clicking a button does not focus it, but keyboard focus works as expected when used separately.

[oliviertassinari]

@iaarnio Yeap, I think that they would agree with you: https://github.com/WICG/focus-visible/issues/167. I would propose the same fix here. What do you think of?

diff --git a/packages/material-ui/src/utils/focusVisible.js b/packages/material-ui/src/utils/focusVisible.js
index f20ddb884..e5d860f0d 100644
--- a/packages/material-ui/src/utils/focusVisible.js
+++ b/packages/material-ui/src/utils/focusVisible.js
@@ -47,7 +47,18 @@ function focusTriggersKeyboardModality(node) {
   return false;
 }

-function handleKeyDown() {
+/**
+ * Keep track of our keyboard modality state with `hadKeyboardEvent`.
+ * If the most recent user interaction was via the keyboard;
+ * and the key press did not include a meta, alt/option, or control key;
+ * then the modality is keyboard. Otherwise, the modality is not keyboard.
+ * @param {KeyboardEvent} event
+ */
+function handleKeyDown(event) {
+  if (event.metaKey || event.altKey || event.ctrlKey) {
+    return;
+  }
+
   hadKeyboardEvent = true;
 }

@@ -57,7 +68,6 @@ function handleKeyDown() {
  * This avoids the situation where a user presses a key on an already focused
  * element, and then clicks on a different element, focusing it with a
  * pointing device, while we still think we're in keyboard modality.
- * @param {Event} e
  */
 function handlePointerDown() {
   hadKeyboardEvent = false;
@@ -119,7 +129,6 @@ function handleBlurVisible() {
   window.clearTimeout(hadFocusVisibleRecentlyTimeout);
   hadFocusVisibleRecentlyTimeout = window.setTimeout(() => {
     hadFocusVisibleRecently = false;
-    window.clearTimeout(hadFocusVisibleRecentlyTimeout);
   }, 100);
 }

Do you want to submit a pull request?

@mbrookes Did you try on Edge?

[eps1lon]

In Firefox, clicking a button does not focus it, but keyboard focus works as expected when used separately.

That sounds like a bug in firefox. I cannot reproduce this on firefox 68.0.1 (64-bit) ubuntu 19.04. With "focus" do you mean :focus i.e. document.activeElement or :focus-visible?

That being said with experimental web features enabled in chrome I get no focus-visible (native or polyfilled) when switching tabs/windows after click. If we make any change it should be made after https://github.com/WICG/focus-visible/issues/167 is resolved. Any discussion should happen there.

[oliviertassinari]

I have seen that https://github.com/WICG/focus-visible/pull/184 was merged recently (after our last update of the logic on Material-UI side),

[mbrookes]

With "focus" do you mean ...

I'd forgotten that our corporate build forces us onto 60.8.0esr, so not a valid test.

And Safari is behaving correctly today. 🤷‍♂

[adeelibr]

Can I work on this PR kindly.

[oliviertassinari]

No objection from my part. It's slightly opinionated given the :focus-visible spec doesn't account for this behavior. But at the same time, the Chrome team seems to be in favor of it, at least, one of their accessibility advocates, Rob did a pull request in this direction.

@robdodson Would you recommend Material-UI to apply https://github.com/WICG/focus-visible/pull/184 in its codebase? Thanks.

[jayesh-kaza]

@adeelibr are you working on this?

[adeelibr]

Yes, i am just waiting on the go ahead from @robdodson

[oliviertassinari]

I'm personally in favor of moving forward because 1. on Chrome, if you click a button (it gives it the focus), if you cmd+tab to move away and come back, you won't get the focus outline. 2. We can easily revert based on https://github.com/WICG/focus-visible/issues/167 outcome.

[adeelibr]

okay, picking this ticket up then 👍