Closed Bug 1478643 Opened 6 years ago Closed 6 years ago

Web Animation targeting transform freezes at last frame when it should snap back to the unanimated value

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: antoine, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(6 files, 2 obsolete files)

firefox-bug.html
413 bytes, text/html
Details
Not a regressed case I believe
470 bytes, text/html
Details
Bug 1478643 - Add test cases checking finished opacity animation on the compositor.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1478643 - Introduce a new change hint set for added or removed transform style.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1478643 - Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation.
59 bytes, text/x-review-board-request
birtles
: review+
Details
A reftest which doesn't fail without the fix
1.62 KB, patch
Details | Diff | Splinter Review
Attached file firefox-bug.html (obsolete) — 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15

Steps to reproduce:

Running Firefox Nightly 63.0a1 (2018-07-26), open the attached file.


Actual results:

Notice how, except for the first time the page was loaded, the black rectangle moves right by 100px and stays at that position until you click somewhere on the page.


Expected results:

The black rectangle should immediately snap back to the original position since the call to Element.animate() does not provide a "fillMode" and thus should use the default "none" value.
Hiro, looks like we fail to update the animation on the compositor sometimes.

I know I fixed a bug like this several years ago so perhaps this is a regression.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Animation
Ever confirmed: true
Priority: -- → P3
Product: Firefox → Core
Version: 63 Branch → Trunk
Yeah looks like a regression.  But I had a quick check on Firefox 57, the issue happens. :/
Not regressed by stylo either. :/
Someone please. :)
Keywords: regressionwindow-wanted
Attached file firefox-bug.html — 
Updated test case that doesn't use implicit keyframes (so we can test versions that don't support them).
Attachment #8995167 - Attachment is obsolete: true
So far I can see that this bug is not present in Firefox 48.

(Unfortunately there are quite a few versions between 48~64 that just crash for me so I'm not sure how far I can get with mozregression.)
Keywords: regression
I managed to narrow it down to somewhere between 52 and 53, specifically [2016-10-17, 2016-12-01], but builds in that range crash for me so I can't narrow it down any further on this machine.

(Curiously builds towards the end of that range have some odd behavior where there seems to be one frame shown mid-way between the last frame and returning to the initial frame--it's like we end up showing one frame using the main thread style.)
This doesn't quite fit the date range (which might be slightly wrong), but bug 1223658 is one suspect.
For what it's worth, here is the range mozregression gave me (again, I'm not completely confident in it):

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=94b0fddf96b43942bdd851a3275042909ea37e09&tochange=cd4cdcc9ad6c45dad8b8d8c0d40e459db2bca8a1

Animation related bugs include:

* Bug 1320474 - CSS animation-name should accept <string> as well as <custom-ident>
* Bug 1318697 - Elements disappear after sliding in
* Bug 1317178 - Stylo: Use single_value_to_css in Servo_DeclarationBlock_SerializeOneValue
* Bug 1304886 - Assertion failure: "accumulateResult || prop.mProperty == eCSSProperty_filter (could not accumulate value)" with animate(...,{iterationComposite:"accumulate"})
* Bug 1064937 - CSS Animations should support @keyframes animations of non-interpolable properties
* Bug 1311196 - Treeherder navigation between jobs is jumpy on recent nightlies; animations and transitions stutter / flicker
* Bug 1287983 - Implement transitionstart/transitionrun event
* Bug 1272549 - Support paced spacing for transform
* Bug 1319378 - Rotated element cropped when animated
* Bug 1289701 - Crash in mozilla::AnimValuesStyleRule::AddValue
* Bug 1286151 - Support paced spacing for filter property
* Bug 1273784 - Implement keyframe effect copy constructors
* Bug 1286150 - Support paced spacing for basic shapes

Of those, the following look particularly suspicious:

* Bug 1311196 - Treeherder navigation between jobs is jumpy on recent nightlies; animations and transitions stutter / flicker
* Bug 1319378 - Rotated element cropped when animated
(In reply to Brian Birtles (:birtles) from comment #9)
Nice!

> * Bug 1311196 - Treeherder navigation between jobs is jumpy on recent
> nightlies; animations and transitions stutter / flicker

Yeah this one looks the culprit, and bug 1223658.

> * Bug 1319378 - Rotated element cropped when animated

This shouldn't affect at all.
That said, I'm not sure bug 1311196 is strictly at fault here.

I think it simply sets the fill mode which means that when we fail to schedule a paint on the main thread it becomes noticeable (since the compositor is applying a fill). It actually fixes the problem I mentioned in comment 7 where, it seems like if there's a delay between finishing the animation and removing it from the compositor, we don't end up seeing an intermediate frame using the main thread's style.

So it seems like the fundamental problem is failing to schedule a paint after removing the animation from the compositor. Bug 1316764 fixed a similar bug but my guess is this one is a race between doing the paint and removing the animation.
Yeah agree.  Bug 1316764 un-wallpapered the fundamental problem.  Now I think I understand what's wrong.

When the transform animation finishes, the target element has no transform style, thus any nsDisplayTransform is generated (it's destroying instead?), then we have no chance to clear animations in AddAnimationsForProperty on the compositor.  But what I don't quite understand is that why the transform animation without delay doesn't cause the problem?  And IIRC there is another place we clear animations on the compositor, but I don't recall yet.
See Also: → 1452165
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> And IIRC there is another place we clear animations on the compositor, but I don't recall yet.

Found the place, it's DisplayItemData::ClearAnimationCompositorState.  Currently we just clear isRunningOnTheCompositor flag, but we should clear the compositor animations here in the case where we destroy the display items, AFAICT.

https://hg.mozilla.org/mozilla-central/file/8f2f847b2f9d/layout/painting/FrameLayerBuilder.cpp#l451
Oh, thank you! So you're saying we don't actually clear the animation from the compositor? That's interesting. That sounds like we should be able to write an automated test for that.

I was assuming that we _do_ clear it from the compositor but that we fail to do another paint after clearing it--but that was just a guess.
(In reply to Brian Birtles (:birtles) from comment #15)
> Oh, thank you! So you're saying we don't actually clear the animation from
> the compositor? That's interesting. That sounds like we should be able to
> write an automated test for that.

Yes, absolutely right.  What I thought initially when I saw this bug, "oh I should have written test cases checking the state of *finished* animations on the compositor. 

> I was assuming that we _do_ clear it from the compositor but that we fail to
> do another paint after clearing it--but that was just a guess.

That's just a guess, but I am 99% sure it is true.

Anyway, now I think this is the case what I've been looking for in bug 1459775.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
I was totally wrong. :) DisplayItemData::ClearAnimationCompositorState is not called at all. So the place we have to look at is places happen prior to paint.  HasEffectiveAnimation() might cause this situation?
Though I am still debugging, what I can tell is that in RestyleManager::AddLayerChangesForAnimation we have to apply  nsChangeHint_AddOrRemoveTransform and other change hints produced when the transform style is removed.
Ok, this is really annoying case. 

1. The transform animation in question has a positive delay and the target element has no transform style, which means that the initial style on the main-thread is 'transform:none'
2. After the initial styling on the main-thread the animation has been running on the main-thread
3. When the animation fisnishes we restyling the animation on the main-thread, the transform value is 'transform:none'
4. We do compare the first style at 1 and the last style at 3
5. As a result there is no change hint produced 
6. Also we have a special handling for the case of 'transform:none' in RestyleManager::AddLayerChangesForAnimation

To fix this, we somehow generate change hints that we apply in nsStyleDisplay::CalcDifference [1]

[1] https://hg.mozilla.org/mozilla-central/file/8f2f847b2f9d/layout/style/nsStyleStruct.cpp#l3821
This is also a regression by bug 1223658 what Brian suspected.
Blocks: 1223658
All test cases fail on WebRender for some reason (I guess getOMTA still has some problems or we don't clear animationInfo on WebRender?).  I will care it in a later bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9867d5d3d9c11ae164edadd0dbe8d3309e48cfcd
Depends on: 1479133
FWIW, here is a reftest I tried to write, but it doesn't fail without the fix.

We've been eliminating style/layout flushes when 'reftest-no-flush' is specified, but there might be another flush.  So I decided to write mochitests instead.
Comment on attachment 8995683 [details]
Bug 1478643 - Introduce a new change hint set for added or removed transform style.

https://reviewboard.mozilla.org/r/260072/#review267186

::: dom/animation/test/mozilla/test_style_after_finished_on_compositor.html:36
(Diff revision 2)
> +  await waitForNextFrame();
> +
> +  const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
> +
> +  assert_equals(opacity, '', 'No opacity animation runs on the compositor');
> +}, 'Opacity animation starts with positive delay');

Nit: This should probably describe what is being tested.

e.g.

'Opacity animation with positive delay is removed from compositor when finished'

::: dom/animation/test/mozilla/test_style_after_finished_on_compositor.html:52
(Diff revision 2)
> +  await waitForNextFrame();
> +
> +  const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
> +
> +  assert_equals(opacity, '', 'No opacity animation runs on the compositor');
> +}, 'Opacity animation starts with opacity: 1 for a while');

'Opacity animation that is initially opacity: 1 is removed from compositor when finished'

::: dom/animation/test/mozilla/test_style_after_finished_on_compositor.html:74
(Diff revision 2)
> +  await waitForNextFrame();
> +
> +  const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
> +
> +  assert_equals(opacity, '', 'No opacity animation runs on the compositor');
> +}, 'Opacity animation updates at the offset generating opacity: 1');

'Opacity animation is removed from compositor even when it only visits exactly the point where the opacity: 1 value was set'
Attachment #8995683 - Flags: review?(bbirtles) → review+
Comment on attachment 8995685 [details]
Bug 1478643 - Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation.

https://reviewboard.mozilla.org/r/260076/#review267190

::: dom/animation/test/mozilla/test_style_after_finished_on_compositor.html:90
(Diff revision 2)
> +  await waitForNextFrame();
> +
> +  const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
> +
> +  assert_equals(transform, '', 'No transform animation runs on the compositor');
> +}, 'Transform animation starts with positive delay');

As with the opacity test patch, it would be nice to change the descriptions to describe better what we are testing. I think we can just use the test descriptions from that patch and s/opacity/transform/, s/opacity: 1/transform: none/ etc.

::: layout/base/RestyleManager.h:402
(Diff revision 2)
> +  // There are some cases that we have to forcibly apply change hints for
> +  // animations even if there is no change hint produced to synchronize with
> +  // animations running on the compositor (i.e. updating animations or
> +  // discarding animations on the compositor).  For examples;
> +  // a) Pausing animations via the Web Animations API
> +  // b) The previous animation style is exactly same as the current style.

I'm not sure I really understand this comment. I added some minor tweaks:

  // There are some cases where we forcibly apply change hints for animations
  // even if there is no change hint produced in order to synchronize with
  // animations running on the compositor.
  //
  // For example:
  //
  // a) Pausing animations via the Web Animations API
  // b) When the previous animation style is exactly same as the current style.

Does (b) mean to say, "the style before sending the animation to the
compositor is exactly the same as the current style"?

::: layout/base/RestyleManager.cpp:1768
(Diff revision 2)
>        // If we have a transform layer but don't have any transform style, we
>        // probably just removed the transform but haven't destroyed the layer
>        // yet. In this case we will add the appropriate change hint
>        // (nsChangeHint_UpdateContainingBlock) when we compare styles so we can
>        // skip adding any change hint here. (If we *were* to add
>        // nsChangeHint_UpdateTransformLayer, ApplyRenderingChangeToTree would
>        // complain that we're updating a transform layer without a transform).
>        if (layerInfo.mProperty == eCSSProperty_transform &&
>            !aFrame->StyleDisplay()->HasTransformStyle()) {
> +        // If the animation generation has changed but appropriate change hint
> +        // hasn't been produced, which means that the transform style was 'none'
> +        // when the transform animation was sent to the compositor and the
> +        // current transform style is also 'none', that said, we have no idea
> +        // what was going on the compositor during this period so we have to add
> +        // all change hints what we apply for the case where transform style is
> +        // removed.
> +        if (!(NS_IsHintSubset(
> +                nsChangeHint_ComprehensiveAddOrRemoveTransform,
> +                aHintForThisFrame))) {
> +          hint |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
> +        }
>          continue;

I think we need to update the previous comment too. Something like:

      // If we have a transform layer but don't have any transform style, we
      // probably just removed the transform but haven't destroyed the layer
      // yet. In this case we will typically add the appropriate change hint
      // (nsChangeHint_UpdateContainingBlock) when we compare styles so in
      // theory we could skip adding any change hint here.
      //
      // However, sometimes when we compare styles we'll get no change. For
      // example, if the transform style was 'none' when we sent the transform
      // animation to the compositor and the current transform style is now
      // 'none' we'll think nothing changed but actually we still need to
      // trigger an update to clear whatever style the transform animation set
      // on the compositor. To handle this case we simply set all the change
      // hints relevant to removing transform style (since we don't know exactly
      // what changes happened while the animation was running on the
      // compositor).
      //
      // Note that we *don't* add nsChangeHint_UpdateTransformLayer since if we
      // did, ApplyRenderingChangeToTree would complain that we're updating
      // a transform layer without a transform.
      if (layerInfo.mLayerType == DisplayItemType::TYPE_TRANSFORM &&
          !aFrame->StyleDisplay()->HasTransformStyle()) {
        // Add all the hints for a removing a transform if they are not already
        // set for this frame.
        if (!(NS_IsHintSubset(
                nsChangeHint_ComprehensiveAddOrRemoveTransform,
                aHintForThisFrame))) {
          hint |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
        }
        continue;


Does that sound correct?
Attachment #8995685 - Flags: review?(bbirtles) → review+
Comment on attachment 8995684 [details]
Bug 1478643 - Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation.

https://reviewboard.mozilla.org/r/260074/#review267188

('comprehensive' feels a little awkward but I can't think of anything better!)

::: layout/style/nsStyleStruct.cpp:3814
(Diff revision 2)
> -    // We do not need to apply nsChangeHint_UpdateTransformLayer since
> -    // nsChangeHint_RepaintFrame will forcibly invalidate the frame area and
> -    // ensure layers are rebuilt (or removed).
> +    /* See nsChangeHint_ComprehensiveAddOrRemoveTransform in nsChangeHint.h what
> +     * change hints are exactly applied.
> +     */

I wonder if this comment is necessary? If a reader wants to know what this represents, they'll just grep/searchfox for it won't they?

And if we keep this comment, we should s/what change hints are exactly/for exactly what change hints are/
Attachment #8995684 - Flags: review?(bbirtles) → review+
Comment on attachment 8995682 [details]
Bug 1478643 - Add test cases checking finished opacity animation on the compositor.

https://reviewboard.mozilla.org/r/260070/#review267194

As discussed on IRC, I'm not sure we want to change this since it seems like when we come to support running individual transform component properties on the compositor it would be better to check the display item type.
Attachment #8995682 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #33)

> ::: layout/base/RestyleManager.h:402
> (Diff revision 2)
> > +  // There are some cases that we have to forcibly apply change hints for
> > +  // animations even if there is no change hint produced to synchronize with
> > +  // animations running on the compositor (i.e. updating animations or
> > +  // discarding animations on the compositor).  For examples;
> > +  // a) Pausing animations via the Web Animations API
> > +  // b) The previous animation style is exactly same as the current style.
> 
> I'm not sure I really understand this comment. I added some minor tweaks:
> 
>   // There are some cases where we forcibly apply change hints for animations
>   // even if there is no change hint produced in order to synchronize with
>   // animations running on the compositor.
>   //
>   // For example:
>   //
>   // a) Pausing animations via the Web Animations API
>   // b) When the previous animation style is exactly same as the current
> style.
> 
> Does (b) mean to say, "the style before sending the animation to the
> compositor is exactly the same as the current style"?

Right, the previous style means the style before sending.  I will use the revised one.
Thanks!

> ::: layout/base/RestyleManager.cpp:1768
> (Diff revision 2)
> >        // If we have a transform layer but don't have any transform style, we
> >        // probably just removed the transform but haven't destroyed the layer
> >        // yet. In this case we will add the appropriate change hint
> >        // (nsChangeHint_UpdateContainingBlock) when we compare styles so we can
> >        // skip adding any change hint here. (If we *were* to add
> >        // nsChangeHint_UpdateTransformLayer, ApplyRenderingChangeToTree would
> >        // complain that we're updating a transform layer without a transform).
> >        if (layerInfo.mProperty == eCSSProperty_transform &&
> >            !aFrame->StyleDisplay()->HasTransformStyle()) {
> > +        // If the animation generation has changed but appropriate change hint
> > +        // hasn't been produced, which means that the transform style was 'none'
> > +        // when the transform animation was sent to the compositor and the
> > +        // current transform style is also 'none', that said, we have no idea
> > +        // what was going on the compositor during this period so we have to add
> > +        // all change hints what we apply for the case where transform style is
> > +        // removed.
> > +        if (!(NS_IsHintSubset(
> > +                nsChangeHint_ComprehensiveAddOrRemoveTransform,
> > +                aHintForThisFrame))) {
> > +          hint |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
> > +        }
> >          continue;
> 
> I think we need to update the previous comment too. Something like:
> 
>       // If we have a transform layer but don't have any transform style, we
>       // probably just removed the transform but haven't destroyed the layer
>       // yet. In this case we will typically add the appropriate change hint
>       // (nsChangeHint_UpdateContainingBlock) when we compare styles so in
>       // theory we could skip adding any change hint here.
>       //
>       // However, sometimes when we compare styles we'll get no change. For
>       // example, if the transform style was 'none' when we sent the
> transform
>       // animation to the compositor and the current transform style is now
>       // 'none' we'll think nothing changed but actually we still need to
>       // trigger an update to clear whatever style the transform animation
> set
>       // on the compositor. To handle this case we simply set all the change
>       // hints relevant to removing transform style (since we don't know
> exactly
>       // what changes happened while the animation was running on the
>       // compositor).
>       //
>       // Note that we *don't* add nsChangeHint_UpdateTransformLayer since if
> we
>       // did, ApplyRenderingChangeToTree would complain that we're updating
>       // a transform layer without a transform.
>       if (layerInfo.mLayerType == DisplayItemType::TYPE_TRANSFORM &&
>           !aFrame->StyleDisplay()->HasTransformStyle()) {
>         // Add all the hints for a removing a transform if they are not
> already
>         // set for this frame.
>         if (!(NS_IsHintSubset(
>                 nsChangeHint_ComprehensiveAddOrRemoveTransform,
>                 aHintForThisFrame))) {
>           hint |= nsChangeHint_ComprehensiveAddOrRemoveTransform;
>         }
>         continue;
> 
> 
> Does that sound correct?

Yep, perfect.:)
Attachment #8995685 - Attachment is obsolete: true
Hmm, MozReview-Commit-ID isn't preserved on a new setup machine.
Comment on attachment 8995682 [details]
Bug 1478643 - Add test cases checking finished opacity animation on the compositor.

https://reviewboard.mozilla.org/r/260070/#review267200
Attachment #8995682 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c62a303f82a
Add test cases checking finished opacity animation on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4bdbf9fdf090
Introduce a new change hint set for added or removed transform style. r=birtles
https://hg.mozilla.org/integration/autoland/rev/fcde37a994ae
Apply change hints for transform animation in the case where the current transform style is 'none' and no change hint produced in AddLayerChangesForAnimation. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: