Should not ignore entire <source-size-list> if something is invalid

RESOLVED FIXED in Firefox 50

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
`sizes` attribute is a comma-separated list of component values, currently we ignore entire attribute if something is invalid. But according to the step 2 and 4 of https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-sizes-attribute, we should jump to next comma-separated entry and continue to parse.

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, for example

Test case:
<img srcset="/images/green-1x1.png?e24 50w, /images/green-16x16.png?e24 51w" sizes="x(),1px">

Expect result:
Use `1px` and select `/images/green-1x1.png`

Actual result:
Use default 100vw due to the parsing error and select `/images/green-16x16.png`
(Assignee)

Updated

2 years ago
Assignee: nobody → echen
Blocks: 1017878
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8756678 [details]
Bug 1275511 - Should not ignore entire <source-size-list> if something is invalid;

Review commit: https://reviewboard.mozilla.org/r/55062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55062/
Attachment #8756678 - Flags: review?(cam)
Comment on attachment 8756678 [details]
Bug 1275511 - Should not ignore entire <source-size-list> if something is invalid;

https://reviewboard.mozilla.org/r/55062/#review56600

Sorry for the delay in reviewing.

::: layout/style/nsCSSParser.cpp:2175
(Diff revision 1)
> -    // Per spec, a parse failure in this list invalidates it
> +      // Per spec, a parse failure in this list invalidates it
> -    // entirely. Currently, this grammar is specified standalone and not part of
> +      // entirely. Currently, this grammar is specified standalone and not part of
> -    // any larger grammar, so it doesn't make sense to try to advance the token
> +      // any larger grammar, so it doesn't make sense to try to advance the token
> -    // beyond it.
> +      // beyond it.

This comment needs to be updated to say that we just skip the current item if there was a parse error.

::: layout/style/nsCSSParser.cpp:2186
(Diff revision 1)
> +      // Jumps to next entry of <source-size-list> which is a comma-separated list.
> +      if (!SkipUntil(',')) {
> +        hitEnd = true;
> +      }
> -  }
> +    }
> +  } while(!hitEnd);

Nit: space after "while".
Attachment #8756678 - Flags: review?(cam) → review+
(Assignee)

Comment 4

2 years ago
Comment on attachment 8756678 [details]
Bug 1275511 - Should not ignore entire <source-size-list> if something is invalid;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55062/diff/1-2/
Attachment #8756678 - Attachment description: MozReview Request: Bug 1275511 - Should not ignore entire <source-size-list> if something is invalid; r?heycam → Bug 1275511 - Should not ignore entire <source-size-list> if something is invalid;

Comment 6

2 years ago
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3c6f8d57ab
Should not ignore entire <source-size-list> if something is invalid; r=heycam

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf3c6f8d57ab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.