Closed Bug 1391169 Opened 7 years ago Closed 7 years ago

stylo: fails to roundtrip <style> element textContent and document.styleSheets[0].cssRules

Categories

(Core :: CSS Parsing and Computation, defect, P2)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: bradwerth)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file test.html
The attached test case passes in Gecko, Chrome, and WebKit, logging the following to the devtools console:

INPUT: "* > *{ }"
OUTPUT: "* > * { }"
ROUNDTRIP: "* > * { }"
PASS

But fails in Stylo:

INPUT: "* > *{ }"
OUTPUT: "** { }"
ROUNDTRIP: ""
FAIL

This test case is a simplified version of the output of Jesse Ruderman's CSS fuzzer:

http://www.squarefree.com/2009/03/16/css-grammar-fuzzer/
Component: General → CSS Parsing and Computation
I'll take a look.
Assignee: nobody → bwerth
See Also: → 1392956
Attachment #8900930 - Flags: review?(simon.sapin)
Simon, it appears that Servo parser can't handle legal compound selectors like "* > *" which would match any elements that are children of something. I looked through the parser and confirmed that all the atoms are read correctly. However, the builder structure holds the tokens without specifying an order between simple_selectors and combinators: http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/servo/components/selectors/builder.rs#56.

If you agree that this is a bug (and not an acceptable deviation from Gecko behavior), I'll work on a patch to change the builder structure to impose an ordering.
Flags: needinfo?(simon.sapin)
(In reply to Brad Werth [:bradwerth] from comment #4)
> However, the builder structure holds the tokens without specifying an order between simple_selectors and combinators.

I see that I spoke too soon. I see that the structure attempts to order the combinators against the selectors by carrying an corresponding length. I'll see if I can figure out what's going on. I'm still interested in your insights, of course.
Attachment #8901350 - Flags: review?(simon.sapin)
Comment on attachment 8900930 [details]
Bug 1391169 Part 2: Add a test of cssText with combinations of namespaces, universal selectors, and combinators.

https://reviewboard.mozilla.org/r/172370/#review178292

::: dom/base/test/test_style_cssText.html:8
(Diff revision 2)
> +<meta charset="UTF-8">
> +<title>Test for Bug 1391169</title>
> +<script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +<style id="style"></style>
> +<style>@namespace blahBLAH url(test_style_cssText.css)</style>

An @namespace rule’s URL does not need to point to a valid file, so test_style_cssText.css can be removed.  (It’s just an identifier, http://example.net/foo/bar would work just as well.)

The scope of a @namespace rule is the stylesheet that contains it. Separate <style> elements introduce separate stylesheets, so this rule is doing nothing. It should instead be prepended to `input` in `testData()`.

I think this fails on try because the blahBLAH|* selectors cause a parse error because of the undeclared namespace prefix.
Comment on attachment 8901350 [details]
Bug 1391169 Part 1: Servo-side change Selector to_css function to handle combinators in between universal selectors.

https://reviewboard.mozilla.org/r/172812/#review178294

::: servo/components/selectors/parser.rs
(Diff revision 1)
>                              // Iterate over everything so we serialize the namespace
>                              // too.
>                              for simple in compound.iter() {
>                                  simple.to_css(dest)?;
>                              }
> -                            continue

This seems wrong, or I’m missing something. The 'for' loop just above this 'continue' is intended to run instead of the one a few lines below, not in addition to. Why remove this 'continue'?
Right, this is a bit confusing. For cache locality, we want the Selector struct to contain a flat array of 'Component's (mixed simple selectors and combinators) stored in matching order. At the level of compound selectors (the things separated by combinators), we need matching to be right-to-left (in CSS source order) because we’re starting from the DOM node being matched and then walking up the tree. Within one compound selector the simple selectors can be matched in any order but we do it left-to-right because for example an element name can be matched more quickly than an attribute lookup, and we can stop at the first negative result.

This is to obtain this non-linear ordering without doing extra memory allocation during parsing that SelectorBuilder is so convoluted. It stores a flat array of simple selectors, and separately tuples of (combinators, number of simple selectors since the previous combinator or start of the selector).
Flags: needinfo?(simon.sapin)
Comment on attachment 8901350 [details]
Bug 1391169 Part 1: Servo-side change Selector to_css function to handle combinators in between universal selectors.

https://reviewboard.mozilla.org/r/172812/#review178294

> This seems wrong, or I’m missing something. The 'for' loop just above this 'continue' is intended to run instead of the one a few lines below, not in addition to. Why remove this 'continue'?

You're right -- I was oversimplifying. The updated version behaves correctly for the test cases I can think to exercise. I added some tests within Servo to further highlight the issue, which was this: The four step algorithm at https://drafts.csswg.org/cssom/#serializing-selectors is written in the form "1, otherwise 2, 3, 4". The current implementation treats this as "either 1 and 3 and 4, or 2" and this patch implements this as "either 1 or 2, then 3 and 4", which I believe is the correct interpretation. It appears to be the necessary interpretation to correctly serialize "* > *".
Priority: -- → P2
(In reply to Brad Werth [:bradwerth] from comment #13)

> The four step algorithm at
> https://drafts.csswg.org/cssom/#serializing-selectors is written in the form
> "1, otherwise 2, 3, 4". The current implementation treats this as "either 1
> and 3 and 4, or 2" 

Correction: the current implementation does "either 1, or 2 and 3 and 4".
Comment on attachment 8901350 [details]
Bug 1391169 Part 1: Servo-side change Selector to_css function to handle combinators in between universal selectors.

https://reviewboard.mozilla.org/r/172812/#review180314
Attachment #8901350 - Flags: review?(simon.sapin) → review+
Comment on attachment 8900930 [details]
Bug 1391169 Part 2: Add a test of cssText with combinations of namespaces, universal selectors, and combinators.

https://reviewboard.mozilla.org/r/172370/#review180316
Attachment #8900930 - Flags: review?(simon.sapin) → review+
Looks good. Please make a pull request on https://github.com/servo/servo/ for part 1 and ping me or one of the other Servo reviewers to mark it r=SimonSapin.
Attachment #8901350 - Attachment is obsolete: true
Pushed by simon.sapin@exyr.org:
https://hg.mozilla.org/integration/autoland/rev/e68ecb05643f
Part 2: Add a test of cssText with combinations of namespaces, universal selectors, and combinators. r=SimonSapin
https://hg.mozilla.org/mozilla-central/rev/e68ecb05643f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Too late for 56. Mass won't fix for 56.
You need to log in before you can comment on or make changes to this bug.