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

Change wpt tests for <meta name=color-scheme> to not depend on the color-scheme computed value (before whatwg/html#7226) #31268

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Oct 15, 2021

This makes the tests valid both with and without the spec change
proposed above. Will send a follow-up PR to test the spec change.

This makes the assumption that the initial browser's color-scheme is
light, which I think is a reasonable assumption to run these tests
under.

An alternative would have to allow both "light" and "dark" wherever we
check for "normal" now (which loses test coverage in practice), or I
could complicate the test to compute the initial color-scheme using an
iframe or what not (but that complicates everything more).

Copy link
Member

@lilles lilles left a comment

Choose a reason for hiding this comment

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

These tests are valid even without the spec pull request, right?

@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

These tests are valid even without the spec pull request, right?

No, because they assert that getComputedStyle(document.documentElement).colorScheme is normal. Otherwise, yes.

@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

@lilles I guess I could remove that assert and land it, if you want?

@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

(then add that assert in another PR)

@lilles
Copy link
Member

lilles commented Oct 15, 2021

@lilles I guess I could remove that assert and land it, if you want?

sgtm, what suits you best.

…lor-scheme computed value (before whatwg/html#7226)

This makes the tests valid both with and without the spec change
proposed above. Will send a follow-up PR to test the spec change.

This makes the assumption that the initial browser's color-scheme is
light, which I think is a reasonable assumption to run these tests
under.

An alternative would have to allow both "light" and "dark" wherever we
check for "normal" now (which loses test coverage in practice), or I
could complicate the test to compute the initial color-scheme using an
iframe or what not (but that complicates everything more).
@emilio emilio changed the title Change wpt tests for <meta name=color-scheme> as per https://github.com/whatwg/html/pull/7226 Change wpt tests for <meta name=color-scheme> to not depend on the color-scheme computed value (before whatwg/html#7226) Oct 15, 2021
@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

Alright, let's do that.

@emilio emilio enabled auto-merge (rebase) October 15, 2021 15:30
@emilio emilio merged commit 705c808 into web-platform-tests:master Oct 15, 2021
@emilio emilio deleted the color-scheme branch October 15, 2021 15:38
@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

Sent the follow-up in #31268.

@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

Err, wrong tab-complete :) #31271

@tabatkins
Copy link
Contributor

This makes the assumption that the initial browser's color-scheme is
light, which I think is a reasonable assumption to run these tests
under.

This is reasonable, but it should be documented in the test as an assumption, so anyone producing a default-dark browser can tell why they're failing the test and that the test should be fixed, rather than their browser. ^_^

@emilio
Copy link
Contributor Author

emilio commented Oct 15, 2021

This is reasonable, but it should be documented in the test as an assumption, so anyone producing a default-dark browser can tell why they're failing the test and that the test should be fixed, rather than their browser. ^_^

#31274

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.

5 participants