1260655 - Set up CSS animations using Keyframes instead of AnimationProperty objects

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

Splitting this off from the work in bug 1245748 because that bug is getting too big and MozReview can't cope with landing in stages.

Comment 1

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Add KeyframeEffectReadOnly::SetFrames; r?heycam

Earlier in this patch series we divided keyframe processing into two stages:

  (1) Turning javascript objects into an array of Keyframe objects
  (2) Calculating AnimationProperty arrays from the Keyframe objects

This patch creates a SetFrames method so that CSS animations and
CSS transitions can skip (1) and pass the frames constructed from CSS syntax
into (2).

It also adds the following additional processing:

a. Notifying animation mutation observers when the set of frames has changed.

   This is currently performed by nsAnimationManager but ultimately we should
   encapsulate this logic inside the effect itself. Furthermore, it will be
   needed when we implement effect.setFrames() (i.e. the Javascript-facing
   wrapper for this method).

b. Preserving the mWinsInCascade and mIsRunningOnCompositor state on properties
   when updating them.

   This is currently performed by:

      bool KeyframeEffectReadOnly::UpdateProperties(
          const InfallibleTArray<AnimationProperty>& aProperties)

   which is what nsAnimationManager currently uses. We will ultimately remove
   the above method and here we are just moving this code to the new version
   of UpdateProperties.

c. Requesting a restyle when the set of AnimationProperty objects has changed.

   Again, this is currently performed by the existing UpdateProperties method
   so we are just moving it here. This behavior will also be required when
   we implement effect.setFrames() and when we call UpdateProperties from
   elsewhere in restyling code.

   This is bug 1235002 but we fix it here and leave that bug to just do further
   cleanup work (e.g. re-instating the check for an empty property set before
   requesting a restyle in NotifyAnimationTimingUpdated).

d. Marking the cascade as needing an update when the set of AnimationProperty
   objects has changed.

   This is in preparation for calling UpdateProperties from elsewhere in
   restyling (e.g. when the nsStyleContext is updated).

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

Comment 2

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Wrap lines in keyframe-effect/constructor.html to 80 chars; r=whitespace-only

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

Comment 3

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam

These types have been removed from the normative part of the Web Animations
spec.

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

Comment 4

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam

Before switching CSS animations over to using KeyframeEffectReadOnly::SetFrames
we update the getFrames() API to return the set frame objects (when available)
so that we can test that we are setting the correct frames.

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

Comment 5

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam

When we go to switch CSS Animations over to using
KeyframeEffectReadOnly::SetFrames we will need a way to represent any filled-in
from/to values as nsCSSValue objects. These objects are built from the current
computed style. We currently use StyleAnimationValue::ExtractComputedValue for
this which returns a StyleAnimationValue. In order to convert this to an
nsCSSValue we can use StyleAnimationValue::UncomputeValue. However, in some
cases, the nsCSSValue objects returned by that method are dependent on the
passed-in StyleAnimationValue object.

This patch adds an overload to UncomputeValue that takes an rvalue
StyleAnimationValue reference and produces an nsCSSValue that is independent
of the StyleAnimationValue through a combination of copying data and
transferring ownership of data.

This patch also adjusts the return value for the case of filter and shadow
lists when the list is empty so that we return a none value in this case.
These are the only list types which are allowed to have a null list value.
Not only does this produce the correct result when these values are serialized
(the initial value for 'filter', 'text-shadow', and 'box-shadow' is 'none') it
also means that UncomputeValue should never return an nsCSSValue whose unit is
null which is important because when we later pass that value to BuildStyleRule
it will treat a null nsCSSValue as an error case (specifically, "longhand failed
to parse").

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

Comment 6

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam

This is needed in order to use std::stable_sort with this type since some
implementations of std::stable_sort require this (as opposed to simply a move
constructor).

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

Comment 7

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam

We will call this method in the next patch in this series.

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

Comment 8

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam

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

Comment 9

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam

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

Comment 10

[Brian Birtles (:birtles)]

Attachment: Add a test function to simulate bad keyframe values passed to SetFrames

I wrote this test function because I was worried about what would happen if
we got bad values from CSS (e.g. of ExtractComputedValue failed). Having
written this patch, however, I'm not sure I want to land it.

We either have to disable a bunch of assertions (which, to be fair, we will
eventually drop anyway) or write some macro that conditionally turns them
off depending on some debug flag we pass around. And, having confirmed that the
test passes and nothing blows up I'm less worried about horrible things
happenning if we get bad values from CSS.

Comment 11

[Brian Birtles (:birtles)]

The other thing we might want to consider here is whether we should really animate 'display' or not. With these patches it will work, but setting to display:none will cancel animations. It seems like that could generate some bad recursive behavior but I haven't yet succeeded in producing any (e.g. http://codepen.io/anon/pen/yOooOg).

Maybe it's ok? CSS Transitions and CSS Animations don't say you can't animate display, but I believe Chrome doesn't allow it.

Comment 12

[Brian Birtles (:birtles)]

Forgive the oncoming MozReview spam (I've been pushing to get that bug fixed, but apparently it's going to be difficult). I need to add another patch into this series since apparently Mac/Android want a copy constructor for Keyframe.

Comment 13

[Brian Birtles (:birtles)]

Attachment: MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam

It turns out that std::stable_sort on Mac and Android use this.

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

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8736189 [details]
MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam

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

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8736190 [details]
MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam

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

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8736191 [details]
MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam

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

Comment 17

[Brian Birtles (:birtles)]

Sorry, more spam coming. Looks like Mac/Android what a copy assignment operator too. I'll use a follow-up bug to add a move constructor for nsCSSValue and PropertyValuePair but if Mac/Android are copying these data structures with std::stable_sort, maybe I need to introduce another wrapper type to store the initial index and use our non-stable sort with a comparator that references the index.

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8736602 [details]
MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam

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

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8736189 [details]
MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam

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

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8736190 [details]
MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam

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

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8736191 [details]
MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam

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

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8736183 [details]
MozReview Request: Bug 1260655 - Add KeyframeEffectReadOnly::SetFrames; r?heycam

https://reviewboard.mozilla.org/r/43161/#review41899

::: dom/animation/KeyframeEffect.cpp:483
(Diff revision 1)
> +  if (mAnimation) {
> +    nsNodeUtils::AnimationChanged(mAnimation);
> +  }

Do we need to check mAnimation->IsRelevant() before calling AnimationChanged here?  Or can we never get in here if IsRelevant() would return false?

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8736185 [details]
MozReview Request: Bug 1260655 - Update keyframe-effect/constructor.html to no longer refer to PropertyIndexedKeyframes or Keyframe; r?heycam

https://reviewboard.mozilla.org/r/43165/#review41903

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8736186 [details]
MozReview Request: Bug 1260655 - Return the stored Keyframe objects from GetFrames, when available ; r?heycam

https://reviewboard.mozilla.org/r/43167/#review41905

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8736187 [details]
MozReview Request: Bug 1260655 - Allow StyleAnimationValue::UncomputeValue to produce values whose storage is independent of the passed-in computed value; r?heycam

https://reviewboard.mozilla.org/r/43169/#review41907

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8736188 [details]
MozReview Request: Bug 1260655 - Add an assignment operator to Keyframe that takes an rvalue reference; r?heycam

https://reviewboard.mozilla.org/r/43171/#review41909

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8736602 [details]
MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam

https://reviewboard.mozilla.org/r/43445/#review41911

r=me but is this an indication that stable_sort will do a bunch of copying that we would prefer to avoid, on those platforms?

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8736189 [details]
MozReview Request: Bug 1260655 - Add methods to CSSAnimationBuilder to build a set of Keyframe objects; r?heycam

https://reviewboard.mozilla.org/r/43173/#review41913

::: layout/style/nsAnimationManager.cpp:917
(Diff revision 3)
> +                                          const nsCSSKeyframesRule* aRule)
> +{
> +  // Ideally we'd like to build up a set of Keyframe objects that more-or-less
> +  // reflects the keyframes as-specified in the @keyframes rule(s). However,
> +  // that proves to be difficult because the way CSS declarations are processed
> +  // differs from how we are able to represent keyframes as Javascript objects.

Nit: "JavaScript" (and below).

::: layout/style/nsAnimationManager.cpp:1055
(Diff revision 3)
> +      existingKeyframe->
> +        mPropertyValues.AppendElements(Move(uniquePropertyValues));

Note, if you haven't realised (since it's not obvious), but the only time this will move elements and not copy them is if mPropertyValues is empty (in which case it swaps the arrays).

::: layout/style/nsAnimationManager.cpp:1087
(Diff revision 3)
> +  RefPtr<nsStyleContext> keyframeRuleContext =
> +    mResolvedStyles.Get(aPresContext, mStyleContext,
> +                        aKeyframeRule->Declaration());

Maybe move this into the if statement?

::: layout/style/nsAnimationManager.cpp:1104
(Diff revision 3)
> +
> +  return result;
> +}
> +
> +static Maybe<ComputedTimingFunction>
> +ConvertTimingFunction(const nsTimingFunction& aTimingFunction) {

Nit: brace on new line.

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8736190 [details]
MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam

https://reviewboard.mozilla.org/r/43175/#review41915

::: layout/style/nsAnimationManager.cpp:333
(Diff revision 3)
>  
>    return nullptr;
>  }
>  
>  static void
>  UpdateOldAnimationPropertiesWithNew(

Should we rename this method now that it's working on Keyframes?

::: layout/style/nsAnimationManager.cpp:349
(Diff revision 3)
>    if (aOld.GetEffect()) {
>      KeyframeEffectReadOnly* oldEffect = aOld.GetEffect();
>      animationChanged =
>        oldEffect->SpecifiedTiming() != aNewTiming;
>      oldEffect->SetSpecifiedTiming(aNewTiming);
> -    animationChanged |=
> +    oldEffect->SetFrames(Move(aNewFrames), aStyleContext);

Why do we no longer have to update animationChanged?

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8736191 [details]
MozReview Request: Bug 1260655 - Drop some no-longer-needed code for setting up CSS animations using AnimationProperty objects; r?heycam

https://reviewboard.mozilla.org/r/43177/#review41917

Comment 31

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #29)
> Comment on attachment 8736190 [details]
> MozReview Request: Bug 1260655 - Use
> CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using
> Keyframe objects; r?heycam
> 
> https://reviewboard.mozilla.org/r/43175/#review41915
> 
> ::: layout/style/nsAnimationManager.cpp:333
> (Diff revision 3)
> >  
> >    return nullptr;
> >  }
> >  
> >  static void
> >  UpdateOldAnimationPropertiesWithNew(
> 
> Should we rename this method now that it's working on Keyframes?
> 
> ::: layout/style/nsAnimationManager.cpp:349
> (Diff revision 3)
> >    if (aOld.GetEffect()) {
> >      KeyframeEffectReadOnly* oldEffect = aOld.GetEffect();
> >      animationChanged =
> >        oldEffect->SpecifiedTiming() != aNewTiming;
> >      oldEffect->SetSpecifiedTiming(aNewTiming);
> > -    animationChanged |=
> > +    oldEffect->SetFrames(Move(aNewFrames), aStyleContext);
> 
> Why do we no longer have to update animationChanged?

Because SetFrames does the work in the case of a change to the frames.

Comment 32

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles, away 13 April) from comment #31)
> Because SetFrames does the work in the case of a change to the frames.

Does that mean we can end up calling nsNodeUitls::AnimationChanged twice (and is that OK due to coalescing)?

Comment 33

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #32)
> (In reply to Brian Birtles (:birtles, away 13 April) from comment #31)
> > Because SetFrames does the work in the case of a change to the frames.
> 
> Does that mean we can end up calling nsNodeUitls::AnimationChanged twice
> (and is that OK due to coalescing)?

Right. I plan to file a follow-up bug to make SetSpecifiedTiming etc. also do observer notifications so we can remove animationChanged altogether.

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8736190 [details]
MozReview Request: Bug 1260655 - Use CSSAnimationBuilder::BuildAnimationFrames to set up CSS animations using Keyframe objects; r?heycam

https://reviewboard.mozilla.org/r/43175/#review41929

OK, thanks for the clarification.

Comment 35

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #22)
> > +  if (mAnimation) {
> > +    nsNodeUtils::AnimationChanged(mAnimation);
> > +  }
> 
> Do we need to check mAnimation->IsRelevant() before calling AnimationChanged
> here?  Or can we never get in here if IsRelevant() would return false?

No, I think you're right, I'll add the IsRelevant() check here.

Comment 36

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #27)
> Comment on attachment 8736602 [details]
> MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment
> operator to Keyframe; r?heycam
> 
> https://reviewboard.mozilla.org/r/43445/#review41911
> 
> r=me but is this an indication that stable_sort will do a bunch of copying
> that we would prefer to avoid, on those platforms?

Yes. I've filed bug 1263500 for this.

Comment 37

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #28)
> ::: layout/style/nsAnimationManager.cpp:1055
> (Diff revision 3)
> > +      existingKeyframe->
> > +        mPropertyValues.AppendElements(Move(uniquePropertyValues));
> 
> Note, if you haven't realised (since it's not obvious), but the only time
> this will move elements and not copy them is if mPropertyValues is empty (in
> which case it swaps the arrays).

Oh, I hadn't. That's a shame.

Comment 38

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #29)
> ::: layout/style/nsAnimationManager.cpp:333
> (Diff revision 3)
> >  
> >    return nullptr;
> >  }
> >  
> >  static void
> >  UpdateOldAnimationPropertiesWithNew(
> 
> Should we rename this method now that it's working on Keyframes?

I left this as it was because this method updates both keyframes and timing properties and I figured "animation properties" was a reasonable generic term to cover both?

Comment 39

[Cameron McCormack (:heycam)]

(In reply to Brian Birtles (:birtles, away 13 April) from comment #38)
> I left this as it was because this method updates both keyframes and timing
> properties and I figured "animation properties" was a reasonable generic
> term to cover both?

OK, no problem.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5db982657c9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8272d22371b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/852340c877fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/33b9f509fab7
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a410cc49c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd357fbe9688
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d2df1822a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d846bcdce3e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/063370aa01ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/52c0128aff10

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5db982657c9b
https://hg.mozilla.org/mozilla-central/rev/8272d22371b7
https://hg.mozilla.org/mozilla-central/rev/852340c877fb
https://hg.mozilla.org/mozilla-central/rev/33b9f509fab7
https://hg.mozilla.org/mozilla-central/rev/51a410cc49c8
https://hg.mozilla.org/mozilla-central/rev/bd357fbe9688
https://hg.mozilla.org/mozilla-central/rev/77d2df1822a9
https://hg.mozilla.org/mozilla-central/rev/d846bcdce3e9
https://hg.mozilla.org/mozilla-central/rev/063370aa01ab
https://hg.mozilla.org/mozilla-central/rev/52c0128aff10

Comment 42

[Brian Birtles (:birtles)]

I'm not sure if this actually needs documentation or not, but, as a result of this bug, CSS animations on various enum-type properties will now exhibit the 50% switch behavior described here:

  https://drafts.csswg.org/css-transitions/#step-types

Note that, like Chrome, we don't apply this to CSS transitions (yet, anyway).

The set of properties affected is, I think, all those in [1] with eStyleAnimType_EnumU8, although there may be other cases of properties where certain combinations of values where not interpolable but now exhibit the 50% switch behavior.

Also, there are probably other properties marked as eStyleAnimType_None which should also exhibit this behavior. We'll fix that in bug 1260572 after checking what makes sense and what Chrome/etc. do.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h