1380258 - stylo: site issue: FastMail refresh animation continues running after it shouldn't

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Cameron McCormack (:heycam)]

Attachment: reduced test

In FastMail, there is an animated, rotating arrow that is shown next to a mail folder, when mail is being fetched.  Hovering over the mail folder should show the arrow icon without an animation, if that folder is not currently being updated.  If this animation starts, then stops, and then I hover over the mail folder, the arrow is shown animating when it shouldn't.

Attached is a reduced test.

Hiro, is this something you could look into?

Comment 1

[Darkspirit]

This may be a duplicate of bug 1379425.

Comment 2

[Cameron McCormack (:heycam)]

It might be, although I just tried adjusting the test to animate a property that can't be animated on the compositor instead (font-size) and still encountered the bug, so if bug 1379425 only manifests when a compositor animation is involved, it might be unrelated.

Comment 3

[Cameron McCormack (:heycam)]

My wild guess is it's related to ::before/::after changes.  Maybe we fail to generate an animation update task in that case?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Ah, bug 1367278 might be the cause of this.  The bug is assigned to me...

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Ah, bug 1367278 might be the cause of this.  The bug is assigned to me...

It seems not.  The pseudo element is not traversed after setting class='y'.

      <div id=v100> (0x7f209c87e160) dd=true data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x7f2098f44ba0 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } })
        <text node> (0x7f2093703d00)
        <span> (0x7f209c87e1f0) dd=false data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x7f2090dc6d80 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } })
          <text node> (0x7f2093703e00)
        <text node> (0x7f2093703e80)
        <div id=target class="y"> (0x7f209c87e280) dd=false data=Some(ElementData { styles: ElementStyles { primary: Some(ComputedValues { rules: Some(StrongRuleNode { p: 0x7f2098f44ba0 }), .. }), pseudos: EagerPseudoStyles(None) }, restyle: RestyleData { hint: , flags: , damage: GeckoRestyleDamage(nsChangeHint(0)) } })
        <text node> (0x7f2093703f00)
      <text node> (0x7f209c8e1800)

id=target element does not have EagerPseudoStyles and there is no _moz_generated_content_before element either.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Oops, sorry I did check different point. The animation on pseudo element should be discarded when setting className to "".

Comment 7

[Hiroyuki Ikezoe (:hiro)]

This is somewhat related to bug 1367278.  We set ElementHasAnimations flag to the parent element in case of pseudo elements. Also in the test case we call Element::UnbindFromTree() for the pseudo element, yes unfortunately we check MayHaveAnimations() for the pseudo element not for the parent [1].

[1] https://hg.mozilla.org/mozilla-central/file/03bcd6d65af6/dom/base/Element.cpp#l1904

Interestingly, gecko also calls UnbindFromTree() for the pseudo element, it means gecko stops animations on the pseudo elements somewhere else?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I think I should take this.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

This is related to bug 1330815. We should fix both of them altogether.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Did not run on stylo.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37251822e9d468e4bd2f1c8ff02993b2c3ad1f80

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1380258 - Check corresponding frame in case of pseudo element instead of parent frame.

In case of pseudo element, AnimationsWithDestroyedFrame holds the parent
element instead of generated pseudo content, so the primary frame of the
holding content is not the primary frame for pseudo elements. We need to
check correct primary frame respectively.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated.

This test case picked from a patch for bug 1330815.
This test fails without the first patch.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1380258 - A reftest for stopping CSS animations on discarded pseudo elements.

This reftest fails without the first patch on stylo.

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

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8888671 [details]
Bug 1380258 - Check corresponding frame in case of pseudo element instead of parent frame.

https://reviewboard.mozilla.org/r/159702/#review165538

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8888672 [details]
Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated.

https://reviewboard.mozilla.org/r/159704/#review165540

::: commit-message-047f2:3
(Diff revision 1)
> +Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated. r?birtles
> +
> +This test case picked from a patch for bug 1330815.

(Not sure we need this sentence.)

::: commit-message-047f2:4
(Diff revision 1)
> +Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated. r?birtles
> +
> +This test case picked from a patch for bug 1330815.
> +This test fails without the first patch.

(This might be a little more clear as "first patch in this series" or something like that)

::: dom/animation/test/mozilla/file_hide_and_show.html:182
(Diff revision 1)
> +    // and rAF callbacks are processed before re-framing process, so waiting for
> +    // an rAF callback is not sufficient.

Nit: ... waiting for one rAF callback ...

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8888673 [details]
Bug 1380258 - A reftest for stopping CSS animations on discarded pseudo elements.

https://reviewboard.mozilla.org/r/159706/#review165544

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Thank you!
A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84aa7ba0e2f5c5b371d6cda921770c8888ff76cf

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888671 [details]
Bug 1380258 - Check corresponding frame in case of pseudo element instead of parent frame.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888672 [details]
Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8888673 [details]
Bug 1380258 - A reftest for stopping CSS animations on discarded pseudo elements.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6903156877e9
Check corresponding frame in case of pseudo element instead of parent frame. r=birtles
https://hg.mozilla.org/integration/autoland/rev/174b43f8eeb9
Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated. r=birtles
https://hg.mozilla.org/integration/autoland/rev/61f7864e2701
A reftest for stopping CSS animations on discarded pseudo elements. r=birtles

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6903156877e9
https://hg.mozilla.org/mozilla-central/rev/174b43f8eeb9
https://hg.mozilla.org/mozilla-central/rev/61f7864e2701

Comment 25

[Darkspirit]

attachment 8885610 [details]: "The blue box should spin once, disappear, then re-appear without spinning."
Exactly this happens in Nightly 56 x64 20170725100346 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480).