[Button] Fix visual focus state according to spec #13134

eps1lon · 2018-10-07T13:11:45Z

Breaking change

Button has no focus ripple by default

- <Button />
+ <Button focusRipple />

Deprecation

deprecate disableFocusRipple

Changes

Buttons have the "correct" visual focus state
- button spec (scroll to states)
- state spec
deprecatedPropType does not cache warnings in test environment

Button demos in latest vs. this pr

Some variations on this PR:

don't deprecate disableFocusRipple (although it adds quite a bit of complexity to a non-standard behavior)
remove focus ripple immediately (should at least add an example with focus ripple that is doable in userland)

Future work:

Implement https://material.io/design/interaction/states.html in the theme. From what I gathered e.g. hover does not add opacity but an overlay to the button which has its own color and opacity.

oliviertassinari · 2018-10-07T16:02:08Z

package.json

@@ -125,6 +125,7 @@
    "nyc": "^13.0.0",
    "postcss": "^7.0.0",
    "prettier": "^1.8.2",
+    "prop-types-extra": "^1.1.0",


This package isn't bundle size friendly, for instance:
https://github.com/mui-org/material-ui/blob/965776002fcf482b65d66040393cf75caa18d356/packages/material-ui/src/utils/exactProp.js#L9-L12. What do you think of inlining it as we did in v0.x?

@oliviertassinari
I didn't really look into but isn't the hole propTypes object removed in production i.e. it doesn't matter how many dependencies propTypes pulls in because they will be stripped?

The imports aren't striped, at least not in the wrap mode.

oliviertassinari · 2018-10-07T16:03:46Z

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

-  disableRipple: PropTypes.bool,
+  disableRipple: deprecated(
+    PropTypes.bool,
+    'Focus ripple will be removed because it does not follow Material design.',


I don't understand. We do want to keep the ripple. It's the pulsating focusing ripple we want to remove no?

You're right. That should be at disableFocusRipple.

oliviertassinari · 2018-10-07T16:04:52Z

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

@@ -365,7 +387,7 @@ Button.defaultProps = {
  color: 'default',
  component: 'button',
  disabled: false,
-  disableFocusRipple: false,
+  disableFocusRipple: true,


Should we wait for a minor or can it be released in a patch? I would vote for a minor.

Probably any choice would be wrong. It's a fix if you want strict material design but breaking if you rely on a focus ripple for some reason. Minor sounds like a good compromise

mbrookes · 2018-10-07T18:24:05Z

Something doesn't look right with the API docs:

eps1lon · 2018-10-07T20:31:16Z

@mbrookes Should be fixed now. Matching of deprecated props is done on text only which did not support whitespace.

mbrookes · 2018-10-07T22:14:39Z

pages/api/button.md

@@ -24,7 +24,7 @@ import Button from '@material-ui/core/Button';
 | <span class="prop-name">color</span> | <span class="prop-type">enum:&nbsp;'default'&nbsp;&#124;<br>&nbsp;'inherit'&nbsp;&#124;<br>&nbsp;'primary'&nbsp;&#124;<br>&nbsp;'secondary'<br> | <span class="prop-default">'default'</span> | The color of the component. It supports those theme colors that make sense for this component. |
 | <span class="prop-name">component</span> | <span class="prop-type">union:&nbsp;string&nbsp;&#124;<br>&nbsp;func&nbsp;&#124;<br>&nbsp;object<br> | <span class="prop-default">'button'</span> | The component used for the root node. Either a string to use a DOM element or a component. |
 | <span class="prop-name">disabled</span> | <span class="prop-type">bool | <span class="prop-default">false</span> | If `true`, the button will be disabled. |
-| <span class="prop-name">disableFocusRipple</span> | <span class="prop-type">bool | <span class="prop-default">false</span> | If `true`, the  keyboard focus ripple will be disabled. `disableRipple` must also be true. |
+| ~~<span class="prop-name">disableFocusRipple</span>~~ | <span class="prop-type">bool | <span class="prop-default">true</span> | *Deprecated*. Focus ripple will be removed because it does not follow Material design.<br><br>If `true`, the  keyboard focus ripple will be disabled. `disableRipple` must also be true. |


Perhaps I'm being over sensitive, but it isn't that it doesn't follow Material Design, so much that the spec has changed since this component was written. (For the worse IMHO. And not saying I'm a big fan of the focus ripple, but the subtle shade change is not enough to distinguish which component is focused).

@mbrookes I would agree on that with contained and fab buttons. One of the issues here is that hover and focus states are not correct in the first place since the actual button should always be opaque.

Maybe we should rework this and lighten or darken those states instead of adding opacity. That might improve focus visibility.

And text.

Put a text button next to an outlined button, and focus the text button. To the uninitiated, which one is focused? The focused text button looks like a (lightly) filled button, with neither focused.

While the focus ripple might be unattractive, it's visually distinctive, and draws attention to the focused element (much as the native browser highlight does). I guess in the google employee churn, that critical requirement got lost, and the v2 focus state is the result.

Not really a fair comparison to put an outlined button next to a text button. I will work on it again and see how it looks without opacity.

Not really a fair comparison to put an outlined button next to a text button.

???

eps1lon · 2019-01-15T13:34:04Z

Test is passing locally and failing with different callcounts on CI in test_browsers and test_unit. Seems like spying on console.error doesn't integrate well with mocha

oliviertassinari · 2019-03-29T18:01:25Z

What's the status of this pull request?

eps1lon · 2019-04-03T09:12:58Z

I'll need to look into that again. It doesn't have that high of a priority since we deprioritized spec changes and this likely needs a bigger picture approach i.e. implement states according to spec.

I would like to leave this open if somebody wants to pick this up.

oliviertassinari · 2019-04-03T09:21:34Z

@eps1lon Do we move the effort to v5?

eps1lon · 2019-04-03T09:34:16Z

@eps1lon Do we move the effort to v5?

Probably. Maybe we can move this into a feature release since matching the spec is more of a bug fix but we'll see when we get there.

eps1lon · 2019-07-03T10:03:04Z

Revisiting this once work for implementing States begins.

eps1lon added component: button This is the name of the generic UI component, not the React module! deprecation New deprecation message labels Oct 7, 2018

eps1lon force-pushed the fix/core/Button/visual-focused-state branch 3 times, most recently from 9cab252 to 248cdde Compare October 7, 2018 13:35

oliviertassinari reviewed Oct 7, 2018

View reviewed changes

eps1lon force-pushed the fix/core/Button/visual-focused-state branch from 0bd2389 to 2673e9f Compare October 7, 2018 17:25

eps1lon added the design: material This is about Material Design, please involve a visual or UX designer in the process label Oct 7, 2018

mbrookes reviewed Oct 7, 2018

View reviewed changes

eps1lon added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 8, 2018

eps1lon added 6 commits October 22, 2018 21:40

[Button] Fix visual focus state according to spec

73e71b4

Test deprecatedProp

570a3fd

[Button] focusRipple is deprecated not ripple

d821675

[docs] Fix deprecated props not recognized on multiline

f62402f

[Button] Improve (disable)FocusRipple logic

ed1ba33

Merge branch 'master' into fix/core/Button/visual-focused-state

d8f5f45

eps1lon force-pushed the fix/core/Button/visual-focused-state branch from aa75f05 to d8f5f45 Compare January 15, 2019 12:17

eps1lon added breaking change and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 15, 2019

[Button] Fix failing test

b1a121a

eps1lon added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 3, 2019

eps1lon added the low priority label Apr 3, 2019

oliviertassinari force-pushed the master branch from 10b9d6a to 89687f3 Compare May 23, 2019 15:26

eps1lon closed this Jul 3, 2019

eps1lon mentioned this pull request Sep 23, 2019

[Button] Increase elevation on hover when contained #17537

Merged

eps1lon deleted the fix/core/Button/visual-focused-state branch September 14, 2021 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Fix visual focus state according to spec #13134

[Button] Fix visual focus state according to spec #13134

eps1lon commented Oct 7, 2018 •

edited

Loading

oliviertassinari Oct 7, 2018

eps1lon Oct 7, 2018 •

edited

Loading

oliviertassinari Oct 8, 2018 •

edited

Loading

oliviertassinari Oct 7, 2018

eps1lon Oct 7, 2018

oliviertassinari Oct 7, 2018

eps1lon Oct 7, 2018

mbrookes commented Oct 7, 2018

eps1lon commented Oct 7, 2018

mbrookes Oct 7, 2018

eps1lon Oct 7, 2018

mbrookes Oct 8, 2018

eps1lon Oct 8, 2018

mbrookes Oct 8, 2018

eps1lon commented Jan 15, 2019

oliviertassinari commented Mar 29, 2019

eps1lon commented Apr 3, 2019

oliviertassinari commented Apr 3, 2019

eps1lon commented Apr 3, 2019

eps1lon commented Jul 3, 2019

[Button] Fix visual focus state according to spec #13134

[Button] Fix visual focus state according to spec #13134

Conversation

eps1lon commented Oct 7, 2018 • edited Loading

Breaking change

Deprecation

Changes

Future work:

Choose a reason for hiding this comment

eps1lon Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 8, 2018 • edited Loading

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

mbrookes commented Oct 7, 2018

eps1lon commented Oct 7, 2018

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

eps1lon commented Jan 15, 2019

oliviertassinari commented Mar 29, 2019

eps1lon commented Apr 3, 2019

oliviertassinari commented Apr 3, 2019

eps1lon commented Apr 3, 2019

eps1lon commented Jul 3, 2019

eps1lon commented Oct 7, 2018 •

edited

Loading

eps1lon Oct 7, 2018 •

edited

Loading

oliviertassinari Oct 8, 2018 •

edited

Loading