1229317 - weird animations behaviour

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[inscription]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151105173914

Steps to reproduce:

FF dev Edition with e10s enabled
go to http://www.streamroot.io/jobs
scroll to “Full time offers”
Hover the offers.


Actual results:

The animations shows weird artefacts on the edges, the back of the cards is not displayed.


Expected results:

Smooth animation, showing the back of the card after flipping it.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: break web page


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db

Regressed by; Bug 1097464

Comment 2

[Alice0775 White]

oops

Comment 3

[Tracy Walker [:tracy]]

I see the same behavior (no backs of cards) in non-e10s window.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Bug 1097464 was backed out from Fx44, which should be available on the beta channel in the next day or so. Fx45+ remain affected, however.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 45 and 46 since this is a recent regression (from bug 1097464) 
Thinker, is this something you can help with? Thanks.

Comment 6

[Thinker Li [:sinker]]

I will look into it.

Comment 7

[Thinker Li [:sinker]]

Just a note.  We would not build a layer if an item is backface hidden and showing backface.  So, there is no corresponding layer on layer tree while compositor playing animation.  It causes the layer never showed even it turning foreface to the user.

Comment 8

[Thinker Li [:sinker]]

Hi Robert and David,

The case here is with a DOM tree looking like

<style>
 .container {
   transform-style: preserve-3d;
   opacity: 0.9;
 }
 .front {
   backface-visibility: hidden;
 }
 .back {
   transform-style: preserve-3d;
   transform: rotateY(-180deg);
   backface-visibility: hidden;
 }
</style>
<div id="container">
 <div id="front"> ... </div>
 <div id="back"> ... </div>
</div>

see http://www.w3.org/TR/css3-transforms/#transform-style-property
I have found opacity is not on the list here, but gecko forces it as a layer with an intermediate buffer, meaning override preserve-3d.  And, our implementation seems reasonable.  But, it make this page broken, |back| is never seen.

So, my question is should we fix the code or fix the w3c spec? Or any other suggestion.

Comment 9

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

'opacity' is on the list in the editor's draft at https://drafts.csswg.org/css-transforms/#grouping-property-values .  (And you should generally view the editor's draft as more up-to-date than the draft on TR, although it might occasionally have more errors -- like nightly vs. release.)  Does that answer your question?

Comment 10

[Thinker Li [:sinker]]

David, yes!  Thank you!  I think we could mark this as wontfix.

inscription,
http://www.streamroot.io/jobs should remove opacity from the style for |obj-offer| class.

Comment 11

[Thinker Li [:sinker]]

I would like to mark this bug wontfix.  How do you think?

Comment 12

[inscription]

Thanks for your answers. 
What's pretty curious is that I don't reproduce the bug anymore (FF 45 dev edition) even with the opacity property.
Did you guys change anything ? (in this case the bug should rather be marked as fixed instead of wontfix I guess)

Comment 13

[Alice0775 White]

I can still reproduce the problem on Aurora45.0a2 as well as Nightly46.0a1.

If I disabled HWA, the problem is gone.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

I usually leave that up to the engineers. If you feel it passes the spec and there's nothing to be fixed on our end then go ahead.

Comment 15

[Thinker Li [:sinker]]

Alice, how do you disable HWA?  I would like to know and try.

Comment 16

[Alice0775 White]

(In reply to Alice0775 White from comment #13)
> I can still reproduce the problem on Aurora45.0a2 as well as Nightly46.0a1.
> 
> If I disabled HWA, the problem is gone.

Oops,
If I disabled *both HWA and e10s*, the problem is gone.


(In reply to Thinker Li [:sinker] from comment #15)
> Alice, how do you disable HWA?  I would like to know and try.


Option  > Advanced > General > un-ckeck "Use hardware acceleration when available"
Option  > General > un-ckeck "Enable multi-process Nightly"
Restart

Comment 17

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

(In reply to Alice0775 White from comment #16)
> (In reply to Alice0775 White from comment #13)
> > I can still reproduce the problem on Aurora45.0a2 as well as Nightly46.0a1.
> > 
> > If I disabled HWA, the problem is gone.
> 
> Oops,
> If I disabled *both HWA and e10s*, the problem is gone.

If there are behavior differences between these configurations, then there's still a bug somewhere.

Comment 18

[inscription]

This is getting really weird : today the problem is back … but the symptoms are not the same from one time to another.
I load the page once: the cards flip and I see nothing (blank) on the back of the card.
I reload the page several time and some time when a card flip, I can see the front of the card written backward …

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The change that caused this regression was backed out from Firefox 45. A current Developer Edition build should now work correctly, or you can wait until 45 goes to Beta some time next week.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

Comment 20

[Matt Woodrow (:mattwoodrow)]

I had a play with this:

The content is invisible when any OMTC compositor is enabled, the content is visible only when using non-OMTC BasicLayers.

This is because BasicLayerManager propagates transforms down through temporary surfaces, whereas all OMTC ones do not.

The simplest fix to get all LayerManagers to have the same behaviour is to change nsIFrame::Extend3DContext to detect opacity and disable preserve-3d for those frames. That seems safer than relying on the LayerManagers to disable it by accident because of an opacity layer being in the middle.

We still need to decide if this is what we want though, since it's a change in behaviour from what we had previously.

The latest spec draft says to disable preserve-3d for opacity, but neither Chrome nor Safari are doing this on my machine.

We have a clear example of how this would break web-compat, so it's not obvious that we'd want to do this. Do we know how many other websites would be affected by this change?

I would prefer to avoid making changes in our behaviour while we're in a rush, but I don't have any ideas right now for retaining the previous behaviour.

Comment 21

[Matt Woodrow (:mattwoodrow)]

I had an idea for a compromise (that could go into the spec), that would fix this page.

If opacity is set on the *root* element of a preserve-3d chain, then we should be able to render the entire preserve-3d with opacity. It's only when opacity is set on an intermediate node that we don't have any way to get a surface representing that subtree.

In terms of gecko code that would mean updating Extend3DContext to check for this, and create the nsDisplayOpacity after the nsDisplayTransforms within BuildDisplayListForStackingContext.

roc, Thinker, does that seem workable?

Comment 22

[Matt Woodrow (:mattwoodrow)]

I think we can also change the BasicLayerManager behaviour to match that of the OMTC compositors.

We previously had different behaviour since it was more efficient, but non-OMTC is incredibly rare now, so I think we'd benefit from the consistency.

Comment 23

[Thinker Li [:sinker]]

Matt, if you mean making Extend3DContext() returning false for opacity < 1, I think it would work.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Attachment: opacity-preserve3d

This is basically what I was thinking, though the clipping is still buggy.

It disallows preserve-3d on any node except the root one, puts nsDisplayOpacity outside the transform when the transform is the root of a preserve-3d context, and makes sure layers doesn't propagate opacity through preserve-3d contexts.

The layers piece could be more efficient since we technically can propagate opacity if the preserve-3d context only has a single leaf, I doubt it's worth adding that complexity right now.

We could also allow opacity on preserve-3d intermediate nodes if there is only a single leaf underneath that node, but trying to spec that sounds like insanity.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Note that we can't unconditionally move opacity to be outside transform since opacity on a preserve-3d leaf needs to be inside the transform to avoid interfering with the chain.

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Thinker Li [:sinker] from comment #23)
> Matt, if you mean making Extend3DContext() returning false for opacity < 1,
> I think it would work.

That was my initial though, and it would make our behaviour match the draft spec.

It still leaves this website broken, even though it works in Chrome and Safari. I don't like the idea of breaking real websites, and we might be better off pushing back on the spec change.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> I had an idea for a compromise (that could go into the spec), that would fix
> this page.
> 
> If opacity is set on the *root* element of a preserve-3d chain, then we
> should be able to render the entire preserve-3d with opacity. It's only when
> opacity is set on an intermediate node that we don't have any way to get a
> surface representing that subtree.
> 
> In terms of gecko code that would mean updating Extend3DContext to check for
> this, and create the nsDisplayOpacity after the nsDisplayTransforms within
> BuildDisplayListForStackingContext.
> 
> roc, Thinker, does that seem workable?

Yes, that sounds good.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8713423 [details] [diff] [review]
opacity-preserve3d

Review of attachment 8713423 [details] [diff] [review]:
-----------------------------------------------------------------

This'll need a test.

::: gfx/thebes/gfxUtils.cpp
@@ +1556,5 @@
>  }
>  
>  /* static */ bool
>  gfxUtils::DumpDisplayList() {
> +  return gfxPrefs::LayoutDumpDisplayList() || XRE_IsContentProcess();

I don't think you want to land this

::: layout/generic/nsFrame.cpp
@@ +2274,5 @@
>            GetContainingBlock()->GetContent()->GetPrimaryFrame(), &resultList));
>      }
>  
>    }
> + 

trailing whitespace

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: opacity-preserve3d

This includes a workaround for the fact that PostProcessLayers doesn't handle preserve-3d properly.

I'm going to file a follow-up to handle this properly, but I think it's reasonable to do the simple thing in an already complex patch that we want to uplift.

Comment 30

[Matt Woodrow (:mattwoodrow)]

In terms of spec compliance, this actually matches the current draft.

In the draft spec, transform-style:flat establishes a 3d rendering context and transform-style:preserve-3d extends one. Opacity forces a value of preserve-3d to be treated as flat, which ensures that the element with opacity is the root of the rendering context.

The TR doesn't mention opacity as a reason to flatten.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8714578 [details] [diff] [review]
opacity-preserve3d

Review of attachment 8714578 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/transform-3d/preserve3d-7-ref.html
@@ +5,5 @@
> +    <div style="opacity:0.5">
> +    <div style="transform: rotatey(90deg); transform-style: preserve-3d; width:100px;">
> +        <div style="transform: rotatey(90deg); width: 100px; height: 100px; background-color: #00FF00"></div>
> +    </div>
> +</div>

fix indent

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e751d638ab4b

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc0852c8651 for, let's see,

https://treeherder.mozilla.org/logviewer.html#?job_id=21197663&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21199224&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21198073&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21198134&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21198199&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21199142&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=21199265&repo=mozilla-inbound

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/258389005e0c

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/258389005e0c

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Matt this looks like a lot of changes for late in aurora, but should it be going along with the preserve-3d changes in 46?

Comment 37

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8714578 [details] [diff] [review]
opacity-preserve3d

Approval Request Comment
[Feature/regressing bug #]: Bug 1097464
[User impact if declined]: Broken rendering on some 3d websites.
[Describe test coverage new/current, TreeHerder]: Reftest added, had a few weeks on m-c
[Risks and why]: Fairly low, tries to go back to our previous behaviour regarding opacity+preserve-3d.
[String/UUID change made/needed]: None

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8714578 [details] [diff] [review]
opacity-preserve3d

Adds a test along with more fixes for preserve-3d.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9253c17568e0