Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. r?bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 27 Oct 2017 18:42:39 +0200
changeset 687760 6e917c6869e7ea2b41acf4c6f5f6f9efc9823e1e
parent 687759 d9dedb79ea0c1cdc3e4e48b04c790c033c9a83cb
child 737734 93bc26ad0571bf05a8a98b2de935d08ac8a2fc07
push id86596
push userbmo:emilio@crisal.io
push dateFri, 27 Oct 2017 18:18:19 +0000
reviewersbz
bugs1412251
milestone58.0a1
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules. r?bz MozReview-Commit-ID: 861FcEy7O26
layout/reftests/webcomponents/reftest.list
layout/reftests/webcomponents/style-sharing-across-shadow.html
layout/reftests/webcomponents/style-sharing-ref.html
layout/reftests/webcomponents/style-sharing.html
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/sharing/checks.rs
servo/components/style/sharing/mod.rs
servo/components/style/stylist.rs
--- a/layout/reftests/webcomponents/reftest.list
+++ b/layout/reftests/webcomponents/reftest.list
@@ -10,8 +10,10 @@ pref(dom.webcomponents.enabled,true) == 
 pref(dom.webcomponents.enabled,true) == nested-insertion-point-1.html nested-insertion-point-1-ref.html
 pref(dom.webcomponents.enabled,true) == update-dist-node-descendants-1.html update-dist-node-descendants-1-ref.html
 pref(dom.webcomponents.enabled,true) fuzzy-if(Android,2,7) == input-transition-1.html input-transition-1-ref.html
 pref(dom.webcomponents.enabled,true) == dynamic-insertion-point-distribution-1.html dynamic-insertion-point-distribution-1-ref.html
 pref(dom.webcomponents.enabled,true) == dynamic-insertion-point-distribution-2.html dynamic-insertion-point-distribution-2-ref.html
 pref(dom.webcomponents.enabled,true) == remove-append-shadow-host-1.html remove-append-shadow-host-1-ref.html
 pref(dom.webcomponents.enabled,true) == reframe-shadow-child-1.html reframe-shadow-child-ref.html
 pref(dom.webcomponents.enabled,true) == reframe-shadow-child-2.html reframe-shadow-child-ref.html
+pref(dom.webcomponents.enabled,true) == style-sharing.html style-sharing-ref.html
+pref(dom.webcomponents.enabled,true) == style-sharing-across-shadow.html style-sharing-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/webcomponents/style-sharing-across-shadow.html
@@ -0,0 +1,22 @@
+<!doctype html>
+<style>
+  div { display: contents }
+</style>
+<div></div>
+<div></div>
+<script>
+  let divs = document.querySelectorAll('div');
+  divs[0].createShadowRoot().innerHTML = `
+    <style>
+      * { color: green; }
+    </style>
+    <span>Should be green</span>
+  `;
+  divs[1].createShadowRoot().innerHTML = `
+    <style>
+      * { color: initial; }
+      [foo] { }
+    </style>
+    <span>Should not be green</span>
+  `;
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/webcomponents/style-sharing-ref.html
@@ -0,0 +1,5 @@
+<!doctype html>
+<div>
+  <span style="color: green">Should be green</span>
+  <span>Should not be green</span>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/webcomponents/style-sharing.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<div id="host"></div>
+<script>
+  let root = host.createShadowRoot();
+  root.innerHTML = `
+    <style>
+      #test {
+        color: green;
+      }
+    </style>
+    <span id="test">Should be green</span>
+    <span id="test2">Should not be green</span>
+  `;
+</script>
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -397,16 +397,25 @@ pub trait TElement
         while let Some(parent) = curr.traversal_parent() {
             depth += 1;
             curr = parent;
         }
 
         depth
     }
 
+    /// The style scope of this element is a node that represents which rules
+    /// apply to the element.
+    ///
+    /// In Servo, where we don't know about Shadow DOM or XBL, the style scope
+    /// is always the document.
+    fn style_scope(&self) -> Self::ConcreteNode {
+        self.as_node().owner_doc().as_node()
+    }
+
     /// Get this node's parent element from the perspective of a restyle
     /// traversal.
     fn traversal_parent(&self) -> Option<Self> {
         self.as_node().traversal_parent()
     }
 
     /// Get this node's children from the perspective of a restyle traversal.
     fn traversal_children(&self) -> LayoutIterator<Self::TraversalChildrenIterator>;
@@ -747,24 +756,26 @@ pub trait TElement
     /// Returns the anonymous content for the current element's XBL binding,
     /// given if any.
     ///
     /// This is used in Gecko for XBL and shadow DOM.
     fn xbl_binding_anonymous_content(&self) -> Option<Self::ConcreteNode> {
         None
     }
 
-    /// Returns the rule hash target given an element.
+    /// Return the element which we can use to look up rules in the selector
+    /// maps.
+    ///
+    /// This is always the element itself, except in the case where we are an
+    /// element-backed pseudo-element, in which case we return the originating
+    /// element.
     fn rule_hash_target(&self) -> Self {
         let is_implemented_pseudo =
             self.implemented_pseudo_element().is_some();
 
-        // NB: This causes use to rule has pseudo selectors based on the
-        // properties of the originating element (which is fine, given the
-        // find_first_from_right usage).
         if is_implemented_pseudo {
             self.closest_non_native_anonymous_ancestor().unwrap()
         } else {
             *self
         }
     }
 
     /// Implements Gecko's `nsBindingManager::WalkRules`.
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -534,18 +534,23 @@ impl<'le> GeckoElement<'le> {
     fn get_extended_slots(
         &self,
     ) -> Option<&structs::FragmentOrElement_nsExtendedDOMSlots> {
         self.get_dom_slots()
             .and_then(|s| unsafe { s.mExtendedSlots.mPtr.as_ref() })
     }
 
     #[inline]
+    fn may_be_in_binding_manager(&self) -> bool {
+        self.flags() & (structs::NODE_MAY_BE_IN_BINDING_MNGR as u32) != 0
+    }
+
+    #[inline]
     fn get_xbl_binding(&self) -> Option<GeckoXBLBinding<'le>> {
-        if self.flags() & (structs::NODE_MAY_BE_IN_BINDING_MNGR as u32) == 0 {
+        if !self.may_be_in_binding_manager() {
             return None;
         }
 
         unsafe { bindings::Gecko_GetXBLBinding(self.0).map(GeckoXBLBinding) }
     }
 
     #[inline]
     fn get_xbl_binding_with_content(&self) -> Option<GeckoXBLBinding<'le>> {
@@ -568,19 +573,21 @@ impl<'le> GeckoElement<'le> {
             // where the binding parent is stored in a member variable
             // rather than in slots.  So just get it through FFI for now.
             unsafe {
                 bindings::Gecko_GetBindingParent(self.0).map(GeckoElement)
             }
         } else {
             let binding_parent = unsafe {
                 self.get_non_xul_xbl_binding_parent_raw_content().as_ref()
-            }.map(GeckoNode::from_content)
-                .and_then(|n| n.as_element());
-            debug_assert!(binding_parent == unsafe { bindings::Gecko_GetBindingParent(self.0).map(GeckoElement) });
+            }.map(GeckoNode::from_content).and_then(|n| n.as_element());
+
+            debug_assert!(binding_parent == unsafe {
+                bindings::Gecko_GetBindingParent(self.0).map(GeckoElement)
+            });
             binding_parent
         }
     }
 
     fn get_non_xul_xbl_binding_parent_raw_content(&self) -> *mut nsIContent {
         debug_assert!(!self.is_xul_element());
         self.get_extended_slots()
             .map_or(ptr::null_mut(), |slots| slots.mBindingParent)
@@ -940,16 +947,45 @@ impl<'le> TElement for GeckoElement<'le>
     fn before_pseudo_element(&self) -> Option<Self> {
         self.get_before_or_after_pseudo(/* is_before = */ true)
     }
 
     fn after_pseudo_element(&self) -> Option<Self> {
         self.get_before_or_after_pseudo(/* is_before = */ false)
     }
 
+    /// FIXME(emilio): if XBL ever goes away, convert the style scope in either
+    /// a document or a ShadowRoot.
+    ///
+    /// Also, ensure this accurately represents the rules that an element may
+    /// match, even in the native anonymous content case.
+    fn style_scope(&self) -> Self::ConcreteNode {
+        if !self.may_be_in_binding_manager() {
+            return self.as_node().owner_doc().as_node();
+        }
+
+        if self.is_native_anonymous() {
+            if let Some(ancestor) = self.closest_non_native_anonymous_ancestor() {
+                return ancestor.style_scope();
+            }
+
+            return self.as_node().owner_doc().as_node();
+        }
+
+        if self.get_xbl_binding().is_some() {
+            return self.as_node();
+        }
+
+        if let Some(parent) = self.get_xbl_binding_parent() {
+            return parent.as_node();
+        }
+
+        return self.as_node().owner_doc().as_node();
+    }
+
     /// Execute `f` for each anonymous content child element (apart from
     /// ::before and ::after) whose originating element is `self`.
     fn each_anonymous_content_child<F>(&self, mut f: F)
     where
         F: FnMut(Self),
     {
         let array: *mut structs::nsTArray<*mut nsIContent> =
             unsafe { bindings::Gecko_GetAnonymousContentForElement(self.0) };
--- a/servo/components/style/sharing/checks.rs
+++ b/servo/components/style/sharing/checks.rs
@@ -1,17 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Different checks done during the style sharing process in order to determine
 //! quickly whether it's worth to share style, and whether two different
 //! elements can indeed share the same style.
 
-use Atom;
 use bloom::StyleBloom;
 use context::{SelectorFlagsMap, SharedStyleContext};
 use dom::TElement;
 use selectors::NthIndexCache;
 use sharing::{StyleSharingCandidate, StyleSharingTarget};
 
 /// Determines whether a target and a candidate have compatible parents for sharing.
 pub fn parents_allow_sharing<E>(
@@ -91,24 +90,26 @@ pub fn have_same_class<E>(target: &mut S
 
 /// Whether a given element and a candidate match the same set of "revalidation"
 /// selectors.
 ///
 /// Revalidation selectors are those that depend on the DOM structure, like
 /// :first-child, etc, or on attributes that we don't check off-hand (pretty
 /// much every attribute selector except `id` and `class`.
 #[inline]
-pub fn revalidate<E>(target: &mut StyleSharingTarget<E>,
-                     candidate: &mut StyleSharingCandidate<E>,
-                     shared_context: &SharedStyleContext,
-                     bloom: &StyleBloom<E>,
-                     nth_index_cache: &mut NthIndexCache,
-                     selector_flags_map: &mut SelectorFlagsMap<E>)
-                     -> bool
-    where E: TElement,
+pub fn revalidate<E>(
+    target: &mut StyleSharingTarget<E>,
+    candidate: &mut StyleSharingCandidate<E>,
+    shared_context: &SharedStyleContext,
+    bloom: &StyleBloom<E>,
+    nth_index_cache: &mut NthIndexCache,
+    selector_flags_map: &mut SelectorFlagsMap<E>,
+) -> bool
+where
+    E: TElement,
 {
     let stylist = &shared_context.stylist;
 
     let for_element = target.revalidation_match_results(
         stylist,
         bloom,
         nth_index_cache,
         selector_flags_map,
@@ -125,30 +126,39 @@ pub fn revalidate<E>(target: &mut StyleS
     // comparing represent the same set of selectors.
     debug_assert_eq!(for_element.len(), for_candidate.len());
 
     for_element == for_candidate
 }
 
 /// Checks whether we might have rules for either of the two ids.
 #[inline]
-pub fn may_have_rules_for_ids(shared_context: &SharedStyleContext,
-                              element_id: Option<&Atom>,
-                              candidate_id: Option<&Atom>) -> bool
+pub fn may_match_different_id_rules<E>(
+    shared_context: &SharedStyleContext,
+    element: E,
+    candidate: E,
+) -> bool
+where
+    E: TElement,
 {
-    // We shouldn't be called unless the ids are different.
-    debug_assert!(element_id.is_some() || candidate_id.is_some());
+    let element_id = element.get_id();
+    let candidate_id = candidate.get_id();
+
+    if element_id == candidate_id {
+        return false;
+    }
+
     let stylist = &shared_context.stylist;
 
     let may_have_rules_for_element = match element_id {
-        Some(id) => stylist.may_have_rules_for_id(id),
+        Some(ref id) => stylist.may_have_rules_for_id(id, element),
         None => false
     };
 
     if may_have_rules_for_element {
         return true;
     }
 
     match candidate_id {
-        Some(id) => stylist.may_have_rules_for_id(id),
+        Some(ref id) => stylist.may_have_rules_for_id(id, candidate),
         None => false
     }
 }
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -201,20 +201,21 @@ impl ValidationData {
     #[inline]
     fn revalidation_match_results<E, F>(
         &mut self,
         element: E,
         stylist: &Stylist,
         bloom: &StyleBloom<E>,
         nth_index_cache: &mut NthIndexCache,
         bloom_known_valid: bool,
-        flags_setter: &mut F
+        flags_setter: &mut F,
     ) -> &SmallBitVec
-        where E: TElement,
-              F: FnMut(&E, ElementSelectorFlags),
+    where
+        E: TElement,
+        F: FnMut(&E, ElementSelectorFlags),
     {
         if self.revalidation_match_results.is_none() {
             // The bloom filter may already be set up for our element.
             // If it is, use it.  If not, we must be in a candidate
             // (i.e. something in the cache), and the element is one
             // of our cousins, not a sibling.  In that case, we'll
             // just do revalidation selector matching without a bloom
             // filter, to avoid thrashing the filter.
@@ -225,20 +226,22 @@ impl ValidationData {
             } else {
                 if bloom.current_parent() == element.traversal_parent() {
                     Some(bloom.filter())
                 } else {
                     None
                 }
             };
             self.revalidation_match_results =
-                Some(stylist.match_revalidation_selectors(&element,
-                                                          bloom_to_use,
-                                                          nth_index_cache,
-                                                          flags_setter));
+                Some(stylist.match_revalidation_selectors(
+                    element,
+                    bloom_to_use,
+                    nth_index_cache,
+                    flags_setter,
+                ));
         }
 
         self.revalidation_match_results.as_ref().unwrap()
     }
 }
 
 /// Information regarding a style sharing candidate, that is, an entry in the
 /// style sharing cache.
@@ -656,16 +659,25 @@ impl<E: TElement> StyleSharingCache<E> {
 
         if target.is_native_anonymous() {
             debug_assert!(!candidate.element.is_native_anonymous(),
                           "Why inserting NAC into the cache?");
             trace!("Miss: Native Anonymous Content");
             return None;
         }
 
+        // Note that in the XBL case, we should be able to assert that the
+        // scopes are different, since two elements with different XBL bindings
+        // need to necessarily have different style (and thus children of them
+        // would never pass the parent check).
+        if target.element.style_scope() != candidate.element.style_scope() {
+            trace!("Miss: Different style scopes");
+            return None;
+        }
+
         if *target.get_local_name() != *candidate.element.get_local_name() {
             trace!("Miss: Local Name");
             return None;
         }
 
         if *target.get_namespace() != *candidate.element.get_namespace() {
             trace!("Miss: Namespace");
             return None;
@@ -685,25 +697,27 @@ impl<E: TElement> StyleSharingCache<E> {
         // We do not ignore visited state here, because Gecko
         // needs to store extra bits on visited style contexts,
         // so these contexts cannot be shared
         if target.element.get_state() != candidate.get_state() {
             trace!("Miss: User and Author State");
             return None;
         }
 
-        let element_id = target.element.get_id();
-        let candidate_id = candidate.element.get_id();
-        if element_id != candidate_id {
-            // It's possible that there are no styles for either id.
-            if checks::may_have_rules_for_ids(shared, element_id.as_ref(),
-                                              candidate_id.as_ref()) {
-                trace!("Miss: ID Attr");
-                return None;
-            }
+        // It's possible that there are no styles for either id.
+        let may_match_different_id_rules =
+            checks::may_match_different_id_rules(
+                shared,
+                target.element,
+                candidate.element,
+            );
+
+        if may_match_different_id_rules {
+            trace!("Miss: ID Attr");
+            return None;
         }
 
         if !checks::have_same_style_attribute(target, candidate) {
             trace!("Miss: Style Attr");
             return None;
         }
 
         if !checks::have_same_class(target, candidate) {
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -1404,36 +1404,55 @@ impl Stylist {
         } else {
             debug!("skipping transition rules");
         }
     }
 
     /// Given an id, returns whether there might be any rules for that id in any
     /// of our rule maps.
     #[inline]
-    pub fn may_have_rules_for_id(&self, id: &Atom) -> bool {
-        self.cascade_data
-            .iter_origins()
-            .any(|(d, _)| d.mapped_ids.might_contain_hash(id.get_hash()))
+    pub fn may_have_rules_for_id<E>(
+        &self,
+        id: &Atom,
+        element: E,
+    ) -> bool
+    where
+        E: TElement,
+    {
+        let hash = id.get_hash();
+        for (data, _) in self.cascade_data.iter_origins() {
+            if data.mapped_ids.might_contain_hash(hash) {
+                return true;
+            }
+        }
+
+        let mut xbl_rules_may_contain = false;
+
+        element.each_xbl_stylist(|stylist| {
+            xbl_rules_may_contain = xbl_rules_may_contain ||
+                stylist.cascade_data.author.mapped_ids.might_contain_hash(hash)
+        });
+
+        xbl_rules_may_contain
     }
 
     /// Returns the registered `@keyframes` animation for the specified name.
     #[inline]
     pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> {
         self.cascade_data
             .iter_origins()
             .filter_map(|(d, _)| d.animations.get(name))
             .next()
     }
 
     /// Computes the match results of a given element against the set of
     /// revalidation selectors.
     pub fn match_revalidation_selectors<E, F>(
         &self,
-        element: &E,
+        element: E,
         bloom: Option<&BloomFilter>,
         nth_index_cache: &mut NthIndexCache,
         flags_setter: &mut F
     ) -> SmallBitVec
     where
         E: TElement,
         F: FnMut(&E, ElementSelectorFlags),
     {
@@ -1449,32 +1468,50 @@ impl Stylist {
         // Note that, by the time we're revalidating, we're guaranteed that the
         // candidate and the entry have the same id, classes, and local name.
         // This means we're guaranteed to get the same rulehash buckets for all
         // the lookups, which means that the bitvecs are comparable. We verify
         // this in the caller by asserting that the bitvecs are same-length.
         let mut results = SmallBitVec::new();
         for (data, _) in self.cascade_data.iter_origins() {
             data.selectors_for_cache_revalidation.lookup(
-                *element,
+                element,
                 self.quirks_mode,
                 &mut |selector_and_hashes| {
                     results.push(matches_selector(
                         &selector_and_hashes.selector,
                         selector_and_hashes.selector_offset,
                         Some(&selector_and_hashes.hashes),
-                        element,
+                        &element,
                         &mut matching_context,
                         flags_setter
                     ));
                     true
                 }
             );
         }
 
+        element.each_xbl_stylist(|stylist| {
+            stylist.cascade_data.author.selectors_for_cache_revalidation.lookup(
+                element,
+                stylist.quirks_mode,
+                &mut |selector_and_hashes| {
+                    results.push(matches_selector(
+                        &selector_and_hashes.selector,
+                        selector_and_hashes.selector_offset,
+                        Some(&selector_and_hashes.hashes),
+                        &element,
+                        &mut matching_context,
+                        flags_setter
+                    ));
+                    true
+                }
+            );
+        });
+
         results
     }
 
     /// Computes styles for a given declaration with parent_style.
     pub fn compute_for_declarations(
         &self,
         guards: &StylesheetGuards,
         parent_style: &ComputedValues,