stylo: audit layout/reftests/css-parsing/invalid-url-handling.xhtml

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: emilio, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

9 months ago
It's the last css-parsing reftest that remains. Gecko passes this, but both Stylo and Blink seem to disagree with it in the same way.
(Reporter)

Updated

9 months ago
Blocks: 1243581
(Reporter)

Comment 1

9 months ago
I went through this with Simon:

17:10 <@SimonSapin> emilio: #three in http://searchfox.org/mozilla-central/source/layout/reftests/css-parsing/invalid-url-handling.xhtml is a bad-url token per spec, so the double-quote does not start a quoted string
17:10 <@SimonSapin> same with four, and similar with fourteen
17:11 <emilio> SimonSapin: I see
17:11 <@SimonSapin> https://drafts.csswg.org/css-syntax/#consume-url-token
17:13 <emilio> SimonSapin: fourteen would be a bad-url too, right?
17:13 <@SimonSapin> yes
17:13 <emilio> SimonSapin: err, twelve
17:14 <emilio> SimonSapin: err, wait, maybe not, let me check
17:14 <@SimonSapin> twelve is green?
17:14 <emilio> SimonSapin: it's red
17:15 <@SimonSapin> that sounds right to me (per L3 spec)
17:15 <@SimonSapin> url(}{""{) is the bad-url token
17:15 <@SimonSapin> the } after than ends the style rule
17:15 <@SimonSapin> ); background-color: green; }
17:15 <@SimonSapin> is an invalid selector

So it seems we're agreeing with the spec, and with other browsers... I'm going to move this to stylo-behavior-changes tentatively. David, does this sound right to you?
Blocks: 1365771
Flags: needinfo?(dbaron)
See Also: → bug 790997
Given bug 790997 I think we agree there are changes we need to make here (in Gecko).  The spec has changed enough times that I'm not particularly confident, though, but I *guess* it's safe to assume that the current state of the spec reflects working group decisions.  If the current spec actually does match Chromium then it probably is safe to implement.

(We've changed the Gecko behavior here a number of times in the past as the spec changed...)
Flags: needinfo?(dbaron)
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1380934
(Assignee)

Comment 4

9 months ago
I'm going to change Gecko's behavior to have it closer to the spec (though still not strictly follow, because that may require more changes than worth).
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)

Comment 7

9 months ago
mozreview-review
Comment on attachment 8891210 [details]
Bug 1383075 part 1 - Handle URL token in a closer way to the spec.

https://reviewboard.mozilla.org/r/162428/#review167706

::: devtools/shared/css/lexer.js:740
(Diff revision 1)
> +      // Consume until right parenthesis
> +      if (aToken.mType == eCSSToken_Bad_URL) {

You should probably ask someone who knows lexer.js to review this.

::: layout/style/nsCSSScanner.cpp:754
(Diff revision 1)
> +    // Consume until right parenthesis
> +    if (aToken.mType == eCSSToken_Bad_URL) {
> +      while (true) {
> +        int32_t ch = Peek();
> +        if (IsVertSpace(ch)) {
> +          AdvanceLine();
> +        } else {
> +          Advance();
> +        }
> +        if (ch < 0 || ch == ')') {
> +          break;
> +        }
> +      }
> +    }

So I think it would be both simpler and closer to the spec if you:

 (a) moved this inside of NextURL, and did it only in the non-string case
 
 (b) didn't consume the closing parenthesis (avoiding the need to make changes to the parser)
 
 (c) didn't bother with the code you *did* add to NextURL() to consume stuff in the string case
 
This would mean that:
 - for the case the spec describes as the bad-url token, the scanner would always leave the ) as the next character, and the SkipUntil() would be trivial
 - for the case the spec describes at the function-and-string case, the SkipUntil() would do () [] and {} matching as the spec requires

What do you think of that approach?

::: layout/style/nsCSSScanner.cpp:757
(Diff revision 1)
> +        int32_t ch = Peek();
> +        if (IsVertSpace(ch)) {
> +          AdvanceLine();
> +        } else {
> +          Advance();
> +        }

Perhaps this should append to aToken.mIdent?  It might at least help for error reporting.

::: layout/style/nsCSSScanner.cpp:1211
(Diff revision 1)
>      if (ch < 0) {
>        AddEOFCharacters(eEOFCharacters_CloseParen);
>      }
>    } else {
> +    // Skip any string token inside the url function if we starts with
> +    // a string. In spec, an "url(" followed by string is handled as a

"In spec" should be "In the spec", but maybe it's worth clarifying to "In css-syntax-3 (unlike in CSS 2.1)".

::: testing/web-platform/meta/css/CSS2/syntax/uri-013.xht.ini:1
(Diff revision 1)
>  [uri-013.xht]
>    type: reftest
> -  expected:
> +  expected: FAIL

Is there a bug open on web-platform-tests to fix this?
Attachment #8891210 - Flags: review?(dbaron) → review-
needinfo? for the "What do you think of that approach?" above.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 9

9 months ago
That sounds good. I'll try that way.
(Assignee)

Comment 10

9 months ago
mozreview-review-reply
Comment on attachment 8891210 [details]
Bug 1383075 part 1 - Handle URL token in a closer way to the spec.

https://reviewboard.mozilla.org/r/162428/#review167706

> Perhaps this should append to aToken.mIdent?  It might at least help for error reporting.

It may actually be confusing, because we have skipped whitespace by here. It would probably be more helpful if there is an easy way to add back the whitespaces we skipped.
(Assignee)

Comment 11

9 months ago
mozreview-review-reply
Comment on attachment 8891210 [details]
Bug 1383075 part 1 - Handle URL token in a closer way to the spec.

https://reviewboard.mozilla.org/r/162428/#review167706

> Is there a bug open on web-platform-tests to fix this?

I'm going to open an issue there when this change gets merged, so that I can refer the mutated test here.

Or probably we can just update the web-platform test in this commit as well? Maybe even remove the reftest in favor of web-platform test?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

9 months ago
ni? for the question regarding the test update in comment 11.
Flags: needinfo?(xidorn+moz) → needinfo?(dbaron)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> Or probably we can just update the web-platform test in this commit as well?
> Maybe even remove the reftest in favor of web-platform test?

I think it probably makes more sense to have a web-platform-test for this sort of behavior -- we just need to make sure that all the cases covered in the reftest are covered in any web-platform-test.
Flags: needinfo?(dbaron)

Comment 16

9 months ago
mozreview-review
Comment on attachment 8891210 [details]
Bug 1383075 part 1 - Handle URL token in a closer way to the spec.

https://reviewboard.mozilla.org/r/162428/#review168298

r=dbaron with the following

::: layout/reftests/css-parsing/invalid-url-handling.xhtml:70
(Diff revision 2)
> -  /* not a URI token; brace matching should work only after invalid URI token */
> -  #twelve { background: url(}{""{)}); background-color: green; }
> +  /* not a URI token; bad-url token is consumed until the first closing )
> +     so later brace closes the declaration block */
> +  #twelve { background-color: green; }
> +  #twelve { background: url(}{""{)}); background-color: red; }

I'm not sure I believe this comment.  Doesn't the fact that the earlier ) ends the bad-url token mean that the *earlier* brace closes the declaration block?  (That also seems to fit better with the placement of red and green.)
Attachment #8891210 - Flags: review?(dbaron) → review+

Comment 17

9 months ago
mozreview-review
Comment on attachment 8891833 [details]
Bug 1383075 part 2 - Update CSS lexer in devtools to match the change.

https://reviewboard.mozilla.org/r/162860/#review168436

Thanks for updating the DevTools version as well! :)

::: devtools/shared/css/lexer.js:1097
(Diff revision 1)
>        if (ch < 0) {
>          this.AddEOFCharacters(eEOFCharacters_CloseParen);
>        }
>      } else {
>        aToken.mType = eCSSToken_Bad_URL;
> +      // Consume until right parenthesis

Copy the same large comment you added to the C++ version.  I believe this file tries its best to match the C++ version as closely as is reasonable to do so.
Attachment #8891833 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

9 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a6ea79cb3d7
part 1 - Handle URL token in a closer way to the spec. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/5ab2855930cd
part 2 - Update CSS lexer in devtools to match the change. r=jryans
(Assignee)

Comment 21

9 months ago
(In reply to David Baron :dbaron: ⌚️UTC+2 from comment #15)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> > Or probably we can just update the web-platform test in this commit as well?
> > Maybe even remove the reftest in favor of web-platform test?
> 
> I think it probably makes more sense to have a web-platform-test for this
> sort of behavior -- we just need to make sure that all the cases covered in
> the reftest are covered in any web-platform-test.

Filed issue w3c/web-platform-tests#6666.
https://hg.mozilla.org/mozilla-central/rev/1a6ea79cb3d7
https://hg.mozilla.org/mozilla-central/rev/5ab2855930cd
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
So did this fix bug 790997?  As in, did it fully implement the syntax draft?

If not, what exact behavior did it implement, and was it tested against things like bug 1315368 and whatnot?  My experience when working on this stuff in bug 790997 was that anything that didn't either exactly match existing Gecko or exactly match the syntax ED / Chrome broke webpages.  So if we did some halfway measure here, we need to test the living daylights out of it if we're at all considering shipping it.
Flags: needinfo?(xidorn+moz)
I'm not aware of ways this differs observably from the css-syntax editor's draft, though the underlying tokenization is different for url() functions with a string (which the spec tokenizes as "url(" function plus string), and for the inclusion of the closing parenthis in the token for all valid URLs.
OK, great.  That sounds like it fixes bug 790997, then...
(Assignee)

Comment 26

9 months ago
As far as we don't support the <url-modifier> defined in css-values-3, I'm not aware of any observable way from the content to distinguish between the new behavior and the css-syntax behavior.

It probably doesn't completely fix bug 790997 (at the tokenization level), but stylo implemented the new rule from the very beginning IIUC.

And I checked the case in bug 1315368. I believe we shouldn't regress on that case with this patch.
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.