Skip to content

Commit

Permalink
Do not allow first-line-inherited styles to start transitions
Browse files Browse the repository at this point in the history
If a computed property value is inherited from ::first-line styles into

the styles for inline boxes on the first line, no transitions should
be started on those elements since those inherited values are not part
of those elements' computed styles.

However, transitions and animations running on the element without any
::first-line styles applied still need to be applied for ::first-line
rendering.

The way we accomplish that is to pass a parameter through StyleRequest
for that behavior.

Bug: 1422029
Change-Id: Ic957d4a596771d6ee5303bf5b43f8321ce47c034
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4315914
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1115630}
  • Loading branch information
Rune Lillesveen authored and Chromium LUCI CQ committed Mar 10, 2023
1 parent 69aba5a commit 352b384
Show file tree
Hide file tree
Showing 14 changed files with 79 additions and 23 deletions.
12 changes: 8 additions & 4 deletions third_party/blink/renderer/core/animation/css/css_animations.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1068,12 +1068,14 @@ void CSSAnimations::CalculateAnimationUpdate(
Element& element,
const ComputedStyleBuilder& style_builder,
const ComputedStyle* parent_style,
StyleResolver* resolver) {
StyleResolver* resolver,
bool can_trigger_animations) {
ElementAnimations* element_animations =
animating_element.GetElementAnimations();

bool is_animation_style_change =
element_animations && element_animations->IsAnimationStyleChange();
!can_trigger_animations ||
(element_animations && element_animations->IsAnimationStyleChange());

#if !DCHECK_IS_ON()
// If we're in an animation style change, no animations can have started, been
Expand Down Expand Up @@ -2064,7 +2066,8 @@ void CSSAnimations::CalculateTransitionUpdate(
CSSAnimationUpdate& update,
Element& animating_element,
const ComputedStyleBuilder& style_builder,
const ComputedStyle* old_style) {
const ComputedStyle* old_style,
bool can_trigger_animations) {
if (animating_element.GetDocument().FinishingOrIsPrinting())
return;

Expand All @@ -2078,7 +2081,8 @@ void CSSAnimations::CalculateTransitionUpdate(
style_builder.GetWritingDirection();

const bool animation_style_recalc =
element_animations && element_animations->IsAnimationStyleChange();
!can_trigger_animations ||
(element_animations && element_animations->IsAnimationStyleChange());

HashSet<PropertyHandle> listed_properties;
bool any_transition_had_transition_all = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ class CORE_EXPORT CSSAnimations final {
Element&,
const ComputedStyleBuilder&,
const ComputedStyle* parent_style,
StyleResolver*);
StyleResolver*,
bool can_trigger_animations);
static void CalculateCompositorAnimationUpdate(
CSSAnimationUpdate&,
const Element& animating_element,
Expand All @@ -108,7 +109,8 @@ class CORE_EXPORT CSSAnimations final {
static void CalculateTransitionUpdate(CSSAnimationUpdate&,
Element& animating_element,
const ComputedStyleBuilder&,
const ComputedStyle* old_style);
const ComputedStyle* old_style,
bool can_trigger_animations);

static void SnapshotCompositorKeyframes(Element&,
CSSAnimationUpdate&,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,11 @@ class TestCascade {
void CalculateInterpolationUpdate() {
CSSAnimations::CalculateTransitionUpdate(
state_.AnimationUpdate(), state_.GetElement(), state_.StyleBuilder(),
state_.OldStyle());
state_.OldStyle(), true /* can_trigger_animations */);
CSSAnimations::CalculateAnimationUpdate(
state_.AnimationUpdate(), state_.GetElement(), state_.GetElement(),
state_.StyleBuilder(), state_.ParentStyle(),
&GetDocument().GetStyleResolver());
&GetDocument().GetStyleResolver(), true /* can_trigger_animations */);
}

CascadeOrigin current_origin_ = CascadeOrigin::kUserAgent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1492,7 +1492,7 @@ void StyleResolver::ApplyBaseStyle(
StyleCascade& cascade) {
DCHECK(style_request.pseudo_id != kPseudoIdFirstLineInherited);

if (state.CanCacheBaseStyle() && CanReuseBaseComputedStyle(state)) {
if (state.CanTriggerAnimations() && CanReuseBaseComputedStyle(state)) {
const ComputedStyle* animation_base_computed_style =
CachedAnimationBaseComputedStyle(state);
DCHECK(animation_base_computed_style);
Expand Down Expand Up @@ -1880,10 +1880,11 @@ bool StyleResolver::ApplyAnimatedStyle(StyleResolverState& state,

CSSAnimations::CalculateAnimationUpdate(
state.AnimationUpdate(), *animating_element, state.GetElement(),
state.StyleBuilder(), state.ParentStyle(), this);
state.StyleBuilder(), state.ParentStyle(), this,
state.CanTriggerAnimations());
CSSAnimations::CalculateTransitionUpdate(
state.AnimationUpdate(), *animating_element, state.StyleBuilder(),
state.OldStyle());
state.OldStyle(), state.CanTriggerAnimations());

bool apply = !state.AnimationUpdate().IsEmpty();
if (apply) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@ namespace blink {

namespace {

bool CanCacheBaseStyle(const StyleRequest& style_request) {
return style_request.IsPseudoStyleRequest() ||
(!style_request.parent_override &&
!style_request.layout_parent_override &&
style_request.matching_behavior == kMatchAllRules);
}

Element* ComputeStyledElement(const StyleRequest& style_request,
Element& element) {
Element* styled_element = style_request.styled_element;
Expand Down Expand Up @@ -84,7 +77,7 @@ StyleResolverState::StyleResolverState(
is_for_highlight_(IsHighlightPseudoElement(style_request.pseudo_id)),
uses_highlight_pseudo_inheritance_(
::blink::UsesHighlightPseudoInheritance(style_request.pseudo_id)),
can_cache_base_style_(blink::CanCacheBaseStyle(style_request)) {
can_trigger_animations_(style_request.can_trigger_animations) {
DCHECK(!!parent_style_ == !!layout_parent_style_);

if (UsesHighlightPseudoInheritance()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ class CORE_EXPORT StyleResolverState {
return uses_highlight_pseudo_inheritance_;
}

bool CanCacheBaseStyle() const { return can_cache_base_style_; }
bool CanTriggerAnimations() const { return can_trigger_animations_; }

bool HadNoMatchedProperties() const { return had_no_matched_properties_; }
void SetHadNoMatchedProperties() { had_no_matched_properties_ = true; }
Expand Down Expand Up @@ -265,9 +265,13 @@ class CORE_EXPORT StyleResolverState {
// should be used for this highlight pseudo.
const bool uses_highlight_pseudo_inheritance_;

// True if the base style can be cached to optimize style recalculations for
// animation updates or transition retargeting.
bool can_cache_base_style_ = false;
// True if this style resolution can start or stop animations and transitions.
// One case where animations and transitions can not be triggered is when we
// resolve FirstLineInherited style for an element on the first line. Styles
// inherited from the ::first-line styles should not cause transitions to
// start on such elements. Still, animations and transitions in progress still
// need to apply the effect for theses styles as well.
bool can_trigger_animations_ = false;

// Set to true if a given style resolve produced an empty MatchResult.
// This is used to return a nullptr style for pseudo-element style resolves.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ TEST_F(StyleResolverTest, AnimationBaseComputedStyle) {
StyleRequest style_request;
style_request.parent_override = parent_style;
style_request.layout_parent_override = parent_style;
style_request.can_trigger_animations = false;
EXPECT_EQ(
10,
resolver.ResolveStyle(div, recalc_context, style_request)->FontSize());
Expand Down
1 change: 1 addition & 0 deletions third_party/blink/renderer/core/css/style_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class StyleRequest {
CustomScrollbar* scrollbar{nullptr};
AtomicString pseudo_argument{g_null_atom};
RulesToInclude rules_to_include{kAll};
bool can_trigger_animations{true};

explicit StyleRequest(const ComputedStyle* parent_override)
: parent_override(parent_override),
Expand Down
1 change: 1 addition & 0 deletions third_party/blink/renderer/core/dom/element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7025,6 +7025,7 @@ scoped_refptr<const ComputedStyle> Element::StyleForPseudoElement(
first_line_inherited_request.pseudo_id =
IsPseudoElement() ? To<PseudoElement>(this)->GetPseudoId()
: kPseudoIdNone;
first_line_inherited_request.can_trigger_animations = false;
StyleRecalcContext local_recalc_context(style_recalc_context);
local_recalc_context.old_style = PostStyleUpdateScope::GetOldStyle(*this);
Element* target = IsPseudoElement() ? parentElement() : this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const ComputedStyle* SVGElementRareData::OverrideComputedStyle(
style_request.parent_override = parent_style;
style_request.layout_parent_override = parent_style;
style_request.matching_behavior = kMatchAllRulesExcludingSMIL;
style_request.can_trigger_animations = false;

// The style computed here contains no CSS Animations/Transitions or SMIL
// induced rules - this is needed to compute the "base value" for the SMIL
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<!DOCTYPE html>
<title>CSS Test Reference</title>
<p>
<span style="font-weight:bold">Should be bold</span><br>
Should not be bold
</p>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>
<title>CSS Pseudo-Element Test: ::first-line style should not trigger transitions on elements</title>
<link rel="help" href="https://drafts.csswg.org/css-pseudo-4/#first-line-pseudo">
<link rel="match" href="first-line-inherited-no-transition-ref.html">
<style>
p::first-line { font-weight: bold; }
span {
transition-property: font-weight;
transition-duration: 100s;
transition-delay: -50s;
}
</style>
<p><span>Should be bold<br>Should not be bold</span></p>
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<title>CSS Test Reference</title>
<style>
span { background-color: green; }
#orange { color: orange; }
</style>
<p>
<span id="orange">Orange on green</span><br>
<span>Black on green</span>
</p>
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!DOCTYPE html>
<title>CSS Pseudo-Element Test: ::first-line style should not trigger transitions on elements</title>
<link rel="help" href="https://drafts.csswg.org/css-pseudo-4/#first-line-pseudo">
<link rel="match" href="first-line-inherited-with-transition-ref.html">
<style>
p::first-line { color: orange; }
span {
background-color: black;
transition: background-color 1000s steps(2, start);
}
span.lime {
background-color: lime;
}
</style>
<p><span id="s">Orange on green<br>Black on green</span></p>
<script>
s.offsetTop;
s.className = "lime";
</script>

0 comments on commit 352b384

Please sign in to comment.