Skip to content

Commit

Permalink
Bug 1822432 - Restyle pseudo-elements as well on part attribute chang…
Browse files Browse the repository at this point in the history 
…es. 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
  • Loading branch information
@emilio
emilio committed Mar 27, 2023
1 parent d63eeb8 commit c53e6e7
Show file tree
Hide file tree
Showing 8 changed files with 157 additions and 178 deletions.
2 changes: 1 addition & 1 deletion layout/base/RestyleManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
89 changes: 65 additions & 24 deletions servo/components/style/data.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
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<RestyleKind> {
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.
Expand Down Expand Up @@ -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
Expand Down
46 changes: 0 additions & 46 deletions servo/components/style/dom.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down
76 changes: 35 additions & 41 deletions servo/components/style/invalidation/element/restyle_hints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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]
Expand All @@ -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
Expand All @@ -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]
Expand All @@ -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);
}
}

Expand Down
7 changes: 0 additions & 7 deletions servo/components/style/matching.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 0 additions & 5 deletions servo/components/style/sharing/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit c53e6e7

Please sign in to comment.