Closed Bug 1395151 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Web Painting, defect, P3)

Product:
Core
Component:
Web Painting
Version:
41 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: r.otten, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(2 files)

reporter's testcase
911 bytes, text/html
Details
Bug 1395151 - Don't apply ancestors' opacity if the opacity item has an async opacity animation.
59 bytes, text/x-review-board-request
birtles
: review+
mattwoodrow
: review+
Details 
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
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: enable-omt-animations
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Keywords: regressionwindow-wanted → regression
indeed, setting layers.offmainthreadcomposition.async-animations=false fixes the problem.
Version: 55 Branch → 41 Branch
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.
Attached file 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...)
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.
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.
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
(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.
(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 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+
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 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?
(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
Attachment #8920014 - Flags: review?(matt.woodrow)
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+
Priority: -- → P3
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
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: qe-verify+
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: