Don't persist styles on elements not in the document. r?emilio draft
authorCameron McCormack <cam@mcc.id.au>
Thu, 29 Dec 2016 15:04:45 +0800
changeset 454386 c410308ed8a51649f8ecca8c085ff778a128c129
parent 454385 34562a53079525f3dec755ebc861068e1838e9c2
child 540691 756f2529d4961d4d6ab2fe5ead25d98b0efaf37a
push id39910
push userbmo:cam@mcc.id.au
push dateThu, 29 Dec 2016 07:06:54 +0000
reviewersemilio
milestone53.0a1
Don't persist styles on elements not in the document. r?emilio MozReview-Commit-ID: ExR7rnNRnGn
servo/components/script/layout_wrapper.rs
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/gecko_bindings/bindings.rs
servo/components/style/traversal.rs
--- a/servo/components/script/layout_wrapper.rs
+++ b/servo/components/script/layout_wrapper.rs
@@ -194,16 +194,20 @@ impl<'ln> TNode for ServoLayoutNode<'ln>
         self.node.set_flag(CAN_BE_FRAGMENTED, value)
     }
 
     fn parent_node(&self) -> Option<ServoLayoutNode<'ln>> {
         unsafe {
             self.node.parent_node_ref().map(|node| self.new_with_this_lifetime(&node))
         }
     }
+
+    fn is_in_doc(&self) -> bool {
+        self.node.is_in_doc()
+    }
 }
 
 pub struct ServoChildrenIterator<'a> {
     current: Option<ServoLayoutNode<'a>>,
 }
 
 impl<'a> Iterator for ServoChildrenIterator<'a> {
     type Item = ServoLayoutNode<'a>;
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -94,16 +94,18 @@ pub trait TNode : Sized + Copy + Clone +
 
     unsafe fn set_dirty_on_viewport_size_changed(&self);
 
     fn can_be_fragmented(&self) -> bool;
 
     unsafe fn set_can_be_fragmented(&self, value: bool);
 
     fn parent_node(&self) -> Option<Self>;
+
+    fn is_in_doc(&self) -> bool;
 }
 
 /// Wrapper to output the ElementData along with the node when formatting for
 /// Debug.
 pub struct ShowData<N: TNode>(pub N);
 impl<N: TNode> Debug for ShowData<N> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         fmt_with_data(f, self.0)
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -152,16 +152,20 @@ impl<'ln> TNode for GeckoNode<'ln> {
         // FIXME(SimonSapin): Servo uses this to implement CSS multicol / fragmentation
         // Maybe this isn’t useful for Gecko?
     }
 
     fn parent_node(&self) -> Option<Self> {
         unsafe { bindings::Gecko_GetParentNode(self.0).map(GeckoNode) }
     }
 
+    fn is_in_doc(&self) -> bool {
+        unsafe { bindings::Gecko_IsInDocument(self.0) }
+    }
+
     fn needs_dirty_on_viewport_size_changed(&self) -> bool {
         // Gecko's node doesn't have the DIRTY_ON_VIEWPORT_SIZE_CHANGE flag,
         // so we force them to be dirtied on viewport size change, regardless if
         // they use viewport percentage size or not.
         // TODO(shinglyu): implement this in Gecko: https://github.com/servo/servo/pull/11890
         true
     }
 
--- a/servo/components/style/gecko_bindings/bindings.rs
+++ b/servo/components/style/gecko_bindings/bindings.rs
@@ -263,16 +263,19 @@ extern "C" {
 }
 extern "C" {
     pub fn Gecko_ChildrenCount(node: RawGeckoNodeBorrowed) -> u32;
 }
 extern "C" {
     pub fn Gecko_NodeIsElement(node: RawGeckoNodeBorrowed) -> bool;
 }
 extern "C" {
+    pub fn Gecko_IsInDocument(node: RawGeckoNodeBorrowed) -> bool;
+}
+extern "C" {
     pub fn Gecko_GetParentNode(node: RawGeckoNodeBorrowed)
      -> RawGeckoNodeBorrowedOrNull;
 }
 extern "C" {
     pub fn Gecko_GetFirstChild(node: RawGeckoNodeBorrowed)
      -> RawGeckoNodeBorrowedOrNull;
 }
 extern "C" {
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -320,26 +320,33 @@ pub fn resolve_style<E, F, G, H>(context
     // Resolve styles up the tree.
     let display_none_root = resolve_style_internal(context, element, ensure_data);
 
     // Make them available for the scope of the callback. The callee may use the
     // argument, or perform any other processing that requires the styles to exist
     // on the Element.
     callback(element.borrow_data().unwrap().styles());
 
-    // Clear any styles in display:none subtrees to leave the tree in a valid state.
-    if let Some(root) = display_none_root {
+    // Clear any styles in display:none subtrees or subtrees not in the document,
+    // to leave the tree in a valid state.  For display:none subtrees, we leave
+    // the styles on the display:none root, but for subtrees not in the document,
+    // we clear styles all the way up to the root of the disconnected subtree.
+    let in_doc = element.as_node().is_in_doc();
+    if !in_doc || display_none_root.is_some() {
         let mut curr = element;
         loop {
             unsafe { curr.unset_dirty_descendants(); }
-            if curr == root {
+            if in_doc && curr == display_none_root.unwrap() {
                 break;
             }
             clear_data(curr);
-            curr = curr.parent_element().unwrap();
+            curr = match curr.parent_element() {
+                Some(parent) => parent,
+                None => break,
+            };
         }
     }
 }
 
 /// Calculates the style for a single node.
 #[inline]
 #[allow(unsafe_code)]
 pub fn recalc_style_at<E, D>(traversal: &D,