Skip to content

Commit

Permalink
[scroll-animations] Commit start time and fix pause calculation.
Browse files Browse the repository at this point in the history
We should always use the start time from main for scroll driven
animations. This also fixes a bug in the pause calculation
which was leading to the start time effectively being ignored.

Bug: 1445543,1401368,1444324,1428836,1445137
Change-Id: I5c5dc4e12e9e9cd695a19bbc92cc64cc4c493736
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4549163
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Robert Flack <flackr@chromium.org>
Reviewed-by: Kevin Ellis <kevers@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1146658}
  • Loading branch information
Robert Flack authored and Chromium LUCI CQ committed May 19, 2023
1 parent ee91878 commit 7d59fcb
Show file tree
Hide file tree
Showing 9 changed files with 223 additions and 8 deletions.
3 changes: 1 addition & 2 deletions cc/animation/animation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ void Animation::Tick(base::TimeTicks tick_time) {
// time and then ticks it which side-steps the start time altogether. See
// crbug.com/1076012 for alternative design choices considered for future
// improvement.
keyframe_effect()->Pause(tick_time - base::TimeTicks(),
PauseCondition::kAfterStart);
keyframe_effect()->Pause(tick_time, PauseCondition::kAfterStart);
keyframe_effect()->Tick(base::TimeTicks());
} else {
DCHECK(!tick_time.is_null());
Expand Down
6 changes: 4 additions & 2 deletions cc/animation/keyframe_effect.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ void KeyframeEffect::UpdateTickingState() {
}
}

void KeyframeEffect::Pause(base::TimeDelta pause_offset,
void KeyframeEffect::Pause(base::TimeTicks timeline_time,
PauseCondition pause_condition) {
bool did_pause = false;
for (auto& keyframe_model : keyframe_models()) {
Expand All @@ -184,7 +184,9 @@ void KeyframeEffect::Pause(base::TimeDelta pause_offset,
gfx::KeyframeModel::WAITING_FOR_TARGET_AVAILABILITY ||
keyframe_model->run_state() == gfx::KeyframeModel::STARTING))
continue;
keyframe_model->Pause(pause_offset);
// Convert the timeline_time to the effective local time for each
// KeyframeModel's start time.
keyframe_model->Pause(timeline_time - keyframe_model->start_time());
did_pause = true;
}

Expand Down
2 changes: 1 addition & 1 deletion cc/animation/keyframe_effect.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class CC_ANIMATION_EXPORT KeyframeEffect : public gfx::KeyframeEffect {
void UpdateState(bool start_ready_keyframe_models, AnimationEvents* events);
void UpdateTickingState();

void Pause(base::TimeDelta pause_offset,
void Pause(base::TimeTicks timeline_time,
PauseCondition = PauseCondition::kUnconditional);

void AddKeyframeModel(
Expand Down
2 changes: 1 addition & 1 deletion cc/animation/worklet_animation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void WorkletAnimation::Tick(base::TimeTicks monotonic_time) {
// animations lifecycle. To avoid this we pause the underlying keyframe effect
// at the local time obtained from the user script - essentially turning each
// call to |WorkletAnimation::Tick| into a seek in the effect.
keyframe_effect()->Pause(local_time_.Read(*this).value());
keyframe_effect()->Pause(base::TimeTicks() + local_time_.Read(*this).value());
keyframe_effect()->Tick(base::TimeTicks());
}

Expand Down
4 changes: 3 additions & 1 deletion third_party/blink/renderer/core/animation/animation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2168,7 +2168,8 @@ void Animation::StartAnimationOnCompositor(
// the playback rate preserve current time even if the start time is set.
// Asynchronous updates have an associated pending play or pending pause
// task associated with them.
if (start_time_ && !PendingInternal()) {
if (start_time_ &&
(timeline()->IsScrollSnapshotTimeline() || !PendingInternal())) {
start_time = timeline_->ZeroTime() + start_time_.value();
if (reversed) {
start_time =
Expand Down Expand Up @@ -2297,6 +2298,7 @@ void Animation::UpdateStartTimeForViewTimeline() {
: default_offset;
AnimationTimeDelta duration = timeline_->GetDuration().value();
start_time_ = duration * relative_offset;
SetCompositorPending(true);
}

void Animation::OnRangeUpdate() {
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/web_tests/VirtualTestSuites
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@
"bases": ["fast/scroll-behavior",
"fast/scroll-snap",
"fast/scrolling",
"external/wpt/css/cssom-view"],
"external/wpt/css/cssom-view",
"external/wpt/scroll-animations"],
"exclusive_tests": ["fast/scrolling/scrollbars/mouse-scrolling-on-div-scrollbar-thumb-scaled.html"],
"args": ["--enable-threaded-compositing",
"--enable-prefer-compositing-to-lcd-text"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<link rel="help" src="https://drafts.csswg.org/scroll-animations-1/#named-timeline-range">
<link rel="stylesheet" href="support/animation-range.css">
<style>
.meter {
animation: active-interval linear 100s paused;
animation-timeline: auto;
}

.bar {
animation: slide-in linear 100s paused;
animation-timeline: auto;
}
</style>
</head>
<body>
<h3>View timeline</h3>
<template id="meters">
<div class="meters">
<div class="cover"><div class="meter"><div class="bar"></div></div><div>Cover</div></div>
<div class="contain"><div class="meter"><div class="bar"></div></div><div>Contain</div></div>
<div class="entry"><div class="meter"><div class="bar"></div></div><div>Entry</div></div>
<div class="exit"><div class="meter"><div class="bar"></div></div><div>Exit</div></div>
</div>
</template>
<div class="flex">
<div>
<div class="scroller">
<div class="subject" style="margin-top: 90px;" data-progress=".08333,-1,.5,-1"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: 70px;" data-progress=".25,.125,2,-1"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: 10px;" data-progress=".75,.875,2,-1"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: -10px;" data-progress=".91667,2,2,.5"></div>
<div class="spacer"></div>
</div>
</div>
</div>
</body>
<script>
let template = document.querySelector('#meters');
let subjects = document.querySelectorAll('.subject');
for (let i = 0; i < subjects.length; i++) {
let clone = template.content.cloneNode(true);
let meters = clone.querySelectorAll('.meter');
let progress = subjects[i].getAttribute('data-progress').split(',').map(s => parseFloat(s));
for (let meter of meters) {
let bar = meter.querySelector('.bar');
let startTime = -progress.splice(0, 1)[0] * 100;
meter.style.animationDelay = `${startTime}s`;
bar.style.animationDelay = `${startTime}s`;
}
subjects[i].appendChild(clone);
}
</script>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<!DOCTYPE html>
<html class="reftest-wait">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="help" src="https://drafts.csswg.org/scroll-animations-1/#named-timeline-range">
<link rel="match" href="animation-range-visual-test-ref.html">
<link rel="stylesheet" href="support/animation-range.css">
<script src="/common/reftest-wait.js"></script>
<script src="/web-animations/testcommon.js"></script>
</head>
<body>
<h3>View timeline</h3>
<template id="meters">
<div class="meters">
<div class="cover"><div class="meter"><div class="bar"></div></div><div>Cover</div></div>
<div class="contain"><div class="meter"><div class="bar"></div></div><div>Contain</div></div>
<div class="entry"><div class="meter"><div class="bar"></div></div><div>Entry</div></div>
<div class="exit"><div class="meter"><div class="bar"></div></div><div>Exit</div></div>
</div>
</template>
<div class="flex">
<div>
<div class="scroller">
<div class="subject" style="margin-top: 90px;"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: 70px;"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: 10px;"></div>
<div class="spacer"></div>
</div>
</div>
<div>
<div class="scroller">
<div class="subject" style="margin-top: -10px;"></div>
<div class="spacer"></div>
</div>
</div>
</div>
</body>
<script>
let template = document.querySelector('#meters');
let subjects = document.querySelectorAll('.subject');
for (let i = 0; i < subjects.length; i++) {
subjects[i].appendChild(template.content.cloneNode(true));
}
waitForCompositorReady().then(takeScreenshot);
</script>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
.flex {
display: flex;
}

.flex > div {
position: relative;
height: 160px;
margin: 0 10px;
}

.scroller {
width: 100px;
height: 100px;
overflow: auto;
border: 1px solid black;
}

.subject {
view-timeline-name: view;
width: 20px;
height: 20px;
margin: 0 auto;
background: green;
}

.meters {
position: absolute;
left: 0;
top: 110px;
height: 50px;
}

.meters > div {
display: flex;
align-items: center;
}

@keyframes active-interval {
0% { opacity: 1; }
100% { opacity: 1; }
}

.meter {
width: 50px;
position: relative;
border: 2px solid black;
height: 5px;
overflow: clip;
opacity: 0.4;
animation: active-interval linear;
animation-timeline: view;
}

@keyframes slide-in {
0% { transform: translateX(-100%)}
100% { transform: translateX(0%)}
}

.bar {
width: 100%;
height: 100%;
background: blue;
transform: translateX(-100%);
animation: slide-in linear;
animation-timeline: view;
}

.spacer {
height: 400px;
}

.contain .bar, .contain .meter {
animation-range: contain;
}

.entry .bar, .entry .meter {
animation-range: entry;
}

.exit .bar, .exit .meter {
animation-range: exit;
}

0 comments on commit 7d59fcb

Please sign in to comment.