From c338745b175765e4ed8fc2440dcede537d5ebe1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 7 Mar 2018 02:04:56 +0100 Subject: [PATCH 1/2] style: Allow to share style across elements with similar XBL bindings. --- components/layout_thread/dom_wrapper.rs | 4 ++-- components/style/dom.rs | 11 +-------- components/style/gecko/wrapper.rs | 30 ++++++------------------- components/style/sharing/mod.rs | 18 +++++++++------ 4 files changed, 21 insertions(+), 42 deletions(-) diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 5cf29e8e159..5c5fc4be052 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/components/layout_thread/dom_wrapper.rs @@ -151,10 +151,10 @@ impl<'ln> NodeInfo for ServoLayoutNode<'ln> { } } -#[derive(Clone, Copy)] +#[derive(Clone, Copy, PartialEq)] enum Impossible { } -#[derive(Clone, Copy)] +#[derive(Clone, Copy, PartialEq)] pub struct ShadowRoot<'lr>(Impossible, PhantomData<&'lr ()>); impl<'lr> TShadowRoot for ShadowRoot<'lr> { diff --git a/components/style/dom.rs b/components/style/dom.rs index 59c31572c56..cfd3c80e890 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -321,7 +321,7 @@ fn fmt_subtree(f: &mut fmt::Formatter, stringify: &F, n: N, indent: } /// The ShadowRoot trait. -pub trait TShadowRoot : Sized + Copy + Clone { +pub trait TShadowRoot : Sized + Copy + Clone + PartialEq { /// The concrete node type. type ConcreteNode: TNode; @@ -390,15 +390,6 @@ pub trait TElement 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 { diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index b4e7862bce8..321673d7c81 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -135,6 +135,13 @@ impl<'ld> TDocument for GeckoDocument<'ld> { #[derive(Clone, Copy)] pub struct GeckoShadowRoot<'lr>(pub &'lr structs::ShadowRoot); +impl<'lr> PartialEq for GeckoShadowRoot<'lr> { + #[inline] + fn eq(&self, other: &Self) -> bool { + self.0 as *const _ == other.0 as *const _ + } +} + impl<'lr> TShadowRoot for GeckoShadowRoot<'lr> { type ConcreteNode = GeckoNode<'lr>; @@ -1040,29 +1047,6 @@ impl<'le> TElement for GeckoElement<'le> { self.before_or_after_pseudo(/* is_before = */ false) } - /// Ensure this accurately represents the rules that an element may ever - /// match, even in the native anonymous content case. - fn style_scope(&self) -> Self::ConcreteNode { - if self.implemented_pseudo_element().is_some() { - return self.closest_non_native_anonymous_ancestor().unwrap().style_scope(); - } - - if self.is_in_native_anonymous_subtree() { - return self.as_node().owner_doc().as_node(); - } - - if self.xbl_binding().is_some() || self.shadow_root().is_some() { - return self.as_node(); - } - - if let Some(parent) = self.xbl_binding_parent() { - return parent.as_node(); - } - - self.as_node().owner_doc().as_node() - } - - #[inline] fn is_html_element(&self) -> bool { self.namespace_id() == (structs::root::kNameSpaceID_XHTML as i32) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 6b3bbb9bccc..788ad3f5e3d 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -679,12 +679,14 @@ impl StyleSharingCache { 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"); + // If two elements belong to different shadow trees, different rules may + // apply to them, from the respective trees. + // + // Note that we don't need the same for XBL case, 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.containing_shadow() != candidate.element.containing_shadow() { + trace!("Miss: Different containing shadow roots"); return None; } @@ -695,7 +697,9 @@ impl StyleSharingCache { // shadow root, they could match different rules, due to the slot being // assigned to yet another slot in another shadow root. if target.element.assigned_slot() != candidate.element.assigned_slot() { - trace!("Miss: Different style scopes"); + // TODO(emilio): We could have a look at whether the shadow roots + // actually have slotted rules and such. + trace!("Miss: Different assigned slots"); return None; } From 9364cd21b82b370a6cc3c21e98f5de0b8832449a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 7 Mar 2018 15:01:03 +0100 Subject: [PATCH 2/2] style: Don't share style with an element that has a different proto binding than us. See the lengthy comment about why... XBL sucks. MozReview-Commit-ID: 8UgeJAVkj6a --- components/style/dom.rs | 3 +++ components/style/gecko/wrapper.rs | 12 ++++++++++-- components/style/sharing/mod.rs | 22 ++++++++++++++++++---- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index cfd3c80e890..93c587257a6 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -739,6 +739,9 @@ pub trait TElement /// The shadow root which roots the subtree this element is contained in. fn containing_shadow(&self) -> Option<::ConcreteShadowRoot>; + /// XBL hack for style sharing. :( + fn has_same_xbl_proto_binding_as(&self, _other: Self) -> bool { true } + /// Return the element which we can use to look up rules in the selector /// maps. /// diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 321673d7c81..e774b1cfb33 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1092,8 +1092,16 @@ impl<'le> TElement for GeckoElement<'le> { unsafe { slots._base.mContainingShadow.mRawPtr.as_ref().map(GeckoShadowRoot) } } - /// Execute `f` for each anonymous content child element (apart from - /// ::before and ::after) whose originating element is `self`. + fn has_same_xbl_proto_binding_as(&self, other: Self) -> bool { + match (self.xbl_binding(), other.xbl_binding()) { + (None, None) => true, + (Some(a), Some(b)) => { + a.0.mPrototypeBinding == b.0.mPrototypeBinding + } + _ => false, + } + } + fn each_anonymous_content_child(&self, mut f: F) where F: FnMut(Self), diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 788ad3f5e3d..f0637742173 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -681,15 +681,29 @@ impl StyleSharingCache { // If two elements belong to different shadow trees, different rules may // apply to them, from the respective trees. - // - // Note that we don't need the same for XBL case, 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.containing_shadow() != candidate.element.containing_shadow() { trace!("Miss: Different containing shadow roots"); return None; } + // Note that in theory we shouldn't need this XBL check. However, XBL is + // absolutely broken in all sorts of ways. + // + // A style change that changes which XBL binding applies to an element + // arrives there, with the element still having the old prototype + // binding attached. And thus we try to match revalidation selectors + // with the old XBL binding, because we can't look at the new ones of + // course. And that causes us to revalidate with the wrong selectors and + // hit assertions. + // + // Other than this, we don't need anything else like the containing XBL + // binding parent or what not, since two elements with different XBL + // bindings will necessarily end up with different style. + if !target.element.has_same_xbl_proto_binding_as(candidate.element) { + trace!("Miss: Different proto bindings"); + return None; + } + // If the elements are not assigned to the same slot they could match // different ::slotted() rules in the slot scope. //