1923208 - Fire transitioncancel event first rather than transitionrun when replacing old transitions

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, defect, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

This bug is the root cause of bug 1916176 (I believe). Setting the same priority and severity as bug 1916176.

Attaching file is exactly same as the one in bug 1916176 comment 19.

Comment 1

[Boris Chiou [:boris]]

A quick look at the CSSTransition::QueueEvents(). It seems we set the timestamp for the cancel event too late. We call QueueEvents() when ticking the transition, so it is much later than the timing we create this transition.

We probably need to store a timestamp for transitioncancel when canceling the replaced transition to make sure we order the events properly.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Fallback to compare the order of event targets for CSSTransition if they have the same time.

Basically, the cancel events are almost always queued synchronously in some
deterministic manner in CSSTransition::QueueEvents(), so we should also
expect to dispatch them in the same order.

In AnimationEventDispatcher, we use nsTArray::StableSort(), which keeps the
relative order if both elements are equal, so we should let the comparator,
i.e. Animation::HasLowerCompositeOrderThan(), return false if two
transition events have the same scheduled time and one of them is a
cancel event.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Brian do you happen to know off-hand where this is defined in the spec? (No worries if not)

Comment 4

[Boris Chiou [:boris]]

Attachment: testcase for CSS Animations

Comment 5

[Boris Chiou [:boris]]

Attachment: testcase for multiple CSS Transitions

Per Hiro's comment in the patch, we should check this testcase as well.

I saw something like this in Chrome/Safari:

transitionrun(opacity) on 770.9
transitionrun(transform) on 770.9
transitioncancel(transform) on 1270.8
transitioncancel(opacity) on 1270.8
transitionrun(transform) on 1270.8
transitionrun(opacity) on 1270.8

but in Gecko:

transitionrun(opacity) on 756.56
transitionrun(transform) on 756.56
transitionrun(transform) on 1289.8
transitioncancel(transform) on 1289.8
transitionrun(opacity) on 1298.14
transitioncancel(opacity) on 1298.14

In this case, we did cancel transform transition and create a new one to replace it first, but Chrome/Safari fired the transitioncancel of transform and opacity together. This is probably implementation-dependent, and maybe we just need to make sure:
transitioncancel({transform|opacity}) is earlier than transitionrun({transform|opacity}), correspondingly.

Comment 6

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Brian do you happen to know off-hand where this is defined in the spec? (No worries if not)

The events that get queued are defined in CSS Transitions Level 2 here:

https://drafts.csswg.org/css-transitions-2/#transition-events

The sorting of those events is defined in Web Animations Level 1 here:

https://drafts.csswg.org/web-animations-1/#animation-frame-loop

It doesn't strictly define this case but I think Boris' suggestion in comment 1 matches the intent of the spec. Specifically this part:

Each Document maintains a pending animation event queue that stores animation events along with their corresponding event targets and scheduled event time. The scheduled event time is a time value relative to the time origin representing when the event would ideally have been dispatched were animations updated at an infinitely high frequency.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Wait so why are we even looking at composite order to sort events? Seems we should not sort them, at least events for the same element and same scheduled time?

But the spec seems to contradict that:

Perform a stable sort of the animation events in events to dispatch as follows:

1. Sort the events by their scheduled event time such that events that were scheduled to occur earlier sort before events scheduled to occur later, and events whose scheduled event time is unresolved sort before events with a resolved scheduled event time.
2. Within events with equal scheduled event times, sort by their composite order.

Note: The purpose of sorting events is to ensure that, as best possible, even on devices with differing capabilities and hence different frame rates, events are dispatched in a consistent order.

Note: The requirement for the sort to be a stable sort is because sometimes multiple events can be queued with the same scheduled event time. For example, a CSS animation with a duration of zero would dispatch both an animationstart and an animationend event with the same scheuled event time, and the order of these events should be preserved.

So what am I missing? We're following the spec to the letter.

WebKit seems to do something a lot more subtle than our AnimationPtrComparator here. Haven't dug into chromium yet.

In particular it seems like they just sort by tree position of the event target here

Comment 8

[Brian Birtles (:birtles)]

I thought our problem was the scheduled event time we set for the cancel event different from the run event?

If the scheduled event times are the same then the algorithm is to sort by composite order. That's going to mean we sort as follows:

1. If A and B’s associated animations differ by class, sort by any inter-class composite order as defined for the corresponding classes.
2. If A and B are still not sorted, sort by any class-specific composite order defined by the common class of A and B’s associated animations.
3. If A and B are still not sorted, sort by the position of their associated animations in the global animation list.

Since the classes are the same ("transition"), we'll sort by the transition-specific composite order:

As I understand it, unfortunately that means we'll hit step 2 there:

2. Otherwise, if only one of A or B has an owning element, let the animation with an owning element sort first.

Since the cancelled transition has no owning element, it will sort last but intuitively, it happened first so it should sort first.

I think we're doing what the spec says but I think the spec doesn't do the intuitive thing.

I think the spec should, like Webkit, sort by the event target. The way we cancel transitions ensures that we have an owning element (and hence event target) just long enough to queue the event but not long enough for it to be there when we later sort events. The event target is still there though.

Comment 9

[Brian Birtles (:birtles)]

In spec terms, it's a bit hard to fix this, since we kind of want to use a different composite order behavior when sorting events in this case.

I think we could introduce a term like "context element" and let that be the owning element when compositing animations generally, but for the specific case of sorting transitions events, let it be the event target.

Comment 10

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #9)

In spec terms, it's a bit hard to fix this, since we kind of want to use a different composite order behavior when sorting events in this case.

I think we could introduce a term like "context element" and let that be the owning element when compositing animations generally, but for the specific case of sorting transitions events, let it be the event target.

Thanks for the suggestion. I'm trying to use the event target to sort (i.e. AnimationEventInfo::CssAnimationOrTransitionData::mOwningAnimationTarget).

BTW, we have to updating this for CSS Animations as well because we may set the same schedule time for animationstart and animationcancel when replacing the CSS Animation, and the cancelled animation doesn't have owning element, either.

So I'd like to handle both cases in AnimationEventInfo::operator<() before we go into Animation::HasLowerCompositeOrderThan().

Comment 11

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #10)

So I'd like to handle both cases in AnimationEventInfo::operator<() before we go into Animation::HasLowerCompositeOrderThan().

I'm not sure but think you might still need to go into HasLowerCompositeOrderThan so that we do the correct inter-class sorting (e.g. sorting transitions before animations) prior to sorting by event target.

Perhaps HasLowerCompositeOrderThan should take a pointer to a context element? Then make asCSSTransitionForSorting check for transition && (transition->IsTiedToMarkup() || aContextElement)? (And similarly update CSSTransition::HasLowerCompositeOrderThan.)

Comment 12

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #11)

Perhaps HasLowerCompositeOrderThan should take a pointer to a context element? Then make asCSSTransitionForSorting check for transition && (transition->IsTiedToMarkup() || aContextElement)? (And similarly update CSSTransition::HasLowerCompositeOrderThan.)

Thanks. This sounds more reasonable. I was thinking that we could add some special handling in AnimationEventInfo::operator<() if both are CSSTransitions or CSSAnimations, but this makes the readability worse. Passing the event target (i.e. dom::Element*) into HasLowerCompositeOrderThan() would be much better.

Comment 13

[Boris Chiou [:boris]]

BTW, I noticed there is one thing we probably missed. In the spec, if the owning element (or the context element) are the same, we have to use the transition generation to sort the composite order.

However, we change this value when cancelling a transition. That means the cancelled transition is still sorted later in the event queue when replacing this transition.

It seems the spec doesn't mention a lot about how to handle the transition generation when we cancelling this transition, and it seems CSS Animations doesn't use the "animation generation" to sort the events (or perhaps it affects its global animation list).

So either we shouldn't change the transition generation when cancelling the transition, or we should skip this check if we pass a context element. Or maybe we just cache the old transition generation (or animation index) when cancelling a transition, for sorting the events.

Comment 14

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #13)

BTW, I noticed there is one thing we probably missed. In the spec, if the owning element (or the context element) are the same, we have to use the transition generation to sort the composite order.

However, we change this value when cancelling a transition. That means the cancelled transition is still sorted later in the event queue when replacing this transition.

Oh, good point. I hadn't thought about that. That makes me wonder if we're approaching this the wrong way. Maybe we simply need to sort cancel events before other events with the same time?

Comment 15

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #14)

(In reply to Boris Chiou [:boris] from comment #13)

BTW, I noticed there is one thing we probably missed. In the spec, if the owning element (or the context element) are the same, we have to use the transition generation to sort the composite order.

However, we change this value when cancelling a transition. That means the cancelled transition is still sorted later in the event queue when replacing this transition.

Oh, good point. I hadn't thought about that. That makes me wonder if we're approaching this the wrong way. Maybe we simply need to sort cancel events before other events with the same time?

It seems we always push the cancel event first, so we probably just follow the implementation of WebKit:

1. if their event targets are the same => return false (so keep the relative order)
2. if not, we compare the order of event targets

And perhaps we put this special case only when comparing two events with the same time (in AnimationEventInfo::operator<(), instead of Animation::HasLowerCompositeOrderThan()) because this is only for two CSSTransition events or two CSSAnimation events.

Comment 16

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Fallback to compare the order of event targets for CSSAnimation if they have the same time.

Same for the CSSAnimation. We have to keep the relative order if their
event targets are the same.

Comment 17

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Set a proper schedule time for transitionrun.

See the testcase. If we replace a transition and flush it in the
meantime, after a timeout, we may fail to set a proper mPendingReadyTime
for the newly-created transition, because the refresh driver is not in
refresh when we call Animation::EnsurePaintIsScheduled() in
Animation::PlayNoUpdate(). So we don't have a pending ready time and
so we cannot trigger this transition at the current tick.

Therefore, this newly-created transition would be triggered at the next tick.
The means it doesn't have mStartTime when we enqueued the transition event
i.e. transitionrun, at the current tick. zeroTimeStamp would be null,
and we fail to sort the events.

In this patch, we just assign a proper schedule time for transitionrun
if zeroTimeStamp is null.

Comment 18

[Brian Birtles (:birtles)]

Just following up on my comments on D225117, I think we shouldn't use a separate path for cancel events because it doesn't completely define the ordering in some cases.

For example, suppose we have multiple transitions fire on an element, e.g. transition-property: width, height. Now suppose that both transitions are cancelled and replaced with new transitions. We should get four events: 2 x transitioncancel, 2 x transitionrun.

Since these transitions have the same event target, simply sorting by event target is not going to provide a fully-specified sort order meaning we'll potentially end up with implementation-specific ordering, either cancel, cancel, run, run or cancel, run, cancel, run.

As a result, I'm suggesting we spec something like the following for transition and CSS animation events:

When sorting cancel events, use the owning element and transition generation from when the transition was cancelled.

(And something similar for CSS animation cancel events but somehow storing the position in the animation-name property? I'm not sure, actually. That will need a bit more thought.)

From an implementation point of view, we are already storing the owning element in the event data as the event target. So, in addition to that we'd need to store the transition generation. Then when we do the composite order sort, we'd need to be able to pass in the owning element and generation to use when sorting.

Comment 19

[Boris Chiou [:boris]]

I filed https://github.com/w3c/csswg-drafts/issues/11064. Hope this makes us easier to update the spec.

Comment 20

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Store animation index into AnimationEventDispatcher.

We need this information when comparing the cancelled transitions/animations
in the following patches.

Note that we have to reorder the calling of Animation::Cancel() and
the update of mAnimationIndex to make sure we enqueue the cancel event
with the correct mAnimationIndex.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f94eadf58850
Store animation index into AnimationEventDispatcher. r=layout-reviewers,birtles,jwatt,emilio
https://hg.mozilla.org/integration/autoland/rev/ff474bf7a64d
Fallback to compare the order of event targets for CSSTransition if they have the same time. r=layout-reviewers,firefox-animation-reviewers,emilio,birtles
https://hg.mozilla.org/integration/autoland/rev/b09adff52148
Fallback to compare the order of event targets for CSSAnimation if they have the same time. r=jwatt,birtles
https://hg.mozilla.org/integration/autoland/rev/b396c8b46c14
Set a proper schedule time for transitionrun. r=layout-reviewers,emilio

Comment 22

[Norisz Fay [:noriszfay]]

Backed out for causing failures on event-dispatch.tentative.html

Backout link

Push with failures

Failure log // Failure log 2

Comment 23

[Boris Chiou [:boris]]

Weird. My previous try on Android looks good. I will send to try again to check what happened. It's unlikely to be reproduced locally.

Comment 24

[Boris Chiou [:boris]]

It's unfortunate I cannot reproduce this on try today. I will push to try again on the tip of tomorrow to see which patch causes this.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Boris, it also failed on desktops, this failure for example, so I think you can try it locally.

Comment 26

[Boris Chiou [:boris]]

Good catch! I will try again locally again. :)

Comment 27

[Boris Chiou [:boris]]

I still cannot reproduce these failures in try today (https://treeherder.mozilla.org/jobs?repo=try&revision=b34e769b9089f0bac0b2a2aeb8c0d851ebdd4a38) and my local build.

I'd like to try to push part of these patches to find the patch which causes these errors.

Comment 28

[Boris Chiou [:boris]]

I suspect this may be a kind of memory layout issue in AnimationEventDispatcher because I added a new field in Variant<CssAnimationData, CssTransitionData, WebAnimationData>. And I probably saw some issues that we mismatch the data type before, e.g. A CSSTransition object but it uses WebAnimationData.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/498d9410b286
Store animation index into AnimationEventDispatcher. r=layout-reviewers,birtles,jwatt,emilio

Comment 30

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/498d9410b286

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7ab9fb6f6a9
Fallback to compare the order of event targets for CSSTransition if they have the same time. r=layout-reviewers,firefox-animation-reviewers,emilio,birtles

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/49185 for changes under testing/web-platform/tests

Comment 33

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f7ab9fb6f6a9

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73c19d1a0269
Set a proper schedule time for transitionrun. r=layout-reviewers,emilio

Comment 35

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/73c19d1a0269

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 37

[Daniel Holbert [:dholbert]]

Hi Boris - this ran across my radar since it's one of our S2 bugs.

Just summing up the status here (as it seems to me at least):

• the patch-stack landed (comment 21) but was backed out for test failures (comment 22)
• Test failures were difficult to repro on-demand, so we re-landed the patches one by one.
• We ended up with 3 of the 4 originally-landed patches as having stuck, and it looks like we haven't attempted to re-land the 4th one yet, specifically https://phabricator.services.mozilla.com/D226212 ("Fallback to compare the order of event targets for CSSAnimation"), so this is still open (and has leave-open keyword)

I'm assuming there's still work to be done to get that 4th patch landable-without-breaking-tests, but wanted to check in since some time has passed, just in case the test issues have already been addressed & to be sure this doesn't slip off the radar....

Is that summary accurate & we've still got a bit more to do here to get that final patch landable (and/or to better-understand the test-failures)?

Thanks!

Comment 38

[Daniel Holbert [:dholbert]]

Also: emilio noticed in a quick test that the patches-that-have-already-landed here may have fixed the originally-affected-site here.

Given that, and to keep things simpler for tracking purposes (now that we've just crossed a version-number threshold), maybe it'd be best to close out this bug as fixed-in-134, and spin off a followup to get the remaining patch landed in the 135 nightly cycle?

(You can migrate the existing phab revision to the hypothetical new bug, just by editing the "Bugzilla Bug ID" field in the edit-revision field at https://phabricator.services.mozilla.com/differential/revision/edit/226212/ )

Comment 39

[Boris Chiou [:boris]]

Yes. Thanks for checking this bug. I landed the first 3 patches because they block the S2 bug (and that's the reason we set this bug S2).

I will do things:

1. File a separate bug which hold my animation event patch. In that bug, I think it is OK to give it S3 because it is just a spec update. Therefore, we can close this one.
2. I probably will use try server to check my patch again to see any failures. If I still cannot reproduce it, I probably will reland it to see if there are still any failures. This takes much more time, so using a different bug is much better.

Comment 40

[Phabricator Automation]

Comment on attachment 9432006 [details]
Bug 1923208 - Fallback to compare the order of event targets for CSSAnimation if they have the same time.

Revision D226212 was moved to bug 1933281. Setting attachment 9432006 [details] to obsolete.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for ESR128 uplift? It grafts cleanly.

Comment 43

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Store animation index into AnimationEventDispatcher.

We need this information when comparing the cancelled transitions/animations
in the following patches.

Note that we have to reorder the calling of Animation::Cancel() and
the update of mAnimationIndex to make sure we enqueue the cancel event
with the correct mAnimationIndex.

Original Revision: https://phabricator.services.mozilla.com/D227110

Comment 44

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Fallback to compare the order of event targets for CSSTransition if they have the same time.

Since the cancelled transition has no owning element and its transition
generation is out-of-date, so we have to compare the event targets and
the transition generations in AnimationEventInfo. We still follow the
composite order defined in the spec but use the event target and transition
generation before cancelling this transition, to get the correct event order
if we have the same schedule time.

Original Revision: https://phabricator.services.mozilla.com/D225117

Comment 45

[Boris Chiou [:boris]]

Attachment: Bug 1923208 - Set a proper schedule time for transitionrun.

See the testcase. If we replace a transition and flush it in the
meantime, after a timeout, we may fail to set a proper mPendingReadyTime
for the newly-created transition, because the refresh driver is not in
refresh when we call Animation::EnsurePaintIsScheduled() in
Animation::PlayNoUpdate(). So we don't have a pending ready time and
so we cannot trigger this transition at the current tick.

Therefore, this newly-created transition would be triggered at the next tick.
The means it doesn't have mStartTime when we enqueued the transition event
i.e. transitionrun, at the current tick. zeroTimeStamp would be null,
and we fail to sort the events.

In this patch, we just assign a proper schedule time for transitionrun
if zeroTimeStamp is null.

Original Revision: https://phabricator.services.mozilla.com/D226213

Comment 46

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: The users may see unexpected behaviors on some website which uses CSS transitions
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This only changed the sorting way for CSS transition events. The only impact is the performance. However, it is low per our tests in Nightly.
• String changes made/needed: No
• Is Android affected?: yes

Comment 47

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: The users may see unexpected behaviors on some website which uses CSS transitions
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This only changed the sorting way for CSS transition events. The only impact is the performance. However, it is low per our tests in Nightly.
• String changes made/needed: No
• Is Android affected?: yes

Comment 48

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: The users may see unexpected behaviors on some website which uses CSS transitions
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Very Low
• Explanation of risk level: This just added one extra parameter in some functions.
• String changes made/needed: No
• Is Android affected?: yes

Comment 49

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: The users may see unexpected behaviors on some website which uses CSS transitions
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Very Low
• Explanation of risk level: This only assigned a proper timestamp for the events in some cases.
• String changes made/needed: No
• Is Android affected?: yes

Comment 50

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/c0001ffdc690
https://hg.mozilla.org/releases/mozilla-esr128/rev/93f557e808d3
https://hg.mozilla.org/releases/mozilla-esr128/rev/238c790933e4