771367 - Consider doing css animations of pseudoelements on the compositor

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Probably an important content use case, but I don't have a good feel for the frequency various properties are animated "in the wild".  We've stepped on this in gaia at least.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: support compositor animation for pseudo elements

This patch makes the animation manager both find the requisite style frame when a collection animates a pseudo-element, and it also sets up the animation properties on the pseudo-element so that any layer/layout code looking for animations will find them there without having to rummage around.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

At the very least, this could use some tests (e.g., in layout/style/test/test_animations_omta.html, maybe just by porting existing tests from test_animations.html).

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: support compositor animation for pseudo elements

This revision of the patch attempts to synchronize the pseudo-element's animationsProperty a bit more zealously by calling EnsurePseudoProperty inside GetAnimations, so that while the layout building code is probing for pseudo-element styles during pseudo-element creation, it will update the property.

While investigating support for testing, it was discovered the "mCalledPropertyDtor" stuff was unsafe - both because the existing direct call within CommonAnimationManager::GetAnimations to "delete collection" on SetProperty triggered assertion failures while debugging, and the property list code required the same dtor function to be supplied for elements (previous patch tried to use a null dtor for pseudo-elements, which triggered various assertions). Therefor, it seemed just using reference counting to track which nodes refer to the collection and handle cleanup was the safest/best of all options to fix these issues.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: update test_animations_omta.html to support testing pseudo-elements

This ports a test for animating pseudo-elements from test_animations.html over to test_animations_omta.html. To make this work, several bits of the test harness had to be updated to support querying info about pseudo-elements.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: support compositor animation for pseudo elements

Remove some accidental debugging code

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: support compositor animation for pseudo elements

As discussed, this version of the patch does "The Right Thing" it - it doesn't do any duplication of properties like the previous patches, but rather indirects onto the original parent element of the pseudo-element to find any animation info.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8624701 [details] [diff] [review]
support compositor animation for pseudo elements

So this would have been better split into multiple patches making the
different API changes that you're making; it would have been substantially
easier to review that way.  (Now that it's done, though, please leave it
as-is.)

AnimationCommon.cpp:

>+    animProp =
>+        content->NodeInfo()->NameAtom() == nsGkAtoms::mozgeneratedcontentbefore ?
>+          GetAnimationsBeforeAtom() :
>+          GetAnimationsAfterAtom();

This isn't strict enough.

You need to explicitly check for both mozgeneratecontentbefore and
mozgeneratedcontentafter, and return null if the NameAtom() is something
other than one of those two.


>+    content = parent->GetContent();

Given that we do interesting things to the frame tree in some cases,
I think this should instead be:

    content = content->GetParent();

>+    if (!content->MayHaveAnimations())
>+      return nullptr;

Given that you're reindenting this, please add {}.

>-      delete collection;
>+      AnimationCollection::PropertyDtor(aElement, propName, collection, nullptr);

I'd prefer to leave this as-is.  Why were you changing it?



nsLayoutUtils.cpp:

>+GetAnimationContent(const nsIFrame* aFrame,
>+                    nsIContent* &outContent,
>+                    nsCSSPseudoElements::Type &outPseudoType)

Please rename outContent to aContent or aContentResult, and outPseudoType
to aPseudoType or aPseudoTypeResult.

Please also expose this as a public method on nsLayoutUtils and use it
from CommonAnimationManager::GetAnimationsForCompositor, which has code
that's almost identical (but will need to convert pseudo types to atoms,
which is fine).  And please apply the review comments that I made above,
on that code, here.

GetAnimationCollection could perhaps be gotten rid of (in favor of
calling the methods on the transition manager or animation manager), but
it's ok.  (If you want to, please do it as a separate followup patch.)



r=dbaron with that

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8623974 [details] [diff] [review]
update test_animations_omta.html to support testing pseudo-elements

Please rev the IID of nsIDOMWindowUtils (i.e., use uuidgen to generated a
new IID).

(It's possible birtles might have something else to say about the test_animations_omta.html changes, but they look ok to me, and I don't think you need to wait for him.)

r=dbaron, and thanks for fixing this

Comment 10

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

Just a few nits:

* In the main patch we're pushing a lot of logic into places like nsLayoutUtils.cpp that really should be handled by the various managers. In particular, all the mapping to different atoms etc. We should tidy that up in a follow-up bug and move as much of that logic as possible back to layout/style.

* Putting the pseudo argument at the end of omta_is etc. seems odd to me. The desc parameter should go last. Maybe it's ok but it would be neat if we could detect the number of arguments and allow the pseudo to appear before the desc.

* In the test patch there are a few lines over 80 characters that need to be wrapped.

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: part 1 - support compositor animation for pseudo elements

Rebased and fixed bitrot.

Exposed nsLayoutUtils::GetAnimationContent so that CommonAnimationManager::GetAnimationsForCompositor can use it.

Fixed animProp detection to check for both mozgeneratedcontentbefore and mozgeneratedcontentafter.

Fixed content parent to use content->GetParent() instead of parent->GetContent().

Added comment to usage of PropertyDtor in failure case to make it evident why this must be done instead of just deleting the collection directly.

Fixed MayHaveAnimations() bracketing.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: part 2 - update test_animations_omta.html to support testing pseudo-elements

Fixed line lengths in patch to respect 80 character limit.

Comment 13

[Lee Salzman [:lsalzman]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #8)
> Comment on attachment 8624701 [details] [diff] [review]
> support compositor animation for pseudo elements
> 
> So this would have been better split into multiple patches making the
> different API changes that you're making; it would have been substantially
> easier to review that way.  (Now that it's done, though, please leave it
> as-is.)
> 
> AnimationCommon.cpp:
> 
> >+    animProp =
> >+        content->NodeInfo()->NameAtom() == nsGkAtoms::mozgeneratedcontentbefore ?
> >+          GetAnimationsBeforeAtom() :
> >+          GetAnimationsAfterAtom();
> 
> This isn't strict enough.
> 
> You need to explicitly check for both mozgeneratecontentbefore and
> mozgeneratedcontentafter, and return null if the NameAtom() is something
> other than one of those two.
> 
> 
> >+    content = parent->GetContent();
> 
> Given that we do interesting things to the frame tree in some cases,
> I think this should instead be:
> 
>     content = content->GetParent();
> 
> >+    if (!content->MayHaveAnimations())
> >+      return nullptr;
> 
> Given that you're reindenting this, please add {}.
> 
> >-      delete collection;
> >+      AnimationCollection::PropertyDtor(aElement, propName, collection, nullptr);
> 
> I'd prefer to leave this as-is.  Why were you changing it?
> 
> 
> 
> nsLayoutUtils.cpp:
> 
> >+GetAnimationContent(const nsIFrame* aFrame,
> >+                    nsIContent* &outContent,
> >+                    nsCSSPseudoElements::Type &outPseudoType)
> 
> Please rename outContent to aContent or aContentResult, and outPseudoType
> to aPseudoType or aPseudoTypeResult.
> 
> Please also expose this as a public method on nsLayoutUtils and use it
> from CommonAnimationManager::GetAnimationsForCompositor, which has code
> that's almost identical (but will need to convert pseudo types to atoms,
> which is fine).  And please apply the review comments that I made above,
> on that code, here.
> 
> GetAnimationCollection could perhaps be gotten rid of (in favor of
> calling the methods on the transition manager or animation manager), but
> it's ok.  (If you want to, please do it as a separate followup patch.)
> 
> 
> 
> r=dbaron with that

Fixed all issues in patch update. With respect to PropertyDtor, the problem is AnimationCollection checks if mCalledPropertyDtor is true in an assertion inside its destructor. Thus, if SetProperty fails, this would have always triggered the assertion, and indeed was triggered while I was writing and debugging the patch. The best way to resolve the issue, IMO, was to just call into PropertyDtor to destroy the collection in this corner-case, so everything lines up implicitly. I added a comment to the patch to make this clear.

Comment 14

[Lee Salzman [:lsalzman]]

Attachment: part 3 - refactor GetAnimationContent and GetAnimationCollection into CommonAnimationManager to hide atom and pseudo-element voodoo

This is a cumulative cleanup of part 1.

It removes GetAnimationContent and GetAnimationCollection from nsLayoutUtils. It replaces them with a version of GetAnimationCollection in CommonAnimationManager that takes in an nsIFrame and spits out the AnimationCollection, hiding all details of atoms and pseudo-elements in between.

All details of mucking with atoms and pseudo-elements are thus gone from nsLayoutUtils. What remains are some small stubs that take in a frame, get an appropriate animation collection from either the animation or transition manager, query some info on it, and return a boolean.

Maybe other things can be cleaned up, but I was just trying to hit the 80/20 side of refactoring with this, within the scope of changes I made as part of this bug. Larger refactoring is maybe better addressed by someone else if desired.

Comment 15

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

Comment on attachment 8628382 [details] [diff] [review]
part 3 - refactor GetAnimationContent and GetAnimationCollection into CommonAnimationManager to hide atom and pseudo-element voodoo

Review of attachment 8628382 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks for doing this.

At some point we need to clarify when "animations" in a function name means "all animations including transitions" versus "only CSS animations". I'll probably do that this quarter as part of bug 1151731 and co.

Comment 16

[Lee Salzman [:lsalzman]]

Try results for all 3 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b93fc6b0c0

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa343e461c43
https://hg.mozilla.org/integration/mozilla-inbound/rev/12729d4f6730
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd98d3d9258

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa343e461c43
https://hg.mozilla.org/mozilla-central/rev/12729d4f6730
https://hg.mozilla.org/mozilla-central/rev/2fd98d3d9258