Closed Bug 1125455 Opened 9 years ago Closed 9 years ago

rewrite cascading of CSS Transitions and Animations to match current spec

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed
relnote-firefox --- 39+

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(8 files, 1 obsolete file)

patch 1 - Add boolean for whether an animation of a property wins in the CSS cascade
3.11 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 3 - Add mochitests for animations overriding transitions
4.51 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 4 - For main-thread application of transitions, don't apply transitions when animations are also running
5.87 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 5 - Add Animation::GetAnimationOfProperty
2.19 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 6 - Only search the properties list of the animation once when adding animations to the compositor
7.38 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 7 - For compositor-thread application of transitions, don't apply transitions when animations are also running
2.86 KB, patch
birtles
: review+
Details | Diff | Splinter Review
diff -w of patch 6
5.74 KB, patch
Details | Diff | Splinter Review
patch 2 - Set mWinsInCascade for transitions based on whether there are animations
9.66 KB, patch
birtles
: review+
Details | Diff | Splinter Review 
This is split off to cover the remainder of the work originally planned in bug 960465:

As I wrote in bug 960465 comment 0:
> Links with more details:
> 
>   http://lists.w3.org/Archives/Public/www-style/2013Mar/0297.html
>    - my initial spec proposal
> 
>  
> https://groups.google.com/d/msg/mozilla.dev.tech.layout/Mu0z1kqTUR0/S5iQj-
> qGeJQJ
>    - a description of what the above means for Gecko
> 
>   http://lists.w3.org/Archives/Public/www-style/2013Jun/0682.html 
>    - CSS WG resolution to accept my proposal with modified version of (B)
> 
>   http://lists.w3.org/Archives/Public/www-style/2013Nov/0192.html
>    - description of completed spec edits
> 
> This bug will have two big but inter-related pieces (quoting from the second
> link above):
...
>  (2) Changing how animations and transitions work in the cascade.
>  Note that currently compositor-driven animations have different
>  cascading behavior than animations not running on the compositor,
>  which is a relatively serious bug 847287.  I admit I don't have a
>  completely concrete idea about how to do this, but I think it's not
>  going to be all that hard; at a concrete level it might come down
>  to having nsIStyleRules in the cascade at two different places
>  (both at the top of the cascade and at the official location) in
>  order to let the rule safely record which properties the animation
>  or transition is the winning rule for, and then recording that
>  information so that we can use it for the compositor-driven
>  animations.  I don't think this is too hard, though; I just haven't
>  implemented it yet.
So the current state is that our cascading order matches the spec's cascading order:

http://dev.w3.org/csswg/css-cascade/#cascade-origin
http://www.w3.org/TR/2013/CR-css-cascade-3-20131003/#cascade-origin

so I think the only change is to update to the rule in
http://dev.w3.org/csswg/css-transitions/#application that:

Implementations must add this value to the cascade if and only if that property is not currently undergoing a CSS Animation ([CSS3-ANIMATIONS]) on the same element.
Thoughts on how to do both this and bug 847287:

1. This bug could be done with a boolean for each transition of a property on an element.  The value could be obtained from the animation manager.  This could live in ElementPropertyTransition.  (It doesn't have to; we could compute it dynamically.  But it's probably a good idea given the next two points.)

2. bug 847287 for CSS Animations requires a boolean for each animation of a property on an element to decide whether to send animations to the compositor -- whether that animation won in the cascade.  The style set could notify the animation manager when it's about to do cascading for an element, and it could set all of the booleans for that element to false, and then set them to true again 

3. bug 847287 for CSS Transitions requires exactly the boolean from (1) to decide whether to send animations to the compositor.

4. These booleans both only need to be recomputed when style changes (nsTransitionManager::StyleContextChanged or nsAnimationManager::CheckAnimationRule) and whenever an animation on the element starts or stops filling.  Though the mechanism for computing (2) might make it easier to compute it more often.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Note that (at this stage) some of the tests in both files fail (which I
have verified locally), as noted by the todos and FIXMEs in the test,
which will be removed in later patches in this bug, as the failures are
fixed.
Attachment #8576362 - Flags: review?(bbirtles)
I've verified locally that this patch (not others in this series) fixes
the test failures that match the test changes in this patch.
Attachment #8576363 - Flags: review?(bbirtles)
This removes the duplication where AddAnimationsForProperty calls
HasAnimationOfProperty which goes over the list once, and then
AddAnimationForProperty searches the list again and skips all but the
item found before.

It also makes it easier, in patch 7, to perform additional tests on the
item that we found.
Attachment #8576365 - Flags: review?(bbirtles)
I've verified locally that this patch (not others in this series) fixes
the test failures that match the test changes in this patch.
Attachment #8576366 - Flags: review?(bbirtles)
Comment on attachment 8576360 [details] [diff] [review]
patch 1 - Add boolean for whether an animation of a property wins in the CSS cascade

>diff --git a/layout/style/nsTransitionManager.cpp b/layout/style/nsTransitionManager.cpp
>--- a/layout/style/nsTransitionManager.cpp
>+++ b/layout/style/nsTransitionManager.cpp
>@@ -537,16 +537,17 @@ nsTransitionManager::ConsiderStartingTra
>   nsRefPtr<ElementPropertyTransition> pt =
>     new ElementPropertyTransition(aElement->OwnerDoc(), aElement,
>                                   aNewStyleContext->GetPseudoType(), timing);
>   pt->mStartForReversingTest = startForReversingTest;
>   pt->mReversePortion = reversePortion;
> 
>   AnimationProperty& prop = *pt->Properties().AppendElement();
>   prop.mProperty = aProperty;
>+  prop.mWinsInCascade = true;

Just checking I've understood this. We set this unconditionally to true here, but then at the call site, nsTransitionManager::StyleContextChanged, we will later call UpdateCascadeResultsWithTransitions which will cause us to set it to false if necessary.

Assuming I've got that right, r=birtles.
Attachment #8576360 - Flags: review?(bbirtles) → review+
Comment on attachment 8576361 [details] [diff] [review]
patch 2 - Set mWinsInCascade for transitions based on whether there are animations

>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
...
>+  void AddPropertiesToSet(nsCSSPropertySet& aSet)
>+  {
>+    for (uint32_t i = 0, i_end = mPropertyValuePairs.Length(); i < i_end; ++i) {
>+      PropertyValuePair &cv = mPropertyValuePairs[i];
>+      aSet.AddProperty(cv.mProperty);
>+    }
>+  }

This should probably be a const method. nsCSSPropertySet::AddProperty already takes its argument by value so we'd probably only need to make cv a const ref.

Also, I think Length() here returns a size_t. If so, we should probably use that or auto.

>+void
>+nsTransitionManager::UpdateCascadeResults(
>+                       AnimationPlayerCollection* aTransitions,
>+                       AnimationPlayerCollection* aAnimations)
>+{
>+  if (!aTransitions || !aAnimations || !aAnimations->mStyleRule) {
>+    // Nothing to do (although we could check the #ifdef DEBUG assertions)

It took me a moment to understand which assertions this was referring to. It might be more clear if we made it refer to the "assertions below" or just dropped the parenthetical altogether.

>+void
>+nsTransitionManager::UpdateCascadeResults(
>+                       AnimationPlayerCollection* aTransitions,
>+                       AnimationPlayerCollection* aAnimations)
>+{
>+  if (!aTransitions || !aAnimations || !aAnimations->mStyleRule) {
>+    // Nothing to do (although we could check the #ifdef DEBUG assertions)
>+    return;
>+  }
>+
>+  nsCSSPropertySet propertiesUsed;

propertiesWithAnimations?

I don't mind either way, but that would make a little more sense to me.

>diff --git a/layout/style/nsTransitionManager.h b/layout/style/nsTransitionManager.h
...
>+  void UpdateCascadeResultsWithTransitions(
>+         AnimationPlayerCollection* aTransitions);
>+  void UpdateCascadeResultsWithAnimations(
>+         AnimationPlayerCollection* aAnimations);
>+  void UpdateCascadeResults(AnimationPlayerCollection* aTransitions,
>+                            AnimationPlayerCollection* aAnimations);

If we make AddPropertiesToSet const, there would probably be a ripple effect meaning aAnimations could be a const pointer. I'll leave it up to you to decide if changing that is appropriate.

r=birtles
Attachment #8576361 - Flags: review?(bbirtles) → review+
Comment on attachment 8576362 [details] [diff] [review]
patch 3 - Add mochitests for animations overriding transitions

>+// Bug 1125455 - Transitions should not run when animations are running.
>+addAsyncAnimTest(function *() {
>+  advance_clock(0);

We don't actually need this line. runAsyncAnimTest in animation_utils.js already does this for us.

>+  new_div("transition: opacity 2s linear; opacity: 0.8");
>+  yield waitForPaintsFlushed();
>+  omta_is("opacity", 0.8, RunningOn.MainThread,
>+          "initial opacity");
>+  gDiv.style.opacity = "0.2";
>+  yield waitForPaintsFlushed();
>+  omta_is("opacity", 0.8, RunningOn.Compositor,
>+          "opacity transition at 0s");
>+  advance_clock(500);
>+  yield waitForPaintsFlushed();

We shouldn't actually need to wait for paints here. advance_clock() should push the time through to the compositor and force a synchronous update. We should only need to wait for paints when we're expecting an animation (or transition) to start or stop and we need to wait for the corresponding layer transaction to happen.

Apart from that r=me.
Attachment #8576362 - Flags: review?(bbirtles) → review+
Comment on attachment 8576363 [details] [diff] [review]
patch 4 - For main-thread application of transitions, don't apply transitions when animations are also running

>diff --git a/dom/animation/Animation.cpp b/dom/animation/Animation.cpp
...
>+    if (!prop.mWinsInCascade) {
>+      // This isn't the winning declaration, so don't add it to style.
>+      // For transitions, this is important, because it's how we
>+      // implement the rule that CSS transitions don't run when a CSS
>+      // animation is running on the same property and element.  For
>+      // animations, this is only skipping this that will otherwise be
>+      // overridden.

I don't understand the last sentence in this comment.

Apart from that, r=me.
Attachment #8576363 - Flags: review?(bbirtles) → review+
Attachment #8576364 - Flags: review?(bbirtles) → review+
Comment on attachment 8576365 [details] [diff] [review]
patch 6 - Only search the properties list of the animation once when adding animations to the compositor

> static void
>-AddAnimationForProperty(nsIFrame* aFrame, nsCSSProperty aProperty,
>+AddAnimationForProperty(nsIFrame* aFrame, const AnimationProperty* aProperty,
>                         AnimationPlayer* aPlayer, Layer* aLayer,
>                         AnimationData& aData, bool aPending)

Since we never expect aProperty to be null, I think a const ref is better but I'll leave it up to your judgement.

> static void
> AddAnimationsForProperty(nsIFrame* aFrame, nsCSSProperty aProperty,
...
>     AnimationPlayer* player = aPlayers[playerIdx];
>     dom::Animation* anim = player->GetSource();
>-    if (!(anim && anim->HasAnimationOfProperty(aProperty) &&
>-          player->IsRunning())) {
>+    if (!anim) {
>+      continue;
>+    }
>+    const AnimationProperty* property =
>+      anim->GetAnimationOfProperty(aProperty);
>+    if (!property || !player->IsRunning()) {
>       continue;
>     }

Nit: It's seems a little odd to test |property| and |player->IsRunning()| together. Would it make sense to test player->IsRunning() before setting anim?
Attachment #8576365 - Flags: review?(bbirtles) → review+
Comment on attachment 8576366 [details] [diff] [review]
patch 7 - For compositor-thread application of transitions, don't apply transitions when animations are also running

>+    if (!property->mWinsInCascade) {
>+      // We have an animation or transition, but it isn't actually
>+      // winning in the CSS cascade, so we don't want to send it to the
>+      // compositor.
>+      // I believe that anything that changes mWinsInCascade should
>+      // trigger this code again, either because of a restyle that
>+      // changes the properties in question, or because of the
>+      // main-thread style update that results when an animation stops
>+      // filling.
>+      continue;
>+    }

I tried to persuade myself of this too. I think it's right, but there's one case I wonder about. When there's no change in animation value I believe that our restyle processing will filter out the apparently redundant change. If a CSS animation started and happened to produce the same value as an existing transition (perhaps they are both using step timing functions) then I wonder if we might fail to update the compositor. To fix this situation for changes to animations using the API, we bump the animation generation in a couple of places and we make the restyle manager look for changes to the animation generation.

In this case, even though nsTransitionManager::UpdateCascadeResults doesn't change the animation generation, subsequent to calling UpdateCascadeResultsWithTransitions() from nsTransitionManager::StyleContextChanged(), we do so we should be ok there. However, for AnimationPlayerCollection::EnsureStyleRuleFor(), which calls UpdateCascadeResultsWithAnimations(), we don't do that. There may be something else that triggers a layer transaction in that case but it still might be interesting to set up an OMTA test which causes a CSS transition to be overridden with a CSS animation such that they have the same value when the override takes place (but which differ later on so we can confirm that the layer got the updated animation).

What do you think?

r=birtles apart from that
Attachment #8576366 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #12)
> patch 1 - Add boolean for whether an animation of a property wins in the CSS
> cascade
> 
> Just checking I've understood this. We set this unconditionally to true
> here, but then at the call site, nsTransitionManager::StyleContextChanged,
> we will later call UpdateCascadeResultsWithTransitions which will cause us
> to set it to false if necessary.

Yes.

(In reply to Brian Birtles (:birtles) from comment #13)
> patch 2 - Set mWinsInCascade for transitions based on whether there are
> animations
> 
> This should probably be a const method. nsCSSPropertySet::AddProperty
> already takes its argument by value so we'd probably only need to make cv a
> const ref.

Indeed.

> Also, I think Length() here returns a size_t. If so, we should probably use
> that or auto.

Yes, although given the 0 I think it has to be size_t.

> It took me a moment to understand which assertions this was referring to. It
> might be more clear if we made it refer to the "assertions below" or just
> dropped the parenthetical altogether.

changed to "(although we could check the assertions below)"

> >+nsTransitionManager::UpdateCascadeResults(
> >+                       AnimationPlayerCollection* aTransitions,
> >+                       AnimationPlayerCollection* aAnimations)
> >+{
> >+  if (!aTransitions || !aAnimations || !aAnimations->mStyleRule) {
> >+    // Nothing to do (although we could check the #ifdef DEBUG assertions)
> >+    return;
> >+  }
> >+
> >+  nsCSSPropertySet propertiesUsed;
> 
> propertiesWithAnimations?
> 
> I don't mind either way, but that would make a little more sense to me.

Perhaps.  I was originally going to add the properties with transitions into propertiesUsed as well (since conceptually that's the right thing), but then realized I could just assert that I don't need to.

I think I'll add a comment above the #ifdef DEBUG block saying "// assert that we don't need to bother adding the transitioned properties into propertiesUsed".

> >+  void UpdateCascadeResultsWithTransitions(
> >+         AnimationPlayerCollection* aTransitions);
> >+  void UpdateCascadeResultsWithAnimations(
> >+         AnimationPlayerCollection* aAnimations);
> >+  void UpdateCascadeResults(AnimationPlayerCollection* aTransitions,
> >+                            AnimationPlayerCollection* aAnimations);
> 
> If we make AddPropertiesToSet const, there would probably be a ripple effect
> meaning aAnimations could be a const pointer. I'll leave it up to you to
> decide if changing that is appropriate.

May as well do so.
(In reply to Brian Birtles (:birtles) from comment #15)
> patch 4 - For main-thread application of transitions, don't apply
> transitions when animations are also running
> 
> >+    if (!prop.mWinsInCascade) {
> >+      // This isn't the winning declaration, so don't add it to style.
> >+      // For transitions, this is important, because it's how we
> >+      // implement the rule that CSS transitions don't run when a CSS
> >+      // animation is running on the same property and element.  For
> >+      // animations, this is only skipping this that will otherwise be
> >+      // overridden.
> 
> I don't understand the last sentence in this comment.

"this" -> "things" should help.
(In reply to Brian Birtles (:birtles) from comment #17)
> patch 7 - For compositor-thread application of transitions, don't apply
> transitions when animations are also running

> I tried to persuade myself of this too. I think it's right, but there's one
> case I wonder about. When there's no change in animation value I believe
> that our restyle processing will filter out the apparently redundant change.
> If a CSS animation started and happened to produce the same value as an
> existing transition (perhaps they are both using step timing functions) then
> I wonder if we might fail to update the compositor. To fix this situation
> for changes to animations using the API, we bump the animation generation in
> a couple of places and we make the restyle manager look for changes to the
> animation generation.
> 
> In this case, even though nsTransitionManager::UpdateCascadeResults doesn't
> change the animation generation, subsequent to calling
> UpdateCascadeResultsWithTransitions() from
> nsTransitionManager::StyleContextChanged(), we do so we should be ok there.
> However, for AnimationPlayerCollection::EnsureStyleRuleFor(), which calls
> UpdateCascadeResultsWithAnimations(), we don't do that. There may be
> something else that triggers a layer transaction in that case but it still
> might be interesting to set up an OMTA test which causes a CSS transition to
> be overridden with a CSS animation such that they have the same value when
> the override takes place (but which differ later on so we can confirm that
> the layer got the updated animation).
> 
> What do you think?

I was actually thinking about the need to call UpdateAnimationGeneration yesterday while working on bug 847287:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/2af1b75dba46

I think I probably should fix patch 2 to call UpdateAnimationGeneration from UpdateCascadeResults.  Does that seem reasonable?
... er, to call UpdateAnimationGeneration when it changes an mWinsInCascade value.

(And while I'm there, I realized that it probably does need to handle null-aAnimations or null aAnimations->mStyleRule better, since there might be false mWinsInCascade values that need to be changed to true.)
Attachment #8578859 - Flags: review?(bbirtles) → review+
(In reply to David Baron [:dbaron] (UTC-7) from comment #20)
> I was actually thinking about the need to call UpdateAnimationGeneration
> yesterday while working on bug 847287:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/2af1b75dba46
> 
> I think I probably should fix patch 2 to call UpdateAnimationGeneration from
> UpdateCascadeResults.  Does that seem reasonable?

Yes, that makes sense to me. If it's possible to create a test case that fails without this, that would be a useful to make sure we don't regress this.
The failures were due to the addition of:
       collection->mPlayers.Clear();
       collection->mStyleRule = nullptr;
so that I could call UpdateCascadeResultsWithAnimations when the animations are about to go away in a way that would both (a) find the appropriate transitions object and (b) act like we have no animations.  I should find a less-hacky way to do that.
I fixed the failures with what seemed like the minimal change to fix them:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/727cd515e764
although I also added a comment to the function afterwards:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/385341e98e6d
For my own reference, the issue here was that were we blowing away the list of players without calling cancel on them so any Promises they generated would just hang.

We should probably check that all CSS-generated animations are cancelled before being destroyed. I added a comment to bug 1134381 so we can look into doing that there.

Also, we can't just call Destroy on the collection first since, as I understand it, that will trigger the dtor for the collection so we won't be able to use its element / pseudo type properties.

The proposed patch seems fine although I'd find it slightly more clear if we simplify introduced UpdateCascadeResultsWithoutAnimations (or possibly ...WithNoAnimations) that took the element and pseudo type as parameters rather than the collection of animations.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #30)
> The proposed patch seems fine although I'd find it slightly more clear if we
> simplify introduced UpdateCascadeResultsWithoutAnimations (or possibly
> ...WithNoAnimations) that took the element and pseudo type as parameters
> rather than the collection of animations.

Then there's the question of whether such a method would:
 (a) not bother getting the animations, and risk having bugs if we tried to use it in a similar case for destroying transitions, or

 (b) get the animations, and require both that we called it after destroying the animations object and that we waste time asking for the property

Given that, I think I prefer leaving it as-is, although I guess I'm not as opposed to (a) as (b).
Release Note Request (optional, but appreciated)
[Why is this notable]: Useful for developers 
[Suggested wording]: Cascading of CSS transitions and animations now matches the current spec
[Links (documentation, blog post, etc)]:  https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Using_CSS_animations.
relnote-firefox: --- → 39+
Blocks: css-cascade-3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: