Add rgb() parsing tests that include non-finite inputs #369

mysteryDate · 2023-06-28T15:23:20Z

Test List

css/css-color/parsing/color-computed-rgb.html
css/css-color/parsing/color-valid-rgb.html

Rationale

See the discussion here: w3c/csswg-drafts#8629

Since the inputs to rgb() include <percentage> and <number> just like the new color functions, they should process non-finite values as well. Since the color channels are bounded to [0, 255], calc(-infinity) should become 0 at computed value time and calc(infinity) should become 255. calc(NaN) should become zero, like with all colors:
https://www.w3.org/TR/css-color-4/#rgb-functions

We should add tests to the two files above along the lines of:
test_valid_value("color", "rgb(calc(infinity), 0, 0)", "rgb(calc(infinity), 0, 0)"); to color-valid-rgb.html and
["rgb(calc(infinity), 0, 0)", "rgb(255, 0, 0)", "Red channel resolves positive infinity to 255"] to color-computed-rgb.html

The text was updated successfully, but these errors were encountered:

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3

nairnandu · 2023-07-05T14:54:11Z

@jgraham / @zcorpan please review for Gecko
@gsnedders / @nt1m please review for WebKit

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3

nt1m · 2023-07-06T07:48:18Z

I don't really see the benefit of adding this, since it's an unlikely use case, but why not.

zcorpan · 2023-07-06T13:30:52Z

@emilio

emilio · 2023-07-06T14:58:42Z

So legacy rgb at least always clamps eagerly during parsing doesn't it? Why would calc(infinity) be different?

document.body.style.color = "rgb(500, 0, 0)"
document.body.style.color // "rgb(255, 0, 0)"

Not saying we shouldn't add these though, but feels rather inconsistent? Or are you suggesting:

document.body.style.color = "rgb(calc(infinity), 0, 0)"
document.body.style.color // "rgb(255, 0, 0)"

Which afaict Chrome and Firefox do now? Or are you suggesting to change the out-of-range handling too?

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3

Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598329 Reviewed-by: Florin Malita <[email protected]> Reviewed-by: Steinar H Gunderson <[email protected]> Commit-Queue: Aaron Krajeski <[email protected]> Cr-Commit-Position: refs/heads/main@{#1166616}

jgraham · 2023-07-07T08:25:26Z

Given that there seems to be substantial ongoing discussion about the desired behaviour in these cases, I'd suggest that perhaps the tests should be held back from Interop 2023. Although it does seem like a case we'll inevitably run into eventually, it doesn't seem like it's a core requirement for most uses of this feature, or evidence that it's a current web compat hazard, and so we don't need to fast track interop changes.

mysteryDate · 2023-07-07T08:44:21Z

So legacy rgb at least always clamps eagerly during parsing doesn't it? Why would calc(infinity) be different?
document.body.style.color = "rgb(500, 0, 0)"
document.body.style.color // "rgb(255, 0, 0)"
Not saying we shouldn't add these though, but feels rather inconsistent? Or are you suggesting:
document.body.style.color = "rgb(calc(infinity), 0, 0)"
document.body.style.color // "rgb(255, 0, 0)"
Which afaict Chrome and Firefox do now? Or are you suggesting to change the out-of-range handling too?

This makes a lot of sense. This would mirror what's currently happening for lightness in lab()/oklab():
https://github.com/web-platform-tests/wpt/blob/master/css/css-color/parsing/color-valid-lab.html#L42

So then, the only actual change would be for calc(NaN), so

document.body.style.color = "rgb(calc(infinity), calc(-infiinity), calc(NaN)"
document.body.style.color // "rgb(255, 0, 0)"

Currently firefox does this, chrome and safari both return rgb(255, 0, 255). calc(NaN) becoming 255 is certainly unexpected and out-of-line with interop goals for color this year. If it's worth bringing the browsers into alignment for non-finite inputs for color for all other color functions, it makes sense to also to rgb, especially when the change is so small.

It's also currently impossible to make any modifications to "color-valid-rgb.html" without also adding them to interop, as the entire file is included. So apologies for adding failures with my recent chromium CL.

If any of you wanted to respond on w3c/csswg-drafts#8629, it would be good to get the discussion going there, where all the context currently is.

web-platform-tests/interop#369 Bug: 1452185 Change-Id: I6817098f6c4a5fa1f37bfd4713251cac98734291

gsnedders · 2023-07-20T15:29:07Z

Given that there seems to be substantial ongoing discussion about the desired behaviour in these cases, I'd suggest that perhaps the tests should be held back from Interop 2023. Although it does seem like a case we'll inevitably run into eventually, it doesn't seem like it's a core requirement for most uses of this feature, or evidence that it's a current web compat hazard, and so we don't need to fast track interop changes.

+1 on this.

With the CSS WG issue unresolved, and with no evidence of this causing ongoing problems, let's just exclude this from Interop 2023.

…a=testonly Automatic update from web-platform-tests Parse rgb() like other color functions Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598329 Reviewed-by: Florin Malita <[email protected]> Reviewed-by: Steinar H Gunderson <[email protected]> Commit-Queue: Aaron Krajeski <[email protected]> Cr-Commit-Position: refs/heads/main@{#1166616} -- wpt-commits: 207ddbd913b3f007271558329c116e8da16b7979 wpt-pr: 40426

…a=testonly Automatic update from web-platform-tests Parse rgb() like other color functions Previously, rgb() and rgba() parsers were compressing all four color channels and alpha to 8-bit integers. This is no longer necessary, as blink::Color stores the parameters as floats. As we move towards implementing relative color, the more unified the color parsing is, the easier the eventual implementation will be. After some discussion for Interop 2023, it was decided that non-finite color parameters should round-trip and be resolved sanely: http://github.com/w3c/csswg-drafts/issues/8629 This new requirement obviously puts us well out of 8-bit integer territory. Unfortunately, there are clearly still some code paths that are independently compressing alpha to be an 8-bit integer and unless we explicitly do this in the parser, many tests fail. A TODO has been added to address this issue. Here is the test change proposal: web-platform-tests/interop#369 Gradient tests with png expectations are pretty brittle, as evidenced by the per-platform expectations. This change requires a rebaseline because color channels are not squashed to [0,255] integer range at the endpoints. So inputs like: conic-gradient(rgb(0%, 75%, 25%), rgb(0%, 25%, 75%)); No longer become equivalent to: conic-gradient(rgb(0, 191, 63), rgb(0, 63, 191)); 25% of 255 is 63.75. 75% of 255 is 191.25. Bug: 1452185 Change-Id: I1e89ad21d0e3007c1f51aa075728732c2856acc3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4598329 Reviewed-by: Florin Malita <fmalitachromium.org> Reviewed-by: Steinar H Gunderson <sessechromium.org> Commit-Queue: Aaron Krajeski <aaronhkchromium.org> Cr-Commit-Position: refs/heads/main{#1166616} -- wpt-commits: 207ddbd913b3f007271558329c116e8da16b7979 wpt-pr: 40426 UltraBlame original commit: bb939cc04443aa870a6a6b6088231ab172cc8c7a

jgraham · 2023-08-10T16:49:05Z

Since there seems to be no disagreement on withholding these tests from Interop 2023, I'm going to close this issue.

web-platform-tests/interop#369 All legacy colors parse to `rgb( , , )` or `rgba( , , , )` with integers in the range [0, 255] for the color channels and [0, 1] for alpha. Since the channels are bounded, non finite inputs should resolve to the upper bound for "infinity" and the lower bound for "-infinity" and "NaN". Bug: 1478766 Change-Id: I85602f4bffa046e1fd7652b33f119e468287e258

web-platform-tests/interop#369 All legacy colors parse to `rgb( , , )` or `rgba( , , , )` with integers in the range [0, 255] for the color channels and [0, 1] for alpha. Since the channels are bounded, non finite inputs should resolve to the upper bound for "infinity" and the lower bound for "-infinity" and "NaN". The parser was also previously allowing unbounded alpha at parse time. Alpha is also a bounded channel and non-finite inputs should be clamped to [0, 1]. This behavior was previously untested, but is tested here in new tests added to color-valid-rgb.html. Bug: 1478766 Change-Id: I85602f4bffa046e1fd7652b33f119e468287e258

web-platform-tests/interop#369 All legacy colors parse to `rgb( , , )` or `rgba( , , , )` with integers in the range [0, 255] for the color channels and [0, 1] for alpha. Since the channels are bounded, non finite inputs should resolve to the upper bound for "infinity" and the lower bound for "-infinity" and "NaN". The parser was also previously allowing unbounded alpha at parse time. Alpha is also a bounded channel and non-finite inputs should be clamped to [0, 1]. This behavior was previously untested, but is tested here in new tests added to color-valid-rgb.html. Bug: 1478766 Change-Id: I85602f4bffa046e1fd7652b33f119e468287e258 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4839697 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Cr-Commit-Position: refs/heads/main@{#1193043}

…formats at parse time, a=testonly Automatic update from web-platform-tests Clamp non-finite inputs to legacy color formats at parse time web-platform-tests/interop#369 All legacy colors parse to `rgb( , , )` or `rgba( , , , )` with integers in the range [0, 255] for the color channels and [0, 1] for alpha. Since the channels are bounded, non finite inputs should resolve to the upper bound for "infinity" and the lower bound for "-infinity" and "NaN". The parser was also previously allowing unbounded alpha at parse time. Alpha is also a bounded channel and non-finite inputs should be clamped to [0, 1]. This behavior was previously untested, but is tested here in new tests added to color-valid-rgb.html. Bug: 1478766 Change-Id: I85602f4bffa046e1fd7652b33f119e468287e258 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4839697 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Cr-Commit-Position: refs/heads/main@{#1193043} -- wpt-commits: 5ef347155a01cffa9b9082c6cc5077e76c268b71 wpt-pr: 41803

web-platform-tests/interop#369 All legacy colors parse to `rgb( , , )` or `rgba( , , , )` with integers in the range [0, 255] for the color channels and [0, 1] for alpha. Since the channels are bounded, non finite inputs should resolve to the upper bound for "infinity" and the lower bound for "-infinity" and "NaN". The parser was also previously allowing unbounded alpha at parse time. Alpha is also a bounded channel and non-finite inputs should be clamped to [0, 1]. This behavior was previously untested, but is tested here in new tests added to color-valid-rgb.html. Bug: 1478766 Change-Id: I85602f4bffa046e1fd7652b33f119e468287e258 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4839697 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Cr-Commit-Position: refs/heads/main@{#1193043}

mysteryDate added the test-change-proposal Proposal to add or remove tests for an interop area label Jun 28, 2023

chromium-wpt-export-bot mentioned this issue Jun 28, 2023

Parse rgb() like other color functions web-platform-tests/wpt#40426

Merged

zcorpan added the focus area: Color Spaces and Functions label Jul 6, 2023

chromium-wpt-export-bot mentioned this issue Jul 7, 2023

Resolve infinity at parse time for rgb web-platform-tests/wpt#40919

Closed

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 7, 2023

Resolve infinity at parse time for rgb

5409ac3

web-platform-tests/interop#369 Bug: 1452185 Change-Id: I6817098f6c4a5fa1f37bfd4713251cac98734291

nairnandu mentioned this issue Jul 19, 2023

Agenda for Jul 20, 2023 #383

Closed

emilio mentioned this issue Jul 20, 2023

Enhance modern component serialization with inf and nan support servo/rust-cssparser#351

Merged

jgraham closed this as completed Aug 10, 2023

meyerweb mentioned this issue Aug 10, 2023

Agenda, Aug 10th 2023 #391

Closed

chromium-wpt-export-bot mentioned this issue Sep 4, 2023

Clamp non-finite inputs to legacy color formats at parse time web-platform-tests/wpt#41803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rgb() parsing tests that include non-finite inputs #369

Add rgb() parsing tests that include non-finite inputs #369

mysteryDate commented Jun 28, 2023 •

edited by nt1m

Loading

nairnandu commented Jul 5, 2023

nt1m commented Jul 6, 2023

zcorpan commented Jul 6, 2023

emilio commented Jul 6, 2023

jgraham commented Jul 7, 2023

mysteryDate commented Jul 7, 2023 •

edited

Loading

gsnedders commented Jul 20, 2023

jgraham commented Aug 10, 2023

Add rgb() parsing tests that include non-finite inputs #369

Add rgb() parsing tests that include non-finite inputs #369

Comments

mysteryDate commented Jun 28, 2023 • edited by nt1m Loading

Test List

Rationale

nairnandu commented Jul 5, 2023

nt1m commented Jul 6, 2023

zcorpan commented Jul 6, 2023

emilio commented Jul 6, 2023

jgraham commented Jul 7, 2023

mysteryDate commented Jul 7, 2023 • edited Loading

gsnedders commented Jul 20, 2023

jgraham commented Aug 10, 2023

mysteryDate commented Jun 28, 2023 •

edited by nt1m

Loading

mysteryDate commented Jul 7, 2023 •

edited

Loading