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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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).

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1383075 part 1 - Handle URL token in a closer way to the spec.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

needinfo? for the "What do you think of that approach?" above.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

That sounds good. I'll try that way.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1383075 part 2 - Update CSS lexer in devtools to match the change.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162428/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

ni? for the question regarding the test update in comment 11.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162428/diff/2-3/

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162860/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1a6ea79cb3d7
https://hg.mozilla.org/mozilla-central/rev/5ab2855930cd

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, great.  That sounds like it fixes bug 790997, then...

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

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.