Closed
Bug 1383075
Opened 7 years ago
Closed 7 years ago
stylo: audit layout/reftests/css-parsing/invalid-url-handling.xhtml
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: emilio, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
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 | ||
Comment 1•7 years 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: stylo-behavior-changes
Flags: needinfo?(dbaron)
Updated•7 years ago
|
Comment 2•7 years ago
|
||
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 | ||
Comment 4•7 years 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) |
Assignee | ||
Comment 6•7 years ago
|
||
Comment 7•7 years 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-
Comment 8•7 years ago
|
||
needinfo? for the "What do you think of that approach?" above.
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 9•7 years ago
|
||
That sounds good. I'll try that way.
Assignee | ||
Comment 10•7 years 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•7 years 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•7 years ago
|
||
ni? for the question regarding the test update in comment 11.
Flags: needinfo?(xidorn+moz) → needinfo?(dbaron)
Comment 15•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years 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.
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1a6ea79cb3d7
https://hg.mozilla.org/mozilla-central/rev/5ab2855930cd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 23•7 years ago
|
||
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)
Comment 24•7 years ago
|
||
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•7 years ago
|
||
OK, great. That sounds like it fixes bug 790997, then...
Assignee | ||
Comment 26•7 years 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.
Description
•