Bug 1441022: Separate the XBL and shadow dom styling bits. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 05 Mar 2018 12:50:04 +0100
changeset 763120 9dcea2205f05833766b2640a1ea4c1a8d92d0145
parent 763119 e98821079e4d03d6bec22383436422641bfe5da5
push id101346
push userbmo:emilio@crisal.io
push dateMon, 05 Mar 2018 13:23:42 +0000
reviewersxidorn
bugs1441022
milestone60.0a1
Bug 1441022: Separate the XBL and shadow dom styling bits. r?xidorn MozReview-Commit-ID: 2W0BmZ8wWXg
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/stylist.rs
testing/web-platform/meta/css/selectors/focus-within-shadow-001.html.ini
--- 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 shadow host that hosts 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_eq!(
+            author_styles.quirks_mode,
+            self.as_node().owner_doc().quirks_mode()
+        );
+
+        &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/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,
--- a/testing/web-platform/meta/css/selectors/focus-within-shadow-001.html.ini
+++ b/testing/web-platform/meta/css/selectors/focus-within-shadow-001.html.ini
@@ -1,5 +1,3 @@
 [focus-within-shadow-001.html]
   disabled:
     if not stylo: Shadow DOM is stylo-only
-  expected: FAIL
-  bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1441022