1383239 - Viewport doesn't repaint for delayed animation moving an item into view.

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski]

Attachment: testcase.html

In the following testcase, a red square should appear at the top/left of the page 1 second after it loads. However, if my mouse isn't on the window and/or moving, it doesn't do so unless the delay is lower than 1s (at least on my devices, perhaps?).

The square is just absolutely positioned and starts off-screen (left:-9999em), before the animation moves it into view (left:1em) after the delay. It appears on cue in Chrome, but not on several Firefox devices (with and without hw accel).

mozregression is giving me curious results; debug builds are broken way back to 2015-01-01, but opt builds somehow bisect to this range:
>INFO: Last good revision: 16663eb3dcfa759f25b5e27b101bc79270c156f2 (2016-05-23)
>INFO: First bad revision: 829d3be6ba648b838ee1953fdfa1a477dace752f (2016-05-24)
>INFO: Pushlog:
>https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16663eb3dcfa759f25b5e27b101bc79270c156f2&tochange=829d3be6ba648b838ee1953fdfa1a477dace752f

But the pushlog for that link shows nothing, so I'm not sure what's going on.

Comment 1

[j.j.]

Attachment: reduced testcase

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

confirming, "reduced testcase" without javascript shows the same error

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Interesting. It works fine on stylo.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Confirmed that on Windows too this reproduces, but not when Stylo is enabled.

Comment 4

[Chris Mills [:cmills]]

I've documented this bug on MDN:

https://developer.mozilla.org/en-US/docs/Web/CSS/animation-delay#Stylo_notes
https://developer.mozilla.org/en-US/Firefox/Releases/57#Stylo_notes

Comment 5

[Michal Čaplygin [:myf]]

On my machine I have been observing strange repaint bugs (Stable Fx, not reproducible in other even very similar environments, so I accounted it to GPU or weird settings; Nightly OK).  Most of them disappeared (namely glitchy stroke SVG animations for really huge values), but one super simple still prevails in the stable version:

Colour cross-fade of empty `body` background-color:

   data:text/html,<style>@keyframes x{from{background:black}to{background:white}}body{animation:x 2s infinite alternate linear}</style>

This animation happens for me during mouse movement over window or some keyboard interaction with chrome that produces own repaints - only then the colour of the viewport (smoothly) changes. If nothing moves, colour remains the same (This makes this testcase pretty "interactive".) Also, there is noticeable "jump" after end of the movement: colour transitions stops, then remains for circa half of second and then jumps further and stops.

Could this be manifestations of this bug?

Comment 6

[Michał Gołębiowski-Owczarek [:mgol]]

Should it be marked as fixed because of Stylo being on by default in 57?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

This is due to bug 1166500.  I should have noticed earlier.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

All property values in keyframes in the test case are the same, so they don't produce any change hints and the target element is out of view initially. But changing non-animation style to the initial animation style (left: -9999em -> left: 1em) produces a change hint, so we should not throttle restyle but we unfortunately do because the animation has a positive delay.

A solution I can think of is to not throttle out-of-view animations if the animation just starts in-effect or ends the in-effect.

Here is a try with the solution.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9288cb8a641159afa6ca7981c34382bd480f22e

Another solution is to calculate cumulative change hints between the base style and keyframe values, but it's inefficient because we don't generally resolve the base styles for normal 'replace' animation and if we take this approach, we can never throttle out-of-view animations if there is any change hints between the base style and keyframe values even if the animation itself does not produce any change hints which cause layout change.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

A new try with slightly revised patch (check the previous in-effect state in CanThrottle directly);
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d6f10f4bd8bb8bfd6526bada5fdc8144bfa93e4

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1383239 - Don't throttle non-visible changes involved animations on out-of-view elements when they are newly in-effect.

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

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8920413 [details]
Bug 1383239 - Don't throttle non-visible changes involved animations on out-of-view elements when they are newly in-effect.

https://reviewboard.mozilla.org/r/191386/#review196634

Looks good with the following comments addressed. Also, in future please add a commit message explaining what the patch is doing.

::: commit-message-cb2a3:1
(Diff revision 1)
> +Bug 1383239 - Don't throttle non-visible changes involved animations on out-of-view elements if it just stars in effect state. r?birtles

"... on out-of-view elements when they are newly in-effect"

This patch could do with more explanation. It took me a while to work out what it is doing.

::: dom/animation/KeyframeEffectReadOnly.cpp
(Diff revision 1)
> -  // Detect changes to "in effect" status since we need to recalculate the
> -  // animation cascade for this element whenever that changes.
> -  bool inEffect = IsInEffect();
> -  if (inEffect != mInEffectOnLastAnimationTimingUpdate) {
> -    MarkCascadeNeedsUpdate();
> -    mInEffectOnLastAnimationTimingUpdate = inEffect;
> -  }
> -
>    // Request restyle if necessary.
>    if (mAnimation && !mProperties.IsEmpty() && HasComputedTimingChanged()) {
>      EffectCompositor::RestyleType restyleType =
>        CanThrottle() ?
>        EffectCompositor::RestyleType::Throttled :
>        EffectCompositor::RestyleType::Standard;
>      RequestRestyle(restyleType);
>    }
>  
> +  // Detect changes to "in effect" status since we need to recalculate the
> +  // animation cascade for this element whenever that changes.
> +  bool inEffect = IsInEffect();
> +  if (inEffect != mInEffectOnLastAnimationTimingUpdate) {
> +    MarkCascadeNeedsUpdate();
> +    mInEffectOnLastAnimationTimingUpdate = inEffect;
> +  }

We need to document the ordering dependency here that we need to call CanThrottle before updating mInEffectOnLastAnimationTimingUpdate.

::: dom/animation/KeyframeEffectReadOnly.cpp:1378
(Diff revision 1)
>      // In either case we can throttle the animation because there is no
>      // need to update on the main thread.
>      return true;
>    }
>  
> -  // We can throttle the animation if the animation is paint only and
> +  // If we are not just starting in-effect state, we can throttle the animation

Unless we are newly in-effect, we can ...

::: layout/reftests/css-animations/animation-initially-out-of-view-with-delay.html:11
(Diff revision 1)
> + position: absolute;
> + left: -9999px;
> + background: orange;
> + width: 20px;
> + height: 20px;
> + animation: anim 100s 0.1s infinite;

Does this test fail consistently without the fix?

According to comment 0, the bug doesn't reproduce if the the delay is lower than 1s.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

> :::
> layout/reftests/css-animations/animation-initially-out-of-view-with-delay.
> html:11
> (Diff revision 1)
> > + position: absolute;
> > + left: -9999px;
> > + background: orange;
> > + width: 20px;
> > + height: 20px;
> > + animation: anim 100s 0.1s infinite;
> 
> Does this test fail consistently without the fix?
> 
> According to comment 0, the bug doesn't reproduce if the the delay is lower
> than 1s.

Oh, I forgot about the comment. Actually I did run it just once, now I tried to run 5 times on debug builds, all runs failed. So it reliably fails?

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Oh, I forgot about the comment. Actually I did run it just once, now I tried
> to run 5 times on debug builds, all runs failed. So it reliably fails?

That's good enough for me!

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3c6692a2fb58c0e936f3910ed39321bdec91d4

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8920413 [details]
Bug 1383239 - Don't throttle non-visible changes involved animations on out-of-view elements when they are newly in-effect.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a477904ed4f
Don't throttle non-visible changes involved animations on out-of-view elements when they are newly in-effect. r=birtles

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7a477904ed4f