CSSOM: Whole ton of things #14241

Manishearth · 2016-11-16T00:30:48Z

CSSOM is now starting to be useful!

Based on #14190. Only the ~~last commit~~ last two commits need review.

cc @Xidorn . This doesn't change the style API, but adds useful methods.

part of #11420

This adds:

insertRule() and deleteRule() on CSSStyleSheet, CSSGroupingRule
.style getters on link and style elements
Keyframes-backed CSSRules and CSSKeyframesRule.cssRules
CSSGroupingRule.cssRules
prefix and namespaceURI attributes of CSSNamespaceRule
Fixups regarding parent stylesheets

r? @SimonSapin

This change is

highfive · 2016-11-16T00:30:51Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/stylesheets.rs
@jgraham: tests/wpt/harness/test/metadata/testharness/firefox/test_pref_set.html.ini, tests/wpt/harness/test/metadata/testharness/firefox/subdir/test_pref_reset.html.ini, tests/wpt/harness/test/metadata/testharness/tags/testharness_2.html.ini, tests/wpt/harness/test/metadata/testharness/subdir/testharness_1.html.ini, tests/wpt/harness/test/metadata/testharness/tags/testharness_1.html.ini, tests/wpt/harness/test/metadata/testharness/tags/testharness_0.html.ini
@wafflespeanut: python/tidy/servo_tidy/tidy.py
@emilio: components/style/selector_matching.rs, ports/geckolib/glue.rs, components/style/stylesheets.rs
@fitzgen: components/script/dom/webidls/CSSStyleDeclaration.webidl, components/script/dom/webidls/CSSRule.webidl, components/script/dom/cssfontfacerule.rs, components/script/dom/mod.rs, components/script/dom/csskeyframesrule.rs, components/script/dom/webidls/StyleSheetList.webidl, components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/cssmediarule.rs, components/script/dom/cssgroupingrule.rs, components/script/dom/cssrulelist.rs, components/script/dom/webidls/CSSStyleSheet.webidl, components/script/dom/cssviewportrule.rs, components/script/dom/stylesheetlist.rs, components/script/dom/stylesheet.rs, components/script/dom/cssrule.rs, components/script/dom/webidls/CSS.webidl, components/script/dom/htmlstyleelement.rs, components/script/dom/webidls/CSSFontFaceRule.webidl, components/script/dom/webidls/CSSMediaRule.webidl, components/script/dom/webidls/CSSGroupingRule.webidl, components/script/dom/htmlmetaelement.rs, components/script/dom/node.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/webidls/StyleSheet.webidl, components/script/dom/webidls/CSSViewportRule.webidl, components/script/dom/webidls/CSSKeyframesRule.webidl, components/script/dom/cssnamespacerule.rs, components/script/dom/webidls/CSSNamespaceRule.webidl, components/script/dom/cssstylesheet.rs, components/script/dom/webidls/CSSRuleList.webidl
@KiChjang: components/script/dom/webidls/CSSStyleDeclaration.webidl, components/script/dom/webidls/CSSRule.webidl, components/script/dom/cssfontfacerule.rs, components/script/dom/mod.rs, components/script/dom/csskeyframesrule.rs, components/script/dom/webidls/StyleSheetList.webidl, components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/cssmediarule.rs, components/script/dom/cssgroupingrule.rs, components/script/dom/cssrulelist.rs, components/script/dom/webidls/CSSStyleSheet.webidl, components/script/dom/cssviewportrule.rs, components/script/dom/stylesheetlist.rs, components/script/dom/stylesheet.rs, components/script/dom/cssrule.rs, components/script/dom/webidls/CSS.webidl, components/script/dom/htmlstyleelement.rs, components/script/dom/webidls/CSSFontFaceRule.webidl, components/script/dom/webidls/CSSMediaRule.webidl, components/script/dom/webidls/CSSGroupingRule.webidl, components/script/dom/htmlmetaelement.rs, components/script/dom/node.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/webidls/StyleSheet.webidl, components/script/dom/webidls/CSSViewportRule.webidl, components/script/dom/webidls/CSSKeyframesRule.webidl, components/script/dom/cssnamespacerule.rs, components/script/dom/webidls/CSSNamespaceRule.webidl, components/script/dom/cssstylesheet.rs, components/script/dom/webidls/CSSRuleList.webidl

Manishearth · 2016-11-16T00:31:36Z

components/script/dom/cssrulelist.rs

+
+        // Step 1, 2
+        // XXXManishearth get url from correct location
+        // XXXManishearth should we also store the namespace map?


I'm considering just filing issues for these

Manishearth · 2016-11-16T00:31:52Z

components/script/dom/cssrulelist.rs

+                return Err(Error::IndexSize);
+            }
+
+            // XXXManishearth Step 5 (throw HierarchyRequestError in invalid situations)


spec is very vague here. will have to check against gecko (and file an issue)

Manishearth · 2016-11-16T01:25:35Z

@bors-servo try

bors-servo · 2016-11-16T01:25:38Z

⌛ Trying commit a5ccd59 with merge 3e29c46...


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)

bors-servo · 2016-11-16T01:55:18Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2016-11-16T01:55:20Z

⌛ Trying commit a5ccd59 with merge 7fbc165...


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)

bors-servo · 2016-11-16T03:20:33Z

💔 Test failed - linux-rel-css

Manishearth · 2016-11-16T03:38:14Z

Also http://build.servo.org/builders/linux-rel-wpt/builds/862

SimonSapin · 2016-11-16T19:40:40Z

Looks good modulo inline comments. (Opening this before #14190 is merged makes review slightly harder…)

Review status: 0 of 75 files reviewed at latest revision, 7 unresolved discussions.

components/script/dom/cssrulelist.rs, line 79 at r6 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

spec is very vague here. will have to check against gecko (and file an issue)

The rules are:

@charset, if present, must be first (but it’s not present in the OM so don’t worry about it)
@import must be before everything other than @charset https://drafts.csswg.org/css-cascade/#at-import
@namespace must be before everything other than @charset or @import https://drafts.csswg.org/css-namespaces/#syntax
None of the above can appear inside a conditional rule (@media or @supports) https://drafts.csswg.org/css-conditional/#contents-of

Tracking this is what the split between TopLevelRuleParser and NestedRuleParser as well as the TopLevelRuleParser.state field are for. As mentioned in another comment, some things probably need to be moved around for this.

components/script/dom/cssrulelist.rs, line 82 at r6 (raw file):

            // Step 6
            if let StyleCssRule::Namespace(..) = new_rule {

Something implicit in CSSOM is that namespace prefixes in selectors in a stylesheet are based on all @namespace rules in the stylesheet, even inserted by CSSOM. So this code will need to update the NamespaceMap that is currently only maintained during stylesheet parsing but will need to be kept in the Stylesheet struct.

(This is why this InvalidStateError exception exists: so that selectors don’t need to be re-parsed.)

components/script/dom/cssrulelist.rs, line 115 at r6 (raw file):

        let mut dom_rules = self.dom_rules.borrow_mut();
        self.rules.0.write().remove(index);
        dom_rules[index].get().map(|r| r.disown());

Nit: Vec::remove returns the removed element, disown can be called on that.

components/style/stylesheets.rs, line 136 at r6 (raw file):

    }

    pub fn from_str(css: &str, origin: Origin,

Nit: I’d prefer to call this parse.

components/style/stylesheets.rs, line 139 at r6 (raw file):

                    base_url: Url, extra_data: ParserContextExtraData) -> Result<Self, ()> {
        let error_reporter = Box::new(MemoryHoleReporter);
        let rule_parser = TopLevelRuleParser {

TopLevelRuleParser is what you want for CSSStyleSheet.insertRule, but for CSSGroupingRule.insertRule it’s NestedRuleParser. Probably these two Rust types should be merged, with a boolean field or something to tell both cases apart. This boolean would be an additional parameter to this method.

If you’d rather not do this now I can do it in a follow-up PR.

components/style/stylesheets.rs, line 145 at r6 (raw file):

        };
        let mut input = Parser::new(css);
        let mut iter = RuleListParser::new_for_stylesheet(&mut input, rule_parser);

This should use cssparser::parse_one_rule instead.

Comments from Reviewable

Manishearth · 2016-11-16T21:46:25Z

Review status: 0 of 75 files reviewed at latest revision, 7 unresolved discussions.

components/script/dom/cssrulelist.rs, line 115 at r6 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Nit: Vec::remove returns the removed element, disown can be called on that.

I'm wary of GC happening within that step.

components/style/stylesheets.rs, line 139 at r6 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

TopLevelRuleParser is what you want for CSSStyleSheet.insertRule, but for CSSGroupingRule.insertRule it’s NestedRuleParser. Probably these two Rust types should be merged, with a boolean field or something to tell both cases apart. This boolean would be an additional parameter to this method.

If you’d rather not do this now I can do it in a follow-up PR.

I didn't merge them, instead I just changed CssRule::parse to accept a nested parameter

Comments from Reviewable

Manishearth · 2016-11-16T22:45:50Z

Made it handle state (and other review fixups)

Manishearth · 2016-11-16T23:05:20Z

@bors-servo try


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)

bors-servo · 2016-11-16T23:05:25Z

⌛ Trying commit 7090797 with merge 852e589...

Manishearth · 2016-11-17T01:33:43Z

Added support for keyframe-backed CSSRuleLists, and CSSKeyframeRule. No insertion/deletion from CSSKeyframesRules yet (but it's easy to implement).


        Add support for LinkStyle's sheet getter on <style> and <link>


        Update test expectations


        Handle parser state in CSSOM insert_rule


        Add support for keyframe-backed CSSRules, CSSKeyframesRule.cssRules a…

…nd CSSKeyframeRule


        Make parent stylesheet optional for CSSRules


        Transitively deparent on removal

Chrome and Firefox do this already, probably a spec oversight


        Add CSSGroupingRule.{cssRules, insertRule(), deleteRule()}


        Add CSSKeyframesRule.{findRule, deleteRule, appendRule}


        Add CSSNamespaceRule.{namespaceURI, prefix}


        Fixup error from #14238


        Update test expectations


        Address review comments

Manishearth · 2016-11-23T18:16:40Z

@bors-servo r=SimonSapin

bors-servo · 2016-11-23T18:16:41Z

📌 Commit 95cbf1a has been approved by SimonSapin

bors-servo · 2016-11-23T18:27:50Z

⌛ Testing commit 95cbf1a with merge 9967566...


        Auto merge of #14241 - Manishearth:mut-cssom, r=SimonSapin

@Xidorn

CSSOM: Whole ton of things CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 This adds: - `insertRule()` and `deleteRule()` on `CSSStyleSheet`, `CSSGroupingRule` - `.style` getters on link and style elements - Keyframes-backed `CSSRules` and `CSSKeyframesRule.cssRules` - `CSSGroupingRule.cssRules` - `prefix` and `namespaceURI` attributes of `CSSNamespaceRule` - Fixups regarding parent stylesheets r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)

bors-servo · 2016-11-23T19:30:57Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

upsuper · 2016-11-23T23:53:52Z

You didn't successfully cc'ed me...

The algorithm of insertRule and deleteRule really should be implemented inside style component so that they can be reused for stylo...

Manishearth · 2016-11-23T23:57:19Z

I tried to make it as reusable as possible. Because of the parallel dom tree the algorithm involves DOM code as well and is hard to separate. The parsing/insertion validity algorithms are in stylesheets.rs though.

highfive assigned SimonSapin Nov 16, 2016

highfive added the S-awaiting-review label Nov 16, 2016

Manishearth reviewed Nov 16, 2016

View changes

Manishearth changed the title ~~CSSOM mutation: Add insertRule() and deleteRule() on CSSStyleSheet~~ CSSOM mutation: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> Nov 16, 2016

Manishearth changed the title ~~CSSOM mutation: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link>~~ CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> Nov 16, 2016

Manishearth mentioned this pull request Nov 16, 2016

servo doesn't render plotly-generated html graphs #13828

Open

highfive added the S-tests-failed label Nov 16, 2016

highfive removed the S-tests-failed label Nov 16, 2016

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Nov 16, 2016

Manishearth force-pushed the Manishearth:mut-cssom branch from 8b26e8d to 7090797 Nov 16, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 16, 2016

Manishearth added 12 commits Nov 16, 2016

Add support for LinkStyle's sheet getter on <style> and <link>

972e9ac

Update test expectations

262408d

Handle parser state in CSSOM insert_rule

2793d5f

Add support for keyframe-backed CSSRules, CSSKeyframesRule.cssRules a…

52a3a71

…nd CSSKeyframeRule

Make parent stylesheet optional for CSSRules

53c9966

Transitively deparent on removal

cada5d7

Chrome and Firefox do this already, probably a spec oversight

Add CSSGroupingRule.{cssRules, insertRule(), deleteRule()}

a9cd17a

Add CSSKeyframesRule.{findRule, deleteRule, appendRule}

6444209

Add CSSNamespaceRule.{namespaceURI, prefix}

8108fc4

Fixup error from #14238

0e19f45

Update test expectations

18742e7

Address review comments

Loading status checks…

95cbf1a

Manishearth force-pushed the Manishearth:mut-cssom branch from adb7a3a to 95cbf1a Nov 23, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 23, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 23, 2016

bors-servo mentioned this pull request Nov 23, 2016

implement StyleSheet.disabled #14340

Merged

2 of 5 tasks complete

bors-servo merged commit 95cbf1a into servo:master Nov 23, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 23, 2016

Manishearth deleted the Manishearth:mut-cssom branch Nov 23, 2016

bors-servo mentioned this pull request Nov 23, 2016

Implement access to CSSStyleRule for stylo #14330

Merged

0 of 5 tasks complete

Manishearth mentioned this pull request Dec 3, 2016

twis84 servo/blog.servo.org#125

Merged

servo / servo

CSSOM: Whole ton of things #14241

CSSOM: Whole ton of things #14241

Manishearth commented Nov 16, 2016 •

edited

highfive commented Nov 16, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

Manishearth commented Nov 16, 2016

SimonSapin commented Nov 16, 2016

Manishearth commented Nov 16, 2016 •

edited

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

Manishearth commented Nov 17, 2016

Manishearth commented Nov 23, 2016

bors-servo commented Nov 23, 2016

bors-servo commented Nov 23, 2016

bors-servo commented Nov 23, 2016

upsuper commented Nov 23, 2016

Manishearth commented Nov 23, 2016

servo / servo

Join GitHub today

CSSOM: Whole ton of things #14241

CSSOM: Whole ton of things #14241

Conversation

Manishearth commented Nov 16, 2016 • edited

highfive commented Nov 16, 2016

This comment has been minimized.

Manishearth Nov 16, 2016

This comment has been minimized.

Manishearth Nov 16, 2016

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

Manishearth commented Nov 16, 2016

SimonSapin commented Nov 16, 2016

Manishearth commented Nov 16, 2016 • edited

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

Manishearth commented Nov 17, 2016

Manishearth commented Nov 23, 2016

bors-servo commented Nov 23, 2016

bors-servo commented Nov 23, 2016

bors-servo commented Nov 23, 2016

upsuper commented Nov 23, 2016

Manishearth commented Nov 23, 2016

Manishearth commented Nov 16, 2016 •

edited

Manishearth commented Nov 16, 2016 •

edited