1267937 - mProgressOnLastCompose should be nullified in somewhere

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: step.html

While I am writing a reftest in bug 1235002, I noticed there are some cases we should nullify mProgressOnLastCompose somewhere.  In UpdateProperties?

Attaching file is a test case to confirm this problem.  The blue box in the test case should be rendered 200px but actually not.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Interesting.  I can not reproduce the problem on today's nightly...

Comment 2

[Hiroyuki Ikezoe (:hiro)]

OK. I can not reproduce the problem on non-E10S.  But why?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: A reftest

I think this reftest covers a test case which I was going to write reftest in bug 1235002.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e23e2e3068d487a85d36f9caf64be953418bd6
I think we can clear the flag if the animation is not in effect in NotifyAnimationTimingUpdated().  But the test on Android failed on try...

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed36a4bf1ee9

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Oops, I did not know that 'mach try' leaves comments on bugzilla.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

Review commit: https://reviewboard.mozilla.org/r/49389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49389/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

We need a long enough duration to ensure that the animation is not finished
when reftest takes a snapshot.  Also, The current time value setting before
changing frames should be far from zero.  That's because if the value is near
by zero, restyles requested by setting currentTime or frames will be processed
with *current time > 0* in the next tick. As a result the animation will be
painted with new frames regardless of the fix for this bug.

Review commit: https://reviewboard.mozilla.org/r/49391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49391/

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

https://reviewboard.mozilla.org/r/49389/#review46219

For my own reference, I think this basically comes down to the fact that we're looking at isRelevant rather than isInEffect. The reason we need to look at "in effect" is that Animation::ComposeStyle now includes an early return for IsInEffect() so we won't update mProgressOnLastCompose there.

My understanding is that if we're not in effect, then GetComputedTiming().mProgress must be null.

Now, if mProgressOnLastCompose is already null, there is nothing to do. However, if it's not-null then assuming mAnimation is set (which it currently always is since we only call NotifyAnimationTimingUpdated from an Animation), GetComputedTiming().mProgress != mProgressOnLastCompose so we will enter the block that is guarded:

  if (mAnimation && GetComputedTiming().mProgress != mProgressOnLastCompose) {
  
However, the problem occurs when we are *relevant* but not *in effect*.

Recall that relevant = "in effect" OR "current". So if we are "current" but NOT "in effect" we won't clear mProgressOnLastCompose here (since the test is for !isRelevant and we *are* still relevant in that case). We can only be current in the before or active phase so I don't think this is a problem for the after phase.

As a result, I suspect we only need to change isRelevant to isInEffect but given that mAnimation may become null in the future, I think this change is fine.

::: dom/animation/KeyframeEffect.cpp:198
(Diff revision 1)
> +  // Once we're not in effect, we need to clear mProgressOnLastCompose.
> +  // Otherwise the cached mProgressOnLastCompose will be used if some animaiton
> +  // properties are changed in the before or after phase.

Nit: Blank line before this comment.

I think we should write this as:

  // If we're no longer "in effect", our ComposeStyle method will never be
  // called and we will never have a chance to update mProgressOnLastCompose.
  // We clear mProgressOnLastCompose here to ensure that if we later become
  // "in effect" we will request a restyle (above).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8746382 [details]
> MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once
> we are not in effect. r?birtles
> 
> https://reviewboard.mozilla.org/r/49389/#review46219
> 
> For my own reference, I think this basically comes down to the fact that
> we're looking at isRelevant rather than isInEffect. The reason we need to
> look at "in effect" is that Animation::ComposeStyle now includes an early
> return for IsInEffect() so we won't update mProgressOnLastCompose there.
> 
> My understanding is that if we're not in effect, then
> GetComputedTiming().mProgress must be null.

Yes, my understanding is the same.

> However, the problem occurs when we are *relevant* but not *in effect*.
> 
> Recall that relevant = "in effect" OR "current". So if we are "current" but
> NOT "in effect" we won't clear mProgressOnLastCompose here (since the test
> is for !isRelevant and we *are* still relevant in that case). We can only be
> current in the before or active phase so I don't think this is a problem for
> the after phase.

Maybe.  I thought we can write a test case for the after phase with negative endDelay, but never tried.

> As a result, I suspect we only need to change isRelevant to isInEffect but
> given that mAnimation may become null in the future, I think this change is
> fine.

To be honest I have not considered much about the case when mAnimation is null and mTarget null either.  I will reconsider the case when it comes in our tree.

> ::: dom/animation/KeyframeEffect.cpp:198
> (Diff revision 1)
> > +  // Once we're not in effect, we need to clear mProgressOnLastCompose.
> > +  // Otherwise the cached mProgressOnLastCompose will be used if some animaiton
> > +  // properties are changed in the before or after phase.
> 
> Nit: Blank line before this comment.
> 
> I think we should write this as:
> 
>   // If we're no longer "in effect", our ComposeStyle method will never be
>   // called and we will never have a chance to update mProgressOnLastCompose.
>   // We clear mProgressOnLastCompose here to ensure that if we later become
>   // "in effect" we will request a restyle (above).

Thanks!!

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

https://reviewboard.mozilla.org/r/49391/#review46227

::: layout/reftests/web-animations/1267937-1.html:31
(Diff revision 1)
> +    // restyles requested by setting currentTime or frames in a next tick.
> +    // If this value is near by zero, say -1, the restyles will be processed in
> +    // the next tick and current time in the next tick must be greater than
> +    // zero at that point, that means the animation with new frame values will
> +    // be painted.  As a result, this test will be useless.
> +    anim.currentTime = -1000;

1s still seems fairly long. Would 500ms be enough? If it occasionally passes accidentally, that would be ok, right?

(We could even check inside the requestAnimationFrame callback that we got at least *one* callback with anim.currentTime < 0 -- or anim.effect.getComputedTiming().progress === null -- and if we didn't, we could log a warning?)

::: layout/reftests/web-animations/1267937-1.html:37
(Diff revision 1)
> +
> +    // Setting another frame does not cause any visual changes because
> +    // the animation is still in the before phase.
> +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> +
> +    // Is there any altenative events of animationstart for web-animations?

Not that I can think of. The only other approach I can think of is to use DOMWindowUtils.advanceTimeAndRefresh().

::: layout/reftests/web-animations/1267937-1.html:39
(Diff revision 1)
> +    // the animation is still in the before phase.
> +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> +
> +    // Is there any altenative events of animationstart for web-animations?
> +    window.requestAnimationFrame(function handleFrame() {
> +      if (anim.currentTime > 0) {

It might be better to use anim.effect.getComputedTiming().progress !== null ?

The reason is that if a frame falls *exactly* at the start of the active interval we should run the test then, right?

Until the animation starts anim.currentTime == 0 so we can't test anim.currentTime >= 0. However, until the animation starts anim.effect.getComputedTiming().progress === null, I think.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #11)
> ::: layout/reftests/web-animations/1267937-1.html:31
> (Diff revision 1)
> > +    // restyles requested by setting currentTime or frames in a next tick.
> > +    // If this value is near by zero, say -1, the restyles will be processed in
> > +    // the next tick and current time in the next tick must be greater than
> > +    // zero at that point, that means the animation with new frame values will
> > +    // be painted.  As a result, this test will be useless.
> > +    anim.currentTime = -1000;
> 
> 1s still seems fairly long. Would 500ms be enough? If it occasionally passes
> accidentally, that would be ok, right?

Good point! Actually I saw the accidentally failure with 500ms on my local emulator once, so I changed it to 1000ms.  But you are right!  It does not matter in this case.

> (We could even check inside the requestAnimationFrame callback that we got
> at least *one* callback with anim.currentTime < 0 -- or
> anim.effect.getComputedTiming().progress === null -- and if we didn't, we
> could log a warning?)

That's a really good idea.  I will do it.

> ::: layout/reftests/web-animations/1267937-1.html:37
> (Diff revision 1)
> > +
> > +    // Setting another frame does not cause any visual changes because
> > +    // the animation is still in the before phase.
> > +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> > +
> > +    // Is there any altenative events of animationstart for web-animations?
> 
> Not that I can think of. The only other approach I can think of is to use
> DOMWindowUtils.advanceTimeAndRefresh().
> 
> ::: layout/reftests/web-animations/1267937-1.html:39
> (Diff revision 1)
> > +    // the animation is still in the before phase.
> > +    anim.effect.setFrames({ marginLeft: [ "0px", "400px" ] });
> > +
> > +    // Is there any altenative events of animationstart for web-animations?
> > +    window.requestAnimationFrame(function handleFrame() {
> > +      if (anim.currentTime > 0) {
> 
> It might be better to use anim.effect.getComputedTiming().progress !== null ?
> 
> The reason is that if a frame falls *exactly* at the start of the active
> interval we should run the test then, right?

Yes, exactly!  I will revise it.

Thanks!

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Brian Birtles (:birtles. away 29 Apr-1 May) from comment #11)
> > ::: layout/reftests/web-animations/1267937-1.html:31
> > (Diff revision 1)
> > > +    // restyles requested by setting currentTime or frames in a next tick.
> > > +    // If this value is near by zero, say -1, the restyles will be processed in
> > > +    // the next tick and current time in the next tick must be greater than
> > > +    // zero at that point, that means the animation with new frame values will
> > > +    // be painted.  As a result, this test will be useless.
> > > +    anim.currentTime = -1000;
> > 
> > 1s still seems fairly long. Would 500ms be enough? If it occasionally passes
> > accidentally, that would be ok, right?
> 
> Good point! Actually I saw the accidentally failure with 500ms on my local

accidentally *success*  It's confusing.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49389/diff/1-2/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49391/diff/1-2/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=429ec814fbaa
500ms seems to work on try.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/49389/#review46219

> Nit: Blank line before this comment.
> 
> I think we should write this as:
> 
>   // If we're no longer "in effect", our ComposeStyle method will never be
>   // called and we will never have a chance to update mProgressOnLastCompose.
>   // We clear mProgressOnLastCompose here to ensure that if we later become
>   // "in effect" we will request a restyle (above).

Fixed. Thanks.

Comment 18

[Jim Mathies [:jimm]]

Is this a serious web compat issue? The test case doesn't seem to work in IE or Chrome. Marking lower e10s priority here but if you feel this should block rollout please renom.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jim Mathies [:jimm] from comment #18)
> Is this a serious web compat issue?

I don't think so.

> The test case doesn't seem to work in IE or Chrome.

That's because Chrome and IE do not yet implement KeyframeEffect.setFrames() which is used in the test case.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8746382 [details]
MozReview Request: Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49389/diff/2-3/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8746383 [details]
MozReview Request: Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49391/diff/2-3/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15070beaee3548018781299762f6c15995e05924
Bug 1267937 - Part 1: Clear mProgressOnLastCompose once we are not in effect. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/c358aa5441c953188a2fd2bc6ae0eeb813285d98
Bug 1267937 - Part 2: A reftest which checks mProgressOnLastCompose is surely cleared in before phase. r=birtles

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15070beaee35
https://hg.mozilla.org/mozilla-central/rev/c358aa5441c9