1250718 - Nightly47 doesn't show correctly after 3d animation. And Aurora46 doesn't show the menu at all

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

+++ This bug was initially created as a clone of Bug #1038652 +++


Steps to reproduce:
1. Go to http://lacigrona.com/carta/comida/
2. Click on "OPEN MENU >" button

Actual Results:
Aurora46 doesn't show the menu at all at step1
Nightly47 doesn't show menu correctly after 3d animation step2

Expected Results:
The menu items should be displayed on all 3 pages instead of the middle page.


#1 Regression window(doesn't show the menu at all):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54

#1 Regressed by: Bug 1097464


#2 Partially progression(doesn't show menu correctly after 3d animation):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d1e05b9116cc9725951440e05c65b2e57fea881f&tochange=258389005e0ce17f81d80e733d78f99bf273ec3c

#2 Regressed by : Bug 1229317

Comment 1

[Alice0775 White]

Attachment: Screenshot Aurora46 .VS. Beta45

Comment 2

[Alice0775 White]

Attachment: Screenshot Nightly47 .VS. Beta45

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]: webpage layout broken.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Reduced testcase

Comment 5

[Matt Woodrow (:mattwoodrow)]

So this test is very similar to bug 1229317, where opacity is being set on a frame with preserve-3d.

The previous bug had opacity on the preserve-3d root, which we solved, this one has it on an intermediate frame.

This is technically invalid under the latest spec draft, but it still works in both chrome and safari.

I don't see how it's possible to support both 3d plane sorting, and opacity for preserve-3d subtrees. I'll try come up with a testcase to see what behaviour chrome and safari have for this.

This is going to be quite hard to fix with our current code.

The two options I've thought of so far:

* Track when we get opacity on an intermediate preserve-3d frames, and create multiple nsDisplayTransforms at each ancestor above it. One for before the opacity, one wrapping the opacity, and one for after. We'd push the wrapping nsDisplayTransforms down so that they are on the inside on the nsDisplayOpacity.

This is very similar to what we used to do and would involve bringing most WrapPreserve3DList back (ew).

* Have the compositor process the layer tree to detect and handle this case. We'd need to have the layer sorting code detect which layers are grouped together with opacity and make sure they stay consecutive. We'd also need to temporarily setup an intermediate surface (with opacity) for compositing for the correct subset of the layer tree. DefaultComputeEffectiveTransforms would need to know about how to continue to propagate preserve-3d transforms through opacity layers.

I think this is slightly nicer (maybe?), but it's all new code.

The other alternative is to just leave this broken, and ask the site to change. Do we have any way of finding out how common this is, other than just waiting for regressions to be filed?

roc, thinker, any better ideas here?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Example layer tree: https://pastebin.mozilla.org/8860954

Comment 7

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> I don't see how it's possible to support both 3d plane sorting, and opacity
> for preserve-3d subtrees. I'll try come up with a testcase to see what
> behaviour chrome and safari have for this.

That would be interesting!

> * Have the compositor process the layer tree to detect and handle this case.
> We'd need to have the layer sorting code detect which layers are grouped
> together with opacity and make sure they stay consecutive. We'd also need to
> temporarily setup an intermediate surface (with opacity) for compositing for
> the correct subset of the layer tree. DefaultComputeEffectiveTransforms
> would need to know about how to continue to propagate preserve-3d transforms
> through opacity layers.

I like this a lot better.

I think we should probably do this. If it turns out that Safari/Chrome have really crazy behavior for this case, maybe we shouldn't do it.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Simplified testcase to show behaviour

This testcase shows the differences in behaviour quite clearly.

Firefox Nightly/Aurora is disabling the preserve-3d effect for the element with opacity. We still sort the 3 top-level nodes by depth though.

Chrome/Safari are applying the opacity to the nodes individually, not as a group.

Firefox Release is applying the opacity as a group (disabling preserve-3d), but it breaks sorting entirely.

Comment 9

[Thinker Li [:sinker]]

I see the same thing as Matt.  So, we can see this as a bug of the site, or we work around it.
How do you think Matt?

Comment 10

[Matt Woodrow (:mattwoodrow)]

I've sent an email to www-style asking for clarification on this. We might end up having to copy the Chrome/Safari behaviour.

Comment 11

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

Tracking, probably something we should settle one way or the other before 46 release. 

I was about to ask if we can tell how common this kind of use of opacity and layers is but then see Matt is asking the same question. If it were very common, maybe we would have seen similar bug reports before this.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Add reftests that match WebKits behaviour

These tests match the behaviour of WebKit. Blink is identical for all except 3, where it's doing something weird with sorting.

Comment 13

[Matt Woodrow (:mattwoodrow)]

I asked on www-style about getting this clarified and the TR spec updated. The spec author isn't interested in updating that, and wants to focus solely on the ED spec (which contains numerous web-compat issues, and isn't implemented by anyone). Given that, I'm working on doing my best to just match the observed behaviour of WebKit/blink since it seems like the pragmatic thing to do here.


Test 3 exposes some real weirdness with how blink and WebKit handle this.

WebKit appears to apply the opacity to each of the leaf layers individually, which seems really broken since it relies entirely on the implementation details of their layerization.

blink retains grouping around sets of layers that aren't transformed (second and third in opacity-preserve3d-3.html of attachment 8728771 [details] [diff] [review]), but gives up on sorting entirely within the opacity. It's pretty weird that they do most of the things for preserve-3d, except sorting.

My current patch does the best of both of those, maintains grouping in a way that isn't dependent on how FrameLayerBuilder decides to layerize, and also retains sorting for opacity within preserve-3d.

Clearly there's no consensus on this (and the spec author isn't interested in trying to spec anything here), so I think picking the 'best' behaviour is the right thing to do.

David, does that sound reasonable to you? (A high level answer is fine, and I'm happy to answer questions if you don't want to spend too long reading the various spec versions)

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Support opacity with preserve-3d

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Support opacity with preserve-3d

Rebased patch.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Improve layer logging

Just adding a few things that were useful for debugging this.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Support opacity with preserve-3d

Fixed rebase issues so that it compiles again.

Comment 19

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> So this test is very similar to bug 1229317, where opacity is being set on a
> frame with preserve-3d.
> 
> The previous bug had opacity on the preserve-3d root, which we solved, this
> one has it on an intermediate frame.
> 
> This is technically invalid under the latest spec draft, but it still works
> in both chrome and safari.

By invalid, you mean that https://drafts.csswg.org/css-transforms/#grouping-property-values says that it no longer is supposed to preserve 3D?

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> I asked on www-style about getting this clarified and the TR spec updated.
> The spec author isn't interested in updating that, and wants to focus solely
> on the ED spec (which contains numerous web-compat issues, and isn't
> implemented by anyone). Given that, I'm working on doing my best to just
> match the observed behaviour of WebKit/blink since it seems like the
> pragmatic thing to do here.

That seems reasonable.

> Test 3 exposes some real weirdness with how blink and WebKit handle this.

Which is test 3?

> WebKit appears to apply the opacity to each of the leaf layers individually,
> which seems really broken since it relies entirely on the implementation
> details of their layerization.
> 
> blink retains grouping around sets of layers that aren't transformed (second
> and third in opacity-preserve3d-3.html of attachment 8728771 [details] [diff] [review]
> [diff] [review]), but gives up on sorting entirely within the opacity. It's
> pretty weird that they do most of the things for preserve-3d, except sorting.

Neither of these seem great.  How confident are you in your understanding of their behavior?

> Clearly there's no consensus on this (and the spec author isn't interested
> in trying to spec anything here), so I think picking the 'best' behaviour is
> the right thing to do.
> 
> David, does that sound reasonable to you? (A high level answer is fine, and
> I'm happy to answer questions if you don't want to spend too long reading
> the various spec versions)

At a high level, that sounds reasonable, although I'm still a bit curious about the details.

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #19) 
> By invalid, you mean that
> https://drafts.csswg.org/css-transforms/#grouping-property-values says that
> it no longer is supposed to preserve 3D?

Yes, exactly.

> Which is test 3?

opacity-preserve3d-3.html in the latest patch.

> Neither of these seem great.  How confident are you in your understanding of
> their behavior?

It's based on my analysis of their rendering of the above tests. Webkit and blink both render differently to both each other, and us (with the patch applied) on test 3. WebKit matches us for 4, but blink is different (because of the broken sorting).

It's possible that there are other corner cases that I haven't thought of and covered with tests.

Comment 21

[Thinker Li [:sinker]]

Comment on attachment 8730079 [details] [diff] [review]
Improve layer logging

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +840,5 @@
>    LayerScopeAutoFrame frame(PR_Now());
>  
>    // Dump to console
>    if (gfxPrefs::LayersDump()) {
> +    this->Dump(/* aSorted= */true);

How about to have a preference to switch between true or false here.

Comment 22

[Thinker Li [:sinker]]

Comment on attachment 8730081 [details] [diff] [review]
Support opacity with preserve-3d

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

L

::: gfx/layers/Layers.cpp
@@ +1388,5 @@
>  #endif
>    } else {
>      float opacity = GetEffectiveOpacity();
>      CompositionOp blendMode = GetEffectiveMixBlendMode();
> +    if (opacity != 1.0f && (HasMultipleChildren() || Creates3DContextWithExtendingChildren()) && !Extend3DContext()) {

|!Extend3DContext()| could be moved to a quote surrounding |HasMultipleChildren()|.

::: layout/reftests/transform-3d/opacity-preserve3d-1.html
@@ +36,5 @@
> +    <div class="preserve" style="opacity:0.5">
> +        <div class="leaf second"></div>
> +        <div class="leaf fourth"></div>
> +    </div>
> +    <div class="leaf third" onclick="alert('yellow');"></div>

Why do we need this? |onclick|

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a099eced7f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/849e85ce8561

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a099eced7f04
https://hg.mozilla.org/mozilla-central/rev/849e85ce8561

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Note that bug 1255130 is a crasher bug that appears to be fixed by this, so it would be good to uplift this if possible.

Comment 27

[Mathieu Marquer]

Still crashing with STR in bug 1255130, but the signature has changed, it's now [@ mozilla::layers::ShmemTextureData::Create ]

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Mathieu, could you please try nightly?

https://nightly.mozilla.org/

I can't reproduce the crash with the STR in bug 1255130 on linux64.

Comment 29

[Mathieu Marquer]

I'm using nightly already (Build ID 20160318030236). Forgot to link to the crash report: https://crash-stats.mozilla.com/report/index/b8dbc58a-ab06-4612-9c31-c53d22160319

Comment 30

[Mathieu Marquer]

Same crash report with a new clean Firefox profile: https://crash-stats.mozilla.com/report/index/803dcbcd-1c56-4e72-b2e8-75dea2160319

Comment 31

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

We are heading into 46 beta 6 which I think is too late for uplift. 

Is the fix for this menu issue verifiable on nightly? Do you want to uplift this (even if it does not perfectly fix the crash in bug 1255130?) If you don't feel this is urgent then my preference is that this should ride with 48.

Comment 32

[Matt Woodrow (:mattwoodrow)]

I'd prefer to ride with 48 too.

This is a non-trivial change (both in terms of implementation and spec compliance), so the more time the better.

Comment 33

[Michelle Funches - QA]

Confirmation
Version 	48.0b1
Build ID 	20160606200529
Update Channel 	beta
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Multiprocess Windows 	0/2 (Disabled) and with Multiprocess Windows 	1/1 (Enabled by default)