style: Look at the snapshots when invalidating due to stylesheet changes.

Otherwise removal of stylesheets may get out of sync with other DOM changes, and
we may fail to invalidate the style of the affected elements.

Bug: 1432850
Reviewed-by: bz
MozReview-Commit-ID: DrMTgLzQcnk

components/layout_thread/lib.rs

@@ -1194,8 +1194,6 @@ impl LayoutThread {
                 debug!("Doc sheets changed, flushing author sheets too");
                 self.stylist.force_stylesheet_origins_dirty(Origin::Author.into());
             }
-
-            self.stylist.flush(&guards, Some(element));
         }
 
         if viewport_size_changed {
@@ -1246,6 +1244,8 @@ impl LayoutThread {
             debug!("Noting restyle for {:?}: {:?}", el, style_data);
         }
 
+        self.stylist.flush(&guards, Some(element), Some(&map));
+
         // Create a layout context for use throughout the following passes.
         let mut layout_context =
             self.build_layout_context(guards.clone(), true, &map);

components/style/gecko/data.rs

@@ -15,6 +15,7 @@ use invalidation::media_queries::{MediaListKey, ToMediaListKey};
 use malloc_size_of::MallocSizeOfOps;
 use media_queries::{Device, MediaList};
 use properties::ComputedValues;
+use selector_parser::SnapshotMap;
 use servo_arc::Arc;
 use shared_lock::{Locked, StylesheetGuards, SharedRwLockReadGuard};
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -202,6 +203,7 @@ impl PerDocumentStyleDataImpl {
         &mut self,
         guard: &SharedRwLockReadGuard,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> bool
     where
         E: TElement,
@@ -209,6 +211,7 @@ impl PerDocumentStyleDataImpl {
         self.stylist.flush(
             &StylesheetGuards::same(guard),
             document_element,
+            snapshots,
         )
     }
 

components/style/gecko/snapshot.rs

@@ -193,7 +193,8 @@ impl ElementSnapshot for GeckoElementSnapshot {
 
     #[inline]
     fn each_class<F>(&self, callback: F)
-        where F: FnMut(&Atom)
+    where
+        F: FnMut(&Atom)
     {
         if !self.has_any(Flags::MaybeClass) {
             return;

components/style/invalidation/element/element_wrapper.rs

@@ -71,7 +71,8 @@ pub struct ElementWrapper<'a, E>
 }
 
 impl<'a, E> ElementWrapper<'a, E>
-    where E: TElement,
+where
+    E: TElement,
 {
     /// Trivially constructs an `ElementWrapper`.
     pub fn new(el: E, snapshot_map: &'a SnapshotMap) -> Self {

components/style/invalidation/stylesheets.rs

@@ -8,12 +8,14 @@
 #![deny(unsafe_code)]
 
 use Atom;
+use CaseSensitivityExt;
 use LocalName as SelectorLocalName;
 use dom::{TElement, TNode};
 use fnv::FnvHashSet;
+use invalidation::element::element_wrapper::{ElementSnapshot, ElementWrapper};
 use invalidation::element::restyle_hints::RestyleHint;
 use media_queries::Device;
-use selector_parser::SelectorImpl;
+use selector_parser::{SelectorImpl, Snapshot, SnapshotMap};
 use selectors::attr::CaseSensitivity;
 use selectors::parser::{Component, LocalName, Selector};
 use shared_lock::SharedRwLockReadGuard;
@@ -43,21 +45,40 @@ impl Invalidation {
         matches!(*self, Invalidation::ID(..) | Invalidation::Class(..))
     }
 
-    fn matches<E>(&self, element: E) -> bool
+    fn matches<E>(&self, element: E, snapshot: Option<&Snapshot>) -> bool
-        where E: TElement,
+    where
+        E: TElement,
     {
+        // FIXME This should look at the quirks mode of the document to
+        // determine case sensitivity.
+        //
+        // FIXME(emilio): Actually write a test and fix this.
+        let case_sensitivity = CaseSensitivity::CaseSensitive;
         match *self {
             Invalidation::Class(ref class) => {
-                // FIXME This should look at the quirks mode of the document to
+                if element.has_class(class, case_sensitivity) {
-                // determine case sensitivity.
+                    return true;
-                element.has_class(class, CaseSensitivity::CaseSensitive)
+                }
+
+                if let Some(snapshot) = snapshot {
+                    if snapshot.has_class(class, case_sensitivity) {
+                        return true;
+                    }
+                }
             }
             Invalidation::ID(ref id) => {
-                match element.get_id() {
+                if let Some(ref element_id) = element.get_id() {
-                    // FIXME This should look at the quirks mode of the document
+                    if case_sensitivity.eq_atom(element_id, id) {
-                    // to determine case sensitivity.
+                        return true;
-                    Some(element_id) => element_id == *id,
+                    }
-                    None => false,
+                }
+
+                if let Some(snapshot) = snapshot {
+                    if let Some(ref old_id) = snapshot.id_attr() {
+                        if case_sensitivity.eq_atom(old_id, id) {
+                            return true;
+                        }
+                    }
                 }
             }
             Invalidation::LocalName { ref name, ref lower_name } => {
@@ -65,9 +86,11 @@ impl Invalidation {
                 // of testing against both names, but it's probably not worth
                 // it.
                 let local_name = element.get_local_name();
-                *local_name == **name || *local_name == **lower_name
+                return *local_name == **name || *local_name == **lower_name
             }
         }
+
+        false
     }
 }
 
@@ -145,11 +168,17 @@ impl StylesheetInvalidationSet {
     /// `document_element` is provided.
     ///
     /// Returns true if any invalidations ocurred.
-    pub fn flush<E>(&mut self, document_element: Option<E>) -> bool
+    pub fn flush<E>(
-        where E: TElement,
+        &mut self,
+        document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
+    ) -> bool
+    where
+        E: TElement,
     {
+        debug!("Stylist::flush({:?}, snapshots: {})", document_element, snapshots.is_some());
         let have_invalidations = match document_element {
-            Some(e) => self.process_invalidations(e),
+            Some(e) => self.process_invalidations(e, snapshots),
             None => false,
         };
         self.clear();
@@ -163,9 +192,17 @@ impl StylesheetInvalidationSet {
         self.fully_invalid = false;
     }
 
-    fn process_invalidations<E>(&self, element: E) -> bool
+    fn process_invalidations<E>(&self, element: E, snapshots: Option<&SnapshotMap>) -> bool
-        where E: TElement,
+    where
+        E: TElement,
     {
+        debug!(
+           "Stylist::process_invalidations({:?}, {:?}, {:?})",
+           element,
+           self.invalid_scopes,
+           self.invalid_elements,
+        );
+
         {
             let mut data = match element.mutate_data() {
                 Some(data) => data,
@@ -185,7 +222,7 @@ impl StylesheetInvalidationSet {
             return false;
         }
 
-        self.process_invalidations_in_subtree(element)
+        self.process_invalidations_in_subtree(element, snapshots)
     }
 
     /// Process style invalidations in a given subtree. This traverses the
@@ -194,9 +231,15 @@ impl StylesheetInvalidationSet {
     ///
     /// Returns whether it invalidated at least one element's style.
     #[allow(unsafe_code)]
-    fn process_invalidations_in_subtree<E>(&self, element: E) -> bool
+    fn process_invalidations_in_subtree<E>(
-        where E: TElement,
+        &self,
+        element: E,
+        snapshots: Option<&SnapshotMap>,
+    ) -> bool
+    where
+        E: TElement,
     {
+        debug!("process_invalidations_in_subtree({:?})", element);
         let mut data = match element.mutate_data() {
             Some(data) => data,
             None => return false,
@@ -212,8 +255,10 @@ impl StylesheetInvalidationSet {
             return false;
         }
 
+        let element_wrapper = snapshots.map(|s| ElementWrapper::new(element, s));
+        let snapshot = element_wrapper.as_ref().and_then(|e| e.snapshot());
         for invalidation in &self.invalid_scopes {
-            if invalidation.matches(element) {
+            if invalidation.matches(element, snapshot) {
                 debug!("process_invalidations_in_subtree: {:?} matched subtree {:?}",
                        element, invalidation);
                 data.hint.insert(RestyleHint::restyle_subtree());
@@ -225,7 +270,7 @@ impl StylesheetInvalidationSet {
 
         if !data.hint.contains(RestyleHint::RESTYLE_SELF) {
             for invalidation in &self.invalid_elements {
-                if invalidation.matches(element) {
+                if invalidation.matches(element, snapshot) {
                     debug!("process_invalidations_in_subtree: {:?} matched self {:?}",
                            element, invalidation);
                     data.hint.insert(RestyleHint::RESTYLE_SELF);
@@ -243,7 +288,8 @@ impl StylesheetInvalidationSet {
                 None => continue,
             };
 
-            any_children_invalid |= self.process_invalidations_in_subtree(child);
+            any_children_invalid |=
+                self.process_invalidations_in_subtree(child, snapshots);
         }
 
         if any_children_invalid {

components/style/stylesheet_set.rs

@@ -7,6 +7,7 @@
 use dom::TElement;
 use invalidation::stylesheets::StylesheetInvalidationSet;
 use media_queries::Device;
+use selector_parser::SnapshotMap;
 use shared_lock::SharedRwLockReadGuard;
 use std::slice;
 use stylesheets::{Origin, OriginSet, OriginSetIterator, PerOrigin, StylesheetInDocument};
@@ -502,6 +503,7 @@ where
     pub fn flush<'a, E>(
         &'a mut self,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> StylesheetFlusher<'a, S>
     where
         E: TElement,
@@ -510,7 +512,9 @@ where
 
         debug!("StylesheetSet::flush");
 
-        let had_invalidations = self.invalidations.flush(document_element);
+        let had_invalidations =
+            self.invalidations.flush(document_element, snapshots);
+
         let origins_dirty =
             mem::replace(&mut self.origins_dirty, OriginSet::empty());
 

components/style/stylist.rs

@@ -25,7 +25,7 @@ use properties::{AnimationRules, PropertyDeclarationBlock};
 use rule_cache::{RuleCache, RuleCacheConditions};
 use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
 use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry};
-use selector_parser::{SelectorImpl, PerPseudoElementMap, PseudoElement};
+use selector_parser::{SelectorImpl, SnapshotMap, PerPseudoElementMap, PseudoElement};
 use selectors::NthIndexCache;
 use selectors::attr::{CaseSensitivity, NamespaceConstraint};
 use selectors::bloom::{BloomFilter, NonCountingBloomFilter};
@@ -504,6 +504,7 @@ impl Stylist {
         &mut self,
         guards: &StylesheetGuards,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> bool
     where
         E: TElement,
@@ -548,7 +549,7 @@ impl Stylist {
             }
         }
 
-        let flusher = self.stylesheets.flush(document_element);
+        let flusher = self.stylesheets.flush(document_element, snapshots);
 
         let had_invalidations = flusher.had_invalidations();
 

ports/geckolib/glue.rs

@@ -1219,21 +1219,23 @@ pub extern "C" fn Servo_StyleSet_RemoveStyleSheet(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_StyleSet_FlushStyleSheets(
+pub unsafe extern "C" fn Servo_StyleSet_FlushStyleSheets(
     raw_data: RawServoStyleSetBorrowed,
     doc_element: RawGeckoElementBorrowedOrNull,
+    snapshots: *const ServoElementSnapshotTable,
 ) {
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
     let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let doc_element = doc_element.map(GeckoElement);
-    let have_invalidations = data.flush_stylesheets(&guard, doc_element);
+
+    let have_invalidations =
+        data.flush_stylesheets(&guard, doc_element, snapshots.as_ref());
+
     if have_invalidations && doc_element.is_some() {
-        // The invalidation machinery propagates the bits up, but we still
+        // The invalidation machinery propagates the bits up, but we still need
-        // need to tell the gecko restyle root machinery about it.
+        // to tell the Gecko restyle root machinery about it.
-        unsafe {
+        bindings::Gecko_NoteDirtySubtreeForInvalidation(doc_element.unwrap().0);
-            bindings::Gecko_NoteDirtySubtreeForInvalidation(doc_element.unwrap().0);
-        }
     }
 }