Closed Bug 1192592 Opened 9 years ago Closed 6 years ago

CSS transition works only the first time when the style change comes from removing a filling animation

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: erekosec, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 2 obsolete files)

Animation Bug Reproduction
502 bytes, text/html
Details
simplified and slowed down testcase
537 bytes, text/html
Details
Naive WIP patch
1.06 KB, patch
Details | Diff | Splinter Review
Testcase with inheritance
580 bytes, text/html
Details
Test case for animation overlapping with transition for some period
539 bytes, text/html
Details
Test case for an animation dropped during its fill
513 bytes, text/html
Details
Various animation/transition test cases with cross-browser results
25.94 KB, text/html
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150806001005

Steps to reproduce:

I have a class with an animation and animation-fill-mode set to forwards and am dynamically changing an element to this class so that it will animate. Then I am dynamically changing the element's class back to remove the animation and reset its appearance after the animation's completion.


Actual results:

Sometimes the element will transition as expected after the animation, but most of the time it jumps states entirely.


Expected results:

An animation should play after the first click. Then it should transition back to its original state after the second click.
OS: Unspecified → Windows 7
Hardware: Unspecified → All
It looks to me like it works the first time but then doesn't work later times.

My guess is that we're leaving the finished transition around, even though we should remove it in response to another style change (in this case driven by the animation).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Summary: Transition Fail After Animation-Fill-Mode Forwards → CSS transition works only the first time when the style change comes from removing a filling animation
Version: 39 Branch → Trunk
(In reply to David Baron [:dbaron] ⏰UTC-4 (busy Aug. 8-Aug. 30) from comment #1)
> It looks to me like it works the first time but then doesn't work later
> times.
> 
> My guess is that we're leaving the finished transition around, even though
> we should remove it in response to another style change (in this case driven
> by the animation).

I'm not sure if I've understood this properly but when we start the animation the second time we should, I suppose, be detecting the style change and dropping the transition.

What I've observed, so far, however, is that when we get to nsTransitionManager::StyleContextChanged we calculate the afterChangeStyle without CSS animations.

I'm not entirely sure how this is supposed to work. That is, is calling nsStyleSet::ResolveStyleWithoutAnimation and passing eRestyle_CSSTransitions as the restyle hint is supposed to return the style without transitions but with animations? The description in nsStyleSet.h seems to suggest this but I'm pretty sure we don't want the animations style included in what we pass to ConsiderStartingTransition so this is probably correct.

In ConsiderStartingTransition we hit this line:

  if (haveCurrentTransition && haveValues &&
      oldPT->Properties()[0].mSegments[0].mToValue == endValue) {
    // GetAnimationRule already called RestyleForAnimation.
    return;
  }

The rationale being:

  // If we got a style change that changed the value to the endpoint
  // of the currently running transition, we don't want to interrupt
  // its timing function.

As a result of this we bail out early and miss the block where we would destroy the animation (since shouldAnimate is false but haveCurrentTransition is true). I suppose, in that case, we should have recognized that the transition had already completed and not applied that logic?

We get another chance to destroy the transition later in nsTransitionManager::StyleContextChanged when we do this check:

      if ((checkProperties &&
           !allTransitionProperties.HasProperty(prop.mProperty)) ||
          // properties whose computed values changed but for which we
          // did not start a new transition (because delay and
          // duration are both zero, or because the new value is not
          // interpolable); a new transition would have segment.mToValue
          // matching currentValue
          !ExtractComputedValueForTransition(prop.mProperty, afterChangeStyle,
                                             currentValue) ||
          currentValue != segment.mToValue) {

It's the last two conditions that are of interest here. PruneCompletedTransitions has these same two conditions.

Again, since afterChangeStyle does not include the animation style we will end up with currentValue == segment.mToValue so we won't follow this branch and drop the transition here.

When we come to update the cascade results later in StyleContextChanged we correctly notice that the transition is now overridden by an animation. When the transition is still running we don't want to drop it but simply mark it as being overridden. Were it finished at this point, however, this might be another opportunity to drop it.

Having said all that, I wonder if we really want to drop the transition based on whether it has finished or not. It seems like if we applied the animation just before the transition finished, then dropped the animation, we would still want the subsequent transition to occur. Perhaps, in that case, we should drop the transition when it finishes if it is being overridden (i.e. its transition property mas mWinsInCascade false). I have to re-read the spec to see what is supposed to happen in that case and re-think this.
(In reply to Brian Birtles (:birtles) from comment #2)
> I'm not entirely sure how this is supposed to work. That is, is calling
> nsStyleSet::ResolveStyleWithoutAnimation and passing eRestyle_CSSTransitions
> as the restyle hint is supposed to return the style without transitions but
> with animations? The description in nsStyleSet.h seems to suggest this but
> I'm pretty sure we don't want the animations style included in what we pass
> to ConsiderStartingTransition so this is probably correct.

I'm not sure if this is actually correct. It may simply be that the animation in this case starts from the underlying value and so the values happen to compare equal. If I change the first keyframe to specify a value of width the problem no longer manifests.

I need to look into this further.
If I've understood the spec correctly then it might need an update. Based on comment 1, I'm assuming the intention is that applying a new animation should cause us to remove the completed transition.

When we trigger the animation the second time and consider starting a transition, I believe our state looks like this:

  completed transitions: [width: 250px -> 200px]
  before-change style: width 200px (end point of transition)
  after-change style: width 200px (start point of animation)

Then applying the steps from 3.1 Start of transitions[1] we get:

  1. If all of the following are true:

    * the element does not have a running transition for the property,
      [true]
    * the before-change style is different from and can be interpolated with the
      after-change style for that property,
      [false]
    * the element does not have a completed transition for the property or the
      end value of the completed transition is different from the after-change
      style for the property,
      [false]
    * there is a matching transition-property value, and
      [true]
    * the combined duration is greater than 0s, 
      [true]

  then implementations must remove the completed transition (if present) from
  the set of completed transitions and start a transition whose... [edited]

  2. Otherwise, if the element has a completed transition for the property and
     the end value of the completed transition is different from the
     after-change style for the property, then implementations must remove the
     completed transition from the set of completed transitions. [false]

So it doesn't seem like the spec would have us remove the completed transition at this point.

[1] https://drafts.csswg.org/css-transitions/#starting
David, based on comment 4, it seems like the spec might need updating here. What do you think?
Flags: needinfo?(dbaron)
Agree that it probably needs updating; I don't think I'll be able to look in detail until at least the week of September 21, so leaving the needinfo request.
I'm looking into this, and I have some questions.

Attached testcase is a simplified and slowed down version of the original testcase.
The initial width of the box is 200px, and it does animation to width:500px in 3seconds for the 1st click, and it does transition back to width:200px in 10seconds for the 2nd click.

When I click 2nd time *before* the 200px=>500px animation completes, the transition starts from the intermediate value, like 350px=>200px.

[Question 1]
If I click 3rd time before the 350px=>200px transition completes, the animation starts from 200px, instead of the intermediate value.  So the width of the box jumps.  Is this expected behavior?

[Question 2]
If I click 4th time before both the 200px=>500px animation and the 350px=>200px transition complete, it resumes the 350px=>200px transition, from the middle of the transition, instead of starting a new transition from the current intermediate value.  The width of the box jumps again.  Is this expected behavior?


About question 2, I feel, we should remove the conflicting transition for a new animation regardless of "completed or not" (maybe, cancel it?), at the 3rd click.  So that the transition to width:200px starts from the current width every time (in the original testcase too).  What could be the issue with this way?
Flags: needinfo?(bbirtles)
(In reply to Tooru Fujisawa [:arai] from comment #7)
> Created attachment 8744666 [details]
> simplified and slowed down testcase
> 
> I'm looking into this, and I have some questions.
> 
> Attached testcase is a simplified and slowed down version of the original
> testcase.
> The initial width of the box is 200px, and it does animation to width:500px
> in 3seconds for the 1st click, and it does transition back to width:200px in
> 10seconds for the 2nd click.

animation: [200px -> 500px]

> When I click 2nd time *before* the 200px=>500px animation completes, the
> transition starts from the intermediate value, like 350px=>200px.

I'm a bit confused about how this should actually work. The spec text here is,

  "Otherwise, define the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time."[1]

[1] https://drafts.csswg.org/css-transitions/#before-change-style

It would seem that in this case we are taking the before-change style to be 350px--but I would expect the CSS Animation to have been cancelled by this point, making the before-change style 200px?

Anyway, assuming this is correct we have:

  before-change style: 350px
  after-change style: 200px
  new transition: 350px -> 200px

> [Question 1]
> If I click 3rd time before the 350px=>200px transition completes, the
> animation starts from 200px, instead of the intermediate value.  So the
> width of the box jumps.  Is this expected behavior?

In that case, we trigger another animation and employ this behavior:

  "If a 0% or from keyframe is not specified, then the user agent constructs a 0% keyframe using the computed values of the properties being animated."[2]

[2] https://drafts.csswg.org/css-animations/#keyframes

So we should create a 0% keyframe with 'width' of, say, 250px. It looks like we're not doing that--that is, we're not including currently running transitions when we create the 0% keyframe.

So, I guess we're not doing the right thing.

(See my final question, however--I'm not sure if we want to include running transitions when calculating this value.)

> [Question 2]
> If I click 4th time before both the 200px=>500px animation and the
> 350px=>200px transition complete, it resumes the 350px=>200px transition,
> from the middle of the transition, instead of starting a new transition from
> the current intermediate value.  The width of the box jumps again.  Is this
> expected behavior?

Firstly, presumably in the previous step we did *not* cancel the transition so we have a running transition in the set with values 350px -> 200px.

(This seems odd---should the transition really keep running? Will we get different behavior depending on if the invisible transition finished or not?)

Note that even though we might not have canceled the transition, you won't see it because of the behavior defined when transitions and animations overlap. In that case for transitions:

  "Implementations must add this value to the cascade if and only if that property is not currently undergoing a CSS Animation ([CSS3-ANIMATIONS]) on the same element."[3]
  
[3] https://drafts.csswg.org/css-transitions/#application

So our state is:

  running-transitions: [ width: 350px -> 200px ]
  before-change style: Do I include the CSS animation or has it been canceled already?
     It seems like Gecko says: include it
                   spec says:  don't?
  after-change style: 200px

In that case we run the "Starting of transitions" behavior[4] and from what I can tell none of the conditions match. 

[4] https://drafts.csswg.org/css-transitions/#starting

> About question 2, I feel, we should remove the conflicting transition for a
> new animation regardless of "completed or not" (maybe, cancel it?), at the
> 3rd click.  So that the transition to width:200px starts from the current
> width every time (in the original testcase too).  What could be the issue
> with this way?

I think that makes sense but I'm pretty confused about this now and probably need to go and debug it again. I think I need to understand also:

1. Does the before-change style include the endpoint of a cancelled animation? (What does the spec intend and what do we implement?)

2. Should the computed value used in synthesized 0%/100% keyframes include the value of transitions animating at the time when the animation was created? (Presumably not because we'd have to define at what *time* the value of the transition was used so that we could maintain that value when we rebuild the animation due to changes to other animation-* properties?)

So you're probably right, but it's been a while since I looked into this and I haven't quite got my head around it at the moment. If you are able to help understand those two questions that would be helpful.
Flags: needinfo?(bbirtles)
Thank you for your feedback :)

(In reply to Brian Birtles (:birtles) from comment #8)
> 1. Does the before-change style include the endpoint of a cancelled
> animation? (What does the spec intend and what do we implement?)

About the spec, I don't think the animation is already cancelled at that point.

  "define the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time."

We're about to start new transition because the context is changed *now* (like, adding ".anim" class), so the the context of "the *previous* style change event" is not yet changed to the state that we should cancel the previous animation or start the next transition.  It's just updating the *time* of the animations to current time.  Maybe it will move the animation to another state and it may complete if it was still running, but that won't cancel them.

About the implementation, Give me some more time to learn about it.


> 2. Should the computed value used in synthesized 0%/100% keyframes include
> the value of transitions animating at the time when the animation was
> created? (Presumably not because we'd have to define at what *time* the
> value of the transition was used so that we could maintain that value when
> we rebuild the animation due to changes to other animation-* properties?)

If we don't remove the conflicting transition before calculating 0%/100% keyframes and use the effect while calculating, |"100%" {}| will end up stopping at the intermediate value.

  "If a ‘100%’ or ‘to’ keyframe is not specified, then the user agent constructs a ‘100%’ keyframe using the computed values of the properties being animated."

Using the intermediate value for 0% will make those transition and animation connect smoothly, but using it in 100% won't be nice.

To make things simpler, maybe we should not include transition effect while calculating 0%/100% keyframes, and make it clear that "transition to animation" don't connect smoothly like "animation to transition" when they're switched before completed.
(In reply to Tooru Fujisawa [:arai] from comment #9)
> Thank you for your feedback :)
> 
> (In reply to Brian Birtles (:birtles) from comment #8)
> > 1. Does the before-change style include the endpoint of a cancelled
> > animation? (What does the spec intend and what do we implement?)
> 
> About the spec, I don't think the animation is already cancelled at that
> point.
> 
>   "define the before-change style as the computed values of all properties
> on the element as of the previous style change event, except with any styles
> derived from declarative animations such as CSS Transitions, CSS Animations
> ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated
> to the current time."
> 
> We're about to start new transition because the context is changed *now*
> (like, adding ".anim" class)

But adding/removing .anim here doesn't trigger transitions by itself, right? There's nothing in that rule that touches a transitionable property. I think the transition we're triggering in this particular case is due to cancelling the animation, right?

> > 2. Should the computed value used in synthesized 0%/100% keyframes include
> > the value of transitions animating at the time when the animation was
> > created? (Presumably not because we'd have to define at what *time* the
> > value of the transition was used so that we could maintain that value when
> > we rebuild the animation due to changes to other animation-* properties?)
> 
> If we don't remove the conflicting transition before calculating 0%/100%
> keyframes and use the effect while calculating, |"100%" {}| will end up
> stopping at the intermediate value.

Yeah, I think the intended behavior is that we don't include transitions (and I think I've tested for that at one point), I'm just having trouble finding where that's specified at the moment (or if it's assumed to fall out of the cascade with transitions being at a higher level).
(In reply to Brian Birtles (:birtles) from comment #10)
> But adding/removing .anim here doesn't trigger transitions by itself, right?
> There's nothing in that rule that touches a transitionable property. I think
> the transition we're triggering in this particular case is due to cancelling
> the animation, right?

Yes, but doesn't removing .anim class cancel the animation instantly?  There seems to be no extra rule about when to cancel the running animation, so I suppose the animation just get cancelled when the rule becomes no more applicable.  Can the style change event for the "removing .anim" and the "cancelling the animation" different?  or perhaps is it just an issue of the execution order in the single style change event?


Here's my understanding, let's think the following case:

  time: T1
    style change:           add .anim class
    animation:              [200px -> 500px] starts
    width of the animation: 200px
    computed width:         200px

  time: T2  (T2 > T1)
    style change:           none
    animation:              [200px -> 500px] is still running
    width of the animation: 270px
    computed width:         270px

  time: T3  (T3 > T2)
    style change:           remove .anim class
    animation:              [200px -> 500px] was still running, and gets cancelled
    width of the animation: 350px (if it's still running)
    computed width:         200px (without transition)

Style change event happens both for T1 and T3, right? (is there any other point that style change event happens?,  hiro-san told me that the style change event doesn't happen for each frame in the running animation)

At the style change event for T3, previous style change event is for T1, and the [200px -> 500px] animation is still running.
About before-change style, we use the rule for T1, and by changing the time to T3 (current time), the width of the animation becomes 350px, and the computed width is also 350px;
About after-change style, we use the rule for T3 without transition, so the animation is no more applied and the computed width is 200px;
Here before-change style and after-change style is different and the transition happens with those values.


Even if the animation completes before removing .anim class, the situation won't become different much, as long as the animation is still applied to the property just before the style change event.


> > > 2. Should the computed value used in synthesized 0%/100% keyframes include
> > > the value of transitions animating at the time when the animation was
> > > created? (Presumably not because we'd have to define at what *time* the
> > > value of the transition was used so that we could maintain that value when
> > > we rebuild the animation due to changes to other animation-* properties?)
> > 
> > If we don't remove the conflicting transition before calculating 0%/100%
> > keyframes and use the effect while calculating, |"100%" {}| will end up
> > stopping at the intermediate value.
> 
> Yeah, I think the intended behavior is that we don't include transitions
> (and I think I've tested for that at one point), I'm just having trouble
> finding where that's specified at the moment (or if it's assumed to fall out
> of the cascade with transitions being at a higher level).

mmm, I cannot find any on the spec.

While constructing 0%/100% keyframes based on the computed value, the animation is not yet applied, so the following rule doesn't apply:

  "Implementations must add this value to the cascade if and only if that property is not currently undergoing a CSS Animation ([CSS3-ANIMATIONS]) on the same element."

and I don't see anything on the CSS Animations spec about the interaction between transition, except the "ISSUE"s.
Yes, thanks, sorry it took me a while to recall how all this is supposed to work.

So, I think we're back to where we started. We need to update the transitions spec to remove completed transitions when we start an animation.

So I think the condition we're getting tripped up on may be:

  "Otherwise, if the element has a completed transition for the property and the end value of the completed transition is different from the after-change style for the property, then implementations must remove the completed transition from the set of completed transitions."

And we're getting tripped up because in this case the end value of the completed transitions is NOT different from the after-change style.

Presumably that condition exists so that if we have a completed transition and something sets a property to its current value, we don't remove the transition---I can't remember why that's important but I think it comes up in some cases of transitions running on inherited properties where the ancestor has a duration of different length: we need to remember that the descendant has already done its transition and no trigger another one due to the ancestor's transition. Or something like that.

As for running transitions---they might just work because the values will differ. I think I'd like to fix the 'completed' case first, anyway.

I can try to look into the required spec changes if necessary but I'd prefer to wait for David because he's familiar with why each of these conditions exist.
(In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #12)
> So I think the condition we're getting tripped up on may be:
> 
>   "Otherwise, if the element has a completed transition for the property and
> the end value of the completed transition is different from the after-change
> style for the property, then implementations must remove the completed
> transition from the set of completed transitions."
> 
> And we're getting tripped up because in this case the end value of the
> completed transitions is NOT different from the after-change style.

Yes, so, for completed transition, I think it should detect that there are new animation for the property, and in that case remove the completed transition regardless of the after-change value, as animation's computed value on the first style-change event doesn't reflect any of its keyframes other than 0%.

Maybe we need to define some extra variable that reflects the set of running animations for "before" and "after" style-change event, for all properties.  so that we can detect there are new animation for the property.


> As for running transitions---they might just work because the values will
> differ. I think I'd like to fix the 'completed' case first, anyway.

I agree that the running transitions case is less critical, as the issue with it is just a "jump", not skipping whole transition.  If they can be solved separately, it would also be okay.


> I can try to look into the required spec changes if necessary but I'd prefer
> to wait for David because he's familiar with why each of these conditions
> exist.

Okay, for now, will continue learning about the code :)
Thank you for your help!
See Also: → 1319384
(In reply to Brian Birtles (:birtles) from comment #12)
> So, I think we're back to where we started. We need to update the
> transitions spec to remove completed transitions when we start an animation.

Actually, I think maybe the spec is OK here, because in the spec's model we have a "style change event" for each movement of the animation, not only when the animation starts.  Thus there should be a style change event when the animation moves from 200px to 201px (or some fractional value) that causes the completed transition to be removed.

I'm guessing we optimize away the code that removes completed transitions when the only style updates are from animations, perhaps?
Flags: needinfo?(dbaron)
Does that analysis make sense to you?
Flags: needinfo?(bbirtles)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #15)
> (In reply to Brian Birtles (:birtles) from comment #12)
> > So, I think we're back to where we started. We need to update the
> > transitions spec to remove completed transitions when we start an animation.
> 
> Actually, I think maybe the spec is OK here, because in the spec's model we
> have a "style change event" for each movement of the animation, not only
> when the animation starts.  Thus there should be a style change event when
> the animation moves from 200px to 201px (or some fractional value) that
> causes the completed transition to be removed.
> 
> I'm guessing we optimize away the code that removes completed transitions
> when the only style updates are from animations, perhaps?

Yes, we don't attempt to run transitions based on animation-only style updates.[1]

I'm not sure we want to either, do we? Supposing we did and the condition in step 1 of "Starting transitions"[2] evaluated to true on a subsequent frame, we'd end up removing the completed transition but we'd *also* end up generating a new transition from width: 200px to width: 201px. We wouldn't add the transition to the cascade but we'd still dispatch events for it. Furthermore, on the next tick, which would most likely occur before the new transition finishes, we'd cancel the transition and generate a new transition for width: 200px to width: 203px and so on.

Also, even if we're ok with all that, I wonder we'd end up relying on having a certain frame rate in order to correctly drop the completed transition.

In terms of the Gecko implementation, if we simply drop the check for an animation-only restyle in nsTransitionManager::StyleContextChanged this test happens to sort-of work (I suspect on a release build you'd see a little flicker though). The reason is that we still don't try to generate transitions on each frame because the animation generations compare equal. *However*, when the animation finishes the animation generation is updated and we *do* trigger a small transition (e.g. from width: 245px to width: 250px).

I think I'm missing something, however. Are you sure we want to trigger transitions based on changes from CSS animations?

[1] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/layout/style/nsTransitionManager.cpp#457
[2] https://drafts.csswg.org/css-transitions/#starting
Flags: needinfo?(bbirtles) → needinfo?(dbaron)
I don't think we want to trigger transitions based on animation-only style updates -- given the spec text, I don't think it's possible for transitions to start.  But given the spec text, it *is* possible to have completed transitions removed, and maybe we could write a small piece of code to do only that?
Flags: needinfo?(dbaron)
I don't follow. The spec text I'm looking at is this part[1]:

  1. If all of the following are true:

    * the element does not have a running transition for the property,
    * the before-change style is different from and can be interpolated with the
      after-change style for that property,
    * the element does not have a completed transition for the property or the
      end value of the completed transition is different from the after-change
      style for the property,
    * there is a matching transition-property value, and
    * the combined duration is greater than 0s, 

  then implementations must remove the completed transition (if present) from
  the set of completed transitions and start a transition whose...

  2. Otherwise, if the element has a completed transition for the property and
     the end value of the completed transition is different from the
     after-change style for the property, then implementations must remove the
     completed transition from the set of completed transitions.

So I think you're saying we want (1) to evaluate false and (2) to evaluate true.

I don't understand why (1) would not evaluate true, however.

[1] https://drafts.csswg.org/css-transitions/#starting
(In reply to Brian Birtles (:birtles) from comment #19)
>   1. If all of the following are true:

This one:

>     * the before-change style is different from and can be interpolated with the
>       after-change style for that property,

isn't true, because the before-change style is defined as:

> define the before-change style as the computed values of all
> properties on the element as of the previous style change event,
> except with any styles derived from declarative animations such
> as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL
> Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time

so the advancing of an animation will produce the before-change style being the same as the after-change style.
Oh, I completely misread that definition as before-change style not having animation applied.
Looking into this a bit further, it looks like our before-change style does not match the spec (it's always behind the after-change style). I thought it because we *don't* call UpdateOnlyAnimationStyles if we only have restyles resulting from animation but that doesn't seem to be it. I need to investigate further.

However, in terms of implementation, I'm pretty sure we don't want to check for new transitions on every tick since that would imply we couldn't throttle compositor animations. I suspect we'll want some other mechanism for cancelling transitions when an animation runs.
I tried to make our implementation produce a before-change style in the same way the spec describes (so we can verify the spec) but that proves difficult. If we *always* run an animation-only restyle as the first step in processing restyles in order to produce the correct before-change style, then as part of doing that animation-only restyle when we come to do the actual restyle, there will be no pending restyles left to do and hence we won't enter nsTransitionManager::StyleContextChanged to look for transitions (which is fine, since we shouldn't find any, anyway).

On the other hand we *will* enter nsTransitionManager::StyleContextChanged while doing the animation-only restyle but at that point the old style context is the one without the animation changes for the current frame applied and hence not suitable for using to generate transitions (i.e. it isn't equivalent to the before-change style).

So, in effect, we're skipping generating transitions which is correct, but we're also missing the opportunity to drop any completed transitions. I wonder if we can just do something in nsTransitionManager::StyleContextChanged where, if we're in an animation-only style update and have completed transitions whose end value does not match the value in the new style context, we drop it. I'll give it a try.
Attached patch Naive WIP patch — — Splinter Review 
I had a look into simply cancelling transitions during animation restyles. This almost works (it fixes the patch in this test case) but comes unstuck with inherited transitions. Test case forthcoming.
This test case demonstrates the problem. We have a transition on 'color' on a parent element, which triggers a transition of shorter length on a child that inherits the property.

The trouble is, once the child transition has finished we will end up cancelling the transition on it because we can't detect that the style change from the transition on the parent is unrelated.

This is very complicated, so allow me to explain.

Normally we ignore style changes due to transitions on ancestors at this point because (a) when we are doing the animation-only restyle we don't update transitions, and (b) when we actually do go to update transitions we are, at that point, comparing the before-change style and the after-change style.

Recall that the before-change style is the previous style with all the transitions brought up to date. Now, since our child transition has finished, it goes back to simply inheriting its parent's style so the before-change style simply is the parent's current transition value.

The after-change style is the style on this element without any transitions. Since the transition on this element has finished, it has no transition style, so, again, the after-change style is just the inherited parent's current transition value.

This seems odd from an author's point of view, and doesn't match what Chrome does, but that seems to be what the spec suggests and that's what we do.

In nsTransitionManager::StyleContextChanged if we want *during an animation-only restyle* to detect changes from CSS animations and cancel completed transitions, it seems like we can't differentiate between a change from an inherited parent transition and a change from an animation.

* aOldStyleContext will represent the style inherited during the previous frame
  (Recall that we are in an animation-only restyle so at this point the old style
  context does *not* include the style from advancing declarative animations.)
* aNewStyleContext will represent the updated style for this frame.
* If we call ResolveStyleWithoutAnimation to generate an afterChangeStyle it will
  simply be the same as aNewStyleContext for our purposes since the transitions we
  care about have all finished.

So all we can compare is aOldStyleContext and aNewStyleContext, which will be != for both CSS animations and inherited CSS transitions.

What's more, if we compare any of those to the end value of the completed transition, it won't match either (which is what PruneCompletedTransitions does, and why the previous patch doesn't fix this case).

Going back to the issue where in this test case we seem to run the transitions 1.5 times (and that seems counter-intuitive and Chrome doesn't do it), I wonder if we could actually make CSS transitions fill forwards.

That would fix the odd behavior where the transition seems to jump backwards and make us compatible with Chrome.

I think it would let us distinguish a change due to CSS animation too. Basically, for a completed transition whose inherit parent is still transitioning in nsTransitionManager::StyleContextChanged we'd have:

* aOldStyleContext - my fill value from last sample
* aNewStyleContext - my fill value still
* afterChangeStyle - my parent's transitioning value
* ToValue - my fill value

If a CSS animation ran, however, since we suppress transition styles while CSS animations are running on the same property we'd see:

* aOldStyleContext - my fill value from last sample
* aNewStyleContext - CSS animation value
* afterChangeStyle - CSS animation value
* ToValue - my fill value

So we could simply compare aNewStyleContext to ToValue and if they don't match, drop the transition (i.e. exactly what PruneCompletedTransitions already does).

That won't work for regular style changes such as those from script (because CSS transitions beat them in the cascade) but I believe our regular handling of CSS transitions for non-animation restyles should detect them.

Well, that's the theory anyway. I'll bet it's more complicated than that, however.
I've been wrestling with this for a few days trying to work out what the spec intends and what we're doing and I *think* that the crux of the issue might be that we do something asymmetric with regards to starting animations and ending animations.

When we start an animation we do our regular animation-only update which brings all declarative animations up-to-date, but then we go to generate the new CSS animation. That may update the animation rule for the animations-level of the cascade which may, in turn, mean the new style context gets updated. We then go to check if we should generate new transitions and now our old and new style contexts don't match.

I'm pretty sure at this point the spec intends that we update the old style context. It says,

"the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time."

So while it's not clear if that means the CSS animations we generated as part of the same style change event, I'm assuming it does. In Gecko, however, we don't do that. As a result, generating a new CSS animation may generate a new transition (if the start of the CSS animation differs from the the style's existing value, which often it doesn't).

However, when an animation updates or ends, that happens as part of the regular animation style update so we don't generate transitions in that case.

I *think* that's the root problem here--our before-change style really needs to include styles from CSS animations generated in the same style change event. After that, we may or may not *also* need to make one or more of the following changes:

* Call to PruneCompletedTransitions in StyleContextChanged for animation-only style updates (hopefully not)
* Make the fill-mode of transitions 'both' (we might need to do this to match Chrome's behavior, I'm not sure)
I managed to get the before-change style updated so it matches the after-change style (and doesn't generate a transition), but now the running transition is cancelled! And that appears to match what the spec requires (but not what Chrome or Edge do).

In the attached test case Chrome and Edge, the CSS animation clobbers the transition while it is active, but after it finishes the transition resumes. According to the spec though, I don't think it should. I actually like the Chrome/Edge behavior and wonder if the spec'ed behavior is intentional. I wonder if CSS animations should really cancel and trigger transitions.
I've spent quite a bit of time investigating this and I notice that our behavior differs from Chrome/Edge in a number of cases. Before I can go any further I'd like to understand the intention behind the spec text and whether or not I've understood it correctly for the following three matters.

1) Should a new CSS animation trigger or cancel transitions?

As an example, see attachment 8827068 [details]. A transition is running and is temporarily interrupted by a CSS animation.

At this point, what is the before-change style and after-change style? Should the before-change style include the newly generated CSS animation's result on the target properties? (i.e. is that included in bringing declarative animations up to date)?

If so, then I think we end up with no change and we end up cancelling the transition.
If not, then I think we end up triggering a new transition (and cancelling the old one).

On the other hand, Chrome's behavior here seems to be that the new CSS animation and it's effect on the target properties are all stored in a CSSAnimationUpdate object and not yet applied at the point where we consider triggering transitions and hence the existing transition is untouched. (On the next tick they will be applied but then the before and after styles will match I believe. In any case, they don't trigger transitions for animation-only style updates.)

The call stack is something like this:

  StyleResolver::applyAnimatedStandardProperties
    StyleResolver::calculateAnimationUpdate
      CSSAnimations::calculateAnimationUpdate
        - Generate new animation
        -> calculateAnimationActiveInterpolations(update, animatingElement);
      => Store in CSSAnimationUpdate object
    CSSAnimations::calculateTransitionUpdate
      Passed in |style| which, does *not* have the new animations applied yet

Edge appears to do likewise although I can't check the source to be sure.

2) Should cancelling a filling CSS animation trigger a new transition?

If an animation finishes normally we don't fire a transition. Presumably because we update animations in the before-change style and so it no longer includes the finished animation.

However, in the Gecko implementation, if we explicitly drop a filling animation (see attachment 1192592), that takes effect during the non-animation restyle phase. So while we drop the animation and its effect from the new style context, we don't update the old context / before-phase style and so we end up detecting a change and triggering a transition. I'm not sure what the spec intends here, i.e. does bringing declarative animations up to the current time include removing the effect of that are dropped as part of the current style change event?

Both Chrome and Edge don't trigger a transition in this case:

In the chromium source I believe this is explicitly avoided by the following check when generating transitions:

  !elementAnimations->cssAnimations()
      .m_previousActiveInterpolationsForAnimations.contains(
          property)
          
For what it's worth, the Chrome/Edge behavior for attachment 8827068 [details] seems more intuitive to me (the animation applies on top and does not interrupt the transition).

3) Should a transition on an inherited property that is shorter than its ancestor's transition restart?

This is basically attachment 8826471 [details]. The second time you click the text you'll notice that the transition runs once then starts again mid-way through. That's because the transition on the child has a shorter transition duration and when it has finished it begins inheriting the transition running on the ancestor. It seems odd and I wonder if this is really the intended behavior.

Looking at Chrome, the transition only runs once. As best as I can tell, that's because it simply runs the transition on the child and not the parent although I'm not entirely sure.

(For my own reference, at one point I thought Chrome used a fill mode of 'both' for transitions, however that's not the case. Looking at the source, there is a comment saying they use 'backwards' in order to implement delay but I believe that comment is old and actually they appear to use 'none'. For the delay phase they actually add an extra keyframe representing the delay phase. I believe that's for the DevTools so you can seek backwards and see the result as it was at the time.)


David, what do you think of these three questions?
Flags: needinfo?(dbaron)
I now have a Mac and can confirm that Safari's behavior matches Chrome for the various test cases mentioned in comment 28 and comment 29. Since we have interop on that behavior for all non-Gecko browsers, the specced behavior (at least as implemented in Gecko) is buggy, and that behavior is arguably more intuitive for at least some test cases, I suspect we should just spec that behavior. I just need to refresh my memory on what that behavior actually is and check that it makes sense as a spec.
Here is my analysis so far. I have a few ideas about how some of the different cases should work but I need to check how they work out in terms of implementation / spec text -- they might end up contradicting each other.
Still continuing to investigate this. I think (hope) I'm closer understanding how this should behave. If I'm right, it would only be a fairly minor clarification to the spec. My next step is to try and implement it in Gecko and see if it works.
Assignee: nobody → bbirtles
Attachment #8840755 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
Awesome!  I don't see the case in bug 1304937, but it's a gecko specific bug?
Hmm, interesting. I'm not sure yet about that one yet.
Attachment 8843824 [details] shows the results of my testing.

My tentative conclusion for how this should work is:

  * The regular ticking of animations should NOT trigger transitions.
  * Animations ending / starting should NOT trigger transitions, including
    when cancelling filling animations
    * This also includes animations ending / starting / being cancelled on
      parents
  * Animations starting / ending should not interrupt running transitions
  * However, the animation style from parents is included in the transition
    start point
  * Triggering changes on the parent also triggers transitions on the child
  * Transitions take into account the inherited animation value of the parent
    (including *both* transitions and animations)

To achieve the above:

  * The before AND after change should include all updated animation styles
    from the parent such that animation changes to the parent do NOT generate
    transitions
  * We should defer applying any animations / transitions starting on this
    tick until *after* evaluating if transitions should run. This will prevent
    starting / stopping animations from triggering transitions. If we do it
    right, it will also prevent cancelled filling animations from triggering
    transitions. I believe this is what Blink does.

In terms of spec changes I think the only real difference is to indicate that
the before and after change style do NOT include changes made to animations.

That is, where we have the sentence:

  "Likewise, define the after-change style as the computed values of all
  properties on the element based on the information known at the start of that
  style change event, but excluding any styles from CSS Transitions in the
  computation, and inheriting from the after-change style of the parent."

We need to clarify that it does not include changes to CSS Animations. Maybe
that is already the intention but we currently have a code comment that suggests
that at least at one point we only interpreted that as applying to CSS transitions:

  // Compute what the css-transitions spec calls the "after-change
  // style", which is the new style without any data from transitions,
  // but still inheriting from data that contains transitions that are
  // not stopping or starting right now.

As for the test case in this bug, we actually could fix it by simply dropping
the step in nsStyleSet::GetContext where we replace the animation rule.[1] The
reason why this works is a bit unexpected, however.

In our animation code we have a number of methods named *NoUpdate which are
intended to *not* trigger further restyles. This is because they are called when
we are, for example, updating CSS Animations. Normally when we are updating CSS
Animations, we will subsequently fetch the animation rule, and, if it has
changed, replace it and continue restyling with the updated animation rule. As
a result, we don't need to post a further restyle.

However, we current *do* post a restyle. The stack is typically something like
the following:

  nsAnimationManager::UpdateAnimations:443
  nsAnimationManager::BuildAnimations:1110
  CSSAnimationBuilder::Build:682
  mozilla::dom::CSSAnimation::PlayFromStyle:106
  mozilla::dom::Animation::PlayNoUpdate:1100
  mozilla::dom::CSSAnimation::UpdateTiming:322
  mozilla::dom::Animation::UpdateTiming:1213
  mozilla::dom::Animation::UpdateEffect:1283
  mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated:147
  mozilla::dom::KeyframeEffectReadOnly::RequestRestyle:1011
  mozilla::EffectCompositor::RequestRestyle:275
  mozilla::EffectCompositor::PostRestyleForAnimation:321
  nsIPresShell::RestyleForAnimation:2939
  mozilla::RestyleManager::PostRestyleEvent:25
> mozilla::GeckoRestyleManager::PostRestyleEvent:628

At least CancelNoUpdate has the same problem.

So, by commenting out the part where we update the animation rule, we simply
defer the update until the next restyle. As a result we partially implement
the approach outlined above in the sense that we defer some animation changes
until after testing if we should trigger transitions or not.

This doesn't, however, prevent us from triggering a transition when a filling
animation is cancelled. The reason for that is that when we cancel an
animation we can end up destroying its EffectSet which means that we lose the
old animation rule, and hence the after-change style will change.

I'm not sure entirely what the correct solution is yet but I am considering
either to:

a) Simply try to defer calling UpdateAnimations() until much later after we
   have triggered transitions for all descendents.

b) Call UpdateAnimations() at the same place but don't apply the changes until
   later (i.e. store some temporary metadata).

Perhaps these will end up being more-or-less the same. I believe in Stylo
we're trying to do (a) so it would be good to try and align our approach with
that. Also (b) is complicated by the fact that we do a number of updates
in-place so we can't, for example, simply store the new set of animations
and swap them in later.


Apart from the unintended restyles posted that I described above, in
investigating this, I noticed a few other issues too:

* The circumstances under which nsTransitionManager::StyleContextChanged
  triggers a restyle seem odd to me. Why don't we trigger a restyle when we
  have cancelled all transitions? i.e. when the collection is null?

  I wonder if this is a bug that is only being covered up by the fact that
  CancelFromStyle triggers a restyle for us.

* In GeckoRestyleManager::UpdateOnlyAnimationStyles we have the following
  comment:

    // FIXME:  We should have the transition manager and animation manager
    // add only the elements for which animations are currently throttled
    // (i.e., animating on the compositor with main-thread style updates
    // suppressed).
    PresContext()->EffectCompositor()->AddStyleUpdatesTo(tracker);

  We actually now know which elements have throttled style updates so we could
  very easily make a method that only adds pending styles for those elements
  with throttled style updates.

  I *think* that would work. The case I am concerned about is when we call
  UpdateOnlyAnimationStyles from
  GeckoRestyleManager::ProcessPendingRestyles(). In that case we have
  non-animation restyles (i.e. things which could trigger transitions) and we
  are trying to make sure all animation styles are up-to-date so that when we
  go to trigger transitions they have the correct starting point.

  Presumably for any non-throttled animations, they will have posted a restyle
  so we will update them before we go to trigger transitions.

[1] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/layout/style/nsStyleSet.cpp#996
(In reply to Brian Birtles (:birtles) from comment #36)
> * In GeckoRestyleManager::UpdateOnlyAnimationStyles we have the following
>   comment:
> 
>     // FIXME:  We should have the transition manager and animation manager
>     // add only the elements for which animations are currently throttled
>     // (i.e., animating on the compositor with main-thread style updates
>     // suppressed).
>     PresContext()->EffectCompositor()->AddStyleUpdatesTo(tracker);
> 
>   We actually now know which elements have throttled style updates so we
> could
>   very easily make a method that only adds pending styles for those elements
>   with throttled style updates.
> 
>   I *think* that would work. The case I am concerned about is when we call
>   UpdateOnlyAnimationStyles from
>   GeckoRestyleManager::ProcessPendingRestyles(). In that case we have
>   non-animation restyles (i.e. things which could trigger transitions) and we
>   are trying to make sure all animation styles are up-to-date so that when we
>   go to trigger transitions they have the correct starting point.
> 
>   Presumably for any non-throttled animations, they will have posted a
> restyle
>   so we will update them before we go to trigger transitions.

I remembered my concern here: If we don't update animations as a separate pass then presumably only the after-change style will be updated and we'll end up triggering transitions from the regular progress of animations.

The reason I recalled this is that I'm trying to work out how we can prevent triggering a transition when the parent's font-size change and we have an animation that uses em-units. In that case we'll call UpdateProperties on the animation which will update its computed values and produce a different animation rule that will only affect the after-change style.

Also, I might be wrong about PlayNoUpdate etc. not needing to post a restyle at all. So long as it is a throttled restyle I think it's ok. If we don't do that, then when we call GetAnimationRule we will end up using the old animation rule.
Filed bug 1345356 to drop the FIXME mentioned in comment 36 and comment 37.
See Also: → 1341372
This is fixed when stylo is enabled (we designed the stylo animation handling with this bug in mind).
Blocks: stylo-behavior-changes
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #39)
> This is fixed when stylo is enabled (we designed the stylo animation
> handling with this bug in mind).

Furthermore, I have confirmed that all of the examples in attachment 8843824 [details] exhibit the desired behavior with Stylo enabled.

I have updated CSS transitions[1] with the changes proposed in comment 36.

[1] https://github.com/w3c/csswg-drafts/commit/15af0e106b562ea6b72d7f11bf4b9eac15eb1f5f
Great to hear!
Marking as part of documentation plan for intentional stylo differences.
Keywords: dev-doc-needed
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #43)
> Documented:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/transition#Quantum_CSS_notes
> https://developer.mozilla.org/en-US/Firefox/Releases/57#Quantum_CSS_notes
> 
> Does this description make sense? I wasn't sure about this one.

Oh, I think that description might be back-to-front.

> Another Gecko bug means that filling animations (e.g. with animation-fill-mode: forwards set) don't trigger
> transitions set on that same element when finished or cancelled

The Gecko bug is that canceling a filling animation should *not* trigger a transition but in Gecko it does (but only once). In general declarative animations like CSS animations / Web Animations / SMIL etc. should *not* trigger transitions.

(Sorry about the delay, I was on PTO last week.)
Flags: needinfo?(cmills)
(In reply to Brian Birtles (:birtles) from comment #44)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #43)
> > Documented:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/CSS/transition#Quantum_CSS_notes
> > https://developer.mozilla.org/en-US/Firefox/Releases/57#Quantum_CSS_notes
> > 
> > Does this description make sense? I wasn't sure about this one.
> 
> Oh, I think that description might be back-to-front.
> 
> > Another Gecko bug means that filling animations (e.g. with animation-fill-mode: forwards set) don't trigger
> > transitions set on that same element when finished or cancelled
> 
> The Gecko bug is that canceling a filling animation should *not* trigger a
> transition but in Gecko it does (but only once). In general declarative
> animations like CSS animations / Web Animations / SMIL etc. should *not*
> trigger transitions.
> 
> (Sorry about the delay, I was on PTO last week.)

No worries Brian - thanks for the explanation. I've have fixed the description in line with your text.
Flags: needinfo?(cmills)
Closing since this works as expected on our new style system (i.e. stylo). And we already dropped the old style system.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: