Reverse composite order of animation effects

RESOLVED DUPLICATE of bug 1304922

Status

()

Core
DOM: Animation
P3
normal
RESOLVED DUPLICATE of bug 1304922
a year ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
This is a prerequisite for implementation of effect composite.

To reverse the order AnimValuesStyleRule::AddValue() needs to replace an old  StyleAnimationValue if there are already an entry for the same CSS property.
Also in case of transition level composite we need to check that composition for animation level has been already done or not instead of checking mWinsInCascade flag.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08cce4c98de
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8791842 [details]
Bug 1303233 - Part 1: Add a const version of EffectSet::AnimationRule().

https://reviewboard.mozilla.org/r/79112/#review77746
Attachment #8791842 - Flags: review?(bbirtles) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8791843 [details]
Bug 1303233 - Part 2: Add AnimValuesStyleRule::HasPropertyValue(). .

https://reviewboard.mozilla.org/r/79114/#review77752
Attachment #8791843 - Flags: review?(bbirtles) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8791844 [details]
Bug 1303233 - Part 1: AnimValuesStyleRule::AddValue replaces the existence entry's mValue. r?

https://reviewboard.mozilla.org/r/79116/#review77750

::: dom/animation/AnimValuesStyleRule.cpp:69
(Diff revision 1)
> +  if (HasPropertyValue(aProperty)) {
> +    for (PropertyStyleAnimationValuePair& pair : mPropertyValuePairs) {
> +      if (pair.mProperty == aProperty) {
> +        pair.mValue = aValue;
> +        return;
> +      }
> +    }
> +  }

Can we avoid this somehow? I'm concerned about the performance characteristics of this when we have a lot of rules or a lot of properties. Should we be using a hashmap instead?
Attachment #8791844 - Flags: review?(bbirtles)

Comment 8

a year ago
mozreview-review
Comment on attachment 8791845 [details]
Bug 1303233 - Part 2: Reverse composite order of effects.

https://reviewboard.mozilla.org/r/79118/#review77734

::: dom/animation/EffectCompositor.cpp:597
(Diff revision 1)
>    // If multiple animations specify behavior for the same property the
> -  // animation with the *highest* composite order wins.
> -  // As a result, we iterate from last animation to first and, if a
> +  // animation with the *highest* composite order wins.  The highest
> +  // effect will override (or composite) to lower one.

If multiple animations affect the same property, animations with higher composite order (priority) override or add to animations with lower priority.

::: dom/animation/KeyframeEffectReadOnly.cpp:321
(Diff revision 1)
> +  // Here, out side the properties' loop, get the animation level rule in the
> +  // case when we are goning to compose styles for transition level since we
> +  // will check each property has been already composed by *animation* level
> +  // effects inside the loop.

If we are currently composing the rule for the transitions level of the cascade, get the rule for the *animations* level. Although the transitions rule is higher in the cascade, transitions have a lower composite order than animations--that is, they should not run if there are animations running on the same property.

::: dom/animation/KeyframeEffectReadOnly.cpp:325
(Diff revision 1)
> +  // XXXXX: Should we pass the cascade level from
> +  // EffectCompositor::ComposeAnimationRule instead of getting the level each
> +  // time?

Why would that be better? This seems fine to me.

(Although we could cache the cascade level outside the loop to save a virtual method call to CascadeLevel() on each iteration of the loop.)

::: dom/animation/KeyframeEffectReadOnly.cpp:328
(Diff revision 1)
> +  // will check each property has been already composed by *animation* level
> +  // effects inside the loop.
> +  // XXXXX: Should we pass the cascade level from
> +  // EffectCompositor::ComposeAnimationRule instead of getting the level each
> +  // time?
> +  const AnimValuesStyleRule* animationLevelRule;

This needs to be initialized to nullptr.

::: dom/animation/KeyframeEffectReadOnly.cpp:350
(Diff revision 1)
> -    if (aSetProperties.HasProperty(prop.mProperty)) {
> -      // Animations are composed by EffectCompositor by iterating
> -      // from the last animation to first. For animations targetting the
> -      // same property, the later one wins. So if this property is already set,
> -      // we should not override it.
> -      continue;
> -    }
> +    // In case of transition level if animation level style has been already
> +    // composed, we don't need to compose transition styles because animation
> +    // level style is higher priority than transition level.
> +    if (mAnimation->CascadeLevel() ==
> +          EffectCompositor::CascadeLevel::Transitions &&
> +        animationLevelRule &&
> +        animationLevelRule->HasPropertyValue(prop.mProperty)) {

I think this depends on composing the animations rule before the transitions rule. That's fine but we should document and assert that assumption (probably outside this loop).

(Basically we'll want to assert that the element is not in EffectCompositor::mElementsToCascade[CascadeLevel::Animations].)

If possible, it would be nice if we check for animations of a property without using the style rule because:

a) Then we could remove the dependency on composing rules in a certain order
b) It will work with stylo (with stylo we will basically have an opaque pointer to computed values so we'd need to call into servo to check if it contains a particular property or not--AnimValuesStyleRule will not be used)

But perhaps soon we will be reworking this to compose everything at once anyway so these will no longer be a problem? (We'll need to rework this anyway once we do additive animation since animations should be able to add on top of transitions.)

For the comment, I think it confuses levels and priorities a little. Perhaps:

"If we are composing the transitions level, skip any properties that are already set in the animations level (which should have been composed before this). This is because although transitions are higher in the cascade, they have a lower composite order."

::: dom/animation/KeyframeEffectReadOnly.cpp
(Diff revision 1)
> -    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 things that will otherwise be
> -      // overridden.

Thanks for the explanation of why this is ok.
Attachment #8791845 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 9

a year ago
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #7)
> Comment on attachment 8791844 [details]
> Bug 1303233 - Part 3: AnimValuesStyleRule::AddValue replaces the existence
> entry's mValue.
> 
> https://reviewboard.mozilla.org/r/79116/#review77750
> 
> ::: dom/animation/AnimValuesStyleRule.cpp:69
> (Diff revision 1)
> > +  if (HasPropertyValue(aProperty)) {
> > +    for (PropertyStyleAnimationValuePair& pair : mPropertyValuePairs) {
> > +      if (pair.mProperty == aProperty) {
> > +        pair.mValue = aValue;
> > +        return;
> > +      }
> > +    }
> > +  }
> 
> Can we avoid this somehow? I'm concerned about the performance
> characteristics of this when we have a lot of rules or a lot of properties.
> Should we be using a hashmap instead?

I was thinking that I will change it to a hash table when I implement using underlying value. Using underlying value needs to get value and replace value repeatedly.  But it's OK doing it in this bug.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #7)
> > Can we avoid this somehow? I'm concerned about the performance
> > characteristics of this when we have a lot of rules or a lot of properties.
> > Should we be using a hashmap instead?
> 
> I was thinking that I will change it to a hash table when I implement using
> underlying value. Using underlying value needs to get value and replace
> value repeatedly.  But it's OK doing it in this bug.

Thank you!

When you're ready for re-review, please ni me since I set the "not accepting review requests" flag.

Also, note that for Stylo we won't be using AnimValuesStyleRule so if there's a way of handling this without using AnimValuesStyleRule that might be even better (although perhaps you already have in mind a way to fix this later).
(Assignee)

Comment 11

a year ago
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #8)
> ::: dom/animation/KeyframeEffectReadOnly.cpp:350
> (Diff revision 1)
> > -    if (aSetProperties.HasProperty(prop.mProperty)) {
> > -      // Animations are composed by EffectCompositor by iterating
> > -      // from the last animation to first. For animations targetting the
> > -      // same property, the later one wins. So if this property is already set,
> > -      // we should not override it.
> > -      continue;
> > -    }
> > +    // In case of transition level if animation level style has been already
> > +    // composed, we don't need to compose transition styles because animation
> > +    // level style is higher priority than transition level.
> > +    if (mAnimation->CascadeLevel() ==
> > +          EffectCompositor::CascadeLevel::Transitions &&
> > +        animationLevelRule &&
> > +        animationLevelRule->HasPropertyValue(prop.mProperty)) {
> 
> I think this depends on composing the animations rule before the transitions
> rule. That's fine but we should document and assert that assumption
> (probably outside this loop).
> 
> (Basically we'll want to assert that the element is not in
> EffectCompositor::mElementsToCascade[CascadeLevel::Animations].)
> 
> If possible, it would be nice if we check for animations of a property
> without using the style rule because:

I have a patch to drop mWinsInCascade, I think it will be used for the check.  In the patch EffectSet will have a hashtable to store which cascade level is the winner.  It looks like this;

  enum class WinnerCascadeLevel {
    None,            // There are animations but overridden by important styles.
    TransitionsOnly, // There are only Transition level animations.
    AnimationsOnly,  // There are only Animations level animations.
    Both,            // Both of Transitions and Animations.
  };

  nsDataHashtable<nsUint32HashKey, WinnerCascadeLevel> mWinnerInCascade;

  WinnerCascadeLevel WinnerInCascade(nsCSSPropertyID aProperty) const
  {
    WinnerCascadeLevel winner;
    if (mWinnerInCascade.Get(aProperty, &winner)) {
      return winner;
    }
    return WinnerCascadeLevel::None;
  }

This hashtable is updated in UpdateCascadeResults(), currently it stores only opacity and transform properties but if we store all of properties we can use this there.  I am not sure it's a good way though.

(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #10)
> When you're ready for re-review, please ni me since I set the "not accepting
> review requests" flag.

Thanks.  I will do it.

> Also, note that for Stylo we won't be using AnimValuesStyleRule so if
> there's a way of handling this without using AnimValuesStyleRule that might
> be even better (although perhaps you already have in mind a way to fix this
> later).

I did not consider it without AnimValuesStyleRule at all.  I can't imagine how Stylo way looks like, but perhaps we can store the value (as a mediator between StyleAnimationValue and stylo's value?) in EffectSet.
(Assignee)

Comment 12

a year ago
It turns out there are two flaky points in part 4 patch.

1) There is a race condition that the effect is not in-effect state in KeyframeReadOnly::ComposeStyle().
2) There is a case that ComposeStyle() for transition level is not called right after ComposeStyle() for animation level.

Regarding 1); I changed MOZ_ASSERT(IsInEffect()) at the top of KeyframeReadOnly::ComposeStyle() to if statement.

Regarding 2); We call MaybeUpdateAnimationRule() (it leads ComposeStyle for transition) in nsTransitionManager::StyleContextChanged() [1],  StyleContextChanged() is, of course, called after ComposeStyle for animation level, in this case it called from GetAnimationRule in nsStyle::GetContext(), but it's a bit far away.  As far as I can tell, currently there is no animation process against the same element but I am not sure it's guaranteed in future. So, I've decided to implement WinnerCascadeLevel in comment 11 prior to this bug, and use it to check whether we need to compose transition level effects regardless animation rules.

[1] http://hg.mozilla.org/mozilla-central/file/f0e6cc636021/layout/style/nsTransitionManager.cpp#l439
(Assignee)

Updated

a year ago
Depends on: 1304922
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8791842 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8791843 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
Now bug 1304922 made this bug simple.
(Assignee)

Comment 16

a year ago
Now we are going to reverse the order in bug 1304922.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1304922
You need to log in before you can comment on or make changes to this bug.