Skip to content

[css-color-hdr-1] use one-or-more for dynamic-range-limit-mix grammar production #11349

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

Merged

Conversation

keithamus
Copy link
Member

Based on the parsing tests in WPT (https://github.com/web-platform-tests/wpt/blob/master/css/css-color-hdr/parsing.html) it seems as though commas are required for each set of ident && groups here, however the spec is using the + sigil which allows for "one or more" but doesn't specify commas. I think what might be desired here is to use # which is "one or more comma separated". This would satisfy the requirement for commas.

I think, in addition, we might want to specify something like #{2,} - considering all examples in the parse tests expect two or more values, and the spec talks about summing all values, I am unsure if it is redundant or not to specify just one value (e.g. dynamic-range-limit-mix(high 10%)).

This doesn't resolve the recursive affordances that the WPTs allude to, but baby steps 😆

@keithamus keithamus requested a review from svgeesus December 10, 2024 23:42
@keithamus
Copy link
Member Author

/cc @ccameron-chromium

@ccameron-chromium
Copy link
Contributor

I think what might be desired here is to use # which is "one or more comma separated". This would satisfy the requirement for commas.

Sorry, this was my mistake. I intended for commas to be required.

I think, in addition, we might want to specify something like #{2,} - considering all examples in the parse tests expect two or more values, and the spec talks about summing all values, I am unsure if it is redundant or not to specify just one value (e.g. dynamic-range-limit-mix(high 10%)).

My inclination would be to allow single-argument issues in dynamic-range-limit-mix. The reason being that the meaning is unambiguous (the computed value of dynamic-range-limit-mix(high 10%) is high), and allowing it is simpler.

@ccameron-chromium
Copy link
Contributor

Uploaded a patch with WPT tests at:
https://chromium-review.googlesource.com/c/chromium/src/+/6087798

@ccameron-chromium
Copy link
Contributor

And uploaded a PR to update
#11351

Is that the extent of the changes that are desired here?

@keithamus
Copy link
Member Author

I think so. Is there any reason this PR won't suffice for the change? I can't see a difference between this one and #11351

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 11, 2024
Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
@svgeesus svgeesus merged commit 93af87a into main Dec 11, 2024
1 check passed
@keithamus keithamus deleted the use-one-or-more-for-dynamic-range-limit-mix-grammar-production branch December 11, 2024 18:02
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 7, 2025
Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromium@keithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1403188}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 7, 2025
Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromium@keithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1403188}
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jan 10, 2025
… a=testonly

Automatic update from web-platform-tests
dynamic-range-limit: Add more WPT tests

Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromium@keithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1403188}

--

wpt-commits: d2d172058342f786290e219b18288f40dc773ba9
wpt-pr: 49639
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 10, 2025
… a=testonly

Automatic update from web-platform-tests
dynamic-range-limit: Add more WPT tests

Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromium@keithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1403188}

--

wpt-commits: d2d172058342f786290e219b18288f40dc773ba9
wpt-pr: 49639
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 14, 2025
Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromium@keithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1403188}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jan 16, 2025
… a=testonly

Automatic update from web-platform-tests
dynamic-range-limit: Add more WPT tests

Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromiumkeithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameronchromium.org>
Cr-Commit-Position: refs/heads/main{#1403188}

--

wpt-commits: d2d172058342f786290e219b18288f40dc773ba9
wpt-pr: 49639

UltraBlame original commit: 7c7d7cd0908d881fd0dd873abe1e28d0a3c4f087
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jan 16, 2025
… a=testonly

Automatic update from web-platform-tests
dynamic-range-limit: Add more WPT tests

Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromiumkeithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameronchromium.org>
Cr-Commit-Position: refs/heads/main{#1403188}

--

wpt-commits: d2d172058342f786290e219b18288f40dc773ba9
wpt-pr: 49639

UltraBlame original commit: 7c7d7cd0908d881fd0dd873abe1e28d0a3c4f087
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jan 16, 2025
… a=testonly

Automatic update from web-platform-tests
dynamic-range-limit: Add more WPT tests

Add tests for the edge cases brought up in:
w3c/csswg-drafts#11349

Change-Id: Ia387c339d8555068cb70899bff850f4005423f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087798
Reviewed-by: Keith Cirkel <chromiumkeithcirkel.co.uk>
Commit-Queue: ccameron chromium <ccameronchromium.org>
Cr-Commit-Position: refs/heads/main{#1403188}

--

wpt-commits: d2d172058342f786290e219b18288f40dc773ba9
wpt-pr: 49639

UltraBlame original commit: 7c7d7cd0908d881fd0dd873abe1e28d0a3c4f087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants