2039745 - Trigger scroll timeline animations in a spec compliant manner

Status: ASSIGNED

(Core :: DOM: Animation, task)

Description

[Hiroyuki Ikezoe (:hiro)]

As of now we trigger pending scroll timeline animations at the very last of DoFlushPendingNotifications. It's not compliant with the spec.

It's the root cause of the failure of scroll-timeline-inactive.html.

This bug aims to eliminate ScrollTimelineAnimationTracker, and it depends on bug 2006258.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2039745 - Trigger pending scroll-driven animations from Animation::Tick directly. r?#layout-scroll-driven-animation-reviewers

The pending branch of Animation::Tick is monotonic-only: when
mPendingReadyTime is null it falls through to
mPendingReadyTime = mTimeline->GetCurrentTimeAsTimeStamp();
which on a ScrollTimeline always yields a null TimeStamp, because
ScrollTimeline overrides ToTimeStamp to return {}. As a result a pending
scroll-driven animation cannot transition out of the pending state via the
rendering-update step on its own. Today the synchronous
ScrollTimelineAnimationTracker masks this by calling TryTriggerNow() directly
after layout flushes; this commit makes Animation::Tick itself handle the
trigger so that the tracker can be removed in the next commit without
regressing scroll-driven animations.

The mPendingReadyTime declaration comment already documents the rule that the
field is wall-clock-only and that finite timelines should use the timeline's
current time directly in TryTriggerNow() (see bug 2017448). Apply that rule
in Animation::Tick's pending branch: for finite timelines, call
TryTriggerNow() directly rather than writing a structurally-meaningless
TimeStamp into mPendingReadyTime. Reuse the existing HasFiniteTimeline()
helper here and in TryTriggerNow() so both sites express the same predicate.

This matches WebKit's WebAnimation::maybeMarkAsReady, which gates the
pending-start-time requirement on m_timeline->isMonotonic() [1].

[1] https://searchfox.org/wubkat/source/Source/WebCore/animation/WebAnimation.cpp#1583

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2039745 - Drop ScrollTimelineAnimationTracker. r?#layout-scroll-driven-animation-reviewers

ScrollTimelineAnimationTracker existed solely to drain pending scroll-driven
animations from a synchronous trigger at the end of
PresShell::DoFlushPendingNotifications. Per
https://drafts.csswg.org/scroll-animations-1/#event-loop that synchronous
trigger violates the spec: "This section has no effect on forced style and
layout calculations triggered by getComputedStyle() or similar."

Now that the prior commit makes Animation::Tick reach TryTriggerNow() for
finite timelines, the refresh-tick path
(AnimationTimelinesController::WillRefresh -> ScrollTimeline::WillRefresh ->
Animation::Tick -> TryTriggerNow) is the sole, spec-aligned trigger point.
ScrollTimelineAnimationTracker, its callers in Animation::Play / Pause /
SetTimeline / CancelPendingTasks, the Document accessors and member, and the
PresShell helper are all removed.

The first subtest of scroll-animations/css/scroll-timeline-inactive.html
("Animation does not apply when timeline becomes inactive") now passes; its
expected:FAIL metadata entry is removed.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

With these patch (and with bug 2006258), content-visibility-animation-with-scroll-timeline-in-auto-subtree.html is timed out (see this try run: https://treeherder.mozilla.org/jobs?repo=try&revision=a38fd36ba58cdb9f4f3f1b5b39a021373050d30e&selectedTaskRun=LvmSL6cgRb2p-cmee34z0w.0).

The test is originally marked as FAIL, so it may not be a big problem?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

With these patch (and with bug 2006258), content-visibility-animation-with-scroll-timeline-in-auto-subtree.html is timed out (see this try run: https://treeherder.mozilla.org/jobs?repo=try&revision=a38fd36ba58cdb9f4f3f1b5b39a021373050d30e&selectedTaskRun=LvmSL6cgRb2p-cmee34z0w.0).

The test is originally marked as FAIL, so it may not be a big problem?

Thanks to Boris, he has filed bug 2005862 for the test failure. So I am just going to change the annotation to FAIL with the bug number,

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I missed there was another test failure. It told that D300667 regressed bug 2024744.

It looks me that the fix for bug 2024744 subtle. We should rather removing the animation from ScrollTimeline in Animation::SetTimelineNoupdate?

Specifically here where we call AnimationTimeline::RemoveAnimation. Shouldn't we make RemoveAnimation virtual and ScrollTimeline needs to override it?

I will look it into detail tomorrow.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2039745 - Clear mAutoAlignStartTime when transitioning away from a finite timeline. r?#layout-scroll-driven-animation-reviewers

mAutoAlignStartTime is only meaningful for finite timelines: it is set in
the toFiniteTimeline branch of Animation::SetTimelineNoUpdate, and the
AutoAlignStartTime procedure asserts that the current timeline is not
monotonically increasing.

The other branch in SetTimelineNoUpdate only runs SetCurrentTimeNoUpdate
(which clears mAutoAlignStartTime as a side effect via SilentlySetCurrentTime)
when previous progress is resolved. For a pause-pending scroll-driven
animation whose start time was never auto-aligned -- mStartTime null,
mHoldTime null, so previousCurrentTime and previousProgress are both null --
that branch is skipped and the animation lands on the new (potentially
monotonic) timeline still carrying mAutoAlignStartTime = true.

Clear the flag unconditionally in the fromFiniteTimeline branch so the
invariant holds regardless of previousProgress, and keep the existing
SetCurrentTimeNoUpdate call nested inside the previous-progress check.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

I missed there was another test failure. It told that D300667 regressed bug 2024744.

It looks me that the fix for bug 2024744 subtle. We should rather removing the animation from ScrollTimeline in Animation::SetTimelineNoupdate?

Specifically here where we call AnimationTimeline::RemoveAnimation. Shouldn't we make RemoveAnimation virtual and ScrollTimeline needs to override it?

I will look it into detail tomorrow.

I've uploaded D300738 to fix the regression.

Comment 8

[Phabricator Automation]

Comment on attachment 9586663 [details]
Bug 2039745 - Clear mAutoAlignStartTime when transitioning away from a finite timeline. r?#layout-scroll-driven-animation-reviewers

Revision D300738 was moved to bug 2031862. Setting attachment 9586663 [details] to obsolete.