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

[Select] Improve visual distinction between selected and focus-visible #16331

Closed
wants to merge 7 commits into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 22, 2019

Adds theme.palette.action.focusVisible and theme.palette.action.focusVisibleOpacity which are used in ListItem and rippleVisible classes.

It has increased opacity compared to the selected state following https://material.io/design/interaction/states.html#anatomy. This introduces a minor color regression which I would consider a bugfix.

This is not considered a complete solution which requires a more holistic approach implementing https://material.io/design/interaction/states.html. I'll draft a proposal next week and post it in #5186

Visual diffs are expected. Waiting for someone else to approve.

@eps1lon eps1lon added accessibility a11y component: select This is the name of the generic UI component, not the React module! labels Jun 22, 2019
@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@eps1lon eps1lon marked this pull request as ready for review June 22, 2019 10:37
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 22, 2019

Details of bundle changes.

Comparing: eade31e...9bd33d3

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.06% 🔺 +0.05% 🔺 320,509 320,690 88,449 88,494
@material-ui/core/Paper +0.06% 🔺 +0.07% 🔺 68,278 68,319 20,363 20,377
@material-ui/core/Paper.esm +0.07% 🔺 +0.08% 🔺 61,573 61,614 19,161 19,176
@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.26% 🔺 +0.21% 🔺 16,009 16,050 5,791 5,803
@material-ui/core/useMediaQuery 0.00% 0.00% 2,597 2,597 1,102 1,102
@material-ui/lab +0.03% 🔺 +0.04% 🔺 140,344 140,389 43,471 43,488
@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.05% 🔺 +0.06% 🔺 84,430 84,475 25,758 25,774
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.05% 🔺 +0.06% 🔺 74,752 74,793 23,240 23,254
colorManipulator 0.00% 0.00% 3,904 3,904 1,544 1,544
docs.landing 0.00% 0.00% 55,119 55,119 13,941 13,941
docs.main +0.03% 🔺 +0.03% 🔺 649,042 649,225 204,800 204,862
packages/material-ui/build/umd/material-ui.production.min.js +0.05% 🔺 +0.06% 🔺 293,672 293,824 84,362 84,413

Generated by 🚫 dangerJS against 9bd33d3

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

https://material.io/design/interaction/states.html: Since when does a tab item have a hover style? They keep changing the specification! That's an interesting new documentation page for me, I have never seen.


Regarding the pull request, I have seen the following issues:


Regarding the "Why?" question I was asking. My bias was the following. Most sequences of interactions are keyboard or mouse only, so why supporting both? But the specification specifically asks for it now, so ok, why not.


Could you rebase on the focus fix: #16323? I have a hard time triggering the focus style without it :)

@eps1lon
Copy link
Member Author

eps1lon commented Jun 22, 2019

The focus opacity rate should be 12% (not 30%).

That is what I meant with holistic approach. None of the values follow the spec right now. Focus is higher than selected and since selected is already 0.2 we shouldn't follow 0.12 for focused.

The hover style should have precedence over the focus style (not the opposite).

How did you arrive at this conclusion? Material guidelines specifically state they should be additive (no point in addressing this here).

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 22, 2019

That is what I meant with holistic approach.

At the same time, I don't think that we should allow the opacity to go above a threshold, it can become "ugly" (too dark). Would it work by reducing the selected opacity?

How did you arrive at this conclusion?

I was looking at the MCW implementation. However, YouTube implements an additive strategy. Is this a breaking change for overriding the styles?
On the same topic, will implementing the additive logic make override significantly harder? If they do, we might want to consider to not support it.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 22, 2019

At the same time, I don't think that we should allow the opacity to go above a threshold

Maybe. Could you elaborate why this would be a bad thing? I want to reduce the possible breaking changes. I can close this and work on a complete overhaul of the states if that is more to your personal liking.

On the same topic, will implementing the additive logic make override significantly harder? If they do, we might want to consider to not support it.

Perhaps. Again this is not something we can handwave away but have to carefully examine. The focus here is to create a minimal working difference between selected and focused state.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 22, 2019

I want to reduce the possible breaking changes

Can we agree on the fact that a style change is breaking if it breaks somebody overrides? This pull request increases the specificity of a pseudo-class, is that breaking?

If we release this change, people will try (issues/prs) to change the values: (is the contrast OK here?, I can't name it but something is off)

Capture d’écran 2019-06-22 à 15 58 40

The change I was proposing don't follow the specification but help to make a difference with the already selected value with minimal changes. It could be an intermediary step before a revamp of what we have, maybe for v5?

this is not something we can handwave away

I agree, as much as not following the spec is not an option to handwave away if it can make things simpler. People value the good looking, functionnal aspect of the components, we should optimize for that first.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 23, 2019

I have sent two different proposals.

  • n°1 and n°2: I have changed the select color to match the specification:
    Capture d’écran 2019-06-23 à 20 11 32
  • n°1: I have made the hover & focus-visible state has the same UI. We can challenge the need for these two states to have a different UI. I assume that most people use the mouse or the keyboard exclusively, rarely the two at the same time. GitHub dropdown menu uses this very strategy. Not supporting the composition as suggested by the Material Design make thing simpler, it's a net advantage for overrides.
    menu states
  • n°2: However, proposal n°1 suffers from one flaw. The focus state might not be contrasted enough. We can help slightly visually impaired users without requiring them to use a screen reader, the solution: a focus-ring.
    menu 2

@eps1lon eps1lon closed this Jun 23, 2019
@eps1lon eps1lon deleted the test/select-visual-states branch June 23, 2019 18:34
@oliviertassinari
Copy link
Member

@eps1lon Do you have a preference on the direction we should take? I see limitation in all the direction we can go into. While this pull request is great for iterating around different ideas, maybe we could expose the different options available under #5186 and weight the pros and cons of each of them?

@eps1lon
Copy link
Member Author

eps1lon commented Jun 23, 2019

I want to find consensus on a general direction first (proposal tomorrow). We're wasting a lot of time on minor implementation details.

@oliviertassinari
Copy link
Member

This sounds like a great idea! As long as it helps to refine the final solution, any step back is a step forward :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants