transitioncancel event not fired when removing element from the document

RESOLVED FIXED in Firefox 68

Status

()

defect
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Regression)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

3 months ago
Posted file Test case

See the attached test case. We should get transitionrun followed by transitioncancel but instead we only get transitionrun. In Chrome we get both.

As per the spec:

If an element is no longer in the document, implementations must cancel any running transitions on it and remove transitions on it from the completed transitions.

Assignee

Updated

3 months ago
Blocks: 1264125
Assignee

Comment 1

2 months ago

In this test case we go to queue the cancel event but we fail to do so because the pres context for the owning element is not available.

mozregression indicates this was regressed by bug 1396041.

Regressed by: 1396041
Assignee

Comment 2

2 months ago

Digging in a little further, the patch from bug 1396041: has

+  ClearInDocument();
+
   // Ensure that CSS transitions don't continue on an element at a
   // different place in the tree (even if reinserted before next
   // animation refresh).
   // We need to delete the properties while we're still in document
   // (if we were in document).
   // FIXME (Bug 522599): Need a test for this.
   if (MayHaveAnimations()) {
     DeleteProperty(nsGkAtoms::transitionsOfBeforeProperty);
     DeleteProperty(nsGkAtoms::transitionsOfAfterProperty);
     DeleteProperty(nsGkAtoms::transitionsProperty);
     DeleteProperty(nsGkAtoms::animationsOfBeforeProperty);
     DeleteProperty(nsGkAtoms::animationsOfAfterProperty);
     DeleteProperty(nsGkAtoms::animationsProperty);
   }
 
-  ClearInDocument();

Notice the comment: "We need to delete the properties while we're still in document"!

The reason for that comment is bug 1223445 comment 22:

So there is yet another leak. We never remove Animations in background tabs 
from PendingAnimationTracker (unless the page is closed or becomes foreground).
The reason is that when we try to remove animation from tracker,
GetComposedDoc already returns null. So Element::UnbindFromTree needs to remove properties earlier.

We have Animation::CancelFromStyleAnimation::CancelNoUpdateAnimation::ResetPendingTasksAnimation::CancelPendingTasks which has:

  if (Document* doc = GetRenderedDocument()) {
    PendingAnimationTracker* tracker = doc->GetPendingAnimationTracker();
    if (tracker) {
      if (mPendingState == PendingState::PlayPending) {
        tracker->RemovePlayPending(*this);
      } else {
        tracker->RemovePausePending(*this);
      }
    }
  }

So if we're not in document we'll fail to get the tracker and fail to remove ourselves.

So it looks like bug 1396041 regressed this too.

Perhaps we need to revert that change and find some other way to avoid posting an animation restyle when dropping an element.

(It's a little bit odd that CancelNoUpdate is posting an update too, I suppose.)

Assignee

Comment 3

2 months ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee

Comment 4

2 months ago

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

Let's see what breaks...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2768d2e1e0a45862ab8c9b86c7094853589dc62c

Quite a bit it turns out.

It looks like Animation::CancelFromStyle sometimes does need to post a restyle.

I believe it didn't pre-Stylo, but with our Style setup we need it to post a restyle so that we run the subsequent traversal.

For example, in the first failing test dom/animation/test/mozilla/test_cascade.html we have:

  div.style.animation = "none";
  assert_equals(cs.marginLeft, '80px',
                'margin-left animation stops overriding transition at 0s');

When we flush style we will cancel the animation (by calling CancelFromStyle) but if we don't post a restyle at that point we'll fail to run the second traversal and update the animated style for margin-left.

The failures in test_animations.html, test_animations_dynamic_changes.html, test_animations_omta.html, etc. are similar.

The failure in test_restyles.html is odd. That test probably shouldn't depend on CancelFromStyle triggering a restyle. I don't know why it does.

Nearly all these failures apply to CSS animations only but there is one related to transitions: WPT css/css-transitions/CSSTransition-canceling.tentative.html.

So it seems like what we really want to do is just treat the case when we are deleting the element as special and ensure we don't trigger restyles then.

Currently the API for canceling we have is:

  • CSSAnimation/CSSTransition::CancelFromStyle
    • Disassociates from the owning element, updates animation index etc.
    • Calls Animation::CancelFromStyle
      • Calls Animation::CancelNoUpdate (which actually does trigger a restyle!)
  • Animation::Cancel
    • Calls Animation::CancelNoUpdate
    • Calls Animation::PostUpdate which triggers a layer restyle

It seems like the API we might want is:

  • CSSAnimation/CSSTransition::CancelFromStyle(PostRestyle aPostRestyle = aPostRestyle::IfNeeded)
    • Disassociates from the owning element, updates animation index etc.
    • Calls Animation::Cancel(aPostRestyle)
  • Animation::Cancel(PostRestyle aPostRestyle = PostRestyle::IfNeeded)
    • If aPostRestyle is IfNeeded, calls Animation::PostUpdate which triggers a layer restyle (assuming we're not already idle)
    • Passes aPostRestyle on to KeyframeEffect::NotifyAnimationTiming so that it too can avoid posting a restyle

i.e. drop CancelNoUpdate since it does post restyles.

Then from AnimationCollection<AnimationType>::PropertyDtor we'd pass PostRestyle::No. Everywhere else would use the default value IfNeeded. (I was thinking of using a default value simply so that we can use this method as-is for the WebIDL binding without needing to introduce an extra CancelFromJS like wrapper.)

Assignee

Comment 6

2 months ago

CancelNoUpdate actually can and does trigger restyles via its call to
KeyframeEffect::NotifyAnimationTimingUpdated so at very least its name is wrong.

Furthermore, we actually want canceling to trigger restyles in most cases since
when an animation is canceled we need to trigger a subsequent restyle to apply
the (no-longer-animated) result.

This wasn't necessary when CancelNoUpdate was first introduced but since then we
have introduced the Servo style engine where we use a separate traversal to
apply the result from creating/deleting/modifying animations.

This change will mean that we now trigger a "layer" restyle when canceling an
animation when we previously didn't. That, however, seems more correct if
anything.

This patch also makes CancelFromStyle no longer virtual since it doesn't seem
necessary anymore (perhaps because we now point to the concrete type:
CSSAnimation/CSSTransition from nsAnimationManager/nsTransitionManager whereas
previously we didn't).

Assignee

Comment 7

2 months ago

Animation::Cancel calls UpdateTiming() which in turns runs the procedure to
update the finished state. However, the spec[1] doesn't require that.
Furthermore, calling UpdateTiming here hides the fact that we end up triggering
a restyle.

It would be better to move the parts of UpdateTiming we require into Cancel
itself so that we align better with the spec and to make it a bit more clear
what side-effects of UpdateTiming we actually rely on.

[1] https://drafts.csswg.org/web-animations-1/#cancel-an-animation

Depends on D28019

Assignee

Comment 8

2 months ago

Currently we avoid posting animation restyles when unbinding an element by
removing the element from the document before deleting its animation
collections. As a result, when canceled animations go to post a restyle, they
can't find a pres context and give up posting a restyle.

However, this is problematic for two reasons:

  • It means we can't remove such canceled animations from the
    PendingAnimationTracker if they are present there (i.e. it regresses the fix
    from bug 1223445).

  • It means we can't post cancel events for such animations/transitions since we
    can't lookup the appropriate AnimationEventDispatcher.

In the next patch in this series we will change that order to fix the above
problems but before we do that, we need to introduce another mechanism to make
sure that we don't post restyles when unbinding an element or else we will
regress bug 1396041.

This patch does that by introducing a flag which causes us to not post restyles
when we are doing DOM surgery. For all other cases we actually do need to post
restyles in order to update the style correctly.

Without this patch, layout/style/crashtests/1396041.html would fail after
applying the next patch in this series.

Depends on D28020

Assignee

Comment 9

2 months ago

See the previous patch in this series for a detailed explanation of why this is
necessary.

The test included in this patch is from some work I am doing to rewrite the CSS
transitions web-platform-tests. As a result, it includes more than is strictly
related to this issue addressed by this bug. Without the code changes in this
patch the fourth test onwards, "Transitions are canceled when an element is
removed from the document", will timeout waiting for the transitioncancel event.

Depends on D28021

Comment 10

2 months ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67900e2680ff
Drop Animation::CancelNoUpdate; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a751128e51f7
Make Animation::Cancel line up with the spec a little better; r=hiro
https://hg.mozilla.org/integration/autoland/rev/0d047d6f122b
Don't post animation restyles when unbinding an element; r=hiro
https://hg.mozilla.org/integration/autoland/rev/928f042ca74e
Drop animations on an element before removing it from the document; r=hiro
Assignee

Updated

2 months ago
Regressions: 1545689
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16523 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.