Closed Bug 1250718 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

46 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- unaffected
firefox46 + wontfix
firefox47 + wontfix
firefox48 + verified

People

(Reporter: alice0775, Assigned: mattwoodrow)

References

Details

(Keywords: regression)

Attachments

(7 files, 2 obsolete files)

+++ 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
Flags: needinfo?(tlee)
Flags: needinfo?(roc)
Flags: needinfo?(matt.woodrow)
[Tracking Requested - why for this release]: webpage layout broken.
Attached file Reduced testcase
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(tlee)
Flags: needinfo?(roc)
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?
Flags: needinfo?(tlee)
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
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.
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?
Flags: needinfo?(tlee)
I've sent an email to www-style asking for clarification on this. We might end up having to copy the Chrome/Safari behaviour.
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.
These tests match the behaviour of WebKit. Blink is identical for all except 3, where it's doing something weird with sorting.
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)
Flags: needinfo?(dbaron)
Attached patch Support opacity with preserve-3d (obsolete) — Splinter Review
Attachment #8730052 - Flags: review?(tlee)
Attached patch Support opacity with preserve-3d (obsolete) — Splinter Review
Rebased patch.
Attachment #8730052 - Attachment is obsolete: true
Attachment #8730052 - Flags: review?(tlee)
Attachment #8730078 - Flags: review?(tlee)
Just adding a few things that were useful for debugging this.
Attachment #8730079 - Flags: review?(tlee)
Fixed rebase issues so that it compiles again.
Attachment #8730078 - Attachment is obsolete: true
Attachment #8730078 - Flags: review?(tlee)
Attachment #8730081 - Flags: review?(tlee)
Blocks: 1255710
Duplicate of this bug: 1255725
(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.
Flags: needinfo?(dbaron)
(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 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.
Attachment #8730079 - Flags: review?(tlee) → review+
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|
Attachment #8730081 - Flags: review?(tlee) → review+
https://hg.mozilla.org/mozilla-central/rev/a099eced7f04
https://hg.mozilla.org/mozilla-central/rev/849e85ce8561
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Still crashing with STR in bug 1255130, but the signature has changed, it's now [@ mozilla::layers::ShmemTextureData::Create ]
Mathieu, could you please try nightly?

https://nightly.mozilla.org/

I can't reproduce the crash with the STR in bug 1255130 on linux64.
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
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.
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
Depends on: 1271058
Depends on: 1278021
Flags: qe-verify+
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)
Status: RESOLVED → VERIFIED
Resolution: FIXED → WORKSFORME
Resolution: WORKSFORME → FIXED
Blocks: 1283827
You need to log in before you can comment on or make changes to this bug.