1439269 - We should call RequestRestyle(Layer) for script animations when the display property for animating element is changed from 'none' to other

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

See bug 1419851 comment 39.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

We can observe this issue if we drop 'waitForNextFrame' here [1].

[1] https://hg.mozilla.org/mozilla-central/file/ad133cd410a7/dom/animation/test/mozilla/file_restyles.html#l1113

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Here is a try with proper fixes that I believe.

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

Note that for old style system, it seems to me that RequestRestyle(Layer) (or equivalent stuff (SchedulePaint?)) is called for the case.  Thought I haven't checked it, I guess it's because of recursive restyling on the old style system.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I did miss the case where CSS animations/transitions might have been destroyed in UpdateAnimations() or UpdateTransitions(), thus an assertion was hit in the previous try.

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1439269 - Factor out a new function to check display property is changed from 'none' to other.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952229 [details]
Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'.

https://reviewboard.mozilla.org/r/221482/#review227298

::: commit-message-24550:2
(Diff revision 1)
> +Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'. r?emilio
> +

I haven't gone through it in detail, but can you elaborate in the commit message what's the failure mode?

That is, why do we need to re-restyle the element when it has script animations?

Is it because we don't run them when it's display: none?

Also, any chance to write a WPT for this? This looks like the kind of thing that every other browser could get wrong, and then be faster just because :)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

ni? for comment 7, that'd make my review much easier tomorrow morning. Thanks for fixing this Hiro!

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Comment on attachment 8952229 [details]
> Bug 1439269 - Add a new sequential task flag for display propery changes
> from 'none'.
> 
> https://reviewboard.mozilla.org/r/221482/#review227298
> 
> ::: commit-message-24550:2
> (Diff revision 1)
> > +Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'. r?emilio
> > +
> 
> I haven't gone through it in detail, but can you elaborate in the commit
> message what's the failure mode?

OK, I will add more message there.

> That is, why do we need to re-restyle the element when it has script
> animations?
> 
> Is it because we don't run them when it's display: none?
> 
> Also, any chance to write a WPT for this? This looks like the kind of thing
> that every other browser could get wrong, and then be faster just because :)

Yeah, I could probably write a reftest for this.  As far as I can tell, without the restyle, we will see flicker when we change the display property from none.  I.e. in the first frame the element is regarded as a non-transformed element, thus no stacking context creates, then in the second frame animation restyling happens, thus new stacking context is created.  Anyway, I haven't yet tried to write the test, it might end up failing though.

Keep NI to me to write the test.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Oops, stacking-context-transform-none-animation-before-appending-element.html[1] is the very test case, but the test does not specify reftest-no-flush so that we haven't observed the flicker at all.  I don't think reftest framework for web platform test has the flag equivalent to reftest-no-flush.

[1] https://hg.mozilla.org/mozilla-central/file/dc70d241f90d/layout/reftests/web-animations/stacking-context-transform-none-animation-before-appending-element.html

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952229 [details]
Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Wrote comments, but gave up writing the test.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952228 [details]
Bug 1439269 - Factor out a new function to check display property is changed from 'none' to other.

https://reviewboard.mozilla.org/r/221480/#review227352

::: servo/components/style/properties/gecko.mako.rs:154
(Diff revision 1)
> +        &self,
> +        old_values: Option<<&ComputedValues>
> +    ) -> bool {
> +        use properties::longhands::display::computed_value::T as Display;
> +
> +        return old_values.map_or(false, |old| {

nit: no need for `return`.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952229 [details]
Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'.

https://reviewboard.mozilla.org/r/221482/#review227354

::: commit-message-24550:7
(Diff revision 2)
> +
> +Unlike CSS animations/transitions, script animations keep alive on display:none
> +elements, so once the display property was changed to others in normal
> +styling, we need to do styling for the script animations in the second
> +animation traversal.  Otherwise, the styling for the script animations will
> +be diferred to the next frame.

nit: `deferred`.

It'd be nice to mention this in the documentation of the flags.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

https://reviewboard.mozilla.org/r/221484/#review227356

::: commit-message-d1778:1
(Diff revision 3)
> +Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations. r?

Did you mean to request review on this one? Maybe Brian is a better reviewer than me for this.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Comment on attachment 8952229 [details]
> Bug 1439269 - Add a new sequential task flag for display propery changes
> from 'none'.
> 
> https://reviewboard.mozilla.org/r/221482/#review227354
> 
> ::: commit-message-24550:7
> (Diff revision 2)
> > +
> > +Unlike CSS animations/transitions, script animations keep alive on display:none
> > +elements, so once the display property was changed to others in normal
> > +styling, we need to do styling for the script animations in the second
> > +animation traversal.  Otherwise, the styling for the script animations will
> > +be diferred to the next frame.
> 
> nit: `deferred`.
> 
> It'd be nice to mention this in the documentation of the flags.

I will add comment there.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> Comment on attachment 8952230 [details]
> Bug 1439269 - Call RequestRestyle() if the display property is changed from
> 'none' in the case the element has script animations.
> 
> https://reviewboard.mozilla.org/r/221484/#review227356
> 
> ::: commit-message-d1778:1
> (Diff revision 3)
> > +Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations. r?
> 
> Did you mean to request review on this one? Maybe Brian is a better reviewer
> than me for this.

Yeah, Brian doesn't accept any reviews for now. :)  So I just pushed patches without reviewers.

Comment 19

[Brian Birtles (:birtles)]

Yes, sorry I'm travelling this week (and in meetings when I'm not). Feel free to needinfo me for any urgent reviews though.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Brian could you please take time to review this (and bug 1439803 and bug 1439428)?  I've made fixes for these issues that block bug 1419851).

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

https://reviewboard.mozilla.org/r/221484/#review227776

::: dom/animation/test/mozilla/file_restyles.html:1168
(Diff revision 3)
>      ok(SpecialPowers.wrap(animation).isRunningOnCompositor,
>         'Opacity script animations restored from "display: none" should be ' +
> -       'run on the compositor');
> +       'run on the compositor in the next frame');

I don't understand this change. This patch drops the part where we wait a frame.

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

https://reviewboard.mozilla.org/r/221484/#review227778

This would be easier to review with a commit message explaining what is happening but perhaps that's in one of the other patches.

::: dom/animation/test/mozilla/file_restyles.html:1168
(Diff revision 3)
>      ok(SpecialPowers.wrap(animation).isRunningOnCompositor,
>         'Opacity script animations restored from "display: none" should be ' +
> -       'run on the compositor');
> +       'run on the compositor in the next frame');

Oh, I see. observeStyling waits a frame. Never mind.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

https://reviewboard.mozilla.org/r/221484/#review227776

> I don't understand this change. This patch drops the part where we wait a frame.

observeStyling() waits a requestAnimationFrame() actually.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Oops. you earlier.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952229 [details]
Bug 1439269 - Add a new sequential task flag for display propery changes from 'none'.

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

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

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

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Oh, MozReview cleared r+ flag.  I will stamp r+.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221484/diff/4-5/

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8952230 [details]
Bug 1439269 - Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations.

https://reviewboard.mozilla.org/r/221484/#review228068

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8657fae79d0
Add a new sequential task flag for display propery changes from 'none'. r=emilio
https://hg.mozilla.org/integration/autoland/rev/647ee5497377
Call RequestRestyle() if the display property is changed from 'none' in the case the element has script animations. r=birtles

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e8657fae79d0
https://hg.mozilla.org/mozilla-central/rev/647ee5497377