servo: Merge #20229 - style: Separate the XBL and shadow dom styling bits (from emilio:finally); r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 07 Mar 2018 10:06:05 -0500
changeset 461997 ec0e046ffe652b6d647e2623fd17b15fb48e73b6
parent 461996 b8b7a46f6a24a81029c6f18bce0802db060e6aa2
child 461998 c9541861b02d4d03f7db0d1fd58e4279379fabe5
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #20229 - style: Separate the XBL and shadow dom styling bits (from emilio:finally); r=xidorn Bug: 1441022 Reviewed-by: xidorn MozReview-Commit-ID: 2W0BmZ8wWXg Source-Repo: https://github.com/servo/servo Source-Revision: 6272233c50071534ddbab118b64ecdb8fdda7c8a
servo/components/layout_thread/dom_wrapper.rs
servo/components/style/data.rs
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/invalidation/element/state_and_attributes.rs
servo/components/style/stylesheet_set.rs
servo/components/style/stylist.rs
--- a/servo/components/layout_thread/dom_wrapper.rs
+++ b/servo/components/layout_thread/dom_wrapper.rs
@@ -69,16 +69,17 @@ use style::dom::{DomChildren, LayoutIter
 use style::dom::{TDocument, TElement, TNode, TShadowRoot};
 use style::element_state::*;
 use style::font_metrics::ServoMetricsProvider;
 use style::properties::{ComputedValues, PropertyDeclarationBlock};
 use style::selector_parser::{AttrValue as SelectorAttrValue, NonTSPseudoClass, PseudoClassStringArg};
 use style::selector_parser::{PseudoElement, SelectorImpl, extended_filtering};
 use style::shared_lock::{SharedRwLock as StyleSharedRwLock, Locked as StyleLocked};
 use style::str::is_whitespace;
+use style::stylist::CascadeData;
 
 pub unsafe fn drop_style_and_layout_data(data: OpaqueStyleAndLayoutData) {
     let ptr = data.ptr.as_ptr() as *mut StyleData;
     let non_opaque: *mut StyleAndLayoutData = ptr as *mut _;
     let _ = Box::from_raw(non_opaque);
 }
 
 #[derive(Clone, Copy)]
@@ -161,16 +162,23 @@ impl<'lr> TShadowRoot for ShadowRoot<'lr
 
     fn as_node(&self) -> Self::ConcreteNode {
         match self.0 { }
     }
 
     fn host(&self) -> ServoLayoutElement<'lr> {
         match self.0 { }
     }
+
+    fn style_data<'a>(&self) -> &'a CascadeData
+    where
+        Self: 'a,
+    {
+        match self.0 { }
+    }
 }
 
 impl<'ln> TNode for ServoLayoutNode<'ln> {
     type ConcreteDocument = ServoLayoutDocument<'ln>;
     type ConcreteElement = ServoLayoutElement<'ln>;
     type ConcreteShadowRoot = ShadowRoot<'ln>;
 
     fn parent_node(&self) -> Option<Self> {
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -255,28 +255,26 @@ impl ElementData {
                 element.has_snapshot(),
                 element.handled_snapshot(),
                 element.implemented_pseudo_element());
 
         if !element.has_snapshot() || element.handled_snapshot() {
             return InvalidationResult::empty();
         }
 
-        let mut xbl_stylists = SmallVec::<[_; 3]>::new();
-        // FIXME(emilio): This is wrong, needs to account for ::slotted rules
-        // that may apply to elements down the tree.
-        let cut_off_inheritance =
+        let mut non_document_styles = SmallVec::<[_; 3]>::new();
+        let matches_doc_author_rules =
             element.each_applicable_non_document_style_rule_data(|data, quirks_mode| {
-                xbl_stylists.push((data, quirks_mode))
+                non_document_styles.push((data, quirks_mode))
             });
 
         let mut processor = StateAndAttrInvalidationProcessor::new(
             shared_context,
-            &xbl_stylists,
-            cut_off_inheritance,
+            &non_document_styles,
+            matches_doc_author_rules,
             element,
             self,
             nth_index_cache,
         );
 
         let invalidator = TreeStyleInvalidator::new(
             element,
             stack_limit_checker,
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -325,16 +325,21 @@ pub trait TShadowRoot : Sized + Copy + C
     /// The concrete node type.
     type ConcreteNode: TNode<ConcreteShadowRoot = Self>;
 
     /// Get this ShadowRoot as a node.
     fn as_node(&self) -> Self::ConcreteNode;
 
     /// Get the shadow host that hosts this ShadowRoot.
     fn host(&self) -> <Self::ConcreteNode as TNode>::ConcreteElement;
+
+    /// Get the style data for this ShadowRoot.
+    fn style_data<'a>(&self) -> &'a CascadeData
+    where
+        Self: 'a;
 }
 
 /// The element trait, the main abstraction the style crate acts over.
 pub trait TElement
     : Eq
     + PartialEq
     + Debug
     + Hash
@@ -755,17 +760,18 @@ pub trait TElement
                 .expect("Trying to collect rules for a detached pseudo-element")
         } else {
             *self
         }
     }
 
     /// Implements Gecko's `nsBindingManager::WalkRules`.
     ///
-    /// Returns whether to cut off the inheritance.
+    /// Returns whether to cut off the binding inheritance, that is, whether
+    /// document rules should _not_ apply.
     fn each_xbl_cascade_data<'a, F>(&self, _: F) -> bool
     where
         Self: 'a,
         F: FnMut(&'a CascadeData, QuirksMode),
     {
         false
     }
 
@@ -773,25 +779,32 @@ pub trait TElement
     /// the main document's data (which stores UA / author rules).
     ///
     /// Returns whether normal document author rules should apply.
     fn each_applicable_non_document_style_rule_data<'a, F>(&self, mut f: F) -> bool
     where
         Self: 'a,
         F: FnMut(&'a CascadeData, QuirksMode),
     {
-        let cut_off_inheritance = self.each_xbl_cascade_data(&mut f);
+        let mut doc_rules_apply = !self.each_xbl_cascade_data(&mut f);
+
+        if let Some(shadow) = self.containing_shadow() {
+            doc_rules_apply = false;
+            f(shadow.style_data(), self.as_node().owner_doc().quirks_mode());
+        }
 
         let mut current = self.assigned_slot();
         while let Some(slot) = current {
-            slot.each_xbl_cascade_data(&mut f);
+            // Slots can only have assigned nodes when in a shadow tree.
+            let data = slot.containing_shadow().unwrap().style_data();
+            f(data, self.as_node().owner_doc().quirks_mode());
             current = slot.assigned_slot();
         }
 
-        cut_off_inheritance
+        doc_rules_apply
     }
 
     /// Does a rough (and cheap) check for whether or not transitions might need to be updated that
     /// will quickly return false for the common case of no transitions specified or running. If
     /// this returns false, we definitely don't need to update transitions but if it returns true
     /// we can perform the more thoroughgoing check, needs_transitions_update, to further
     /// reduce the possibility of false positives.
     #[cfg(feature = "gecko")]
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -142,16 +142,40 @@ impl<'lr> TShadowRoot for GeckoShadowRoo
     fn as_node(&self) -> Self::ConcreteNode {
         GeckoNode(&self.0._base._base._base._base)
     }
 
     #[inline]
     fn host(&self) -> GeckoElement<'lr> {
         GeckoElement(unsafe { &*self.0._base.mHost.mRawPtr })
     }
+
+    #[inline]
+    fn style_data<'a>(&self) -> &'a CascadeData
+    where
+        Self: 'a,
+    {
+        debug_assert!(!self.0.mServoStyles.mPtr.is_null());
+
+        let author_styles = unsafe {
+            &*(self.0.mServoStyles.mPtr
+                as *const structs::RawServoAuthorStyles
+                as *const bindings::RawServoAuthorStyles)
+        };
+
+        let author_styles =
+            AuthorStyles::<GeckoStyleSheet>::from_ffi(author_styles);
+
+        debug_assert!(
+            author_styles.quirks_mode == self.as_node().owner_doc().quirks_mode() ||
+            author_styles.stylesheets.is_empty()
+        );
+
+        &author_styles.data
+    }
 }
 
 /// A simple wrapper over a non-null Gecko node (`nsINode`) pointer.
 ///
 /// Important: We don't currently refcount the DOM, because the wrapper lifetime
 /// magic guarantees that our LayoutFoo references won't outlive the root, and
 /// we don't mutate any of the references on the Gecko side during restyle.
 ///
@@ -1452,36 +1476,16 @@ impl<'le> TElement for GeckoElement<'le>
     {
         // Walk the binding scope chain, starting with the binding attached to
         // our content, up till we run out of scopes or we get cut off.
         //
         // If we are a NAC pseudo-element, we want to get rules from our
         // rule_hash_target, that is, our originating element.
         let mut current = Some(self.rule_hash_target());
         while let Some(element) = current {
-            // TODO(emilio): Deal with Shadow DOM separately than with XBL
-            // (right now we still rely on get_xbl_binding_parent()).
-            //
-            // That will allow to clean up a bunch in
-            // push_applicable_declarations.
-            if let Some(shadow) = element.shadow_root() {
-                debug_assert!(!shadow.0.mServoStyles.mPtr.is_null());
-                let author_styles = unsafe {
-                    &*(shadow.0.mServoStyles.mPtr
-                        as *const structs::RawServoAuthorStyles
-                        as *const bindings::RawServoAuthorStyles)
-                };
-
-                let author_styles: &'a _ = AuthorStyles::<GeckoStyleSheet>::from_ffi(author_styles);
-                f(&author_styles.data, author_styles.quirks_mode);
-                if element != *self {
-                    break;
-                }
-            }
-
             if let Some(binding) = element.xbl_binding() {
                 binding.each_xbl_cascade_data(&mut f);
 
                 // If we're not looking at our original element, allow the
                 // binding to cut off style inheritance.
                 if element != *self && !binding.inherits_style() {
                     // Go no further; we're not inheriting style from
                     // anything above here.
--- a/servo/components/style/invalidation/element/state_and_attributes.rs
+++ b/servo/components/style/invalidation/element/state_and_attributes.rs
@@ -52,44 +52,44 @@ where
     invalidates_self: bool,
 }
 
 /// An invalidation processor for style changes due to state and attribute
 /// changes.
 pub struct StateAndAttrInvalidationProcessor<'a, 'b: 'a, E: TElement> {
     shared_context: &'a SharedStyleContext<'b>,
     shadow_rule_datas: &'a [(&'b CascadeData, QuirksMode)],
-    cut_off_inheritance: bool,
+    matches_document_author_rules: bool,
     element: E,
     data: &'a mut ElementData,
     matching_context: MatchingContext<'a, E::Impl>,
 }
 
 impl<'a, 'b: 'a, E: TElement> StateAndAttrInvalidationProcessor<'a, 'b, E> {
     /// Creates a new StateAndAttrInvalidationProcessor.
     pub fn new(
         shared_context: &'a SharedStyleContext<'b>,
         shadow_rule_datas: &'a [(&'b CascadeData, QuirksMode)],
-        cut_off_inheritance: bool,
+        matches_document_author_rules: bool,
         element: E,
         data: &'a mut ElementData,
         nth_index_cache: &'a mut NthIndexCache,
     ) -> Self {
         let matching_context = MatchingContext::new_for_visited(
             MatchingMode::Normal,
             None,
             Some(nth_index_cache),
             VisitedHandlingMode::AllLinksVisitedAndUnvisited,
             shared_context.quirks_mode(),
         );
 
         Self {
             shared_context,
             shadow_rule_datas,
-            cut_off_inheritance,
+            matches_document_author_rules,
             element,
             data,
             matching_context,
         }
     }
 }
 
 
@@ -243,17 +243,17 @@ where
                 added_id: id_added,
                 classes_removed: &classes_removed,
                 classes_added: &classes_added,
                 descendant_invalidations,
                 sibling_invalidations,
                 invalidates_self: false,
             };
 
-            let document_origins = if self.cut_off_inheritance {
+            let document_origins = if !self.matches_document_author_rules {
                 Origin::UserAgent.into()
             } else {
                 OriginSet::all()
             };
 
             for (cascade_data, origin) in self.shared_context.stylist.iter_origins() {
                 if document_origins.contains(origin.into()) {
                     collector.collect_dependencies_in_invalidation_map(cascade_data.invalidation_map());
--- a/servo/components/style/stylesheet_set.rs
+++ b/servo/components/style/stylesheet_set.rs
@@ -258,17 +258,17 @@ where
 impl<S> Default for SheetCollection<S>
 where
     S: StylesheetInDocument + PartialEq + 'static,
 {
     fn default() -> Self {
         Self {
             entries: vec![],
             data_validity: DataValidity::Valid,
-            dirty: false,
+            dirty: true,
         }
     }
 }
 
 impl<S> SheetCollection<S>
 where
     S: StylesheetInDocument + PartialEq + 'static,
 {
@@ -592,16 +592,21 @@ where
         }
     }
 
     /// Whether anything has changed since the last time this was flushed.
     pub fn dirty(&self) -> bool {
         self.collection.dirty
     }
 
+    /// Whether the collection is empty.
+    pub fn is_empty(&self) -> bool {
+        self.collection.len() == 0
+    }
+
     fn collection_for(
         &mut self,
         _sheet: &S,
         _guard: &SharedRwLockReadGuard,
     ) -> &mut SheetCollection<S> {
         &mut self.collection
     }
 
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -2,17 +2,17 @@
  * 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/. */
 
 //! Selector matching.
 
 use {Atom, LocalName, Namespace, WeakAtom};
 use applicable_declarations::{ApplicableDeclarationBlock, ApplicableDeclarationList};
 use context::{CascadeInputs, QuirksMode};
-use dom::TElement;
+use dom::{TElement, TShadowRoot};
 use element_state::{DocumentState, ElementState};
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")]
 use gecko_bindings::structs::{ServoStyleSetSizes, StyleRuleInclusion};
 use hashglobe::FailedAllocationError;
 use invalidation::element::invalidation_map::InvalidationMap;
 use invalidation::media_queries::{EffectiveMediaQueryResults, ToMediaListKey};
 #[cfg(feature = "gecko")]
@@ -594,21 +594,22 @@ impl Stylist {
         F: FnMut(&CascadeData, QuirksMode) -> bool,
     {
         if f(&self.cascade_data.user_agent.cascade_data, self.quirks_mode()) {
             return true;
         }
 
         let mut maybe = false;
 
-        let cut_off = element.each_applicable_non_document_style_rule_data(|data, quirks_mode| {
-            maybe = maybe || f(&*data, quirks_mode);
-        });
+        let doc_author_rules_apply =
+            element.each_applicable_non_document_style_rule_data(|data, quirks_mode| {
+                maybe = maybe || f(&*data, quirks_mode);
+            });
 
-        if maybe || cut_off {
+        if maybe || !doc_author_rules_apply {
             return maybe;
         }
 
         f(&self.cascade_data.author, self.quirks_mode()) ||
         f(&self.cascade_data.user, self.quirks_mode())
     }
 
     /// Computes the style for a given "precomputed" pseudo-element, taking the
@@ -1246,16 +1247,18 @@ impl Stylist {
                 if cfg!(debug_assertions) {
                     for declaration in &applicable_declarations[length_before_preshints..] {
                         assert_eq!(declaration.level(), CascadeLevel::PresHints);
                     }
                 }
             }
         }
 
+        let mut match_document_author_rules = matches_author_rules;
+
         // XBL / Shadow DOM rules, which are author rules too.
         //
         // TODO(emilio): Cascade order here is wrong for Shadow DOM. In
         // particular, normally document rules override ::slotted() rules, but
         // for !important it should be the other way around. So probably we need
         // to add some sort of AuthorScoped cascade level or something.
         if matches_author_rules && !only_default_rules {
             // Match slotted rules in reverse order, so that the outer slotted
@@ -1263,36 +1266,53 @@ impl Stylist {
             let mut slots = SmallVec::<[_; 3]>::new();
             let mut current = rule_hash_target.assigned_slot();
             while let Some(slot) = current {
                 slots.push(slot);
                 current = slot.assigned_slot();
             }
 
             for slot in slots.iter().rev() {
-                slot.each_xbl_cascade_data(|cascade_data, _quirks_mode| {
-                    if let Some(map) = cascade_data.slotted_rules(pseudo_element) {
-                        map.get_all_matching_rules(
-                            element,
-                            rule_hash_target,
-                            applicable_declarations,
-                            context,
-                            flags_setter,
-                            CascadeLevel::AuthorNormal
-                        );
-                    }
-                });
+                let styles = slot.containing_shadow().unwrap().style_data();
+                if let Some(map) = styles.slotted_rules(pseudo_element) {
+                    map.get_all_matching_rules(
+                        element,
+                        rule_hash_target,
+                        applicable_declarations,
+                        context,
+                        flags_setter,
+                        CascadeLevel::AuthorNormal,
+                    );
+                }
+            }
+
+            // TODO(emilio): We need to look up :host rules if the element is a
+            // shadow host, when we implement that.
+            if let Some(containing_shadow) = rule_hash_target.containing_shadow() {
+                let cascade_data = containing_shadow.style_data();
+                if let Some(map) = cascade_data.normal_rules(pseudo_element) {
+                    map.get_all_matching_rules(
+                        element,
+                        rule_hash_target,
+                        applicable_declarations,
+                        context,
+                        flags_setter,
+                        CascadeLevel::AuthorNormal,
+                    );
+                }
+
+                match_document_author_rules = false;
             }
         }
 
-        // FIXME(emilio): It looks very wrong to match XBL / Shadow DOM rules
-        // even for getDefaultComputedStyle!
+        // FIXME(emilio): It looks very wrong to match XBL rules even for
+        // getDefaultComputedStyle!
         //
         // Also, this doesn't account for the author_styles_enabled stuff.
-        let cut_off_inheritance = element.each_xbl_cascade_data(|cascade_data, quirks_mode| {
+        let cut_xbl_binding_inheritance = element.each_xbl_cascade_data(|cascade_data, quirks_mode| {
             if let Some(map) = cascade_data.normal_rules(pseudo_element) {
                 // NOTE(emilio): This is needed because the XBL stylist may
                 // think it has a different quirks mode than the document.
                 //
                 // FIXME(emilio): this should use the same VisitedMatchingMode
                 // as `context`, write a test-case of :visited not working on
                 // Shadow DOM and fix it!
                 let mut matching_context = MatchingContext::new(
@@ -1309,17 +1329,19 @@ impl Stylist {
                     applicable_declarations,
                     &mut matching_context,
                     flags_setter,
                     CascadeLevel::AuthorNormal,
                 );
             }
         });
 
-        if matches_author_rules && !only_default_rules && !cut_off_inheritance {
+        match_document_author_rules &= !cut_xbl_binding_inheritance;
+
+        if match_document_author_rules && !only_default_rules {
             // Author normal rules.
             if let Some(map) = self.cascade_data.author.normal_rules(pseudo_element) {
                 map.get_all_matching_rules(
                     element,
                     rule_hash_target,
                     applicable_declarations,
                     context,
                     flags_setter,