1245075 - Wrong opacity transition on element with display table

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[mtr]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160123151951

Steps to reproduce:

1. Create DIV with opacity (css property) grater than 0 (i.e. 0.25) and display (css property) "table".
2. Set css transition effect on opacity (i.e. to become 1 on hover)
3. Launch transition effect

Working example: 


Actual results:

Opacity is changing from 0 to 1 regardless of starting and ending css value of that property. After effect is finished opacity is returned its determined by css value.

This problem does not occur on elements with display property set to other value than "table".


Expected results:

Opacity should change in range determined by css properties (in given example from 0.25 to 1) like it does on any other (display property value) elements.

Comment 1

[mtr]

Missing example: https://jsfiddle.net/wkpsmjep/1/

Comment 2

[Loic]

Not 100% sure but it has been already reported.

Comment 3

[Loic]

Attachment: index.html

Comment 4

[Loic]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a530b5c3b713926329f7aa5a133d801e2e9eee0a&tochange=335f1295e99bece2c42c90f0405b1eefb60f5f41

suspected bug:
Brian Birtles — Bug 1061364 - Don't force transitions to refresh their style rule; r=dbaron

Comment 5

[Brian Birtles (:birtles)]

This appears to be an off-main thread animations bug. If I turn off OMTA it works for me.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: slightly simpler testcase

I suspect this has something to do with the table's split between inner (primary) frame and outer (wrapper) frame.  We're probably ending up with opacity on both of them instead of just one.

(Given that the ::-moz-table-outer rule in ua.css doesn't mention opacity, it looks like it's supposed to be only on the inner (primary) frame, but I bet we're somehow applying it to the outer frame as well.)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It looks like everything that should matter here calls either EffectCompositor::HasAnimationsForCompositor or EffectCompositor::GetAnimationsForCompositor.  Those both call FindAnimationsForCompositor.  That, in turn, calls EffectSet::GetEffectSet(const nsIFrame* aFrame), which seems like it might be the right place to fix this (although I should look at the other callers).  I suspect it should be checking that the frame is the primary frame for its content, and returning null otherwise.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The only other caller of the nsIFrame* variant of GetEffectSet is EffectCompositor::ClearIsRunningOnCompositor, and it (and its two callers) seem fine with such a change

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Though neither fixing EffectSet::GetEffectSet(nsIFrame*) nor fixing EffectCompositor::GetAnimationElementAndPseudoForFrame fixes the problem.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The test I wanted was nsLayoutUtils::GetStyleFrame(content) != aFrame, not content->GetPrimaryFrame() != aFrame.  With that fixed, I do have a working fix for this bug.

Writing an automated test might be fun...

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8d80e727c24

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a6232b81189

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd9f4d867449

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1245075 patch 1 - Remove assertion, since it can fire during frame destruction. r?dholbert

Without this patch, patch 2 will cause assertions since
nsFrame::DestroyFrom calls nsFrame::HasCSSAnimations (at a time when the
child frame has been destroyed), which calls into the code modified in
patch 2 to call GetStyleFrame.

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

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1245075 patch 2 - Fix EffectSet::GetEffectSet(nsIFrame*) and EffectCompositor::GetAnimationElementAndPseudoForFrame to only return effects when the frame is the style frame for its content. r?birtles

This means that we won't associate animations with additional frames.
In this case, this fixes associating off-main-thread animations with a
table outer frame, when they should have been associated only with the
table frame.

Locally, the test fails without the patch (with opacity in the test
being 0.36 instead of the expected 0.6), and passes with the patch.
(Opacity 0.36 gives a color of rgb(163,163,255), whereas 0.6 gives
rgb(102,102,255).)

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

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8716704 [details]
MozReview Request: Bug 1245075 patch 1 - Remove assertion, since it can fire during frame destruction.  r?dholbert

https://reviewboard.mozilla.org/r/33925/#review30535

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8716705 [details]
MozReview Request: Bug 1245075 patch 2 - Fix EffectSet::GetEffectSet(nsIFrame*) and EffectCompositor::GetAnimationElementAndPseudoForFrame to only return effects when the frame is the style frame for its content.  r?birtles

https://reviewboard.mozilla.org/r/33927/#review30553

::: dom/animation/EffectCompositor.cpp:519
(Diff revision 1)
> +  } else {
> +    if (nsLayoutUtils::GetStyleFrame(content) != aFrame) {

It seems like we could just make this an 'else if' ? I don't mind either way, but if we do change this we should change EffectSet::GetEffectSet too.

::: layout/reftests/css-animations/animate-display-table-opacity.html:11
(Diff revision 1)
> +  animation: HoldOpacity 5s linear;

Should we make this a little longer than 5s to avoid intermittent failures when running on hardware under load? I think we normally use 100s for this kind of case. (Presumably using 'paused' won't help us here because we don't send paused animations to the compositor.)

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #17)
> ::: dom/animation/EffectCompositor.cpp:519
> (Diff revision 1)
> > +  } else {
> > +    if (nsLayoutUtils::GetStyleFrame(content) != aFrame) {
> 
> It seems like we could just make this an 'else if' ? I don't mind either
> way, but if we do change this we should change EffectSet::GetEffectSet too.

I think I prefer it the way it is because the outer if/else splits the space neatly in half (generated content vs. not), and then we have code to deal with each half.

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #17)
> ::: layout/reftests/css-animations/animate-display-table-opacity.html:11
> (Diff revision 1)
> > +  animation: HoldOpacity 5s linear;
> 
> Should we make this a little longer than 5s to avoid intermittent failures
> when running on hardware under load? I think we normally use 100s for this
> kind of case. (Presumably using 'paused' won't help us here because we don't
> send paused animations to the compositor.)

I also added 'infinite'. :-)

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/417122415962057d0f0ddcb6349e9c3499bbdd9b
Bug 1245075 patch 1 - Remove assertion, since it can fire during frame destruction.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcea3556c181336a12e35d2055626f7e6b7b2f24
Bug 1245075 patch 2 - Fix EffectSet::GetEffectSet(nsIFrame*) and EffectCompositor::GetAnimationElementAndPseudoForFrame to only return effects when the frame is the style frame for its content.  r=birtles

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/417122415962
https://hg.mozilla.org/mozilla-central/rev/dcea3556c181

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8716704 [details]
MozReview Request: Bug 1245075 patch 1 - Remove assertion, since it can fire during frame destruction.  r?dholbert

Approval Request Comment
[Feature/regressing bug #]: bug 980770 (enabling compositor-thread animations)
[User impact if declined]: animations of opacity or transform happen "double" when done on table elements (and maybe other similar elements)
[Describe test coverage new/current, TreeHerder]: reftest in tree
[Risks and why]: has a little bit of risk (in the sense of potentially breaking something different from what it's fixing) for animations on elements that have multiple frames (like tables, overflow:scroll/auto/hidden), but those animations were pretty broken for a while, and the fix should be correct; given that the risk should be scoped to those elements I think this risk is reasonable
[String/UUID change made/needed]: no

Comment 23

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8716704 [details]
MozReview Request: Bug 1245075 patch 1 - Remove assertion, since it can fire during frame destruction.  r?dholbert

Fix for some animations. Regression since 40. 
Has had some time on m-c, ok to uplift to aurora.

Comment 24

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking for 45+ since this is a fairly recent regression. 

It would be good to have some verification of this fix on aurora.

Comment 25

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03143f8b2b64
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ed468649dea

Comment 26

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8716705 [details]
MozReview Request: Bug 1245075 patch 2 - Fix EffectSet::GetEffectSet(nsIFrame*) and EffectCompositor::GetAnimationElementAndPseudoForFrame to only return effects when the frame is the style frame for its content.  r?birtles

Already uplifted, just tweaking the approval flag.

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

[Tracking Requested - why for this release]:  Given that we've been shipping this since 41, I don't think it's worth the risk of landing this patch on beta, and I think we should untrack for 45.  The patch has landed on aurora.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Agreed, untracking.

Comment 29

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial behavior on old Nightly (2016-02-02), verified that the issue is fixed using latest Aurora 47.0a2 and Firefox 46 beta 8 across platforms (Windows 10 32-bit, Mac OS X 10.10.5 and Ubuntu 14.04 64-bit).