Closed Bug 1380258 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: heycam, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

reduced test
833 bytes, text/html
Details
Bug 1380258 - Check corresponding frame in case of pseudo element instead of parent frame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1380258 - Test case for restarting CSS animation on pseudo element once after the pseudo element was re-generated.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1380258 - A reftest for stopping CSS animations on discarded pseudo elements.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file 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?
Flags: needinfo?(hikezoe)
Priority: P1 → P2
This may be a duplicate of bug 1379425.
Has STR: --- → yes
Version: unspecified → Trunk
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.
My wild guess is it's related to ::before/::after changes.  Maybe we fail to generate an animation update task in that case?
Ah, bug 1367278 might be the cause of this.  The bug is assigned to me...
(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.
Flags: needinfo?(hikezoe)
Oops, sorry I did check different point. The animation on pseudo element should be discarded when setting className to "".
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?
I think I should take this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
This is related to bug 1330815. We should fix both of them altogether.
See Also: → 1330815
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
Attachment #8888671 - Flags: review?(bbirtles) → review+
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 ...
Attachment #8888672 - Flags: review?(bbirtles) → review+
Comment on attachment 8888673 [details]
Bug 1380258 - A reftest for stopping CSS animations on discarded pseudo elements.

https://reviewboard.mozilla.org/r/159706/#review165544
Attachment #8888673 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6903156877e9
https://hg.mozilla.org/mozilla-central/rev/174b43f8eeb9
https://hg.mozilla.org/mozilla-central/rev/61f7864e2701
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: