Commit 2ca8188f authored by Kevin Ellis's avatar Kevin Ellis Committed by Commit Bot

Prevent updating the base computed style if the style is not cacheable

A regression was introduced in:

https://chromium-review.googlesource.com/c/chromium/src/+/2252642

The base computed style could get updated even when it should not be
cached.  A style that is computed in a hypothetical scenario (using
non-default parent or matching rules) should not be cached. With this
patch, the style resolver maintains a flag to block the update if the
style should not be cached.

Bug: 1100873,1109045
Change-Id: Iaca30b6b0043a8ca750624dbc2d557ef232f92ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2310969
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: default avatarAnders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792017}
parent d64c2630
...@@ -790,6 +790,9 @@ static void UpdateAnimationBaseComputedStyle(StyleResolverState& state, ...@@ -790,6 +790,9 @@ static void UpdateAnimationBaseComputedStyle(StyleResolverState& state,
if (!state.GetAnimatingElement()) if (!state.GetAnimatingElement())
return; return;
if (!state.CanCacheBaseStyle())
return;
if (forced_update) if (forced_update)
state.GetAnimatingElement()->EnsureElementAnimations(); state.GetAnimatingElement()->EnsureElementAnimations();
...@@ -831,6 +834,7 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement( ...@@ -831,6 +834,7 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement(
bool can_cache_animation_base_computed_style = bool can_cache_animation_base_computed_style =
!default_parent && !default_layout_parent && !default_parent && !default_layout_parent &&
matching_behavior == kMatchAllRules; matching_behavior == kMatchAllRules;
state.SetCanCacheBaseStyle(can_cache_animation_base_computed_style);
STACK_UNINITIALIZED StyleCascade cascade(state); STACK_UNINITIALIZED StyleCascade cascade(state);
...@@ -1105,6 +1109,7 @@ scoped_refptr<ComputedStyle> StyleResolver::PseudoStyleForElement( ...@@ -1105,6 +1109,7 @@ scoped_refptr<ComputedStyle> StyleResolver::PseudoStyleForElement(
MaybeResetCascade(cascade); MaybeResetCascade(cascade);
} }
state.SetCanCacheBaseStyle(true);
if (ApplyAnimatedStyle(state, cascade)) if (ApplyAnimatedStyle(state, cascade))
StyleAdjuster::AdjustComputedStyle(state, nullptr); StyleAdjuster::AdjustComputedStyle(state, nullptr);
......
...@@ -229,6 +229,9 @@ class CORE_EXPORT StyleResolverState { ...@@ -229,6 +229,9 @@ class CORE_EXPORT StyleResolverState {
return dependencies_.size() <= kMaxDependencies; return dependencies_.size() <= kMaxDependencies;
} }
void SetCanCacheBaseStyle(bool state) { can_cache_base_style_ = state; }
bool CanCacheBaseStyle() const { return can_cache_base_style_; }
private: private:
enum class AnimatingElementType { kElement, kPseudoElement }; enum class AnimatingElementType { kElement, kPseudoElement };
...@@ -279,6 +282,10 @@ class CORE_EXPORT StyleResolverState { ...@@ -279,6 +282,10 @@ class CORE_EXPORT StyleResolverState {
// CSSProperty::kComputedValueComparable flag set. // CSSProperty::kComputedValueComparable flag set.
bool has_incomparable_dependency_ = false; bool has_incomparable_dependency_ = false;
// True if the base style can be cached to optimize style recalculations for
// animation updates or transition retargeting.
bool can_cache_base_style_ = false;
DISALLOW_COPY_AND_ASSIGN(StyleResolverState); DISALLOW_COPY_AND_ASSIGN(StyleResolverState);
}; };
......
...@@ -325,6 +325,50 @@ TEST_F(StyleResolverTest, ...@@ -325,6 +325,50 @@ TEST_F(StyleResolverTest,
ComputedValue("font-size", *StyleForId("target"))); ComputedValue("font-size", *StyleForId("target")));
} }
TEST_F(StyleResolverTest, NonCachableStyleCheckDoesNotAffectBaseComputedStyle) {
GetDocument().documentElement()->setInnerHTML(R"HTML(
<style>
.adjust { color: rgb(0, 0, 0); }
</style>
<div>
<div style="color: rgb(0, 128, 0)">
<div id="target" style="transition: color 1s linear"></div>
</div>
</div>
)HTML");
UpdateAllLifecyclePhasesForTest();
Element* target = GetDocument().getElementById("target");
EXPECT_EQ("rgb(0, 128, 0)", ComputedValue("color", *StyleForId("target")));
// Trigger a transition on an inherited property.
target->setAttribute(html_names::kClassAttr, "adjust");
UpdateAllLifecyclePhasesForTest();
ElementAnimations* element_animations = target->GetElementAnimations();
EXPECT_TRUE(element_animations);
Animation* transition = (*element_animations->Animations().begin()).key;
EXPECT_TRUE(transition);
// Advance to the midpoint of the transition.
transition->setCurrentTime(500);
UpdateAllLifecyclePhasesForTest();
EXPECT_EQ("rgb(0, 64, 0)", ComputedValue("color", *StyleForId("target")));
EXPECT_TRUE(element_animations->BaseComputedStyle());
element_animations->ClearBaseComputedStyle();
// Perform a non-cacheable style resolution, and ensure that the base computed
// style is not updated.
GetStyleEngine().GetStyleResolver().StyleForElement(
target, nullptr, nullptr, kMatchAllRulesExcludingSMIL);
EXPECT_FALSE(element_animations->BaseComputedStyle());
// Computing the style with default args updates the base computed style.
EXPECT_EQ("rgb(0, 64, 0)", ComputedValue("color", *StyleForId("target")));
EXPECT_TRUE(element_animations->BaseComputedStyle());
}
class StyleResolverFontRelativeUnitTest class StyleResolverFontRelativeUnitTest
: public testing::WithParamInterface<const char*>, : public testing::WithParamInterface<const char*>,
public StyleResolverTest {}; public StyleResolverTest {};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment