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 color valid color mix test #36668

Merged

Conversation

mysteryDate
Copy link
Contributor

color-mix values should not get resolved at specified value time. See: #34158 and w3c/csswg-drafts#7302 (comment)

This PR changes keeps all the cases in the current test, but does not expect the color mix to resolve, only to resolve the input colors as they normally would at specified value time (i.e. #ff00ff -> rgb(255, 0, 255)).

There are definitely some cases that are missing here: calc values, keyword colors, current color, light dark pairs and hex colors. I'll add those next.

@mysteryDate
Copy link
Contributor Author

@lilles

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.

This looks good to me.

I noticed in reviewing that the test assumes lab and oklab, lch and oklch have similar ranges which they don't. But that could be fixed in a separate PR.

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.

There are a lot of changes here, but generally lgtm. Firefox seems to have a pretty good impl in this area so if you could check if Firefox passes all, or if any failures are expected, before landing, that would make me more confident about these changes.

@mysteryDate
Copy link
Contributor Author

I actually based these changes on Firefox. It still fails most of them because they do not yet handle "none" and is doing some computed-value stuff to the percentages that's not in the spec. Otherwise it matches.

chromium-wpt-export-bot pushed a commit that referenced this pull request Nov 1, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
chromium-wpt-export-bot pushed a commit that referenced this pull request Nov 1, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
chromium-wpt-export-bot pushed a commit that referenced this pull request Nov 2, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 2, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
web-platform-tests/wpt#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3982135
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066497}
chromium-wpt-export-bot pushed a commit that referenced this pull request Nov 2, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3982135
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066497}
chromium-wpt-export-bot pushed a commit that referenced this pull request Nov 2, 2022
From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3982135
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066497}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 12, 2022
…n color mix function, a=testonly

Automatic update from web-platform-tests
Allow percentages to precede the color in color mix function

From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
web-platform-tests/wpt#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3982135
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066497}

--

wpt-commits: d530c994059de2aa42a6cc2c01fd0beee3ac37f8
wpt-pr: 36764
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 14, 2022
…n color mix function, a=testonly

Automatic update from web-platform-tests
Allow percentages to precede the color in color mix function

From https://csswg.sesse.net/css-color-5/#color-mix:
color-mix() = color-mix( <color-interpolation-method> , [ <color> &&
<percentage [0,100]>? ]#{2})

&& means the arguments can happen in any order.

I also forgot the "in" keyword in serialization. Also need to reject
percentage values greater than 100.

Contains the same changes to color-valid-color-mix-function.html as
web-platform-tests/wpt#36668

Bug: 1362022
Change-Id: I9da6b32ef79f1217594f82ca684d6bc9d1d22240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3982135
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Aaron Krajeski <aaronhk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1066497}

--

wpt-commits: d530c994059de2aa42a6cc2c01fd0beee3ac37f8
wpt-pr: 36764
@dbaron
Copy link
Member

dbaron commented Apr 26, 2023

Looks like this was basically ready to merge, but it now has conflicts. Is it worth fixing and merging (and maybe addressing @svgeesus 's comment while doing so)?

@svgeesus
Copy link
Contributor

Yes, it is worth re-basing to fix the conflicts.

@svgeesus
Copy link
Contributor

svgeesus commented May 1, 2023

@mysteryDate could you rebase this, to remove the conflicts?

@mysteryDate
Copy link
Contributor Author

I think in the future we should try have smaller test files to avoid issues like this. There are only a couple changes on main that I'm conflicting with (mostly issues of rounding to 127 vs 128) but it's show as a massive conflict.

@mysteryDate
Copy link
Contributor Author

They are resolved!

@mysteryDate
Copy link
Contributor Author

We're having an issue where for a bunch of tests chrome and safari return 127 for the blue channel, though the upstream changes to this file expect 128.

For hwb(320deg 30% 40%) using the algorithm hwbToRgb as defined in this document, the result is:
[0.5999999999999999, 0.3, 0.5]

So the issue is that chrome and safari are evaluating 0.5 to an integer in the range [0,255] as 127, whereas the test expects 128. Which one is correct? Should the test accept either. This test is already doing a lot, we should probably test integer rounding somewhere else.

@svgeesus
Copy link
Contributor

svgeesus commented May 4, 2023

I think in the future we should try have smaller test files to avoid issues like this

That was how things were (small, focused tests for specific, small specification sections) until there was a massive rewrite which consolidated things into one-test-per-property style.

So the issue is that chrome and safari are evaluating 0.5 to an integer in the range [0,255] as 127, whereas the test expects 128. Which one is correct? Should the test accept either. This test is already doing a lot, we should probably test integer rounding somewhere else.

From CSS Color 4 5.1. The RGB functions: rgb() and rgba() which in turn references CSS Values 4 on rounding:

These values come from the fact that many graphics engines store the color channels internally as a single byte, which can hold integers between 0 and 255. Implementations should honor the precision of the channel as authored or calculated wherever possible. If this is not possible, the channel should be rounded towards +∞.

a = 0.5 * 255
127.5
b = Math.round(a)
128 

From round to the nearest integer:

Unless otherwise specified, in the CSS specifications rounding to the nearest integer requires rounding in the direction of +∞ when the fractional portion is exactly 0.5. (For example, 1.5 rounds to 2, while -1.5 rounds to -1.)

@svgeesus
Copy link
Contributor

svgeesus commented May 4, 2023

we should probably test integer rounding somewhere else.

It should probably be tested more generally in css/css-values, but here it is testing a specific reference from CSS Color 4.

@svgeesus
Copy link
Contributor

svgeesus commented May 4, 2023

I don't know how to get past the failing sink tasks which are blocking merging.

@dbaron
Copy link
Member

dbaron commented May 4, 2023

Closing and reopening to re-trigger running of checks, since it looks like the underlying error was:

requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: https://hg.mozilla.org/mozilla-central/archive/tip.zip/testing/profiles/

@dbaron dbaron closed this May 4, 2023
@dbaron dbaron reopened this May 4, 2023
@mysteryDate
Copy link
Contributor Author

Okay, I've set this to round to 128 and not 127, I'll investigate why chrome is doing the wrong thing here.

@mysteryDate
Copy link
Contributor Author

It should probably be tested more generally in css/css-values, but here it is testing a specific reference from CSS Color 4.

Isn't it just testing how "0.5 * 255" is rounded when converting hwb(320deg 30% 40%) to rgb?
https://jsfiddle.net/6yfqnLhs/

@mysteryDate
Copy link
Contributor Author

Isn't it just testing how "0.5 * 255" is rounded when converting hwb(320deg 30% 40%) to rgb?
https://jsfiddle.net/6yfqnLhs/

I'm adding a simple parsing test to check this here:
https://chromium-review.googlesource.com/c/chromium/src/+/4507865

That was how things were (small, focused tests for specific, small specification sections) until there was a #36353 which consolidated things into one-test-per-property style.

It would be my instinct to call this test something like hwb-float-to-integer-rounding.html but to reflect the structure as is, I just added it to color-valid-hwb.html, at least with a comment explaining the intent of the test.

@svgeesus svgeesus merged commit 3ad9978 into web-platform-tests:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants