Closed Bug 1245075 Opened 6 years ago Closed 6 years ago

Wrong opacity transition on element with display table

Categories

(Core :: Graphics: Layers, defect)

40 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 - wontfix
firefox46 + verified
firefox47 + verified

People

(Reporter: mtr, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

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.
Component: Untriaged → Layout
Product: Firefox → Core
Not 100% sure but it has been already reported.
Whiteboard: DUPEME?
Component: Layout → Layout: Tables
Attached file index.html (obsolete) —
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
Flags: needinfo?(bbirtles)
Keywords: regression, testcase
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: DUPEME?
Version: 44 Branch → 40 Branch
Blocks: 1061364
This appears to be an off-main thread animations bug. If I turn off OMTA it works for me.
Blocks: enable-omt-animations
No longer blocks: 1061364
Component: Layout: Tables → Graphics: Layers
Flags: needinfo?(bbirtles)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
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.)
Attachment #8714773 - Attachment is obsolete: true
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.
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
Though neither fixing EffectSet::GetEffectSet(nsIFrame*) nor fixing EffectCompositor::GetAnimationElementAndPseudoForFrame fixes the problem.
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...
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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/
Attachment #8716704 - Flags: review?(dholbert)
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/
Attachment #8716705 - Flags: review?(bbirtles)
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
Attachment #8716704 - Flags: review?(dholbert) → review+
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.)
Attachment #8716705 - Flags: review?(bbirtles) → review+
(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.
(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'. :-)
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
https://hg.mozilla.org/mozilla-central/rev/417122415962
https://hg.mozilla.org/mozilla-central/rev/dcea3556c181
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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
Attachment #8716704 - Flags: approval-mozilla-aurora?
Attachment #8716705 - Flags: approval-mozilla-beta?
Attachment #8716705 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
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.
Attachment #8716704 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking for 45+ since this is a fairly recent regression. 

It would be good to have some verification of this fix on aurora.
Flags: qe-verify+
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.
Attachment #8716705 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[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.
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.