1330815 - CSS animation applied to pseudo-element on hover does not restart after completing once

Status: RESOLVED DUPLICATE of bug 1380258

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

Description

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

As reported here: https://github.com/webcompat/web-bugs/issues/3352

In the test case (https://jsfiddle.net/4tpar15d/) hovering over the link once produces an animation. However, if you let the animation run to completion then re-hover the link, the animation does not run.

This differs from the behavior in Chrome and Edge (which re-run the animation). It also differs from the behavior if you mouseout while the animation is still running, and then later re-hover.

I can see this bug as far back as Firefox 17 (the animation didn't seem to run at all in Firefox 16 which is where we first unprefixed CSS animations) so I think this is not a regression but a bug that has been there all along.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

A possible reason what I can think of is that when hovering out we don't process StopAnimationsForElement() for the pseudo element for :hover because the content, which is the parent element of the :hover, has primary frame there [1]. We should skip the check of the existence of primary frame only for non-pseudo element.

[1] https://hg.mozilla.org/mozilla-central/file/8eaf154b385b/layout/base/RestyleManager.cpp#l224

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330815 - Don't check the existence of the primary frame in case of pseudo element.

In case of animations on pseudo element AnimationsWithDestroyedFrame has the
parent content of the pseudo element so that the primary frame of the parent
content still exists in some cases.
The test case in this patch fails without this fix, passes with the fix.

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Brian, would you mind reviewing this?  Initially I did request it to dbaron, but unfortunately he don't accept review request now.

Comment 4

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

I don't quite understand the commit description:

"In case of animations on pseudo element AnimationsWithDestroyedFrame has the parent content of the pseudo element so that the primary frame of the parent content still exists in some cases."

What does "has" mean here? Any extra explanation would be helpful.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

In case of pseudo element, we store its parent content in AnimationsWithDestroyedFrame [1] instead of the pseudo element itself.

[1] https://hg.mozilla.org/mozilla-central/file/8eaf154b385b/layout/base/RestyleManager.h#l245

So, 'stores' is a proper word there?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Here is a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc64797f77e4bd92b2c1a77b30e08151348e8644

Comment 7

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

Comment on attachment 8826983 [details]
Bug 1330815 - Don't check the existence of the primary frame in case of pseudo element.

https://reviewboard.mozilla.org/r/104776/#review105514

::: dom/animation/test/mozilla/file_hide_and_show.html:12
(Diff revision 1)
>      transform: translateX(100px);
>    }
>  }
>  
> +div.pseudo::before {
> +  animation: move 0.1s;

I guess 0.01s would work too?

::: dom/animation/test/mozilla/file_hide_and_show.html:183
(Diff revision 1)
> +    div.classList.remove('pseudo');
> +
> +    // We need to wait for two frames to process re-framing.
> +    // The callback of 'animationend' is processed just before rAF callbacks,
> +    // and rAF callbacks are processed before re-framing process, so waiting for
> +    // a rAF callback is not sufficient.

Nit: s/a rAF callback/an rAF callback/

::: layout/base/RestyleManager.cpp:224
(Diff revision 1)
> -    if (content->GetPrimaryFrame()) {
> +    if (content->GetPrimaryFrame() &&
> +        aPseudoType == CSSPseudoElementType::NotPseudo) {

I don't understand why this is the correct fix. Isn't the point of StopAnimationsWithoutFrame() to detect when we *don't* have a primary frame and stop the transition in that case?

With this change, the following lines in StopAnimationsForElementsWithoutFrames() are effectively meaningless:

  StopAnimationsWithoutFrame(mBeforeContents, CSSPseudoElementType::before);
  StopAnimationsWithoutFrame(mAfterContents, CSSPseudoElementType::after);

since aPseudoType != CSSPseudoElementType::NotPseudo so we will never run the body of that loop.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #7)
> Comment on attachment 8826983 [details]
> Bug 1330815 - Don't check the existence of the primary frame in case of
> pseudo element.
> 
> https://reviewboard.mozilla.org/r/104776/#review105514
> 
> ::: dom/animation/test/mozilla/file_hide_and_show.html:12
> (Diff revision 1)
> >      transform: translateX(100px);
> >    }
> >  }
> >  
> > +div.pseudo::before {
> > +  animation: move 0.1s;
> 
> I guess 0.01s would work too?
> 
> ::: dom/animation/test/mozilla/file_hide_and_show.html:183
> (Diff revision 1)
> > +    div.classList.remove('pseudo');
> > +
> > +    // We need to wait for two frames to process re-framing.
> > +    // The callback of 'animationend' is processed just before rAF callbacks,
> > +    // and rAF callbacks are processed before re-framing process, so waiting for
> > +    // a rAF callback is not sufficient.
> 
> Nit: s/a rAF callback/an rAF callback/
> 
> ::: layout/base/RestyleManager.cpp:224
> (Diff revision 1)
> > -    if (content->GetPrimaryFrame()) {
> > +    if (content->GetPrimaryFrame() &&
> > +        aPseudoType == CSSPseudoElementType::NotPseudo) {
> 
> I don't understand why this is the correct fix. Isn't the point of
> StopAnimationsWithoutFrame() to detect when we *don't* have a primary frame
> and stop the transition in that case?
> 
> With this change, the following lines in
> StopAnimationsForElementsWithoutFrames() are effectively meaningless:
> 
>   StopAnimationsWithoutFrame(mBeforeContents, CSSPseudoElementType::before);
>   StopAnimationsWithoutFrame(mAfterContents, CSSPseudoElementType::after);
> 
> since aPseudoType != CSSPseudoElementType::NotPseudo so we will never run
> the body of that loop.

OK. We should store the pseudo element itself in AnimationsWithDestroyedFrame and check the primary frame for the pseudo element in the case where the pseudo frame is re-created during restyles.  Also in case of pseudo element we should pass its *parent* element to nsAnimatitionManager::StopAnimationsForElement().

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

> OK. We should store the pseudo element itself in
> AnimationsWithDestroyedFrame and check the primary frame for the pseudo
> element in the case where the pseudo frame is re-created during restyles. 
> Also in case of pseudo element we should pass its *parent* element to
> nsAnimatitionManager::StopAnimationsForElement().

Hmm, I am not sure this is feasible. From a comment in AnimationsWithDestroyedFrame.

    // mBeforeContents and mAfterContents hold the real element rather than
    // the content node for the generated content (which might change during
    // a reframe)

We should find another way.

Comment 10

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

Can't you just keep the data structure as is, and then see if you have a frame for the ::before / ::after generated content as appropriate?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> Can't you just keep the data structure as is, and then see if you have a
> frame for the ::before / ::after generated content as appropriate?

Hmm, I am not sure yet.  Maybe if we have a way to get ::before / ::after generated content from its parent content in StopAnimationsWithoutFrame(). I don't know the way yet. I am concerned that the generated content is still valid there when the primary frame has been destroyed.  Also, even if we can get the generated content there, but I am not sure that we ensure that the generated content is the correct one, I mean, I guess there is a case that a new generated content is created during reframing.

Comment 12

[WM]

And it seems that Firefox does not trigger animationEnd event on the parent of the animated pseudo element.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I am going to fix this in bug 1380258.