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

[css-ui] expect resolved color in inheritance test #13748

Merged
merged 2 commits into from
Jan 11, 2019

Conversation

ewilligers
Copy link
Contributor

@ewilligers ewilligers commented Oct 28, 2018

getComputedStyle returns the resolved color or 'invert', not 'auto'
https://drafts.csswg.org/cssom/#resolved-value

@frivoal
Copy link
Contributor

frivoal commented Nov 1, 2018

For the invert color of the outline-color property, you should check if the invert keyword is supported at all. If it is, it should be the initial value, but if it is not, having currentColor be the initial value is spec compliant.

frivoal
frivoal previously requested changes Nov 1, 2018
Copy link
Contributor

@frivoal frivoal left a comment

Choose a reason for hiding this comment

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

Looks good, except for the comment I made in #13748 (comment).

Once that's fixed, this should be merged.

@ewilligers
Copy link
Contributor Author

outline-color test now checks if 'invert' is supported.

Blink/Firefox/Safari pass the outline-color test by not supporting 'invert', and returning the current rgb() color.

Edge fails the outline-color test by accepting 'invert' but returning 'invert' from getComputedStyle() instead of the inverted background color.

@frivoal
Copy link
Contributor

frivoal commented Nov 12, 2018

Edge fails the outline-color test by [...] returning 'invert' from getComputedStyle() instead of the inverted background color.

That's actually a pass. returning the inverted background color isn't actually possible, since background may not be a single color. returning 'invert' as a keyword is the right thing to do.

@svgeesus
Copy link
Contributor

That's actually a pass. returning the inverted background color isn't actually possible, since background may not be a single color. returning 'invert' as a keyword is the right thing to do.

Does that mean the UA should check whether the background is a single, opaque solid color (and if so, return that) or should it always return invert?

@frivoal
Copy link
Contributor

frivoal commented Nov 14, 2018

Does that mean the UA should check whether the background is a single, opaque solid color (and if so, return that) or should it always return invert?

It should always return invert. Happy to go clarify that in level 4, but that's the intention behind level 3, and is what microsoft & presto implemented.

@svgeesus
Copy link
Contributor

@ewilligers waiting for your changes to address comments before we merge.

Copy link
Contributor

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

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

Look good except for the point already raised by Florian

getComputedStyle returns the resolved color, not 'invert' or 'auto'
https://drafts.csswg.org/cssom/#resolved-value

outline-color initial value test now passes in Chrome/Firefox/Safari;
they return the current rgb() color.
Fails Edge, which returns 'invert' instead of a rgb() color.
@ewilligers
Copy link
Contributor Author

Test updated, thank you for explaining.

@foolip
Copy link
Member

foolip commented Dec 20, 2018

This PR is blocked on the required "Travis CI - Pull Request" check after #14499. In order to trigger it, I will close and reopen this PR.

@foolip foolip closed this Dec 20, 2018
@foolip foolip reopened this Dec 20, 2018
@ewilligers ewilligers dismissed frivoal’s stale review December 21, 2018 02:46

Comments have been addressed. outline-color test passes in Edge.

@ewilligers
Copy link
Contributor Author

@frivoal's comments have been addressed.

@ewilligers
Copy link
Contributor Author

Ping. I believe this is ready for merging.

@frivoal frivoal merged commit 5d06797 into web-platform-tests:master Jan 11, 2019
@ewilligers ewilligers deleted the outline-color branch January 11, 2019 02:40
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.

7 participants