Skip to content

Commit

Permalink
Transitively deparent on removal
Browse files Browse the repository at this point in the history
Chrome and Firefox do this already, probably a spec oversight
  • Loading branch information
Manishearth committed Nov 23, 2016
1 parent 53c9966 commit cada5d7
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 8 deletions.
7 changes: 6 additions & 1 deletion components/script/dom/csskeyframesrule.rs
Expand Up @@ -55,7 +55,8 @@ impl CSSKeyframesRule {
}

impl CSSKeyframesRuleMethods for CSSKeyframesRule {
fn CssRules(&self) -> Root<CSSRuleList> {
// https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-cssrules
fn CssRules(&self) -> Root<CSSRuleList> {
self.rulelist()
}
}
Expand All @@ -69,4 +70,8 @@ impl SpecificCSSRule for CSSKeyframesRule {
fn get_css(&self) -> DOMString {
self.keyframesrule.read().to_css_string().into()
}

fn deparent_children(&self) {
self.rulelist.get().map(|list| list.deparent_all());
}
}
20 changes: 16 additions & 4 deletions components/script/dom/cssrule.rs
Expand Up @@ -9,8 +9,8 @@ use dom::bindings::js::{JS, MutNullableHeap, Root};
use dom::bindings::reflector::{Reflector, reflect_dom_object};
use dom::bindings::str::DOMString;
use dom::cssfontfacerule::CSSFontFaceRule;
use dom::csskeyframesrule::CSSKeyframesRule;
use dom::csskeyframerule::CSSKeyframeRule;
use dom::csskeyframesrule::CSSKeyframesRule;
use dom::cssmediarule::CSSMediaRule;
use dom::cssnamespacerule::CSSNamespaceRule;
use dom::cssstylerule::CSSStyleRule;
Expand Down Expand Up @@ -78,10 +78,18 @@ impl CSSRule {
}

/// Sets owner sheet/rule to null
pub fn disown(&self) {
self.parent.set(None);
pub fn detach(&self) {
self.deparent();
// should set parent rule to None when we add parent rule support
// Should we disown children as well? (https://github.com/w3c/csswg-drafts/issues/722)
}

/// Sets owner sheet to null (and does the same for all children)
pub fn deparent(&self) {
self.parent.set(None);
// https://github.com/w3c/csswg-drafts/issues/722
// Spec doesn't ask us to do this, but it makes sense
// and browsers implement this behavior
self.as_specific().deparent_children();
}
}

Expand Down Expand Up @@ -110,4 +118,8 @@ impl CSSRuleMethods for CSSRule {
pub trait SpecificCSSRule {
fn ty(&self) -> u16;
fn get_css(&self) -> DOMString;
/// Remove CSSStyleSheet parent from all transitive children
fn deparent_children(&self) {
// most CSSRules do nothing here
}
}
11 changes: 9 additions & 2 deletions components/script/dom/cssrulelist.rs
Expand Up @@ -9,8 +9,8 @@ use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
use dom::bindings::error::{Error, ErrorResult, Fallible};
use dom::bindings::js::{JS, MutNullableHeap, Root};
use dom::bindings::reflector::{Reflectable, Reflector, reflect_dom_object};
use dom::cssrule::CSSRule;
use dom::csskeyframerule::CSSKeyframeRule;
use dom::cssrule::CSSRule;
use dom::cssstylesheet::CSSStyleSheet;
use dom::window::Window;
use parking_lot::RwLock;
Expand Down Expand Up @@ -166,10 +166,17 @@ impl CSSRuleList {

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

// Remove parent stylesheets from all children
pub fn deparent_all(&self) {
for rule in self.dom_rules.borrow().iter() {
rule.get().map(|r| Root::upcast(r).deparent());
}
}
}

impl CSSRuleListMethods for CSSRuleList {
Expand Down
2 changes: 1 addition & 1 deletion components/script/dom/mod.rs
Expand Up @@ -244,8 +244,8 @@ pub mod crypto;
pub mod css;
pub mod cssfontfacerule;
pub mod cssgroupingrule;
pub mod csskeyframesrule;
pub mod csskeyframerule;
pub mod csskeyframesrule;
pub mod cssmediarule;
pub mod cssnamespacerule;
pub mod cssrule;
Expand Down

0 comments on commit cada5d7

Please sign in to comment.