CSS 3D transform visibility is broken after landing 806256

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: alice0775, Assigned: mattwoodrow)

Tracking

({regression})

19 Branch
mozilla20
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+ unaffected, firefox20+ verified)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

7 years ago
This is spin off from bug 807563 

Screen shot;
https://bugzilla.mozilla.org/attachment.cgi?id=678183

Steps to reproduce:
1. Open http://desandro.github.com/3dtransforms/examples/carousel-01.html

Actual results:
Some planes(#3 and #8) are missing at the time of initial load of the page

Expected results:
All planes should draw

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/be0d3f1de9c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121031180512
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/01123067aa58
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121031182652
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=be0d3f1de9c2&tochange=01123067aa58
Assignee: nobody → matt.woodrow
Since we know of major web properties that use 3d transforms for galleries, it makes sense to track this FF19 regression.
Assignee

Comment 2

7 years ago
Alright, think I understand this now.

The regression happened when we stopped calling ComputePreserve3DChildrenOverflow every time we reflowed a preserve-3d frame, and only called it should actually come up with something new.

Unfortuntately, because of the way that we handle overflow rects within a preserve-3d chain, other code that collates the overflow areas of child frames (nsBlockFrame::ComputeOverflowAreas, ConsiderChildOverflow at least, not sure how many others there are) don't actually compute these correctly.

We previously let these compute the wrong values, and then always overwrote them with the right ones.

Finding all the places we do this and making them correct would fix this.

Another alternatives is to change overflow rects to be 3d. This handles the cases where an intermediate frame in a preserve-3d chain has a singular transform, and thus has an empty overflow rect, but the combined chain isn't singular. We currently handle this by lying about the overflow rects, and storing them transformed up to the root of the preserve-3d chain instead of relative to the current frame's transform.

The reason the change was made initially was because when we have a preserve-3d parent with a large number of children, and we change *all* the child transforms, we do them one by one, and recurse all children updating overflows each time. We could try find a way to coalesce these and avoid the O(n^2) behaviour that way instead.

roc: Any thoughts on the above ideas? If we go for the first option, then I might need help finding all the code that needs changing.
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Unfortuntately, because of the way that we handle overflow rects within a
> preserve-3d chain, other code that collates the overflow areas of child
> frames (nsBlockFrame::ComputeOverflowAreas, ConsiderChildOverflow at least,
> not sure how many others there are) don't actually compute these correctly.
> 
> We previously let these compute the wrong values, and then always overwrote
> them with the right ones.

Can you explain further why this isn't adequate?
Assignee

Comment 4

7 years ago
Overwriting no longer works, because we no longer call it all the time.

As for accumulating the overflow, for normal frames we accumulate the combined overflow areas of all children, and then transform that by the frame's transform (if there is any).

For a preserve-3d frame, some children might have overflow rects already in our coordinate space, and others not. So we need to transform (or not transform) each of the child overflow rects individually and accumulate the post-transform regions instead.
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Overwriting no longer works, because we no longer call it all the time.

How about ensuring that ComputePreserve3DChildrenOverflow runs whenever any preserve-3d descendant's overflow area changes?
Assignee

Comment 6

7 years ago
That has O(N^2) behaviour when we change the transform on N descendants. This was the original reason for terrible performance on the periodic table demo.
That's because we have a lot of UpdateTranformLayer hints and a lot of UpdateOverflow calls but no reflow, right?

> We could try find a way to coalesce these and avoid the O(n^2) behaviour that way
> instead.

So I guess we should try this then.

How about having nsCSSFrameConstructor::ProcessRestyledFrames collect a set of frames that need UpdateOverflow called on them (and their ancestors updated), and then process that set in the "right" order?

One way to do that would be to use a worklist of (frame, depth in frame tree), keeping it sorted by depth in frame tree. Then you can repeatedly pull a deepest frame from the worklist, call UpdateOverflow on it, and if that returns true add its parent to the worklist (if it's not already there).
Assignee

Comment 8

7 years ago
GetDepthInFrame tree could potentially be slow, not sure if it's worth computing this and caching it though.

nsTArray also isn't the best for storage, but priority queue/heap doesn't provide a good way to implement Contains(), and our linked list implementation (from mfbt) doesn't implement sorted insertions. I guess that could be changed though.
Attachment #688993 - Flags: review?(roc)
Comment on attachment 688993 [details] [diff] [review]
Add a helper class to coalesce frames that need their overflow updated

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

::: layout/base/RestyleTracker.h
@@ +70,5 @@
> +      
> +      // If the overflow changed, then we want to also update the parent's
> +      // overflow
> +      if (overflowChanged || entry.mFrame->UpdateOverflow()) {
> +        AddFrameInternal(entry.mFrame->GetParent(), false);

If you add the depth as a parameter to AddFrameInternal, then you can pass entry.mDepth - 1 here and avoid a lot of calls to GetDepthInFrameTree.

@@ +116,5 @@
> +    }
> +
> +    Entry entry(aFrame, aInitial);
> +    if (mEntryList.BinaryIndexOf(entry) == mEntryList.NoIndex) {
> +      mEntryList.InsertElementSorted(entry);

Wouldn't we need to set an existing element's mInitial in some cases?

@@ +121,5 @@
> +    }
> +  }
> +
> +  /* A list of frames to process, sorted by their depth in the frame tree */
> +  nsTArray<Entry> mEntryList;

Make this a max-heap. Then insertion, and removing a maximal element, can be done in log(N) time. nsTArray has helpers for this.
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> nsTArray also isn't the best for storage, but priority queue/heap doesn't
> provide a good way to implement Contains(),

Oh, I missed that. Hmm.

I think you can use a max-heap ordered by depth and then frame pointer. Then you can simply not do a Contains() check and allow the same frame to be inserted multiple times. When you remove the max element (which is always at element 0 in the nsTArray), you can check if the new max element is the same frame, and keep removing the max element while it is the same frame.

An alternative would be to use some kind of balanced binary tree, but I don't know of one in our code already.
Comment on attachment 688994 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times

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

I think you uploaded the wrong patch here.
Assignee

Comment 13

7 years ago
Attachment #688993 - Attachment is obsolete: true
Attachment #688993 - Flags: review?(roc)
Attachment #689129 - Flags: review?(roc)
Comment on attachment 689129 [details] [diff] [review]
Add a helper class to coalesce frames that need their overflow updated v2

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

Looks good but that !=/== issue is worrying...

::: layout/base/RestyleTracker.h
@@ +30,5 @@
> +{
> +public:
> +
> +  /**
> +   * Add a frame that has had a style changes, and needs its

a style change

@@ +54,5 @@
> +  void Flush() {
> +    while (!mEntryList.IsEmpty()) {
> +      Entry entry = mEntryList.Pop();
> +
> +

Remove one of these blank lines

@@ +58,5 @@
> +
> +      // Pop off any duplicate entries and copy back mInitial
> +      // if any have it set.
> +      while (!mEntryList.IsEmpty() &&
> +             mEntryList.Top().mFrame != entry.mFrame) {

Shouldn't this line be == ?
Assignee

Comment 16

7 years ago
Yes, it definitely should be ==. I moved some code around and forgot to reverse it. Interestingly that didn't ruin performance on the periodic table demo...
Attachment #689129 - Attachment is obsolete: true
Attachment #689129 - Flags: review?(roc)
Attachment #689135 - Flags: review?(roc)
Assignee

Updated

7 years ago
Duplicate of this bug: 815040
https://hg.mozilla.org/mozilla-central/rev/767accc64cf8
https://hg.mozilla.org/mozilla-central/rev/0319dd845d53
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Looking for an aurora uplift nom here so we don't ship this regression ever.
Assignee

Comment 21

7 years ago
Comment on attachment 689130 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times v2

This approval request is for both patches on this bug.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 806256
User impact if declined: Flicking / missing content on pages with 3d transforms
Testing completed (on m-c, etc.): Been on m-c for over a week.
Risk to taking this patch (and alternatives if risky): Low risk, just coalesces unnecessarily repeated function calls.
String or UUID changes made by this patch: None.
Attachment #689130 - Flags: approval-mozilla-aurora?
Reporter

Updated

7 years ago
Depends on: 822906
Attachment #689130 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 22

7 years ago
We should probably wait until bug 822906 is resolved before uplifting this.
(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> We should probably wait until bug 822906 is resolved before uplifting this.

We're still waiting on this for the uplift to FF19. Note that FF20 (now on Aurora) is impacted by bug 822906 though.
Attachment #689130 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta?
[Relman]We sill need to land patch in Bug 822906 (just landed on mozilla-inbound) and the patch in this bug, at the same time.

Updated

6 years ago
Depends on: 832611
Comment on attachment 689130 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times v2

Approving for Beta alongside bug 822906. Please land asap today to make it into Beta 3.
Attachment #689130 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #689130 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
We're going to be backing out bug 806256 and bug 807563 from FF19, so this bug won't be necessary.
Attachment #689130 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I'd reproduce this issue on Nightly it was reported:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0(20121127030907)

I couldn't reproduce this issue on FF 19b4 and Latest Aurora 20 on Windows 7 x64, Ubuntu x64 and Mac OS 10.8 x64 as following attachments shows:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0(20130130080006)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130130 Firefox/20.0(20130130042019)

Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0(20130130080006)
Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130130 Firefox/20.0
(20130130042019)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0   20130130080006
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130130 Firefox/20.0   20130130042019
Posted image Linux
Based on my verifications done in Comment 29 30 31 and 32 I confirm this fix is verified.
Status: RESOLVED → VERIFIED
Depends on: 840480
You need to log in before you can comment on or make changes to this bug.