691864 - 3d css does not work correctly with iframes

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Gregg Tavares]

Attachment: interactive-3d-css-element.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111004 Firefox/10.0a1
Build ID: 20111004030858

Steps to reproduce:

FF version: 10.0a1 (2011-10-04)

Visit this page 
http://greggman.com/downloads/examples/interactive-3d-css-element.html
or use the attached file


Actual results:

The iframe content is not interactive.


Expected results:

You should be able to click links, type in search boxes, etc.

Note: I know 3d css is brand new to Firefox. I just thought you'd find it useful to have a simple example.

Works in Firefox

Comment 1

[Boris Zbarsky [:bzbarsky]]

The attached testcase worksforme on Mac.  I can click links, type in the search box, etc.

Is the issue Linux-only?

Comment 2

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

It doesn't work for me on Linux, either in a debug build or in the 2011-10-07-03-12-27-mozilla-central x86_64 nightly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Testing it some more on Mac, I can click on the left sides of links and the textbox and things work, but if I click on their right sides the click is not registered.

(I also tried turning off hardware acceleration, and while that made the transformed content all jaggy it had no effect on the click behavior.)

Comment 4

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

Yeah, I just realized the same thing -- if I make the window narrower, I can interact with stuff on the left side of the page -- and as I narrow the window further, I can interact with more.  So it seems like there's one test that's off somehow.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  My window is normally about 900px wide, which probably counts as "narrow"...

Comment 6

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

Sounds like a bug in event handling logic

Comment 7

[Matt Woodrow (:mattwoodrow)]

Thanks for filing this.

I notice that if I resize my window so that the left edge of the page works, and the right edge doesn't, I can start selecting text from the left edge and drag into the right side and get accurate text selection.

Anyone have any ideas what different code paths are taken that could mean that the mouse point during text selection is correct, but wrong for mouse-over/click events? Would be nice to narrow down my search.

I'll take a proper look at this on Monday.

Comment 8

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

Setting gDumpEventList to true in nsLayoutUtils.cpp may be a reasonable way to start debugging.  When I did that briefly this morning it seemed (at a quick glance -- could be wrong) like the decision on whether to *build* the display list inside the iframe was incorrect, but if we decided to build the display list inside the iframe, then the display list inside it was constructed correctly and its HitTest method worked correctly.

Comment 9

[Matt Woodrow (:mattwoodrow)]

So at:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6579

We are calling TransformRect with nsPoint(0, 0) instead of the actual offset of the frame and are under the assumption that this doesn't affect the overflow calculation.

This is not true with perspective transformations, and we are getting an overflow rect that is too small.

Any suggestions on how we can get the actual offset here? Is it even a constant for a frame?

Comment 10

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

The input overflow rect is relative to the top-left of the frame's pre-transform border-box. The output overflow rect is relative to (mRect.x, mRect.y) in the parent frame's coordinate space. Does that help?

Comment 11

[Matt Woodrow (:mattwoodrow)]

I don't believe so.

I think we just need to check ToReferenceFrame(), assuming that value will be correct at this point.

Comment 12

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

There's no ToReferenceFrame, because there's no display list. The reference frame is somewhat arbitrary anyway.

Comment 13

[Matt Woodrow (:mattwoodrow)]

The offset passed here actually changes the size (not just x/y) of the returned overflow.

Comment 14

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

So what coordinate space do you want the coordinates in?  Does it need to be relative to the -moz-transform-origin of the frame with the transform or something like that?

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> The offset passed here actually changes the size (not just x/y) of the
> returned overflow.

This makes no sense at all. It only changes it if you don't adjust the coordinate system of the passed in rectangle too. Must have been half asleep.

I think a more relevant problem is that mRect = {0, 0, 0, 0} and GetParent()->mRect = {0, 0, 0, 0}.

When it comes to drawing time, the parent frame rect has an offset of 12480, 9600.

Since computing the transform for a preserve-3d child gives you a transform that changes to the coordinate space of the rootmost parent of the chain, then we need these offsets.

Comment 16

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> I think a more relevant problem is that mRect = {0, 0, 0, 0} and
> GetParent()->mRect = {0, 0, 0, 0}.

That sounds OK.

> When it comes to drawing time, the parent frame rect has an offset of 12480,
> 9600.

You mean its offset to the reference frame is 12480,9600? That's possible since the reference frame is generally the root frame of the topmost document.

> Since computing the transform for a preserve-3d child gives you a transform
> that changes to the coordinate space of the rootmost parent of the chain,
> then we need these offsets.

You mean the first ancestor that's not preserve-3d? I guess you'll need to find that explicitly.

Comment 17

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> > When it comes to drawing time, the parent frame rect has an offset of 12480,
> > 9600.
> 
> You mean its offset to the reference frame is 12480,9600? That's possible
> since the reference frame is generally the root frame of the topmost
> document.

No I mean GetParent->mRect = { 12480, 9600, width, height }

Since these are different between computing the overflow, and drawing, we get different matrices.

> 
> > Since computing the transform for a preserve-3d child gives you a transform
> > that changes to the coordinate space of the rootmost parent of the chain,
> > then we need these offsets.
> 
> You mean the first ancestor that's not preserve-3d? I guess you'll need to
> find that explicitly.

Yes, I did.

Comment 18

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> No I mean GetParent->mRect = { 12480, 9600, width, height }
> 
> Since these are different between computing the overflow, and drawing, we
> get different matrices.

Ah right. Because when we reflow the child we haven't finished reflowing all the ancestors yet. This is a problem.

When we finish reflowing the root of a preserve-3d subtree, we probably need to traverse the preserve-3d descendants and fix up their overflow areas :-(.

This might depend on bug 524925, but I hope not.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP

Once we get to storing the overflowing for the parent of a preserve-3d hierarchy, fix up the overflow areas of any children.

This seems to get reasonable sized overflow rects set, but it's still at the wrong positions. Both the untransformed overflow, and frame size should be in the same (correct) coordinate space right?

Comment 20

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

+        desiredSize.ScrollableOverflow() = child->GetVisualOverflowRectRelativeToSelf();

GetScrollableOverflowRectRelativeToSelf()?

So we're calling FinishAndStoreOverflow on a preserve-3d child twice, and we're applying transforms *both times* right? That seems wrong. Seems like you should get the pre-transform overflow area here before you call FinishAndStoreOverflow again?

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> This seems to get reasonable sized overflow rects set, but it's still at the
> wrong positions. Both the untransformed overflow, and frame size should be
> in the same (correct) coordinate space right?

Not sure what you mean.

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> +        desiredSize.ScrollableOverflow() =
> child->GetVisualOverflowRectRelativeToSelf();
> 
> GetScrollableOverflowRectRelativeToSelf()?

This doesn't exist yet, but I plan to add it.

> 
> So we're calling FinishAndStoreOverflow on a preserve-3d child twice, and
> we're applying transforms *both times* right? That seems wrong. Seems like
> you should get the pre-transform overflow area here before you call
> FinishAndStoreOverflow again?

GetVisualOverflowRectRelativeToSelf is the pre-transformed overflow rect isn't it?

> 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> > This seems to get reasonable sized overflow rects set, but it's still at the
> > wrong positions. Both the untransformed overflow, and frame size should be
> > in the same (correct) coordinate space right?
> 
> Not sure what you mean.

We're unioning the frame size and the result of GetVisualOverflowRectRelativeToSelf (pre-transform overflow rect). I believe that these are in the same coordinate space.

Comment 22

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> > So we're calling FinishAndStoreOverflow on a preserve-3d child twice, and
> > we're applying transforms *both times* right? That seems wrong. Seems like
> > you should get the pre-transform overflow area here before you call
> > FinishAndStoreOverflow again?
> 
> GetVisualOverflowRectRelativeToSelf is the pre-transformed overflow rect
> isn't it?

Er, right.

> > (In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> > > This seems to get reasonable sized overflow rects set, but it's still at the
> > > wrong positions. Both the untransformed overflow, and frame size should be
> > > in the same (correct) coordinate space right?
> > 
> > Not sure what you mean.
> 
> We're unioning the frame size and the result of
> GetVisualOverflowRectRelativeToSelf (pre-transform overflow rect). I believe
> that these are in the same coordinate space.

Yes.

Actually, passing the pre-transform overflow area as the input to FinishAndStoreOverflow is still wrong since there's still a bunch of processing that will get repeated, such as filter effects. What you really need to do is save the overflow area that was input to FinishAndStoreOverflow the first time and repeat that input again.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Attachment: Reduced test case

I've reduced the test case a fair bit, and it's still broken even with my current patch.

The problem appears to be the margin: 0 auto; set on the intermediate frame.

The problem case is approximately:

<div id="parent">
  <div id="one" style="-moz-transform-style:preserve-3d;">
    <div id="two" style="-moz-transform-style:preserve-3d; margin: auto;">
      <div id="three" style="-moz-transform: <transforms>;"></div>
    </div>
  </div>
</div>

When we calculate the overflow area for three, we convert to the coordinate space of parent (instead of two as we normally would without preserve-3d). The overflow rects for parent, one and two just copy this overflow area (and combine it with any other children they may have) since it's already in the 'right' coordinate space.

For some reason this isn't taking the margin shift (around 200pixels?) into account and our final overflow area is positioned incorrectly. If I remove the margin, then this testcase works.

Any ideas where I should be looking to compensate for this margin?

Comment 24

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP v2

Comment 25

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> The problem case is approximately:
> 
> <div id="parent">
>   <div id="one" style="-moz-transform-style:preserve-3d;">
>     <div id="two" style="-moz-transform-style:preserve-3d; margin: auto;">
>       <div id="three" style="-moz-transform: <transforms>;"></div>
>     </div>
>   </div>
> </div>
> 
> When we calculate the overflow area for three, we convert to the coordinate
> space of parent (instead of two as we normally would without preserve-3d).
> The overflow rects for parent, one and two just copy this overflow area (and
> combine it with any other children they may have) since it's already in the
> 'right' coordinate space.

The overflow rect contribution of "three" needs to be adjusted to account for the x/y offset introduced by the auto margin.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Recompute Preserve-3d children overflow rects after all needed sizes are available

This fixes the test case for me!

Comment 27

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

Comment on attachment 566929 [details] [diff] [review]
Recompute Preserve-3d children overflow rects after all needed sizes are available

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

r+ with that fixed.

But we desperately need tests here.

::: layout/generic/nsFrame.cpp
@@ +6499,5 @@
> +    Properties().Set(nsIFrame::InitialOverflowProperty(),
> +                     new nsOverflowAreas(aOverflowAreas));
> +  } else if (initial != &aOverflowAreas) {
> +    *initial = aOverflowAreas;
> +  }

We cannot add this property to every frame, it would kill us performance-wise.

I think we can add it only for preserves-3d children, and only for the children where the overflow area isn't exactly aNewSize. In which case, we should carefully document when it's present and when it isn't.

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Recompute Preserve-3d children overflow rects after all needed sizes are available v2

Fixed review comments, carrying forward r=roc

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Preserve-3d test with margin

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Preserve-3d test with margin v2

Now with more tests :)

Comment 31

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ecd9735b523
https://hg.mozilla.org/integration/mozilla-inbound/rev/16bd56ed52ee

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4ecd9735b523
https://hg.mozilla.org/mozilla-central/rev/16bd56ed52ee