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-values-4] Specify argument range for resolution #8532

Closed
tcaptan-cr opened this issue Mar 6, 2023 · 11 comments
Closed

[css-values-4] Specify argument range for resolution #8532

tcaptan-cr opened this issue Mar 6, 2023 · 11 comments

Comments

@tcaptan-cr
Copy link
Contributor

Currently the resolution-value is defined as:

The < resolution > unit represents the size of a single "dot" in a graphical representation by indicating how many of these dots fit in a CSS in, cm, or px.

However, no argument range is currently specified for the resolution unit, which would imply that zero and negative resolution values are valid.

Given the resolution is a ratio of physical "dots" to CSS px, in or cm, non-positive values do not make sense.

After discussing this issue with @emilio we thought it would be good if it would be well defined in the spec as [1, ∞].

MDN does currently mention a restriction to "strictly positive" that is not in the spec:

The < resolution > data type consists of a strictly positive < number > followed by one of the units listed below.

@tabatkins
Copy link
Member

There's nothing wrong with a resolution between 1 and 0, and in general CSS tries to avoid open ranges (see the wiki) so "strictly positive" should be avoided if possible. In general, a zero resolution isn't problematic anyway; it's a degenerate resolution, sure, but just as meaningful in practice as any extremely small resolution value.

However, negative resolutions are indeed nonsensical. I think it's reasonable to add a range into V&U; this'll make negative values invalid by default, and clamp them to 0 if put in a math function.

@tabatkins
Copy link
Member

Agenda+ for a resolution just due to the spec's stability. Proposal is to add a default range restriction to <resolution> values requiring they be non-negative. (They'll be treated the same as a negative length in a property that only allows non-negative lengths; this restriction will just apply to every place that accepts a resolution automatically.)

@emilio
Copy link
Collaborator

emilio commented Mar 6, 2023

Sounds good, but just to double-check, media queries too? There's a bunch of tests expecting negative resolutions to parse in media queries, see web-platform-tests/wpt#24433, https://hg.mozilla.org/mozilla-central/rev/50b2f3e731efb6aab243709544a1934982f0c215.

CC @frivoal since he introduced at least a few of them.

Also, we should define how does a zero resolution image render... Probably should be rendered as another invalid image?

@tabatkins
Copy link
Member

I doubt anyone's relying on negative resolutions? So I suspect it's fine to just change the tests.

Also, we should define how does a zero resolution image render... Probably should be rendered as another invalid image?

In image-set()? We could do that, or just clamp the resolution to a UA-defined minimum. Arbitrarily small resolutions would produce image blowups anyway, which are probably not desirable.

@emilio
Copy link
Collaborator

emilio commented Mar 6, 2023

Yeah, agreed it should be safe (we didn't honor them anywhere until recently, that linked commit).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
@atanassov atanassov added this to Overflow in March 2023 VF2F Mar 7, 2023
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 7, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}
@astearns astearns moved this from Overflow to Monday - Mar 13 in March 2023 VF2F Mar 11, 2023
nt1m added a commit to nt1m/WebKit that referenced this issue Mar 24, 2023
https://bugs.webkit.org/show_bug.cgi?id=254388
rdar://107167273

Reviewed by NOBODY (OOPS!).

Resolution ranges are currently not restricted by the specification, so we should accept them.

Corresponding WPT commit with more details: web-platform-tests/wpt@6dd9e79

The two unskipped tests are for rendering, negative resolutions are considered invalid for rendering purposes, so they display as blank.
However, they are still parsed, which means the declaration stays valid, which wasn't the case before.

Also rebaseline parsing tests.

It is possible that the spec changes again in the future: w3c/csswg-drafts#8532

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing-expected.txt:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeImageSetOption):
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 30, 2023
… to image-set, a=testonly

Automatic update from web-platform-tests
Add support for non-positive resolutions to image-set

This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}

--

wpt-commits: 6dd9e79912d5161f24b2bf576e0b115c28cde956
wpt-pr: 38839
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-values-4] Specify argument range for resolution, and agreed to the following:

  • RESOLVED: negative <resolution> values are invalid
The full IRC log of that discussion <dael> TabAtkins: It was noted that it is possible to write a negative resolution and not def what that means. Prop is the resolution type as a general case says negative is invalid
<dael> TabAtkins: IN a calc we would clamp it.
<dael> TabAtkins: V&U is mature spec so need resolution
<dael> TabAtkins: Resolution type is restricted to non-negative values
<dael> fantasai: sgtm
<fantasai> RESOLVED: negative <resolution> values are invalid
<fantasai> scribe+ fantasai
@tabatkins
Copy link
Member

Done; negative resolutions are now defined to always be outside the allowed range. This makes it invalid in all contexts, but will cause it to clamp when used in a math function.

cookiecrook pushed a commit to cookiecrook/wpt that referenced this issue Apr 8, 2023
This change was triggered by the recently added wpt tests that started
failing after auto import:
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering.html
third_party/blink/web_tests/external/wpt/css/css-images/image-set/image-set-zero-resolution-rendering-2.html

Currently the spec does not restrict the range for resolution,
thus non-positive resolutions are valid and should not be
rejected at parse time.

The current consensus is to parse the non-positive resolutions,
but treat them as unsupported options for rendering purposes.

With this change, image-set will match resolution parsing
with media queries.

An issue has been opend for the spec to define the valid
argument range for resolution:
w3c/csswg-drafts#8532

In the discussion on the issue new proposals were added about
how to treat zero resolutions for rendering purposes. Until the spec
is well defined in this area we will remove the zero resolution
rendering tests.

R=futhark, pdr

Change-Id: I7b329519168f3c088f5c9ee8614931b0ead7a4df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312883
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1114116}
@cdoublev
Copy link
Collaborator

This makes it invalid in all contexts, but will cause it to clamp when used in a math function.

calc(-1dppx) is invalid but calc(-1 * 1dppx) is valid (and clamped), right?

@tabatkins
Copy link
Member

Hm, calc(-1dppx) being invalid wasn't explicitly intended, but yes, it does flow from the definition, and that's fine.

calc(-1 * 1dppx) is indeed valid and clamped.

@emilio
Copy link
Collaborator

emilio commented Apr 15, 2023

That's inconsistent with all other calc() dimensions tho, seems unfortunate

@tabatkins
Copy link
Member

Not technically - other usages gain ranges from the property, not the function.

That is, width has <length-percentage [0, ∞]> in its grammar, so a calc() must resolve to a non-negative length, but within the calc() itself negative lengths are fine; it's okay to say width: calc(-1 * 10px) to get a 10px length.

On the other hand, the way I specified this, all <resolution> productions implicitly have a non-negative constraint, which includes the ones allowed in math functions.

Now, this isn't a necessity. It was the easiest way to specify the resolution, but I could explicitly carve out an exception for math functions, I suppose?

nt1m added a commit to web-platform-tests/wpt that referenced this issue May 4, 2023
nt1m added a commit to web-platform-tests/wpt that referenced this issue May 4, 2023
nt1m added a commit to web-platform-tests/wpt that referenced this issue May 4, 2023
aarongable pushed a commit to chromium/chromium that referenced this issue May 11, 2023
Resulting from the discussion of issue 8532 [1] which asked to specify
the argument range for resolution units, negative resolutions should
now be excluded.

Spec definition:
[2]
"The allowed range of <resolution> values always excludes negative
values, in addition to any explicit ranges that might be specified."

[1]
w3c/csswg-drafts#8532

[2]
https://www.w3.org/TR/css-values-4/#resolution-value

R=pdr

Change-Id: I20486e9e506683cb076551149c590da20d6eff08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518748
Commit-Queue: Traian Captan <tcaptan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1142795}
webkit-commit-queue pushed a commit to rreno/WebKit that referenced this issue May 21, 2023
…in calc expressions

https://bugs.webkit.org/show_bug.cgi?id=254388
rdar://107167273

Reviewed by Tim Nguyen.

In a CSSWG resolution it was decided that zero resolutions should be
accepted by image-set but that they should produce an invalid image.
This change accepts zero but will reject negative resolutions. In a
follow-up we should clamp the result of calc expressions to zero.

This change also resyncs the image-set WPT as of upstream commit
c56aec6

w3c/csswg-drafts#8532 (comment)

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-2.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3.html: Copied from LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering.html: Added.

* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeImageSetOption):
* Source/WebCore/rendering/style/StyleImageSet.cpp:
(WebCore::StyleImageSet::bestImageForScaleFactor):

Canonical link: https://commits.webkit.org/264298@main
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 22, 2023
… invalid, a=testonly

Automatic update from web-platform-tests
[image-set] Negative resolutions are now invalid (#39840)

See resolution in w3c/csswg-drafts#8532
--

wpt-commits: f191d6e935b7f2df4bd3103a07179af544244516
wpt-pr: 39840
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 22, 2023
… invalid (part 2), a=testonly

Automatic update from web-platform-tests
[image-set] Negative resolutions are now invalid (part 2) (#39849)

See resolution in w3c/csswg-drafts#8532
--

wpt-commits: b0350b795dbf77ebf53713aca302e5d768d5db26
wpt-pr: 39849
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue May 23, 2023
… invalid, a=testonly

Automatic update from web-platform-tests
[image-set] Negative resolutions are now invalid (#39840)

See resolution in w3c/csswg-drafts#8532
--

wpt-commits: f191d6e935b7f2df4bd3103a07179af544244516
wpt-pr: 39840
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue May 23, 2023
… invalid (part 2), a=testonly

Automatic update from web-platform-tests
[image-set] Negative resolutions are now invalid (part 2) (#39849)

See resolution in w3c/csswg-drafts#8532
--

wpt-commits: b0350b795dbf77ebf53713aca302e5d768d5db26
wpt-pr: 39849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants