Closed Bug 1336772 Opened 3 years ago Closed 2 years ago

CSS animation of fill-opacity doesn't beat transition

Categories

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

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: heycam, Assigned: hiro)

References

Details

(Keywords: regression, stale-bug)

Attachments

(3 files)

Attached file test
See the attached test, which is extracted (and modified) from one of the sub-tests in test_animations.html.  Before bug 1304922, this shows all PASSes.  I don't know why bug 1304922 regressed this for fill-opacity but not for opacity -- something to do with opacity being a compositor animated property?

Hiro, do you know what's going on here?

(I found this while working on bug 1331294.)
Flags: needinfo?(hikezoe)
Interesting.  As you already notice, fill-opacity is not treated as compositor animatable property.  So, I think, before bug 1304922, the test case is unintentionally passed.
Hiro, were you planning on looking at this?
Priority: -- → P1
This is really interesting. I just checked attachment 8833712 [details] on stylo with making fill-opacity animatable, the result is the same as gecko.
Assignee: nobody → hikezoe
Flags: needinfo?(hikezoe)
I had been believing UpdateCascadeResults() is only for properties that are running on the compositor, but it turns out that we are relying on RequestRestyle call in the function to clear/throw away (I am not sure what the word is suitable here) beaten cascade level's properties' styles. Otherwise beaten styles persist as the test case in comment 0.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> I had been believing UpdateCascadeResults() is only for properties that are
> running on the compositor, but it turns out that we are relying on
> RequestRestyle call in the function to clear/throw away (I am not sure what
> the word is suitable here) beaten cascade level's properties' styles.
> Otherwise beaten styles persist as the test case in comment 0.

I mean we need to call RequestRestyle() for properties that the cascade level of the property is changed.
Attached file visible example
I might be wrong. This problem might be a getComputedStyle() problem. 
Attaching example uses margin-left property to be able to confirm the problem visually.  But as far as I can see the margin-left animation works fine with margin-left transition. So, we might need to do something for getComputedStyle(), I am not really sure though.
FWIW here is a try with patches what I was thinking in comment 4;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fad51c5034056d051e2256fddb0439724be72a8

Unfortunately these patches cause a crash on stylo since mElementsToRestyle is modified in UpdateCascadeResults() during we iterate over mElementsToRestyle in PreTraverseInSubtree().
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Comment on attachment 8904351 [details]
Bug 1336772 - Request any restyles required by changes to the cascade result.

https://reviewboard.mozilla.org/r/176140/#review181120

r=me with the following comments address, particularly making sure we request a layer restyle when we change the cascade and have transitions running on the compositor.

::: commit-message-632e4:1
(Diff revision 1)
> +Bug 1336772 - Animation overrides transition immediately. r?birtles

This doesn't really describe what the patch changes

::: commit-message-632e4:7
(Diff revision 1)
> +included the transitions level rule. It resulted that the transitions level
> +style overrides newly created animation style until the next normal restyling
> +process happens (i.e. process transition level restyle request). Vice versa,

Nit: s/It resulted that/As a result,/

::: commit-message-632e4:13
(Diff revision 1)
> +So when an animation is newly created or removed, we had to call RequestRestyle
> +for transitions level.

This patch fixes this problem by triggering a restyle of the transitions level when an animation is created or removed.

::: dom/animation/EffectCompositor.h:282
(Diff revision 1)
>    // Update the mPropertiesWithImportantRules and
>    // mPropertiesForAnimationsLevel members of the given EffectSet.
> +  // And also request restyles if necessary.

Nit: s/EffectSet. And also.../EffectSet, and also request any restyles required by changes to the cascade result./

::: dom/animation/EffectCompositor.cpp:933
(Diff revision 1)
> -  // for animations level is newly added or removed, we need to update layers
> -  // for transitions level because composite order has been changed now.
> -  if (hasCompositorPropertiesForTransition &&
> -      prevCompositorPropertiesForAnimationsLevel !=
> -        compositorPropertiesInSet(propertiesForAnimationsLevel)) {
> +  // If we have transition properties and if the same propery for animations
> +  // level is newly added or removed, we need to update the transition level
> +  // rule since the transition rule needs to be dropped from rule tree when the
> +  // animation level rule is newly added or added to the rule tree when the
> +  // animation level rule is removed.

This is a long sentence. Perhaps we could split it into two sentences, or just shorten it a little e.g.

... we need to update the transition level rule since it will need to be added/removed from the rule tree.

::: dom/animation/EffectCompositor.cpp:944
(Diff revision 1)
> +    prevPropertiesForAnimationsLevel.Xor(propertiesForAnimationsLevel);
> +  if (propertiesForTransitionsLevel.Intersects(
> +        changedPropertiesForAnimationLevel)) {
>      presContext->EffectCompositor()->
>        RequestRestyle(aElement, aPseudoType,
> -                     EffectCompositor::RestyleType::Layer,
> +                     EffectCompositor::RestyleType::Standard,

Don't we still need to update layers in some cases?

As discussed, I wonder if we check for compositorPropertiesInSet(<result of Intersects call>).Empty() and use that to determine the restyle type: Layer or Standard.

::: layout/style/nsCSSPropertyIDSet.h:90
(Diff revision 1)
> +    // Return a new nsCSSPropertyIDSet which is the result of xor operation with
> +    // |aOther|.

How about:

"Returns a new nsCSSPropertyIDSet with all properties that are in either this set or |aOther| but not both."

?
Attachment #8904351 - Flags: review?(bbirtles) → review+
Thanks for the review!

(In reply to Brian Birtles (:birtles) from comment #11)
> ::: dom/animation/EffectCompositor.cpp:944
> (Diff revision 1)
> > +    prevPropertiesForAnimationsLevel.Xor(propertiesForAnimationsLevel);
> > +  if (propertiesForTransitionsLevel.Intersects(
> > +        changedPropertiesForAnimationLevel)) {
> >      presContext->EffectCompositor()->
> >        RequestRestyle(aElement, aPseudoType,
> > -                     EffectCompositor::RestyleType::Layer,
> > +                     EffectCompositor::RestyleType::Standard,
> 
> Don't we still need to update layers in some cases?
> 
> As discussed, I wonder if we check for compositorPropertiesInSet(<result of
> Intersects call>).Empty() and use that to determine the restyle type: Layer
> or Standard.

I had to add nsCSSPropertyIDSet::IsEmpty().  A try looks good so far (not finished yet).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=004eaf6e5a776f3e579631c6e0244e1c6205c4fe

I did intentional use 'for' loop instead of using PodOperations for the IsEmpty() method since the for loop can be expanded for an optimization by compilers, whereas PodEqual() seems not to be able to optimized.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #11)
> > As discussed, I wonder if we check for compositorPropertiesInSet(<result of
> > Intersects call>).Empty() and use that to determine the restyle type: Layer
> > or Standard.
> 
> I had to add nsCSSPropertyIDSet::IsEmpty().

Oh, right, Empty actually empties it!

> A try looks good so far (not
> finished yet).
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=004eaf6e5a776f3e579631c6e0244e1c6205c4fe

Nit: s/Requst/Request/ in the commit message
Also, I guess now that Intersects returns an nsCSSPropertyIDSet instead of a bool it should be called 'Intersect' (no 's').
(In reply to Brian Birtles (:birtles) from comment #14)
> Also, I guess now that Intersects returns an nsCSSPropertyIDSet instead of a
> bool it should be called 'Intersect' (no 's').

That's good to know, thanks!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e18c8795f0d9
Request any restyles required by changes to the cascade result. r=birtles
https://hg.mozilla.org/mozilla-central/rev/e18c8795f0d9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there enough user impact to warrant uplift consideration for Beta? Please request approval if so. It grafts cleanly as-is.
Flags: needinfo?(hikezoe)
Flags: in-testsuite?
Version: unspecified → 52 Branch
Ah indeed. This should be uplifted and should be easily applied to beta. I will prepare it. (I did not have mozilla-beta tree locally!)
Comment on attachment 8904351 [details]
Bug 1336772 - Request any restyles required by changes to the cascade result.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1304922 
[User impact if declined]: The user could see graphical glitches when an element has running transition and if an animation is created/removed on the same element.
[Is this code covered by automated tests?]: Yes, the patch has it.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: This patch does just an additional restyling process for this particular case just like we did before bug 1304922. 
[String changes made/needed]: None
Flags: needinfo?(hikezoe)
Attachment #8904351 - Flags: approval-mozilla-beta?
Comment on attachment 8904351 [details]
Bug 1336772 - Request any restyles required by changes to the cascade result.

Regression fix, has new tests. Let's try this for beta 10.
Attachment #8904351 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.