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

fix(select): Remove blue background in IE on focus #3497

Merged
merged 13 commits into from
Sep 5, 2018

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Sep 2, 2018

Fixes #3496

@mdc-web-bot
Copy link
Collaborator

@mdc-web-bot
Copy link
Collaborator

@mdc-web-bot
Copy link
Collaborator

Copy link
Contributor

@kfranqueiro kfranqueiro left a 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 -->
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@acdvorak
Copy link
Contributor Author

acdvorak commented Sep 5, 2018

The disabled page diff in IE 11 💩 should be fixed by #3513.

@kfranqueiro
Copy link
Contributor

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@acdvorak
Copy link
Contributor Author

acdvorak commented Sep 5, 2018

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.

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kfranqueiro kfranqueiro left a 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?

@williamernest
Copy link
Contributor

Because mdc-typography was added to the body.

@mdc-web-bot
Copy link
Collaborator

@acdvorak acdvorak merged commit 1eb86cc into master Sep 5, 2018
@acdvorak acdvorak deleted the fix/select/ie-highlight branch September 5, 2018 22:11
adrianschmidt pushed a commit to Lundalogik/material-components-web that referenced this pull request Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants