1274519 - Should not append result to the candidate list if got unexpected token at the end of string when parsing sizes attribute

Status: RESOLVED FIXED

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

Description

[Edgar Chen [:edgar]]

CSSParserImpl::ParseSourceSizeList() should not append result to the candidate list if got unexpected token at the end of string when parsing sizes attribute.

Some wpt failures in http://w3c-test.org/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html are caused by this bug.

for example,

<img srcset="/images/green-1x1.png?f33 50w, /images/green-16x16.png?f33 51w" sizes="1px !important">

test expects failing to parse sizes attribute [1] and use default value (100vw), so `/images/green-16x16.png` will be selected.

But we parse sizes as `1px` and then select `/images/green-1x1.png`.

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

Comment 1

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 2

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0fe0fd693f2f6978e5cad5d81a0feea59433148&group_state=expanded

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 8754712 [details] [diff] [review]
Patch, v1

Review of attachment 8754712 [details] [diff] [review]:
-----------------------------------------------------------------

Please see comment #0 for the details, thank you.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 8754712 [details] [diff] [review]
Patch, v1

Review of attachment 8754712 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know the CSS parser code at all. From the point of view of the responsive image code, if the API can tell us whether the string was consumed entirely without errors then I think that's all we require.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8754712 [details] [diff] [review]
Patch, v1

Review of attachment 8754712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: layout/style/nsCSSParser.cpp
@@ +2176,5 @@
>    CLEAR_ERROR();
>    ReleaseScanner();
>    mHTMLMediaMode = false;
>  
> +  return aQueries.Length() > 0;

!aQueries.IsEmpty()

Comment 6

[Edgar Chen [:edgar]]

Attachment: Patch, v2, r=heycam

Address review comment #5.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Patch, v2, r=heycam

(Upload correct one)
Address review comment #5.

Comment 8

[Edgar Chen [:edgar]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=333243e0fea1

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a3bd5421b1

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/10a3bd5421b1