Skip to content

GitLab

T
tangled
Commit 2fb2fc1f authored by Stephen McGruer's avatar Stephen McGruer Committed by Commit Bot
Browse files
Options

CalculateAnimationUpdate - track previous/current css-animation-playstate 

Previously this code compared the current (possibly new) value of
css-animation-playstate to the value of blink::Animation::Paused()
to determine if it should toggle the paused state. This is incorrect;
if blink::Animation::pause() is called directly from JS these values
diverge by spec[0]. We only need to toggle the pause state from
CalculateAnimationUpdate if css-animation-playstate itself has changed.

Note that this incorrect behavior did not actually manifest in release
builds because of a no-DCHECK-only optimization; if is_animation_style_change
is set, we skip the entire state-update step and just calculate interpolations.

[0]: https://drafts.csswg.org/css-animations-2/#animation-play-state

Bug: 838594
Change-Id: I23a9eb0d338171c5a125b54ec4e29b916aceb3b0
Reviewed-on: https://chromium-review.googlesource.com/1053759Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558409}
parent 980ad399
Hide whitespace changes
Inline Side-by-side
Showing with 78 additions and 18 deletions
third_party/WebKit/LayoutTests/animations/stability/pause-set-current-time.html 0 → 100644
View file @ 2fb2fc1f
<!DOCTYPE html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<style>
.animate {
animation: animate_width 10s;
}
@keyframes animate_width {
from { width: 100px; }
to { width: 200px; }
}
</style>
<div id="target"></div>
<script>
'use strict';
// Note that this test is only valid when DCHECK is enabled, as the underlying
// bug was DCHECK-only.
async_test(t => {
target.classList = 'animate';
// Grabbing the animation forces style clean, causing the Animation to be created.
let animation = target.getAnimations()[0];
// In http://crbug.com/838594 it was found that pausing a CSS animation and
// then setting the currentTime would cause it to DCHECK the next time style
// was cleaned (as long as the style clean was only from an animation update).
animation.pause();
animation.currentTime = 500;
// If we make it to the next frame without DCHECKing, we're good. Note that
// we double rAF to ensure that a new frame is produced.
window.requestAnimationFrame(function() {
window.requestAnimationFrame(t.step_func_done(() => {
// Ensure that the animation pause state was not incorrectly toggled.
assert_equals(animation.currentTime, 500);
}));
});
}, 'Pausing a CSS animation and then setting the currentTime should not ' +
'cause a DCHECK');
</script>
third_party/blink/renderer/core/animation/css/css_animation_update.h
View file @ 2fb2fc1f
...@@ -31,13 +31,15 @@ class NewCSSAnimation { ...@@ -31,13 +31,15 @@ class NewCSSAnimation {
size_t name_index, size_t name_index,
const InertEffect& effect, const InertEffect& effect,
Timing timing, Timing timing,
StyleRuleKeyframes* style_rule) StyleRuleKeyframes* style_rule,
const Vector<EAnimPlayState>& play_state_list)
: name(name), : name(name),
name_index(name_index), name_index(name_index),
effect(effect), effect(effect),
timing(timing), timing(timing),
style_rule(style_rule), style_rule(style_rule),
style_rule_version(this->style_rule->Version()) {} style_rule_version(this->style_rule->Version()),
play_state_list(play_state_list) {}
void Trace(blink::Visitor* visitor) { void Trace(blink::Visitor* visitor) {
visitor->Trace(effect); visitor->Trace(effect);
...@@ -50,6 +52,7 @@ class NewCSSAnimation { ...@@ -50,6 +52,7 @@ class NewCSSAnimation {
Timing timing; Timing timing;
Member<StyleRuleKeyframes> style_rule; Member<StyleRuleKeyframes> style_rule;
unsigned style_rule_version; unsigned style_rule_version;
Vector<EAnimPlayState> play_state_list;
}; };
class UpdatedCSSAnimation { class UpdatedCSSAnimation {
...@@ -60,13 +63,15 @@ class UpdatedCSSAnimation { ...@@ -60,13 +63,15 @@ class UpdatedCSSAnimation {
Animation* animation, Animation* animation,
const InertEffect& effect, const InertEffect& effect,
Timing specified_timing, Timing specified_timing,
StyleRuleKeyframes* style_rule) StyleRuleKeyframes* style_rule,
const Vector<EAnimPlayState>& play_state_list)
: index(index), : index(index),
animation(animation), animation(animation),
effect(&effect), effect(&effect),
specified_timing(specified_timing), specified_timing(specified_timing),
style_rule(style_rule), style_rule(style_rule),
style_rule_version(this->style_rule->Version()) {} style_rule_version(this->style_rule->Version()),
play_state_list(play_state_list) {}
void Trace(blink::Visitor* visitor) { void Trace(blink::Visitor* visitor) {
visitor->Trace(animation); visitor->Trace(animation);
...@@ -80,6 +85,7 @@ class UpdatedCSSAnimation { ...@@ -80,6 +85,7 @@ class UpdatedCSSAnimation {
Timing specified_timing; Timing specified_timing;
Member<StyleRuleKeyframes> style_rule; Member<StyleRuleKeyframes> style_rule;
unsigned style_rule_version; unsigned style_rule_version;
Vector<EAnimPlayState> play_state_list;
}; };
} // namespace blink } // namespace blink
...@@ -106,9 +112,11 @@ class CSSAnimationUpdate final { ...@@ -106,9 +112,11 @@ class CSSAnimationUpdate final {
size_t name_index, size_t name_index,
const InertEffect& effect, const InertEffect& effect,
const Timing& timing, const Timing& timing,
StyleRuleKeyframes* style_rule) { StyleRuleKeyframes* style_rule,
const Vector<EAnimPlayState>& play_state_list) {
new_animations_.push_back(NewCSSAnimation(animation_name, name_index, new_animations_.push_back(NewCSSAnimation(animation_name, name_index,
effect, timing, style_rule)); effect, timing, style_rule,
play_state_list));
} }
void CancelAnimation(size_t index, const Animation& animation) { void CancelAnimation(size_t index, const Animation& animation) {
cancelled_animation_indices_.push_back(index); cancelled_animation_indices_.push_back(index);
...@@ -121,9 +129,11 @@ class CSSAnimationUpdate final { ...@@ -121,9 +129,11 @@ class CSSAnimationUpdate final {
Animation* animation, Animation* animation,
const InertEffect& effect, const InertEffect& effect,
const Timing& specified_timing, const Timing& specified_timing,
StyleRuleKeyframes* style_rule) { StyleRuleKeyframes* style_rule,
animations_with_updates_.push_back(UpdatedCSSAnimation( const Vector<EAnimPlayState>& play_state_list) {
index, animation, effect, specified_timing, style_rule)); animations_with_updates_.push_back(
UpdatedCSSAnimation(index, animation, effect, specified_timing,
style_rule, play_state_list));
suppressed_animations_.insert(animation); suppressed_animations_.insert(animation);
} }
void UpdateCompositorKeyframes(Animation* animation) { void UpdateCompositorKeyframes(Animation* animation) {
......
third_party/blink/renderer/core/animation/css/css_animations.cc
View file @ 2fb2fc1f
...@@ -374,10 +374,15 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update, ...@@ -374,10 +374,15 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update,
Animation* animation = existing_animation->animation.Get(); Animation* animation = existing_animation->animation.Get();
const bool was_paused =
CSSTimingData::GetRepeated(existing_animation->play_state_list,
i) == EAnimPlayState::kPaused;
if (keyframes_rule != existing_animation->style_rule || if (keyframes_rule != existing_animation->style_rule ||
keyframes_rule->Version() != keyframes_rule->Version() !=
existing_animation->style_rule_version || existing_animation->style_rule_version ||
existing_animation->specified_timing != specified_timing) { existing_animation->specified_timing != specified_timing ||
is_paused != was_paused) {
DCHECK(!is_animation_style_change); DCHECK(!is_animation_style_change);
update.UpdateAnimation( update.UpdateAnimation(
existing_animation_index, animation, existing_animation_index, animation,
...@@ -386,12 +391,10 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update, ...@@ -386,12 +391,10 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update,
element, &style, parent_style, name, element, &style, parent_style, name,
keyframe_timing_function.get(), i), keyframe_timing_function.get(), i),
timing, is_paused, animation->UnlimitedCurrentTimeInternal()), timing, is_paused, animation->UnlimitedCurrentTimeInternal()),
specified_timing, keyframes_rule); specified_timing, keyframes_rule,
} animation_data->PlayStateList());
if (is_paused != was_paused)
if (is_paused != animation->Paused()) { update.ToggleAnimationIndexPaused(existing_animation_index);
DCHECK(!is_animation_style_change);
update.ToggleAnimationIndexPaused(existing_animation_index);
} }
} else { } else {
DCHECK(!is_animation_style_change); DCHECK(!is_animation_style_change);
...@@ -402,7 +405,7 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update, ...@@ -402,7 +405,7 @@ void CSSAnimations::CalculateAnimationUpdate(CSSAnimationUpdate& update,
&style, parent_style, name, &style, parent_style, name,
keyframe_timing_function.get(), i), keyframe_timing_function.get(), i),
timing, is_paused, 0), timing, is_paused, 0),
specified_timing, keyframes_rule); specified_timing, keyframes_rule, animation_data->PlayStateList());
} }
} }
} }
......
third_party/blink/renderer/core/animation/css/css_animations.h
View file @ 2fb2fc1f
...@@ -114,12 +114,14 @@ class CSSAnimations final { ...@@ -114,12 +114,14 @@ class CSSAnimations final {
name_index(new_animation.name_index), name_index(new_animation.name_index),
specified_timing(new_animation.timing), specified_timing(new_animation.timing),
style_rule(new_animation.style_rule), style_rule(new_animation.style_rule),
style_rule_version(new_animation.style_rule_version) {} style_rule_version(new_animation.style_rule_version),
play_state_list(new_animation.play_state_list) {}
void Update(UpdatedCSSAnimation update) { void Update(UpdatedCSSAnimation update) {
DCHECK_EQ(update.animation, animation); DCHECK_EQ(update.animation, animation);
style_rule = update.style_rule; style_rule = update.style_rule;
style_rule_version = update.style_rule_version; style_rule_version = update.style_rule_version;
play_state_list = update.play_state_list;
specified_timing = update.specified_timing; specified_timing = update.specified_timing;
} }
...@@ -134,6 +136,7 @@ class CSSAnimations final { ...@@ -134,6 +136,7 @@ class CSSAnimations final {
Timing specified_timing; Timing specified_timing;
Member<StyleRuleKeyframes> style_rule; Member<StyleRuleKeyframes> style_rule;
unsigned style_rule_version; unsigned style_rule_version;
Vector<EAnimPlayState> play_state_list;
}; };
struct RunningTransition { struct RunningTransition {
......
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