Parent element opacity not taken into account when opacity is transitioned on element

VERIFIED FIXED in Firefox 58

Status

()

Core
Layout: Web Painting
P3
normal
VERIFIED FIXED
5 months ago
8 hours ago

People

(Reporter: r.otten, Assigned: hiro)

Tracking

({regression})

41 Branch
mozilla58
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 verified)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

5 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Steps to reproduce:

Build a page that contains an element which transitions or animates its CSS opacity, but which is nested in a parent element that also has a non-opaque, i.e, fully or partially transparent CSS opacity value.

E.g. http://jsfiddle.net/fxhv3kyg/


Actual results:

Firefox ignores the opacity value of the parent element while the element transitions its own opacity.


Expected results:

Firefox should have honored the opacity value of the parent element.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Web Painting
Ever confirmed: true
Keywords: regressionwindow-wanted
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All

Comment 1

5 months ago
Regression winsow:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ef53c6c25fb1&tochange=eed5d2d610e2

Regressed by:
eed5d2d610e2	L. David Baron — Bug 980770 - Enable off-main-thread animations on all platforms other than Linux (and other X11 based platforms) with off-main-thread compositing, for nightly/aurora. r=birtles
Blocks: 980770
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Keywords: regressionwindow-wanted → regression

Comment 2

5 months ago
indeed, setting layers.offmainthreadcomposition.async-animations=false fixes the problem.
Version: 55 Branch → 41 Branch
status-firefox56: affected → wontfix
Interesting.  Feels like this is probably somehow related to how we manage the opacity on the layer.

I think there may be some other reports of this bug, but this testcase is particularly clear.
Created attachment 8907956 [details]
reporter's testcase

(with extra junk from jsfiddle removed)
bug 1291309 and *maybe* bug 1213253 could be other (more complex) versions of this bug.
(Also, at this point, I'd say I'm not particularly an expert on any of this code anymore.  Perhaps birtles or hiro is, but probably for after Firefox 57...)
(Assignee)

Comment 7

4 months ago
It seems to me that we don't factor the parent opacity into for opacity animation on the compositor. A solution what I can think of is that

1) If we have opacity animation (and OMTA is eabled I guess), don't multiply |mOpacity| by |aOpacity|, just replace |mOpacity| with |aOpacity| in nsDisplayOpacity::ApplyOpacity()
2) Factor |aLayer->GetOpacity() into the value for SetShadowOpacity() in ApplyAnimatedValue() in AsyncCompositionManager.cpp

Actually I did try this way, and it seems to work, but honestly I am not familiar with around these code so there might be some side effects though.

Anyway, as dbaron said, I also think we should fix this bug after 57.
(Assignee)

Comment 8

3 months ago
The way I wrote in comment 7 does not work well in some cases.  That's because, if animating opacity value is less than 1.0, say 0.9, mOpacity of nsDisplayOpacity already factors the animating value into.
(Assignee)

Comment 9

3 months ago
Ok. we assume that opacity value on the compositor has already incorporated the parent opacity value there. 

In case of non-animating opacity, in nsDisplayOpacity::BuildLayer, We pass mOpacity, which is mFrame->StyleEffects()->mOpacity, to the layer.

  container->SetOpacity(mOpacity);

And in CompositorBridgeParent::SetShadowProperties,

  layerCompositor->SetShadowOpacity(layer->GetOpacity());                 
  layerCompositor->SetShadowOpacitySetByAnimation(false);

(Note that the opacity value set here will be override by animating opacity if opacity animation exists, that's the problem)

So, incorporating the parent opacity into each opacity value in each keyframes before sending the animation animation to the compositor is an easy fix and also seems to be a right thing to do.  One thing I am concerned is that it might cause small calculation errors, but it's definitely small I think.

https://hg.mozilla.org/try/rev/137b0b3bd4fa321935d20a25db92c1c41d08b4bc
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 10

3 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> Ok. we assume that opacity value on the compositor has already incorporated

This is totally wrong. Sorry for the noise.
(Assignee)

Comment 11

3 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > Ok. we assume that opacity value on the compositor has already incorporated
> 
> This is totally wrong. Sorry for the noise.

This is not totally wrong, is correct in some cases.   I did misunderstand what ShouldFlattenAway() does.  I thought it flattens children into the parent, but actually it flattens the parent to children.

So, if there is an element with non-animated opacity and a child element with an opacity animation, the parent non-animated opacity is flattens into the child opacity, as a result mOpacity value for the child includes the parent one.
Comment hidden (mozreview-request)

Comment 14

3 months ago
mozreview-review
Comment on attachment 8920014 [details]
Bug 1395151 - Don't apply ancestors' opacity if the opacity item has an async opacity animation.

https://reviewboard.mozilla.org/r/190992/#review196170

Thanks for doing this. Looks good to me but Matt will know if the handling of display items is correct.

::: commit-message-a0486:4
(Diff revision 1)
> +Bug 1395151 - Incorporate ancestors opacity values into the animation opacity value in each keyframe before sending the animation to the compositor if the ancestors were flattened. r?birtles,mattwoodrow
> +
> +Once ancestor display items are flattened into an nsDisplayOpacity with
> +animations, the flattened opacity values are hardly extracted from

I suppose s/hardly extracted/hard to extract/ unless I misunderstand this.

::: layout/painting/nsDisplayList.h:4292
(Diff revision 1)
> +  // Opacity value for all flattened ancestors.
> +  // This value is incorporated into animation value when we send the animation
> +  // to the compositor, since the animation values does not have this flattened
> +  // opacity value.
> +  float mFlattenedOpacity;

I don't follow what "animation value" means here (and I haven't read the rest of the patch yet). Does it mean the endpoints used for animation? I guess so?

So I think should be something like:

"This value is incorporated into the animation parameters we set up when we send animations to the compositor."

I was going to say "interpolation endpoints" instead of "animation parameters" but now that I've read more of the patch I see this is used for the base style too.

::: layout/painting/nsDisplayList.cpp:471
(Diff revision 1)
> +      aAnimatable =
> +        aAnimationValue.GetOpacity() *
> +          static_cast<const nsDisplayOpacity*>(aItem)->FlattenedOpacity();

(I wish there was a safer way of doing this--other than using templates. Maybe Matt knows if there's a suitable assertion we can use here to be sure we're dealing with a nsDisplayOpacity. Alternatively, we could just pass the flattenedOpacity all the way down from where we call AddAnimationsForProperty I guess.)

::: layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent-ref.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <meta http-equiv="content-type" content="text/html; charset=UTF-8">

(I think <meta charset=utf-8> is sufficient)

::: layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.html:19
(Diff revision 1)
> +  opacity : .001;
> +  height     : 100%;
> +  width      : 100%;

(I'm not sure if we were trying to line these up--we normally don't--but they don't.)

::: layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.html:27
(Diff revision 1)
> +}
> +
> +.error > span {
> +  animation  : test-anim 1s linear infinite;
> +  background : #f00;
> +  content    : "";

Does 'content' actually do anything here? (and in the reference file too for that matter)

I thought it only took effect on pseudo elements?

::: layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.html:29
(Diff revision 1)
> +  height     : 100%;
> +  width      : 100%;

I wonder if we can rework this test to use a width/height other than 100%? e.g. 100px x 100px?

My concern is that this is supposed to be testing compositor animations but we might not run the test on the compositor if the layer size is too bug.
Attachment #8920014 - Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 16

3 months ago
Thanks for the review!

(In reply to Brian Birtles (:birtles) from comment #14)
> :::
> layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.
> html:27
> (Diff revision 1)
> > +}
> > +
> > +.error > span {
> > +  animation  : test-anim 1s linear infinite;
> > +  background : #f00;
> > +  content    : "";
> 
> Does 'content' actually do anything here? (and in the reference file too for
> that matter)
> 
> I thought it only took effect on pseudo elements?

Right. Dropped it.

> :::
> layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.
> html:29
> (Diff revision 1)
> > +  height     : 100%;
> > +  width      : 100%;
> 
> I wonder if we can rework this test to use a width/height other than 100%?
> e.g. 100px x 100px?
> 
> My concern is that this is supposed to be testing compositor animations but
> we might not run the test on the compositor if the layer size is too bug.

Indeed. Changed to 100px overall.

Comment 17

3 months ago
mozreview-review
Comment on attachment 8920014 [details]
Bug 1395151 - Don't apply ancestors' opacity if the opacity item has an async opacity animation.

https://reviewboard.mozilla.org/r/190992/#review197114

Would it be simpler to make nsDisplayOpacity::CanApplyOpacity() return false if we have an async animation?
(Assignee)

Comment 18

3 months ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 8920014 [details]
> Bug 1395151 - Incorporate ancestors opacity values into the animation
> opacity value in each keyframe before sending the animation to the
> compositor if the ancestors were flattened.
> 
> https://reviewboard.mozilla.org/r/190992/#review197114
> 
> Would it be simpler to make nsDisplayOpacity::CanApplyOpacity() return false
> if we have an async animation?

Oh right, it should work.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5739ec536df02455fee2fc95109019406a6cf72
(Assignee)

Updated

3 months ago
Attachment #8920014 - Flags: review?(matt.woodrow)
Comment hidden (mozreview-request)

Comment 20

3 months ago
mozreview-review
Comment on attachment 8920014 [details]
Bug 1395151 - Don't apply ancestors' opacity if the opacity item has an async opacity animation.

https://reviewboard.mozilla.org/r/190992/#review197386
Attachment #8920014 - Flags: review?(matt.woodrow) → review+

Updated

3 months ago
Priority: -- → P3

Comment 22

3 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09f25b91132e
Don't apply ancestors' opacity if the opacity item has an async opacity animation. r=birtles,mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/09f25b91132e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Too late for 57
status-firefox57: affected → wontfix
Flags: qe-verify+

Comment 25

2 months ago
I have reproduced this issue using Firefox  57.0a1 (build ID=20170830220349) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b5 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
(Assignee)

Updated

8 hours ago
Duplicate of this bug: 1291309
You need to log in before you can comment on or make changes to this bug.