From 5dd35ac6cd4ff3d21b8eba55e3676a2dc3467ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 5 Apr 2023 09:19:15 +0000 Subject: [PATCH] style: Simplify NAC setup Make all UA widgets also NAC. Keep the UA widget flag but break at anonymous subtree boundaries, so that only nodes inside the UA widget directly (and not NAC from those) get the flag. This is important because two callers depend on this difference: * The style system, since we still want to match content rules from stylesheets in the UA widget. We also match user rules, which is a bit sketchy, but that was the previous behavior, will file a follow-up for that. * The reflector code, since we want the scope for UA widgets to not include the NAC nodes inside that UA widget. nsINode::IsInUAWidget got it wrong. After this patch, ChromeOnlyAccess is equivalent to IsInNativeAnonymousSubtree, so we should probably unify the naming. That's left for a follow-up patch because I don't have a strong preference. Differential Revision: https://phabricator.services.mozilla.com/D174310 --- components/style/dom.rs | 22 ++--- components/style/gecko/wrapper.rs | 141 +++++++++++++---------------- components/style/rule_collector.rs | 19 ++-- components/style/sharing/mod.rs | 14 +-- components/style/style_resolver.rs | 2 +- components/style/traversal.rs | 2 +- 6 files changed, 87 insertions(+), 113 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index 023c3d7507b..68c8460bfd7 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -388,10 +388,10 @@ pub trait TElement: true } - /// Whether this element should match user and author rules. + /// Whether this element should match user and content rules. /// /// We use this for Native Anonymous Content in Gecko. - fn matches_user_and_author_rules(&self) -> bool { + fn matches_user_and_content_rules(&self) -> bool { true } @@ -651,11 +651,6 @@ pub trait TElement: false } - /// Returns true if this element is in a native anonymous subtree. - fn is_in_native_anonymous_subtree(&self) -> bool { - false - } - /// Returns the pseudo-element implemented by this element, if any. /// /// Gecko traverses pseudo-elements during the style traversal, and we need @@ -791,11 +786,8 @@ pub trait TElement: use crate::rule_collector::containing_shadow_ignoring_svg_use; let target = self.rule_hash_target(); - if !target.matches_user_and_author_rules() { - return false; - } - - let mut doc_rules_apply = true; + let matches_user_and_content_rules = target.matches_user_and_content_rules(); + let mut doc_rules_apply = matches_user_and_content_rules; // Use the same rules to look for the containing host as we do for rule // collection. @@ -843,9 +835,9 @@ pub trait TElement: }, None => { // TODO(emilio): Should probably distinguish with - // MatchesDocumentRules::{No,Yes,IfPart} or - // something so that we could skip some work. - doc_rules_apply = true; + // MatchesDocumentRules::{No,Yes,IfPart} or something so that we could + // skip some work. + doc_rules_apply = matches_user_and_content_rules; break; }, } diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 2d02d31cca1..a504ed0c68a 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -325,32 +325,34 @@ impl<'ln> GeckoNode<'ln> { #[inline] fn is_in_shadow_tree(&self) -> bool { use crate::gecko_bindings::structs::NODE_IS_IN_SHADOW_TREE; - self.flags() & (NODE_IS_IN_SHADOW_TREE as u32) != 0 + self.flags() & NODE_IS_IN_SHADOW_TREE != 0 } - /// WARNING: This logic is duplicated in Gecko's FlattenedTreeParentIsParent. - /// Make sure to mirror any modifications in both places. + /// Returns true if we know for sure that `flattened_tree_parent` and `parent_node` return the + /// same thing. + /// + /// TODO(emilio): Measure and consider not doing this fast-path, it's only a function call and + /// from profiles it seems that keeping this fast path makes the compiler not inline + /// `flattened_tree_parent` as a whole, so we're not gaining much either. #[inline] fn flattened_tree_parent_is_parent(&self) -> bool { use crate::gecko_bindings::structs::*; let flags = self.flags(); - // FIXME(emilio): The shadow tree condition seems it shouldn't be needed - // anymore, if we check for slots. - if self.is_in_shadow_tree() { + let parent = match self.parent_node() { + Some(p) => p, + None => return true, + }; + + if parent.is_shadow_root() { return false; } - let parent = unsafe { self.0.mParent.as_ref() }.map(GeckoNode); - let parent_el = parent.and_then(|p| p.as_element()); - if flags & (NODE_IS_NATIVE_ANONYMOUS_ROOT as u32) != 0 && - parent_el.map_or(false, |el| el.is_root()) - { - return false; - } - - if let Some(parent) = parent_el { - if parent.shadow_root().is_some() { + if let Some(parent) = parent.as_element() { + if flags & NODE_IS_NATIVE_ANONYMOUS_ROOT != 0 && parent.is_root() { + return false; + } + if parent.shadow_root().is_some() || parent.is_html_slot_element() { return false; } } @@ -360,10 +362,6 @@ impl<'ln> GeckoNode<'ln> { #[inline] fn flattened_tree_parent(&self) -> Option { - // TODO(emilio): Measure and consider not doing this fast-path and take - // always the common path, it's only a function call and from profiles - // it seems that keeping this fast path makes the compiler not inline - // `flattened_tree_parent`. if self.flattened_tree_parent_is_parent() { debug_assert_eq!( unsafe { @@ -374,12 +372,10 @@ impl<'ln> GeckoNode<'ln> { self.parent_node(), "Fast path stopped holding!" ); - return self.parent_node(); } - // NOTE(emilio): If this call is too expensive, we could manually - // inline more aggressively. + // NOTE(emilio): If this call is too expensive, we could manually inline more aggressively. unsafe { bindings::Gecko_GetFlattenedTreeParentNode(self.0) .as_ref() @@ -650,20 +646,6 @@ impl<'le> GeckoElement<'le> { snapshot_helpers::find_attr(self.non_mapped_attrs(), &atom!("class")) } - #[inline] - fn closest_anon_subtree_root_parent(&self) -> Option { - debug_assert!(self.is_in_native_anonymous_subtree()); - let mut current = *self; - - loop { - if current.is_root_of_native_anonymous_subtree() { - return current.traversal_parent(); - } - - current = current.traversal_parent()?; - } - } - #[inline] fn may_have_anonymous_children(&self) -> bool { self.as_node() @@ -690,13 +672,13 @@ impl<'le> GeckoElement<'le> { /// Returns true if this element has descendants for lazy frame construction. #[inline] pub fn descendants_need_frames(&self) -> bool { - self.flags() & (NODE_DESCENDANTS_NEED_FRAMES as u32) != 0 + self.flags() & NODE_DESCENDANTS_NEED_FRAMES != 0 } /// Returns true if this element needs lazy frame construction. #[inline] pub fn needs_frame(&self) -> bool { - self.flags() & (NODE_NEEDS_FRAME as u32) != 0 + self.flags() & NODE_NEEDS_FRAME != 0 } /// Returns a reference to the DOM slots for this Element, if they exist. @@ -754,7 +736,7 @@ impl<'le> GeckoElement<'le> { fn has_properties(&self) -> bool { use crate::gecko_bindings::structs::NODE_HAS_PROPERTIES; - (self.flags() & NODE_HAS_PROPERTIES as u32) != 0 + self.flags() & NODE_HAS_PROPERTIES != 0 } #[inline] @@ -822,7 +804,7 @@ impl<'le> GeckoElement<'le> { #[inline] fn is_root_of_native_anonymous_subtree(&self) -> bool { use crate::gecko_bindings::structs::NODE_IS_NATIVE_ANONYMOUS_ROOT; - return self.flags() & (NODE_IS_NATIVE_ANONYMOUS_ROOT as u32) != 0; + return self.flags() & NODE_IS_NATIVE_ANONYMOUS_ROOT != 0; } /// Returns true if this node is the shadow root of an use-element shadow tree. @@ -905,19 +887,19 @@ fn selector_flags_to_node_flags(flags: ElementSelectorFlags) -> u32 { use crate::gecko_bindings::structs::*; let mut gecko_flags = 0u32; if flags.contains(ElementSelectorFlags::HAS_SLOW_SELECTOR) { - gecko_flags |= NODE_HAS_SLOW_SELECTOR as u32; + gecko_flags |= NODE_HAS_SLOW_SELECTOR; } if flags.contains(ElementSelectorFlags::HAS_SLOW_SELECTOR_LATER_SIBLINGS) { - gecko_flags |= NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS as u32; + gecko_flags |= NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS; } if flags.contains(ElementSelectorFlags::HAS_SLOW_SELECTOR_NTH_OF) { - gecko_flags |= NODE_HAS_SLOW_SELECTOR_NTH_OF as u32; + gecko_flags |= NODE_HAS_SLOW_SELECTOR_NTH_OF; } if flags.contains(ElementSelectorFlags::HAS_EDGE_CHILD_SELECTOR) { - gecko_flags |= NODE_HAS_EDGE_CHILD_SELECTOR as u32; + gecko_flags |= NODE_HAS_EDGE_CHILD_SELECTOR; } if flags.contains(ElementSelectorFlags::HAS_EMPTY_SELECTOR) { - gecko_flags |= NODE_HAS_EMPTY_SELECTOR as u32; + gecko_flags |= NODE_HAS_EMPTY_SELECTOR; } gecko_flags @@ -974,8 +956,7 @@ impl<'le> TElement for GeckoElement<'le> { // StyleChildrenIterator::IsNeeded does, except that it might return // true if we used to (but no longer) have anonymous content from // ::before/::after, or nsIAnonymousContentCreators. - if self.is_in_native_anonymous_subtree() || - self.is_html_slot_element() || + if self.is_html_slot_element() || self.shadow_root().is_some() || self.may_have_anonymous_children() { @@ -1296,52 +1277,52 @@ impl<'le> TElement for GeckoElement<'le> { #[inline] fn has_snapshot(&self) -> bool { - self.flags() & (ELEMENT_HAS_SNAPSHOT as u32) != 0 + self.flags() & ELEMENT_HAS_SNAPSHOT != 0 } #[inline] fn handled_snapshot(&self) -> bool { - self.flags() & (ELEMENT_HANDLED_SNAPSHOT as u32) != 0 + self.flags() & ELEMENT_HANDLED_SNAPSHOT != 0 } unsafe fn set_handled_snapshot(&self) { debug_assert!(self.has_data()); - self.set_flags(ELEMENT_HANDLED_SNAPSHOT as u32) + self.set_flags(ELEMENT_HANDLED_SNAPSHOT) } #[inline] fn has_dirty_descendants(&self) -> bool { - self.flags() & (ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) != 0 + self.flags() & ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO != 0 } unsafe fn set_dirty_descendants(&self) { debug_assert!(self.has_data()); debug!("Setting dirty descendants: {:?}", self); - self.set_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) + self.set_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO) } unsafe fn unset_dirty_descendants(&self) { - self.unset_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) + self.unset_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO) } #[inline] fn has_animation_only_dirty_descendants(&self) -> bool { - self.flags() & (ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32) != 0 + self.flags() & ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO != 0 } unsafe fn set_animation_only_dirty_descendants(&self) { - self.set_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32) + self.set_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO) } unsafe fn unset_animation_only_dirty_descendants(&self) { - self.unset_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32) + self.unset_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO) } unsafe fn clear_descendant_bits(&self) { self.unset_flags( - ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 | - ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 | - NODE_DESCENDANTS_NEED_FRAMES as u32, + ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO | + ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO | + NODE_DESCENDANTS_NEED_FRAMES, ) } @@ -1349,21 +1330,19 @@ impl<'le> TElement for GeckoElement<'le> { self.state().intersects(ElementState::VISITED) } - /// This logic is duplicated in Gecko's nsINode::IsInNativeAnonymousSubtree. + /// We want to match rules from the same tree in all cases, except for native anonymous content + /// that _isn't_ part directly of a UA widget (e.g., such generated by form controls, or + /// pseudo-elements). #[inline] - fn is_in_native_anonymous_subtree(&self) -> bool { - use crate::gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE; - self.flags() & (NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE as u32) != 0 - } - - #[inline] - fn matches_user_and_author_rules(&self) -> bool { - !self.is_in_native_anonymous_subtree() + fn matches_user_and_content_rules(&self) -> bool { + use crate::gecko_bindings::structs::{NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE, NODE_HAS_BEEN_IN_UA_WIDGET}; + let flags = self.flags(); + (flags & NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE) == 0 || (flags & NODE_HAS_BEEN_IN_UA_WIDGET) != 0 } #[inline] fn implemented_pseudo_element(&self) -> Option { - if !self.is_in_native_anonymous_subtree() { + if self.matches_user_and_content_rules() { return None; } @@ -1371,8 +1350,7 @@ impl<'le> TElement for GeckoElement<'le> { return None; } - let pseudo_type = unsafe { bindings::Gecko_GetImplementedPseudo(self.0) }; - PseudoElement::from_pseudo_type(pseudo_type) + PseudoElement::from_pseudo_type(unsafe { bindings::Gecko_GetImplementedPseudo(self.0) }) } #[inline] @@ -1396,10 +1374,10 @@ impl<'le> TElement for GeckoElement<'le> { unsafe fn clear_data(&self) { let ptr = self.0.mServoData.get(); self.unset_flags( - ELEMENT_HAS_SNAPSHOT as u32 | - ELEMENT_HANDLED_SNAPSHOT as u32 | + ELEMENT_HAS_SNAPSHOT | + ELEMENT_HANDLED_SNAPSHOT | structs::Element_kAllServoDescendantBits | - NODE_NEEDS_FRAME as u32, + NODE_NEEDS_FRAME, ); if !ptr.is_null() { debug!("Dropping ElementData for {:?}", self); @@ -1826,7 +1804,15 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { #[inline] fn pseudo_element_originating_element(&self) -> Option { debug_assert!(self.is_pseudo_element()); - self.closest_anon_subtree_root_parent() + debug_assert!(!self.matches_user_and_content_rules()); + let mut current = *self; + loop { + if current.is_root_of_native_anonymous_subtree() { + return current.traversal_parent(); + } + + current = current.traversal_parent()?; + } } #[inline] @@ -1977,6 +1963,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { .as_node() .parent_node() .map_or(false, |p| p.is_document())); + // XXX this should always return true at this point, shouldn't it? unsafe { bindings::Gecko_IsRootElement(self.0) } } @@ -2102,7 +2089,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { } true }, - NonTSPseudoClass::MozNativeAnonymous => self.is_in_native_anonymous_subtree(), + NonTSPseudoClass::MozNativeAnonymous => !self.matches_user_and_content_rules(), NonTSPseudoClass::MozUseShadowTreeRoot => self.is_root_of_use_element_shadow_tree(), NonTSPseudoClass::MozTableBorderNonzero => unsafe { bindings::Gecko_IsTableBorderNonzero(self.0) diff --git a/components/style/rule_collector.rs b/components/style/rule_collector.rs index 0e18383455c..058d6823179 100644 --- a/components/style/rule_collector.rs +++ b/components/style/rule_collector.rs @@ -73,7 +73,7 @@ where rule_inclusion: RuleInclusion, rules: &'a mut ApplicableDeclarationList, context: &'a mut MatchingContext<'b, E::Impl>, - matches_user_and_author_rules: bool, + matches_user_and_content_rules: bool, matches_document_author_rules: bool, in_sort_scope: bool, } @@ -102,7 +102,7 @@ where MatchingMode::Normal => element.rule_hash_target(), }; - let matches_user_and_author_rules = rule_hash_target.matches_user_and_author_rules(); + let matches_user_and_content_rules = rule_hash_target.matches_user_and_content_rules(); // Gecko definitely has pseudo-elements with style attributes, like // ::-moz-color-swatch. @@ -123,8 +123,8 @@ where rule_inclusion, context, rules, - matches_user_and_author_rules, - matches_document_author_rules: matches_user_and_author_rules, + matches_user_and_content_rules, + matches_document_author_rules: matches_user_and_content_rules, in_sort_scope: false, } } @@ -179,7 +179,7 @@ where } fn collect_user_rules(&mut self) { - if !self.matches_user_and_author_rules { + if !self.matches_user_and_content_rules { return; } @@ -257,7 +257,7 @@ where while let Some(slot) = current { debug_assert!( - self.matches_user_and_author_rules, + self.matches_user_and_content_rules, "We should not slot NAC anywhere" ); slots.push(slot); @@ -292,7 +292,7 @@ where } fn collect_rules_from_containing_shadow_tree(&mut self) { - if !self.matches_user_and_author_rules { + if !self.matches_user_and_content_rules { return; } @@ -341,11 +341,6 @@ where None => return, }; - debug_assert!( - self.matches_user_and_author_rules, - "NAC should not be a shadow host" - ); - let style_data = match shadow.style_data() { Some(d) => d, None => return, diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 2caab7f7305..dcc92de7f42 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -584,8 +584,8 @@ impl StyleSharingCache { }, }; - if element.is_in_native_anonymous_subtree() { - debug!("Failing to insert into the cache: NAC"); + if !element.matches_user_and_content_rules() { + debug!("Failing to insert into the cache: no tree rules:"); return; } @@ -674,8 +674,8 @@ impl StyleSharingCache { return None; } - if target.is_in_native_anonymous_subtree() { - debug!("{:?} Cannot share style: NAC", target.element); + if !target.matches_user_and_content_rules() { + debug!("{:?} Cannot share style: content rules", target.element); return None; } @@ -699,7 +699,7 @@ impl StyleSharingCache { nth_index_cache: &mut NthIndexCache, shared_context: &SharedStyleContext, ) -> Option { - debug_assert!(!target.is_in_native_anonymous_subtree()); + debug_assert!(target.matches_user_and_content_rules()); // Check that we have the same parent, or at least that the parents // share styles and permit sharing across their children. The latter @@ -762,8 +762,8 @@ impl StyleSharingCache { return None; } - if target.matches_user_and_author_rules() != - candidate.element.matches_user_and_author_rules() + if target.matches_user_and_content_rules() != + candidate.element.matches_user_and_content_rules() { trace!("Miss: User and Author Rules"); return None; diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index dbd4ccb09e5..ee69329e1a9 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -219,7 +219,7 @@ where ) -> PrimaryStyle { // Before doing the cascade, check the sharing cache and see if we can // reuse the style via rule node identity. - let may_reuse = !self.element.is_in_native_anonymous_subtree() && + let may_reuse = self.element.matches_user_and_content_rules() && parent_style.is_some() && inputs.rules.is_some(); diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 929dc5344f2..5102a5d1751 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -422,7 +422,7 @@ pub fn recalc_style_at( if let Some(restyle_kind) = restyle_kind { child_restyle_requirement = compute_style(traversal_data, context, element, data, restyle_kind); - if element.is_in_native_anonymous_subtree() { + if !element.matches_user_and_content_rules() { // We must always cascade native anonymous subtrees, since they // may have pseudo-elements underneath that would inherit from the // closest non-NAC ancestor instead of us.