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

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P3)

Description

[r.otten]

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.

Comment 1

[Alice0775 White]

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

Comment 2

[Alice0775 White]

indeed, setting layers.offmainthreadcomposition.async-animations=false fixes the problem.

Comment 3

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

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.

Comment 4

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

Attachment: reporter's testcase

(with extra junk from jsfiddle removed)

Comment 5

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

bug 1291309 and *maybe* bug 1213253 could be other (more complex) versions of this bug.

Comment 6

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

(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...)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(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 12

[Hiroyuki Ikezoe (:hiro)]

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1395151 - Don't apply ancestors' opacity if the opacity item has an async opacity animation.

Once ancestor display items are flattened into an nsDisplayOpacity with
animations, the flattened opacity values are hardly extracted from
nsDisplayOpacity::mOpacity since the mOpacity includes animating value at
*this* moment.  Also the mOpacity is used if the opacity animation is not able
to be sent to the compositor.  So in this patch, we introduce a new variable,
mFlattenedOpacity to store the flattened opacity values for the ancestors along
with mOpacity.

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

Comment 14

[Brian Birtles (:birtles)]

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.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190992/diff/1-2/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

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

[Matt Woodrow (:mattwoodrow)]

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?

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 19

[Hiroyuki Ikezoe (:hiro)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190992/diff/2-3/

Comment 20

[Matt Woodrow (:mattwoodrow)]

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Thank you :mattwoodrow!
A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f734317ee9e4e1a92909cda52744257003438f48

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/09f25b91132e

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Too late for 57

Comment 25

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.