From c53e6e707de36d51a82270f5d05bd0bb004846ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 27 Mar 2023 18:17:56 +0000 Subject: [PATCH] Bug 1822432 - Restyle pseudo-elements as well on part attribute changes. r=jwatt Refactor a bit the code to unify how we deal with this conditional restyling (we had similar code for MustCascadeChildrenIfInheritResetStyle). Differential Revision: https://phabricator.services.mozilla.com/D172890 --- layout/base/RestyleManager.cpp | 2 +- servo/components/style/data.rs | 89 ++++++++++++++----- servo/components/style/dom.rs | 46 ---------- .../invalidation/element/restyle_hints.rs | 76 ++++++++-------- servo/components/style/matching.rs | 7 -- servo/components/style/sharing/mod.rs | 5 -- servo/components/style/traversal.rs | 83 ++++++----------- .../part-mutation-pseudo.html | 27 ++++++ 8 files changed, 157 insertions(+), 178 deletions(-) create mode 100644 testing/web-platform/tests/css/css-shadow-parts/part-mutation-pseudo.html diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index 19de8db1078b1..c323f53506fe6 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -3505,7 +3505,7 @@ void RestyleManager::AttributeChanged(Element* aElement, int32_t aNameSpaceID, } else if (aElement->IsInShadowTree() && aAttribute == nsGkAtoms::part) { // TODO(emilio, bug 1598094): Maybe finer-grained invalidation for part // attribute changes? - restyleHint |= RestyleHint::RESTYLE_SELF; + restyleHint |= RestyleHint::RESTYLE_SELF | RestyleHint::RESTYLE_PSEUDOS; } if (nsIFrame* primaryFrame = aElement->GetPrimaryFrame()) { diff --git a/servo/components/style/data.rs b/servo/components/style/data.rs index a39513220046b..587c1e3a1a8e6 100644 --- a/servo/components/style/data.rs +++ b/servo/components/style/data.rs @@ -4,6 +4,7 @@ //! Per-node data used in style calculation. +use crate::computed_value_flags::ComputedValueFlags; use crate::context::{SharedStyleContext, StackLimitChecker}; use crate::dom::TElement; use crate::invalidation::element::invalidator::InvalidationResult; @@ -193,8 +194,6 @@ impl ElementStyles { /// Whether this element uses viewport units. pub fn viewport_unit_usage(&self) -> ViewportUnitUsage { - use crate::computed_value_flags::ComputedValueFlags; - fn usage_from_flags(flags: ComputedValueFlags) -> ViewportUnitUsage { if flags.intersects(ComputedValueFlags::USES_VIEWPORT_UNITS_ON_CONTAINER_QUERIES) { return ViewportUnitUsage::FromQuery; @@ -366,52 +365,88 @@ impl ElementData { /// Returns the kind of restyling that we're going to need to do on this /// element, based of the stored restyle hint. - pub fn restyle_kind(&self, shared_context: &SharedStyleContext) -> RestyleKind { + pub fn restyle_kind(&self, shared_context: &SharedStyleContext) -> Option { if shared_context.traversal_flags.for_animation_only() { return self.restyle_kind_for_animation(shared_context); } - if !self.has_styles() { - return RestyleKind::MatchAndCascade; + let style = match self.styles.primary { + Some(ref s) => s, + None => return Some(RestyleKind::MatchAndCascade), + }; + + let hint = self.hint; + if hint.is_empty() { + return None; } - if self.hint.match_self() { - return RestyleKind::MatchAndCascade; + let needs_to_match_self = hint.intersects(RestyleHint::RESTYLE_SELF) || + (hint.intersects(RestyleHint::RESTYLE_SELF_IF_PSEUDO) && style.is_pseudo_style()); + if needs_to_match_self { + return Some(RestyleKind::MatchAndCascade); } - if self.hint.has_replacements() { + if hint.has_replacements() { debug_assert!( - !self.hint.has_animation_hint(), + !hint.has_animation_hint(), "Animation only restyle hint should have already processed" ); - return RestyleKind::CascadeWithReplacements(self.hint & RestyleHint::replacements()); + return Some(RestyleKind::CascadeWithReplacements( + hint & RestyleHint::replacements(), + )); } - debug_assert!( - self.hint.has_recascade_self(), - "We definitely need to do something: {:?}!", - self.hint - ); - return RestyleKind::CascadeOnly; + let needs_to_recascade_self = hint.intersects(RestyleHint::RECASCADE_SELF) || + (hint.intersects(RestyleHint::RECASCADE_SELF_IF_INHERIT_RESET_STYLE) && + style.flags.contains(ComputedValueFlags::INHERITS_RESET_STYLE)); + if needs_to_recascade_self { + return Some(RestyleKind::CascadeOnly); + } + + None } /// Returns the kind of restyling for animation-only restyle. - fn restyle_kind_for_animation(&self, shared_context: &SharedStyleContext) -> RestyleKind { + fn restyle_kind_for_animation( + &self, + shared_context: &SharedStyleContext, + ) -> Option { debug_assert!(shared_context.traversal_flags.for_animation_only()); debug_assert!( self.has_styles(), - "Unstyled element shouldn't be traversed during \ - animation-only traversal" + "animation traversal doesn't care about unstyled elements" ); - // return either CascadeWithReplacements or CascadeOnly in case of + // FIXME: We should ideally restyle here, but it is a hack to work around our weird + // animation-only traversal stuff: If we're display: none and the rules we could + // match could change, we consider our style up-to-date. This is because re-cascading with + // and old style doesn't guarantee returning the correct animation style (that's + // bug 1393323). So if our display changed, and it changed from display: none, we would + // incorrectly forget about it and wouldn't be able to correctly style our descendants + // later. + // XXX Figure out if this still makes sense. + let hint = self.hint; + if self.styles.is_display_none() && hint.intersects(RestyleHint::RESTYLE_SELF) { + return None; + } + + let style = self.styles.primary(); + // Return either CascadeWithReplacements or CascadeOnly in case of // animation-only restyle. I.e. animation-only restyle never does // selector matching. - if self.hint.has_animation_hint() { - return RestyleKind::CascadeWithReplacements(self.hint & RestyleHint::for_animations()); + if hint.has_animation_hint() { + return Some(RestyleKind::CascadeWithReplacements( + hint & RestyleHint::for_animations(), + )); } - return RestyleKind::CascadeOnly; + let needs_to_recascade_self = hint.intersects(RestyleHint::RECASCADE_SELF) || + (hint.intersects(RestyleHint::RECASCADE_SELF_IF_INHERIT_RESET_STYLE) && + style.flags.contains(ComputedValueFlags::INHERITS_RESET_STYLE)); + if needs_to_recascade_self { + return Some(RestyleKind::CascadeOnly); + } + return None; } /// Drops any restyle state from the element. @@ -482,7 +517,13 @@ impl ElementData { ) { return false; } - if !self.styles.primary().get_box().clone_container_type().is_normal() { + if !self + .styles + .primary() + .get_box() + .clone_container_type() + .is_normal() + { return false; } true diff --git a/servo/components/style/dom.rs b/servo/components/style/dom.rs index 00941c49f7ffc..5c0c9b20e70fe 100644 --- a/servo/components/style/dom.rs +++ b/servo/components/style/dom.rs @@ -17,7 +17,6 @@ use crate::properties::{AnimationDeclarations, ComputedValues, PropertyDeclarati use crate::selector_parser::{AttrValue, Lang, PseudoElement, SelectorImpl}; use crate::shared_lock::{Locked, SharedRwLock}; use crate::stylist::CascadeData; -use crate::traversal_flags::TraversalFlags; use crate::values::AtomIdent; use crate::values::computed::Display; use crate::{LocalName, Namespace, WeakAtom}; @@ -587,51 +586,6 @@ pub trait TElement: /// Flags this element as having handled already its snapshot. unsafe fn set_handled_snapshot(&self); - /// Returns whether the element's styles are up-to-date for |traversal_flags|. - fn has_current_styles_for_traversal( - &self, - data: &ElementData, - traversal_flags: TraversalFlags, - ) -> bool { - if traversal_flags.for_animation_only() { - // In animation-only restyle we never touch snapshots and don't care - // about them. But we can't assert '!self.handled_snapshot()' - // here since there are some cases that a second animation-only - // restyle which is a result of normal restyle (e.g. setting - // animation-name in normal restyle and creating a new CSS - // animation in a SequentialTask) is processed after the normal - // traversal in that we had elements that handled snapshot. - if !data.has_styles() { - return false; - } - - if !data.hint.has_animation_hint_or_recascade() { - return true; - } - - // FIXME: This should ideally always return false, but it is a hack - // to work around our weird animation-only traversal - // stuff: If we're display: none and the rules we could match could - // change, we consider our style up-to-date. This is because - // re-cascading with and old style doesn't guarantee returning the - // correct animation style (that's bug 1393323). So if our display - // changed, and it changed from display: none, we would incorrectly - // forget about it and wouldn't be able to correctly style our - // descendants later. - if data.styles.is_display_none() && data.hint.match_self() { - return true; - } - - return false; - } - - if self.has_snapshot() && !self.handled_snapshot() { - return false; - } - - data.has_styles() && !data.hint.has_non_animation_invalidations() - } - /// Returns whether the element's styles are up-to-date after traversal /// (i.e. in post traversal). fn has_current_styles(&self, data: &ElementData) -> bool { diff --git a/servo/components/style/invalidation/element/restyle_hints.rs b/servo/components/style/invalidation/element/restyle_hints.rs index c423d8539056b..6020e429ec65c 100644 --- a/servo/components/style/invalidation/element/restyle_hints.rs +++ b/servo/components/style/invalidation/element/restyle_hints.rs @@ -9,38 +9,48 @@ use crate::traversal_flags::TraversalFlags; bitflags! { /// The kind of restyle we need to do for a given element. #[repr(C)] - pub struct RestyleHint: u8 { + pub struct RestyleHint: u16 { /// Do a selector match of the element. const RESTYLE_SELF = 1 << 0; + /// Do a selector match of the element's pseudo-elements. Always to be combined with + /// RESTYLE_SELF. + const RESTYLE_PSEUDOS = 1 << 1; + + /// Do a selector match if the element is a pseudo-element. + const RESTYLE_SELF_IF_PSEUDO = 1 << 2; + /// Do a selector match of the element's descendants. - const RESTYLE_DESCENDANTS = 1 << 1; + const RESTYLE_DESCENDANTS = 1 << 3; /// Recascade the current element. - const RECASCADE_SELF = 1 << 2; + const RECASCADE_SELF = 1 << 4; + + /// Recascade the current element if it inherits any reset style. + const RECASCADE_SELF_IF_INHERIT_RESET_STYLE = 1 << 5; /// Recascade all descendant elements. - const RECASCADE_DESCENDANTS = 1 << 3; + const RECASCADE_DESCENDANTS = 1 << 6; /// Replace the style data coming from CSS transitions without updating /// any other style data. This hint is only processed in animation-only /// traversal which is prior to normal traversal. - const RESTYLE_CSS_TRANSITIONS = 1 << 4; + const RESTYLE_CSS_TRANSITIONS = 1 << 7; /// Replace the style data coming from CSS animations without updating /// any other style data. This hint is only processed in animation-only /// traversal which is prior to normal traversal. - const RESTYLE_CSS_ANIMATIONS = 1 << 5; + const RESTYLE_CSS_ANIMATIONS = 1 << 8; /// Don't re-run selector-matching on the element, only the style /// attribute has changed, and this change didn't have any other /// dependencies. - const RESTYLE_STYLE_ATTRIBUTE = 1 << 6; + const RESTYLE_STYLE_ATTRIBUTE = 1 << 9; /// Replace the style data coming from SMIL animations without updating /// any other style data. This hint is only processed in animation-only /// traversal which is prior to normal traversal. - const RESTYLE_SMIL = 1 << 7; + const RESTYLE_SMIL = 1 << 10; } } @@ -70,11 +80,7 @@ impl RestyleHint { /// Returns whether we need to restyle this element. pub fn has_non_animation_invalidations(&self) -> bool { - self.intersects( - RestyleHint::RESTYLE_SELF | - RestyleHint::RECASCADE_SELF | - (Self::replacements() & !Self::for_animations()), - ) + !(*self & !Self::for_animations()).is_empty() } /// Propagates this restyle hint to a child element. @@ -98,16 +104,19 @@ impl RestyleHint { mem::replace(self, Self::empty()).propagate_for_non_animation_restyle() } - /// Returns a new `CascadeHint` appropriate for children of the current - /// element. + /// Returns a new `RestyleHint` appropriate for children of the current element. fn propagate_for_non_animation_restyle(&self) -> Self { if self.contains(RestyleHint::RESTYLE_DESCENDANTS) { return Self::restyle_subtree(); } + let mut result = Self::empty(); + if self.contains(RestyleHint::RESTYLE_PSEUDOS) { + result |= Self::RESTYLE_SELF_IF_PSEUDO; + } if self.contains(RestyleHint::RECASCADE_DESCENDANTS) { - return Self::recascade_subtree(); + result |= Self::recascade_subtree(); } - Self::empty() + result } /// Returns a hint that contains all the replacement hints. @@ -123,12 +132,6 @@ impl RestyleHint { RestyleHint::RESTYLE_CSS_TRANSITIONS } - /// Returns whether the hint specifies that the currently element must be - /// recascaded. - pub fn has_recascade_self(&self) -> bool { - self.contains(RestyleHint::RECASCADE_SELF) - } - /// Returns whether the hint specifies that an animation cascade level must /// be replaced. #[inline] @@ -140,7 +143,7 @@ impl RestyleHint { /// be replaced. #[inline] pub fn has_animation_hint_or_recascade(&self) -> bool { - self.intersects(Self::for_animations() | RestyleHint::RECASCADE_SELF) + self.intersects(Self::for_animations() | Self::RECASCADE_SELF | Self::RECASCADE_SELF_IF_INHERIT_RESET_STYLE) } /// Returns whether the hint specifies some restyle work other than an @@ -150,13 +153,6 @@ impl RestyleHint { !(*self & !Self::for_animations()).is_empty() } - /// Returns whether the hint specifies that selector matching must be re-run - /// for the element. - #[inline] - pub fn match_self(&self) -> bool { - self.intersects(RestyleHint::RESTYLE_SELF) - } - /// Returns whether the hint specifies that some cascade levels must be /// replaced. #[inline] @@ -169,16 +165,14 @@ impl RestyleHint { pub fn remove_animation_hints(&mut self) { self.remove(Self::for_animations()); - // While RECASCADE_SELF is not animation-specific, we only ever add and - // process it during traversal. If we are here, removing animation - // hints, then we are in an animation-only traversal, and we know that - // any RECASCADE_SELF flag must have been set due to changes in - // inherited values after restyling for animations, and thus we want to - // remove it so that we don't later try to restyle the element during a - // normal restyle. (We could have separate RECASCADE_SELF_NORMAL and - // RECASCADE_SELF_ANIMATIONS flags to make it clear, but this isn't - // currently necessary.) - self.remove(RestyleHint::RECASCADE_SELF); + // While RECASCADE_SELF is not animation-specific, we only ever add and process it during + // traversal. If we are here, removing animation hints, then we are in an animation-only + // traversal, and we know that any RECASCADE_SELF flag must have been set due to changes in + // inherited values after restyling for animations, and thus we want to remove it so that + // we don't later try to restyle the element during a normal restyle. + // (We could have separate RECASCADE_SELF_NORMAL and RECASCADE_SELF_ANIMATIONS flags to + // make it clear, but this isn't currently necessary.) + self.remove(Self::RECASCADE_SELF | Self::RECASCADE_SELF_IF_INHERIT_RESET_STYLE); } } diff --git a/servo/components/style/matching.rs b/servo/components/style/matching.rs index ffdb030e0c725..4a2e72b6c83b0 100644 --- a/servo/components/style/matching.rs +++ b/servo/components/style/matching.rs @@ -75,13 +75,6 @@ pub enum ChildRestyleRequirement { MustMatchDescendants = 4, } -impl ChildRestyleRequirement { - /// Whether we can unconditionally skip the cascade. - pub fn can_skip_cascade(&self) -> bool { - matches!(*self, ChildRestyleRequirement::CanSkipCascade) - } -} - /// Determines which styles are being cascaded currently. #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum CascadeVisitedMode { diff --git a/servo/components/style/sharing/mod.rs b/servo/components/style/sharing/mod.rs index 28883d6d13949..2caab7f730578 100644 --- a/servo/components/style/sharing/mod.rs +++ b/servo/components/style/sharing/mod.rs @@ -800,11 +800,6 @@ impl StyleSharingCache { return None; } - debug_assert!(target.has_current_styles_for_traversal( - &candidate.element.borrow_data().unwrap(), - shared.traversal_flags, - )); - debug!( "Sharing allowed between {:?} and {:?}", target.element, candidate.element diff --git a/servo/components/style/traversal.rs b/servo/components/style/traversal.rs index ea8dd297d1a1b..1b6f2e1088f15 100644 --- a/servo/components/style/traversal.rs +++ b/servo/components/style/traversal.rs @@ -5,7 +5,7 @@ //! Traversing the DOM tree; the bloom filter. use crate::context::{ElementCascadeInputs, SharedStyleContext, StyleContext}; -use crate::data::{ElementData, ElementStyles}; +use crate::data::{ElementData, ElementStyles, RestyleKind}; use crate::dom::{NodeInfo, OpaqueNode, TElement, TNode}; use crate::invalidation::element::restyle_hints::RestyleHint; use crate::matching::{ChildRestyleRequirement, MatchMethods}; @@ -217,16 +217,12 @@ pub trait DomTraversal: Sync { el, traversal_flags, data ); - // In case of animation-only traversal we need to traverse the element - // if the element has animation only dirty descendants bit, - // animation-only restyle hint or recascade. + // In case of animation-only traversal we need to traverse the element if the element has + // animation only dirty descendants bit, animation-only restyle hint. if traversal_flags.for_animation_only() { return data.map_or(false, |d| d.has_styles()) && (el.has_animation_only_dirty_descendants() || - data.as_ref() - .unwrap() - .hint - .has_animation_hint_or_recascade()); + data.as_ref().unwrap().hint.has_animation_hint_or_recascade()); } // Non-incremental layout visits every node. @@ -411,13 +407,11 @@ pub fn recalc_style_at( "Should've handled snapshots here already" ); - let compute_self = !element.has_current_styles_for_traversal(data, flags); - + let restyle_kind = data.restyle_kind(&context.shared); debug!( - "recalc_style_at: {:?} (compute_self={:?}, \ - dirty_descendants={:?}, data={:?})", + "recalc_style_at: {:?} (restyle_kind={:?}, dirty_descendants={:?}, data={:?})", element, - compute_self, + restyle_kind, element.has_dirty_descendants(), data ); @@ -425,8 +419,8 @@ pub fn recalc_style_at( let mut child_restyle_requirement = ChildRestyleRequirement::CanSkipCascade; // Compute style for this element if necessary. - if compute_self { - child_restyle_requirement = compute_style(traversal_data, context, element, data); + 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() { // We must always cascade native anonymous subtrees, since they @@ -468,8 +462,7 @@ pub fn recalc_style_at( "animation restyle hint should be handled during \ animation-only restyles" ); - let propagated_hint = data.hint.propagate(&flags); - + let mut propagated_hint = data.hint.propagate(&flags); trace!( "propagated_hint={:?}, restyle_requirement={:?}, \ is_display_none={:?}, implementing_pseudo={:?}", @@ -478,11 +471,23 @@ pub fn recalc_style_at( data.styles.is_display_none(), element.implemented_pseudo_element() ); - debug_assert!( - element.has_current_styles_for_traversal(data, flags), - "Should have computed style or haven't yet valid computed \ - style in case of animation-only restyle" - ); + + // Integrate the child cascade requirement into the propagated hint. + match child_restyle_requirement { + ChildRestyleRequirement::CanSkipCascade => {}, + ChildRestyleRequirement::MustCascadeDescendants => { + propagated_hint |= RestyleHint::RECASCADE_SELF | RestyleHint::RECASCADE_DESCENDANTS; + }, + ChildRestyleRequirement::MustCascadeChildrenIfInheritResetStyle => { + propagated_hint |= RestyleHint::RECASCADE_SELF_IF_INHERIT_RESET_STYLE; + }, + ChildRestyleRequirement::MustCascadeChildren => { + propagated_hint |= RestyleHint::RECASCADE_SELF; + }, + ChildRestyleRequirement::MustMatchDescendants => { + propagated_hint |= RestyleHint::restyle_subtree(); + }, + } let has_dirty_descendants_for_this_restyle = if flags.for_animation_only() { element.has_animation_only_dirty_descendants() @@ -496,14 +501,12 @@ pub fn recalc_style_at( // // * We have the dirty descendants bit. // * We're propagating a restyle hint. - // * We can't skip the cascade. // * This is a servo non-incremental traversal. // // We only do this if we're not a display: none root, since in that case // it's useless to style children. let mut traverse_children = has_dirty_descendants_for_this_restyle || !propagated_hint.is_empty() || - !child_restyle_requirement.can_skip_cascade() || is_servo_nonincremental_layout(); traverse_children = traverse_children && !data.styles.is_display_none(); @@ -515,7 +518,6 @@ pub fn recalc_style_at( element, data, propagated_hint, - child_restyle_requirement, is_initial_style, note_child, ); @@ -548,6 +550,7 @@ fn compute_style( context: &mut StyleContext, element: E, data: &mut ElementData, + kind: RestyleKind, ) -> ChildRestyleRequirement where E: TElement, @@ -555,8 +558,6 @@ where use crate::data::RestyleKind::*; context.thread_local.statistics.elements_styled += 1; - let kind = data.restyle_kind(context.shared); - debug!("compute_style: {:?} (kind={:?})", element, kind); if data.has_styles() { @@ -733,7 +734,6 @@ fn note_children( element: E, data: &ElementData, propagated_hint: RestyleHint, - restyle_requirement: ChildRestyleRequirement, is_initial_style: bool, mut note_child: F, ) where @@ -769,32 +769,7 @@ fn note_children( ); if let Some(ref mut child_data) = child_data { - let mut child_hint = propagated_hint; - match restyle_requirement { - ChildRestyleRequirement::CanSkipCascade => {}, - ChildRestyleRequirement::MustCascadeDescendants => { - child_hint |= RestyleHint::RECASCADE_SELF | RestyleHint::RECASCADE_DESCENDANTS; - }, - ChildRestyleRequirement::MustCascadeChildrenIfInheritResetStyle => { - use crate::computed_value_flags::ComputedValueFlags; - if child_data - .styles - .primary() - .flags - .contains(ComputedValueFlags::INHERITS_RESET_STYLE) - { - child_hint |= RestyleHint::RECASCADE_SELF; - } - }, - ChildRestyleRequirement::MustCascadeChildren => { - child_hint |= RestyleHint::RECASCADE_SELF; - }, - ChildRestyleRequirement::MustMatchDescendants => { - child_hint |= RestyleHint::restyle_subtree(); - }, - } - - child_data.hint.insert(child_hint); + child_data.hint.insert(propagated_hint); // Handle element snapshots and invalidation of descendants and siblings // as needed. diff --git a/testing/web-platform/tests/css/css-shadow-parts/part-mutation-pseudo.html b/testing/web-platform/tests/css/css-shadow-parts/part-mutation-pseudo.html new file mode 100644 index 0000000000000..d0e0072d51e65 --- /dev/null +++ b/testing/web-platform/tests/css/css-shadow-parts/part-mutation-pseudo.html @@ -0,0 +1,27 @@ + + +CSS Shadow Parts - Invalidation Change Part Name on pseudo + + + + + + + + + +The following text should be green: +
+