[WIP] Fix bug #17182: "Simplify `an+b` when serializing" #17241

jyc · 2017-06-08T18:34:26Z

Simplify serialization of an+b, and implement CSSStyleRule.selectorText and fix serialization of namespace components which map to the default one so that we are able to pass the existing web platform tests.

--

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #17182
There are tests for these changes (existing cssom WPT)

This change is

highfive · 2017-06-08T18:34:32Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

highfive · 2017-06-08T18:34:33Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/node.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/characterdata.rs, components/script/dom/nodelist.rs and 1 more
@KiChjang: components/script/dom/node.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/characterdata.rs, components/script/dom/nodelist.rs and 1 more


        Correctly test CSSStyleRule.style.

Remove the assert_readonly test and add one to verify that assigning to CSSStyleRule.style correctly forwards to CSSStyleRule.style.cssText. We currently test for whether CSSStyleRule.style is read-only by trying to assign to it; however, the spec has it as actually: interface CSSStyleRule : CSSRule { ... [SameObject, PutForwards=cssText] readonly attribute CSSStyleDeclaration style; }; See: https://drafts.csswg.org/cssom/ The `PutForwards=cssText` means that assigning to CSSStyleRule.style should actually assign to style.cssText.


        Replace Namespace components which map to the default namespace with …

…DefaultNamespace. It seems we don't need to preserve the original prefix name, and this conveniently leads to_css to now implement CSSOM's requirement to elide the namespace prefix during serialization when it maps to the default namespace. https://drafts.csswg.org/cssom/#serialize-a-simple-selector


        Simplify <an+b> in selector args when serializing.


        Add variant ChildrenMutation::CharacterData for when a CharacterData …

…is modified. A future commit in this series will have CharacterData use this variant in calling children_changed on it's parent, so that HTMLStyleElement parents can re-parse.

Manishearth · 2017-06-08T20:10:41Z

components/script/dom/cssstylerule.rs

+
+    // https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext
+    fn SetSelectorText(&self, value: DOMString) {
+        let namespaces = Namespaces::default();


@upsuper when setting selectors should we use the default namespace map or get the map from the stylesheet?

(for future readers, there is now an issue here: w3c/csswg-drafts#1511)

Manishearth · 2017-06-08T20:10:50Z

so far looks good!

jyc · 2017-06-08T23:05:36Z

Weird! The test that CI is showing as failing actually seems to be contrary to what CSSOM says!

If there is an attribute value specified, append "=", "~=", "|=", "^=", "$=", or "*=" as appropriate (depending on the type of attribute selector), followed by the serialization of the attribute value as a string, to s.

See the "attribute selector" section of https://drafts.csswg.org/cssom/#serializing-selectors .

Going to check that.

UPDATE: Line 1959 of components/selectors/parser.rs tests the parsing of [attr |= "foo"], and in particular expects the serialization to be identical. But CSSOM doesn't say to add the space in between (and none of Chrome, Firefox, or Safari do this). So it seems like this test is wrong, and I'll modify the test in the commit that also modifies the output of operators.


        Have CharacterData call children_changed on its parent when data is set.

This fixes an issue where an empty <style> element's data is set but the style is not updated. An HTMLStyleElement parent re-parses in its children_changed implementation. The ChildrenMutation::CharacterData variant introduced in the previous commit is used.


        Implement CSSStyleRule.selectorText.

We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext


        Force HTMLStyleElement to create a new CSSStyleSheet when re-parsing.

We already call Document::invalidate_style_sheets and set the stylesheet member to a new Stylesheet. This matches the behavior of Firefox, and means the new CSSStyleSheet you get from accessing .sheet from JS will be correct instead of stale. (::get_cssom_stylesheet already tries to use the new Stylesheet, but MutNullableJS::or_init is called, so if we already created a CSSStyleSheet we will continue to return that one). ./mach update-wpt also made some formatting changes to: - tests/wpt/mozilla/meta/mozilla/webgl/context_creation_error.html.ini - tests/wpt/mozilla/meta/mozilla/worklets/test_paint_worklet.html.ini ... for some reason (removed quotation marks and removed a line).


        Enable "selectors" Web Platform Tests.

jyc · 2017-06-09T03:41:05Z

Oops, the selectorText commit actually fixes Bug #17181 as well (did that so that the selectors tests would pass). Can remove that from this if that's better.

Manishearth · 2017-06-09T03:44:16Z

It's already in #17203 so yeah, remove it for now. We'll try to land that first.

Manishearth · 2017-06-09T03:44:26Z

@bors-servo try

bors-servo · 2017-06-09T03:44:30Z

⌛ Trying commit 9b73014 with merge d2a89f1...


        Auto merge of #17241 - jyc:an-b, r=<try>

[WIP] Fix bug #17182: "Simplify `an+b` when serializing" Simplify serialization of `an+b`, and implement CSSStyleRule.selectorText and fix serialization of namespace components which map to the default one so that we are able to pass the existing web platform tests. -- - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #17182 - [X] There are tests for these changes (existing cssom WPT)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17241)

bors-servo · 2017-06-09T04:22:53Z

💔 Test failed - linux-rel-css

jyc · 2017-06-09T04:28:47Z

Weird! Does this mean I need to update expected results? (Seems like it's all expected failures becoming passes). Some of those seem to be things I didn't update, but the serialize-namespaced-type-selectors.htm test should already have its manifest updated.

upsuper · 2017-06-09T04:46:11Z

Yes. You need to remove the corresponding meta files (or some lines in those files) from https://github.com/servo/servo/tree/master/tests/wpt/metadata-css

jyc · 2017-06-09T04:55:56Z

Hi @upsuper ! Don't know if you remember me, but we met briefly at the All Hands last year, good to see you (in a way) again!

Cool, I will try that when I head in tomorrow, along with removing the repeated work from the other bug.

emilio · 2017-06-09T05:29:17Z

components/script/dom/cssstylerule.rs

+            let mut stylerule = self.stylerule.write_with(&mut guard);
+            mem::swap(&mut stylerule.selectors, &mut s);
+        }
+    }


This needs to somehow invalidate stylesheets, how do changes to the rules get reflected on the page otherwise?

You're right!

emilio · 2017-06-09T05:31:01Z

components/selectors/parser.rs

-            NthLastOfType(a, b) => write!(dest, ":nth-last-of-type({}n{:+})", a, b),
+            NthChild(a, b) | NthLastChild(a, b) | NthOfType(a, b) | NthLastOfType(a, b) => {
+                match *self {
+                    NthChild(_, _) => write!(dest, ":nth-child(")?,


nit: use dest.write_str instead.

emilio · 2017-06-09T05:31:01Z

components/selectors/parser.rs

@@ -1929,7 +1956,7 @@ pub mod tests {
                ].into_boxed_slice())
            ), specificity(0, 0, 1))
        ))));
-        assert_eq!(parse("[attr |= \"foo\"]"), Ok(SelectorList::from_vec(vec!(
+        assert_eq!(parse("[attr|=\"foo\"]"), Ok(SelectorList::from_vec(vec!(


Does this test need to be changed? Whitespace should be ignored while parsing, is it testing also that serialization roundtrips?

nox · 2017-06-09T08:57:58Z

I'm confused, why are there so many commits in this PR that seem unrelated to what the PR title claim to fix?

nox · 2017-06-09T09:00:41Z

components/script/dom/node.rs

+    /// This doesn't change the structure of the list, which is what the other
+    /// variants' fields are stored for at the moment, so this can just have no
+    /// arguments.
+    CharacterData,


This should be Text, we never have to reparse anything if a Comment or a ProcessingInstruction is changed.

nox · 2017-06-09T09:02:00Z

I would greatly appreciate it if this was split in at least 2 PRs, ~~and if you squashed "Have CharacterData call children_changed on its parent when data is set." and "Force HTMLStyleElement to create a new CSSStyleSheet when re-parsing." together.~~

And also if you squashed the two following commits:

Don't make separate commits when one of them introduce code that isn't yet called anywhere.

nox · 2017-06-09T09:04:35Z

components/script/dom/characterdata.rs

+        if let Some(parent_node) = node.GetParentNode() {
+            let mutation = ChildrenMutation::CharacterData;
+            vtable_for(&parent_node).children_changed(&mutation);
+        }


This should be done only for Text nodes.

jyc · 2017-06-09T17:06:09Z

Splitting up the pull request will mean that the parts from the end will fail CI until everything else has been merged--I've split it up into all of its parts, and can just wait for that before pushing.

jyc · 2017-06-09T18:44:06Z

OK, I have split up the PR into different parts. I think we will need to land them in something this particular order to avoid intermediate test failures: (will push WIP PRs to correct test expectations as we go)

new-CSSStyleSheet, CSSStyleRule-style-test
Text-children_changed
default-namespace-serialization
selectorText, simplify-an-b
selectors-wpt

highfive assigned nox Jun 8, 2017

highfive added the S-awaiting-review label Jun 8, 2017

jyc added 4 commits Jun 7, 2017

Simplify <an+b> in selector args when serializing.

4de74f6

Add variant ChildrenMutation::CharacterData for when a CharacterData …

7f0a269

…is modified. A future commit in this series will have CharacterData use this variant in calling children_changed on it's parent, so that HTMLStyleElement parents can re-parse.

jyc force-pushed the jyc:an-b branch from 5f086cb to 81e1f67 Jun 8, 2017

Manishearth reviewed Jun 8, 2017

View changes

jyc force-pushed the jyc:an-b branch 2 times, most recently from a18b928 to fd92c02 Jun 8, 2017

jyc force-pushed the jyc:an-b branch from fd92c02 to dec3a49 Jun 8, 2017

jyc added 4 commits Jun 7, 2017

Enable "selectors" Web Platform Tests.

Loading status checks…

9b73014

jyc force-pushed the jyc:an-b branch from dec3a49 to 9b73014 Jun 8, 2017

highfive added the S-tests-failed label Jun 9, 2017

emilio reviewed Jun 9, 2017

View changes

nox reviewed Jun 9, 2017

View changes

jyc closed this Jun 9, 2017

jyc deleted the jyc:an-b branch Jun 9, 2017

servo / servo

[WIP] Fix bug #17182: "Simplify `an+b` when serializing" #17241

[WIP] Fix bug #17182: "Simplify `an+b` when serializing" #17241

jyc commented Jun 8, 2017 •

edited by larsbergstrom

highfive commented Jun 8, 2017

highfive commented Jun 8, 2017

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 8, 2017

jyc commented Jun 8, 2017 •

edited

jyc commented Jun 9, 2017

Manishearth commented Jun 9, 2017

Manishearth commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jyc commented Jun 9, 2017

upsuper commented Jun 9, 2017

jyc commented Jun 9, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Jun 9, 2017

This comment has been minimized.

nox commented Jun 9, 2017 •

edited

This comment has been minimized.

jyc commented Jun 9, 2017

jyc commented Jun 9, 2017 •

edited

servo / servo

Join GitHub today

[WIP] Fix bug #17182: "Simplify `an+b` when serializing" #17241

[WIP] Fix bug #17182: "Simplify `an+b` when serializing" #17241

Conversation

jyc commented Jun 8, 2017 • edited by larsbergstrom

highfive commented Jun 8, 2017

highfive commented Jun 8, 2017

This comment has been minimized.

This comment has been minimized.

Manishearth commented Jun 8, 2017

jyc commented Jun 8, 2017 • edited

jyc commented Jun 9, 2017

Manishearth commented Jun 9, 2017

Manishearth commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jyc commented Jun 9, 2017

upsuper commented Jun 9, 2017

jyc commented Jun 9, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Jun 9, 2017

This comment has been minimized.

nox commented Jun 9, 2017 • edited

This comment has been minimized.

jyc commented Jun 9, 2017

jyc commented Jun 9, 2017 • edited

jyc commented Jun 8, 2017 •

edited by larsbergstrom

jyc commented Jun 8, 2017 •

edited

nox commented Jun 9, 2017 •

edited

jyc commented Jun 9, 2017 •

edited