Set up CSS animations using Keyframes instead of AnimationProperty objects

RESOLVED FIXED in Firefox 48

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments)

58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
11.35 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8736183 [details]
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/
Attachment #8736183 - Flags: review?(cam)
Attachment #8736185 - Flags: review?(cam)
Attachment #8736186 - Flags: review?(cam)
Attachment #8736187 - Flags: review?(cam)
Attachment #8736188 - Flags: review?(cam)
Attachment #8736189 - Flags: review?(cam)
Attachment #8736190 - Flags: review?(cam)
Attachment #8736191 - Flags: review?(cam)
(Assignee)

Comment 2

2 years ago
Created attachment 8736184 [details]
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/
(Assignee)

Comment 3

2 years ago
Created attachment 8736185 [details]
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/
(Assignee)

Comment 4

2 years ago
Created attachment 8736186 [details]
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/
(Assignee)

Comment 5

2 years ago
Created 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

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/
(Assignee)

Comment 6

2 years ago
Created attachment 8736188 [details]
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/
(Assignee)

Comment 7

2 years ago
Created attachment 8736189 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8736190 [details]
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/
(Assignee)

Comment 9

2 years ago
Created attachment 8736191 [details]
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/
(Assignee)

Comment 10

2 years ago
Created attachment 8736195 [details] [diff] [review]
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.
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1260976
(Assignee)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
Created attachment 8736602 [details]
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/
Attachment #8736602 - Flags: review?(cam)
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Comment 15

2 years ago
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/
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
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/
Attachment #8736602 - Attachment description: MozReview Request: Bug 1260655 - Add a copy constructor to Keyframe → MozReview Request: Bug 1260655 - Add a copy constructor and copy assignment operator to Keyframe; r?heycam
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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 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?
Attachment #8736183 - Flags: review?(cam) → review+
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
Attachment #8736185 - Flags: review?(cam) → review+
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
Attachment #8736186 - Flags: review?(cam) → review+
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
Attachment #8736187 - Flags: review?(cam) → review+
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
Attachment #8736188 - Flags: review?(cam) → review+
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?
Attachment #8736602 - Flags: review?(cam) → review+
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.
Attachment #8736189 - Flags: review?(cam) → review+
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?
Attachment #8736190 - Flags: review?(cam)
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
Attachment #8736191 - Flags: review?(cam) → review+
(Assignee)

Comment 31

2 years ago
(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.
(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)?
(Assignee)

Comment 33

2 years ago
(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 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.
Attachment #8736190 - Flags: review+
(Assignee)

Updated

2 years ago
Blocks: 1263491
(Assignee)

Comment 35

2 years ago
(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.
(Assignee)

Updated

2 years ago
Blocks: 1263500
(Assignee)

Comment 36

2 years ago
(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.
(Assignee)

Comment 37

2 years ago
(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.
(Assignee)

Comment 38

2 years ago
(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?
Blocks: 1235002
(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.
(Assignee)

Comment 42

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1064937

Updated

2 years ago
Depends on: 1264396
Blocks: 1264787

Updated

2 years ago
Depends on: 1264800

Updated

2 years ago
Depends on: 1268858
Depends on: 1279819
You need to log in before you can comment on or make changes to this bug.