style: Restyle pseudo-elements as well on part attribute changes

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

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<RestyleKind> {
         if shared_context.traversal_flags.for_animation_only() {
             return self.restyle_kind_for_animation(shared_context);
         }
 
-        if !self.has_styles() {
+        let style = match self.styles.primary {
-            return RestyleKind::MatchAndCascade;
+            Some(ref s) => s,
+            None => return Some(RestyleKind::MatchAndCascade),
+        };
+
+        let hint = self.hint;
+        if hint.is_empty() {
+            return None;
         }
 
-        if self.hint.match_self() {
+        let needs_to_match_self = hint.intersects(RestyleHint::RESTYLE_SELF) ||
-            return RestyleKind::MatchAndCascade;
+            (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!(
+        let needs_to_recascade_self = hint.intersects(RestyleHint::RECASCADE_SELF) ||
-            self.hint.has_recascade_self(),
+            (hint.intersects(RestyleHint::RECASCADE_SELF_IF_INHERIT_RESET_STYLE) &&
-            "We definitely need to do something: {:?}!",
+                style.flags.contains(ComputedValueFlags::INHERITS_RESET_STYLE));
-            self.hint
+        if needs_to_recascade_self {
-        );
+            return Some(RestyleKind::CascadeOnly);
-        return 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<RestyleKind> {
         debug_assert!(shared_context.traversal_flags.for_animation_only());
         debug_assert!(
             self.has_styles(),
-            "Unstyled element shouldn't be traversed during \
+            "animation traversal doesn't care about unstyled elements"
-             animation-only traversal"
         );
 
-        // 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 restyle. I.e. animation-only restyle never does
+        // animation-only traversal stuff: If we're display: none and the rules we could
-        // selector matching.
+        // match could change, we consider our style up-to-date. This is because re-cascading with
-        if self.hint.has_animation_hint() {
+        // and old style doesn't guarantee returning the correct animation style (that's
-            return RestyleKind::CascadeWithReplacements(self.hint & RestyleHint::for_animations());
+        // 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;
         }
 
-        return RestyleKind::CascadeOnly;
+        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 hint.has_animation_hint() {
+            return Some(RestyleKind::CascadeWithReplacements(
+                hint & RestyleHint::for_animations(),
+            ));
+        }
+
+        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

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 {

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(
+        !(*self & !Self::for_animations()).is_empty()
-            RestyleHint::RESTYLE_SELF |
-                RestyleHint::RECASCADE_SELF |
-                (Self::replacements() & !Self::for_animations()),
-        )
     }
 
     /// 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
+    /// Returns a new `RestyleHint` appropriate for children of the current element.
-    /// element.
     fn propagate_for_non_animation_restyle(&self) -> Self {
         if self.contains(RestyleHint::RESTYLE_DESCENDANTS) {
             return Self::restyle_subtree();
         }
-        if self.contains(RestyleHint::RECASCADE_DESCENDANTS) {
+        let mut result = Self::empty();
-            return Self::recascade_subtree();
+        if self.contains(RestyleHint::RESTYLE_PSEUDOS) {
+            result |= Self::RESTYLE_SELF_IF_PSEUDO;
         }
-        Self::empty()
+        if self.contains(RestyleHint::RECASCADE_DESCENDANTS) {
+            result |= Self::recascade_subtree();
+        }
+        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
+        // While RECASCADE_SELF is not animation-specific, we only ever add and process it during
-        // process it during traversal.  If we are here, removing animation
+        // traversal.  If we are here, removing animation hints, then we are in an animation-only
-        // hints, then we are in an animation-only traversal, and we know that
+        // traversal, and we know that any RECASCADE_SELF flag must have been set due to changes in
-        // 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
-        // inherited values after restyling for animations, and thus we want to
+        // we don't later try to restyle the element during a normal restyle.
-        // remove it so that we don't later try to restyle the element during a
+        // (We could have separate RECASCADE_SELF_NORMAL and RECASCADE_SELF_ANIMATIONS flags to
-        // normal restyle.  (We could have separate RECASCADE_SELF_NORMAL and
+        // make it clear, but this isn't currently necessary.)
-        // RECASCADE_SELF_ANIMATIONS flags to make it clear, but this isn't
+        self.remove(Self::RECASCADE_SELF | Self::RECASCADE_SELF_IF_INHERIT_RESET_STYLE);
-        // currently necessary.)
-        self.remove(RestyleHint::RECASCADE_SELF);
     }
 }
 

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 {

components/style/sharing/mod.rs

@@ -800,11 +800,6 @@ impl<E: TElement> StyleSharingCache<E> {
             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

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<E: TElement>: Sync {
             el, traversal_flags, data
         );
 
-        // In case of animation-only traversal we need to traverse the element
+        // In case of animation-only traversal we need to traverse the element if the element has
-        // if the element has animation only dirty descendants bit,
+        // animation only dirty descendants bit, animation-only restyle hint.
-        // animation-only restyle hint or recascade.
         if traversal_flags.for_animation_only() {
             return data.map_or(false, |d| d.has_styles()) &&
                 (el.has_animation_only_dirty_descendants() ||
-                    data.as_ref()
+                 data.as_ref().unwrap().hint.has_animation_hint_or_recascade());
-                        .unwrap()
-                        .hint
-                        .has_animation_hint_or_recascade());
         }
 
         // Non-incremental layout visits every node.
@@ -411,13 +407,11 @@ pub fn recalc_style_at<E, D, F>(
         "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={:?}, \
+        "recalc_style_at: {:?} (restyle_kind={:?}, dirty_descendants={:?}, data={:?})",
-         dirty_descendants={:?}, data={:?})",
         element,
-        compute_self,
+        restyle_kind,
         element.has_dirty_descendants(),
         data
     );
@@ -425,8 +419,8 @@ pub fn recalc_style_at<E, D, F>(
     let mut child_restyle_requirement = ChildRestyleRequirement::CanSkipCascade;
 
     // Compute style for this element if necessary.
-    if compute_self {
+    if let Some(restyle_kind) = restyle_kind {
-        child_restyle_requirement = compute_style(traversal_data, context, element, data);
+        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<E, D, F>(
         "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<E, D, F>(
         data.styles.is_display_none(),
         element.implemented_pseudo_element()
     );
-    debug_assert!(
+
-        element.has_current_styles_for_traversal(data, flags),
+    // Integrate the child cascade requirement into the propagated hint.
-        "Should have computed style or haven't yet valid computed \
+    match child_restyle_requirement {
-         style in case of animation-only restyle"
+        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<E, D, F>(
     //
     //  * 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<E, D, F>(
             element,
             data,
             propagated_hint,
-            child_restyle_requirement,
             is_initial_style,
             note_child,
         );
@@ -548,6 +550,7 @@ fn compute_style<E>(
     context: &mut StyleContext<E>,
     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<E, D, F>(
     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<E, D, F>(
         );
 
         if let Some(ref mut child_data) = child_data {
-            let mut child_hint = propagated_hint;
+            child_data.hint.insert(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);
 
             // Handle element snapshots and invalidation of descendants and siblings
             // as needed.