-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(select): Remove blue background in IE on focus #3497
Conversation
🤖 Beep boop! Screenshot test report 🚦5 screenshots changed from Details |
🤖 Beep boop! Screenshot test report 🚦5 screenshots changed from Details |
🤖 Beep boop! Screenshot test report 🚦5 screenshots changed from Details |
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'm a little confused that we seem to be adding a new screenshot test page that is identical to an existing page aside from indentation and one comment...
<main class="test-viewport test-viewport--mobile"> | ||
<div class="test-layout"> | ||
|
||
<!-- This tests that select suppresses IE's default blue background on focus --> |
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 don't think we need a new screenshot page for this. We can add these comments to the 3230.html page stating that the first select is verifying both issues. Maybe rename the file 3230_3496.html
?
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
The disabled page diff in IE 11 💩 should be fixed by #3513. |
Why is there a diff in IE and not Edge? Shouldn't these changes affect both? Or does it have to do with the browser's default styles? |
@@ -66,6 +66,11 @@ | |||
display: none; | |||
} | |||
|
|||
&::-ms-value { | |||
background-color: transparent; | |||
color: inherit; |
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.
Would replacing this with @include mdc-select-ink-color($mdc-select-ink-color);
obviate the need for #3513?
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 we still need both PRs.
mdc-select-ink-color()
specifically excludes --disabled
, which is why the browser's default font color was getting used.
We either need to update mdc-select-ink-color()
so that it includes --disabled
, or set a disabled-specific color
.
It seems like the simplest thing is to set the color once at a high level, and have all children just inherit
it.
It's due to the browser's default styles. Earlier versions of Edge had the same problem, but they fixed it around version 15 or 16 I think. |
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.
LGTM
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 this might need to be updated from master... but also, if this is a fix for IE, then why are there updated goldens for all browsers?
Because |
🤖 Beep boop! Screenshot test report 🚦5 screenshots changed from Details |
Fixes #3496