-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix: improved accessibility in high contrast mode #1941
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
Conversation
@@ -79,6 +80,10 @@ | |||
border-radius: inherit; | |||
pointer-events: none; | |||
opacity: 0; | |||
|
|||
@include md-high-contrast { | |||
background-color: rgba(white, 0.5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be an issue with dark themes (where the text is white)?
(same for other components where you use a white background)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably would, but I only went for the dark modes in this PR since supporting all modes would add more bloat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make these styles part of the theme, they can simply use the theme's background and foreground palettes to draw an appropriate color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would look something like
// in _button-theme.scss
@include md-high-contrast {
background-color: if($theme.is-dark, white, black);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, although I had to flip the conditions since the colors are inverted. Might have to re-test this after we have dark themes implemented.
// Add an outline to make it more visible in high contrast mode. | ||
@include md-high-contrast { | ||
[md-button], [md-raised-button], [md-icon-button], [md-fab], [md-mini-fab] { | ||
border: solid 1px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why border
and not outline
, since the latter won't affect layout?
(here and elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I hadn't though about it. I'll give it a try and see how it works out. One nice side effect of using the border is that we don't have to specify the color, it inherits from the text color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do the same for outline using currentColor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is acceptable for high contrast mode.
d95ddc0
to
1206867
Compare
Switched over all of the |
Slide-toggle colors also be affected by the theme? Dark themes should already be working; you can see an example on the sample app (toggle is on the top right). |
I guess, although it seems like IE behaves the same both with the dark and the light theme. It doesn't try to invert colors. |
@crisbeto in that case it seems fine, then |
LGTM, just needs rebase |
I'll rebase it, but it probably makes sense to revert the light/dark distinction for the button overlay. |
That's fine if IE renders it the same either way |
I mean IE renders the light and dark theme the same. In that case we should also use the same overlay color. |
a11y improvements to the following components in high contrast mode: * `button` - Adds an outline since only the text was visible. Also inverts the tint since it was hiding the text. * `card`, `dialog`, `menu`, `snack-bar`, `tooltip` - Adds an outline. * `select` - Adds an outline and prevents the ripple from hiding the option text. * `sidenav` - Fixes the backdrop not being opaque. * `slide-toggle` - Fixes component blending in completely with the background. Fixes angular#421. Fixes angular#1769.
3486e30
to
5954d42
Compare
@crisbeto yeah, I meant that if IE treats light and dark the same way then it's fine to go back to how you originally had it. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
a11y improvements to the following components in high contrast mode:
button
- Adds an outline since only the text was visible. Also inverts the tint since it was hiding the text.card
,dialog
,menu
,snack-bar
,tooltip
- Adds an outline.select
- Adds an outline and prevents the ripple from hiding the option text.sidenav
- Fixes the backdrop not being opaque.slide-toggle
- Fixes component blending in completely with the background.Fixes #421.
Fixes #1769.