Closed Bug 1383239 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: wisniewskit, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [fixed by stylo])

Attachments

(3 files)

Attached file 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.
Attached file 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
Interesting. It works fine on stylo.
Confirmed that on Windows too this reproduces, but not when Stylo is enabled.
Priority: -- → P3
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?
Should it be marked as fixed because of Stylo being on by default in 57?
Whiteboard: [fixed by stylo]
This is due to bug 1166500.  I should have noticed earlier.
Assignee: nobody → hikezoe
Blocks: 1166500, 1303235
Status: NEW → ASSIGNED
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.
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 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.
Attachment #8920413 - Flags: review?(bbirtles) → review+
(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?
(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!
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7a477904ed4f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.