Closed Bug 1229317 Opened 4 years ago Closed 4 years ago

weird animations behaviour

Categories

(Core :: Layout, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 + unaffected
firefox46 + fixed
firefox47 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: inscription, Assigned: mattwoodrow)

References

Details

(Keywords: regression, Whiteboard: [2016-GBT-N])

Attachments

(1 file, 1 obsolete file)

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.
[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
Blocks: 1097464
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 44 Branch → 42 Branch
oops
Version: 42 Branch → 44 Branch
I see the same behavior (no backs of cards) in non-e10s window.
Summary: weird animations behaviour using e10s → weird animations behaviour
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.
Tracking for 45 and 46 since this is a recent regression (from bug 1097464) 
Thinker, is this something you can help with? Thanks.
Flags: needinfo?(tlee)
I will look into it.
Flags: needinfo?(tlee)
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.
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.
Flags: needinfo?(roc)
Flags: needinfo?(dbaron)
'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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(roc) → needinfo?(inscription)
I would like to mark this bug wontfix.  How do you think?
Flags: needinfo?(lhenry)
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)
Flags: needinfo?(inscription) → needinfo?(tlee)
I can still reproduce the problem on Aurora45.0a2 as well as Nightly46.0a1.

If I disabled HWA, the problem is gone.
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.
Flags: needinfo?(lhenry)
Alice, how do you disable HWA?  I would like to know and try.
Flags: needinfo?(tlee) → needinfo?(alice0775)
(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
Flags: needinfo?(alice0775)
(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.
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 …
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
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.
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?
Flags: needinfo?(tlee)
Flags: needinfo?(roc)
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.
Matt, if you mean making Extend3DContext() returning false for opacity < 1, I think it would work.
Flags: needinfo?(tlee)
Attached patch opacity-preserve3d (obsolete) — Splinter Review
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.
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.
(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.
(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.
Flags: needinfo?(roc)
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
Assignee: nobody → matt.woodrow
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.
Attachment #8714578 - Flags: review?(roc)
Blocks: 1244943
Attachment #8713423 - Attachment is obsolete: true
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 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
Attachment #8714578 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/258389005e0c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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?
Flags: needinfo?(matt.woodrow)
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
Flags: needinfo?(matt.woodrow)
Attachment #8714578 - Flags: approval-mozilla-aurora?
Depends on: 1250718
Comment on attachment 8714578 [details] [diff] [review]
opacity-preserve3d

Adds a test along with more fixes for preserve-3d.
Attachment #8714578 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.