1017878 - [meta] Make picture sizes grammar spec compliant and add tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[John Schoenick [:johns]]

The initial sizes syntax in bug 870022 is out of date with the spec, and needs to be updated and given tests for the newest version.

Comment 1

[Mike Taylor [:miketaylr]]

Should Bug 1139560 be considered a dupe of this?

Comment 2

[Simon Pieters [:zcorpan]]

Mike, no, this bug is about parsing sizes="".

Comment 3

[Simon Pieters [:zcorpan]]

Some analysis of failures in http://web-platform.test:8000/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html

* sizes="all 100vw, 1px" - media queries are not allowed, only <meta-condition>. Might cause pages to rely on it working.

* sizes="0.1%" - percentages are not allowed, only <length>. Might cause pages to rely on it working.

* sizes="x(),1px" - interferes with graceful fallback when using future extensions. It seems like the entire attribute is ignored if something is invalid.

Some fails are due to changes between MQ L3 and MQ L4 or new CSS units etc; these I would say are low-prio.

Comment 4

[Simon Pieters [:zcorpan]]

(In reply to Simon Pieters from comment #3)
> * sizes="all 100vw, 1px" - media queries are not allowed, only
> <meta-condition>. Might cause pages to rely on it working.

Never mind, it seems Gecko correctly doesn't allow media queries. It's just that the fallback mechanism doesn't work and 100vw is the default.

> * sizes="0.1%" - percentages are not allowed, only <length>. Might cause
> pages to rely on it working.

Any percentage value seems to make Gecko use the image's actual width as intrinsic width instead of acting like sizes="100vw". So Gecko doesn't "allow" percentages but there's still a bug and pages might rely on it.

Comment 5

[Edgar Chen [:edgar]]

Spec: https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-attribute

Comment 6

[Edgar Chen [:edgar]]

Hi John, not sure if you're still working on this. Do you mind if I take it?

Comment 7

[Edgar Chen [:edgar]]

Besides the analysis in comment #3 and comment #4, there are two more different failures,

* sizes="1q" - not sure why test expects the parsing result will be `1px`.

* sizes="calc(1px" - test expects getting error in parsing and falling back to default value, 100vw. But  gecko allows an open construct be closed by EOF, https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#3075-3083.

Comment 8

[Edgar Chen [:edgar]]

Summarizing the analysis we have now,

1. sizes="all 100vw, 1px" - media queries are not allowed, only <meta-condition>. And we should fallback to `1px` instead of ignoring entire attribute. See step 4 in https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-attribute. 

2. sizes="x(),1px" - interferes with graceful fallback when using future extensions. It seems like the entire attribute is ignored if something is invalid. See step 2 in https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-attribute. 

3. sizes="0.1%" - percentages are not allowed. See https://html.spec.whatwg.org/multipage/embedded-content.html#source-size-value.

4. sizes="calc(1px" - test expects getting error in parsing and falling back to default value, 100vw. But  gecko allows an open construct be closed by EOF, https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#3075-3083.

5. sizes="1q" - not sure why test expects the parsing result will be `1px`.

6. sizes="(min-width:0) or (min-width:0) 1px" - due to changes between MQ L3 and MQ L4 or new CSS units etc;

Comment 9

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> Summarizing the analysis we have now,
> 
> 1. sizes="all 100vw, 1px" - media queries are not allowed, only
> <meta-condition>. And we should fallback to `1px` instead of ignoring entire
> attribute. See step 4 in
> https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-
> attribute. 
> 
> 2. sizes="x(),1px" - interferes with graceful fallback when using future
> extensions. It seems like the entire attribute is ignored if something is
> invalid. See step 2 in
> https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-
> attribute. 
> 
> 3. sizes="0.1%" - percentages are not allowed. See
> https://html.spec.whatwg.org/multipage/embedded-content.html#source-size-
> value.
> 
> 4. sizes="calc(1px" - test expects getting error in parsing and falling back
> to default value, 100vw. But  gecko allows an open construct be closed by
> EOF,
> https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.
> cpp#3075-3083.
> 
> 5. sizes="1q" - not sure why test expects the parsing result will be `1px`.
> 
> 6. sizes="(min-width:0) or (min-width:0) 1px" - due to changes between MQ L3
> and MQ L4 or new CSS units etc;

7. sizes="1px !important" - ParseSourceSizeList() returns parsing error, but still appends the parsing result to the list.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 1: Don't append result to the candidate list if got unexpected token at the end of size string, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 2: Percentages are not allowed in a <source-size-value>, v1

Comment 12

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> Summarizing the analysis we have now,
> 
> 5. sizes="1q" - not sure why test expects the parsing result will be `1px`.

q stands for "quarter-millimeters", see https://www.w3.org/TR/css-values/#absolute-lengths.

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 2: Support using quarter-millimeters as css length value, v2

Comment 14

[John Schoenick [:johns]]

\o/ thanks for working on this

Comment 15

[Edgar Chen [:edgar]]

Attachment: MozReview Request: Bug 1017878 - Part 1: Don't append result to the candidate list if got unexpected token at the end of size string

Review commit: https://reviewboard.mozilla.org/r/45549/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45549/

Comment 16

[Edgar Chen [:edgar]]

Attachment: MozReview Request: Bug 1017878 - Part 2: Support using quarter-millimeters as css length value

Review commit: https://reviewboard.mozilla.org/r/45551/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45551/

Comment 17

[Edgar Chen [:edgar]]

Attachment: MozReview Request: Bug 1017878 - Part 3: Jump to next component values if any parsing error

Review commit: https://reviewboard.mozilla.org/r/45779/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45779/

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8741895 [details]
MozReview Request: Bug 1017878 - Part 1: Don't append result to the candidate list if got unexpected token at the end of size string

Sorry, there are some works need to do, canceling the review first.

Comment 19

[Edgar Chen [:edgar]]

Since there are multiple things we need to fix, I'd like to file separated bugs for each of one and leave this bug as meta bug to track the status.

Comment 20

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> 
> 4. sizes="calc(1px" - test expects getting error in parsing and falling back
> to default value, 100vw. But  gecko allows an open construct be closed by
> EOF,
> https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.
> cpp#3075-3083.

This test is wrong and gecko behaviour is correct, see https://github.com/w3c/web-platform-tests/issues/3066.

Comment 21

[Edgar Chen [:edgar]]

> 6. sizes="(min-width:0) or (min-width:0) 1px" - due to changes between MQ L3 and MQ L4 or new CSS units etc;

IIRC, this is the only thing we need to fix which depends on bug 1312621.

Comment 22

[quasicomputational]

I think that got done in bug 1422225, unless I'm misinterpreting something.

Comment 23

[Edgar Chen [:edgar]]

Yes, thanks for this information. I will udpate the dependency and also close this bug.