1285407 - Animations which can be run on the compositor don't start when "!important" style is removed while it's in effect

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

A transform animation in attaching test case does not initially run on the compositor because the target element has "transform: translateX(0px) !important" style.  But after removing the important style, the transform animation still stays original position.

A reason I can think of is that we don't call UpdateCascadeResults in such case.  But what I don't quite understand is non-compositor animations, say margin-left, *do* start its animation.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: margin-lest case

This test case works as expected.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> Created attachment 8769001 [details]
> A test case
> 
> A transform animation in attaching test case does not initially run on the
> compositor because the target element has "transform: translateX(0px)
> !important" style.  But after removing the important style, the transform
> animation still stays original position.
> 
> A reason I can think of is that we don't call UpdateCascadeResults in such
> case.  But what I don't quite understand is non-compositor animations, say
> margin-left, *do* start its animation.

Oh-oh, I did not realized that the flag is not useful for main-threaded animations. And this issue only happens for web-animations because we call UpdateCascadeResults.
http://hg.mozilla.org/mozilla-central/file/23dc78b7b57e/layout/style/nsAnimationManager.cpp#l443

*AND* now I realized a cumbersome problem that we use an old nsStyleContext in EffectCompositor::MaybeUpdateCascadeResults.
http://hg.mozilla.org/mozilla-central/file/23dc78b7b57e/dom/animation/EffectCompositor.cpp#l473

In EffectCompositor::MaybeUpdateCascadeResults we get nsStyleContext by calling nsIFrame::StyleContext(), but when it's called from nsStyleSet::GetContext, the nsStyleContext obtained by nsIFrame::StyleContext() is the previous one.  We need to pass the new nsStyleContext generated in nsStyleSet::GetContext.

I am not sure this cumbersome problem is related to the nested GetContext calls.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> > Created attachment 8769001 [details]
> > A test case
> > 
> > A transform animation in attaching test case does not initially run on the
> > compositor because the target element has "transform: translateX(0px)
> > !important" style.  But after removing the important style, the transform
> > animation still stays original position.
> > 
> > A reason I can think of is that we don't call UpdateCascadeResults in such
> > case.  But what I don't quite understand is non-compositor animations, say
> > margin-left, *do* start its animation.
> 
> Oh-oh, I did not realized that the flag is not useful for main-threaded
> animations. And this issue only happens for web-animations because we call
> UpdateCascadeResults.

because we call UpdateCascadeResults in case of CSS animations.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1285407 - Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule.

While resolving style context, the primary frame of the target element
has previous style context so if we don't pass the newly created nsStyleContext,
UpdateCascadeResults uses the previous style to get overridden properties, it
will result unexpected cascading results.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1285407 - Part 2: We need to call MarkCascadeNeedsUpdate() when the style context is changed.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1285407 - Part 3: Remove UpdateCascadeResults call because it's called against the same nsStyleContext from MaybeUpdateAnimationRule.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1285407 - Part 4: Drop EffectCompositor::MaybeUpdateCascadeResults(Element*, CSSPseudoElementType) because it's essentially the same as another one.

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

Comment 8

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

Comment on attachment 8769515 [details]
Bug 1285407 - Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule.

https://reviewboard.mozilla.org/r/63374/#review60232

::: dom/animation/EffectCompositor.h:122
(Diff revision 1)
>    // Updates the animation rule stored on the EffectSet for the
>    // specified (pseudo-)element for cascade level |aLevel|.
>    // If the animation rule is not marked as needing an update,
>    // no work is done.
>    void MaybeUpdateAnimationRule(dom::Element* aElement,
>                                  CSSPseudoElementType aPseudoType,
> -                                CascadeLevel aCascadeLevel);
> +                                CascadeLevel aCascadeLevel,
> +                                nsStyleContext *aStyleContext);

We should document what style context is used for and if it can be nullptr or not.

::: dom/animation/EffectCompositor.h:131
(Diff revision 1)
>    nsIStyleRule* GetAnimationRule(dom::Element* aElement,
>                                   CSSPseudoElementType aPseudoType,
> -                                 CascadeLevel aCascadeLevel);
> +                                 CascadeLevel aCascadeLevel,
> +                                 nsStyleContext* aStyleContext);

It might be worth adding a comment saying why we need to pass in a style context here since it seems a bit odd to require one. Also, we should indicate if it can be nullptr or not.

::: dom/animation/EffectCompositor.h:163
(Diff revision 1)
> +  // NOTE: If aStyleContext is not specified, we try to use nsStyleContext
> +  // of the primary frame of the specified (pseudo-)element.
>    //
>    // This method does NOT detect if other styles that apply above the
>    // animation level of the cascade have changed.

I think we don't need NOTE: here since it looks like a warning.

Instead, how about:

  // aStyleContext may be nullptr in which case we will use the nsStyleContext
  // of the primary frame of the specified (pseudo-)element.

Comment 9

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

Comment on attachment 8769516 [details]
Bug 1285407 - Part 2: We need to call MarkCascadeNeedsUpdate() when the style context is changed.

https://reviewboard.mozilla.org/r/63376/#review60234

Comment 10

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

Comment on attachment 8769517 [details]
Bug 1285407 - Part 3: Remove UpdateCascadeResults call because it's called against the same nsStyleContext from MaybeUpdateAnimationRule.

https://reviewboard.mozilla.org/r/63378/#review60236

Comment 11

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

Comment on attachment 8769518 [details]
Bug 1285407 - Part 4: Drop EffectCompositor::MaybeUpdateCascadeResults(Element*, CSSPseudoElementType) because it's essentially the same as another one.

https://reviewboard.mozilla.org/r/63380/#review60238

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769515 [details]
Bug 1285407 - Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769516 [details]
Bug 1285407 - Part 2: We need to call MarkCascadeNeedsUpdate() when the style context is changed.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769517 [details]
Bug 1285407 - Part 3: Remove UpdateCascadeResults call because it's called against the same nsStyleContext from MaybeUpdateAnimationRule.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769518 [details]
Bug 1285407 - Part 4: Drop EffectCompositor::MaybeUpdateCascadeResults(Element*, CSSPseudoElementType) because it's essentially the same as another one.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a3508cf3c41
Part 1: Pass a newly created nsStyleContext to GetAnimationRule and MaybeUpdateAnimationRule. r=birtles
https://hg.mozilla.org/integration/autoland/rev/538877b1d33a
Part 2: We need to call MarkCascadeNeedsUpdate() when the style context is changed. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d4964fe5465e
Part 3: Remove UpdateCascadeResults call because it's called against the same nsStyleContext from MaybeUpdateAnimationRule. r=birtles
https://hg.mozilla.org/integration/autoland/rev/cd0d40f88145
Part 4: Drop EffectCompositor::MaybeUpdateCascadeResults(Element*, CSSPseudoElementType) because it's essentially the same as another one. r=birtles

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a3508cf3c41
https://hg.mozilla.org/mozilla-central/rev/538877b1d33a
https://hg.mozilla.org/mozilla-central/rev/d4964fe5465e
https://hg.mozilla.org/mozilla-central/rev/cd0d40f88145