Bug 37164 - Poor rendering on lala.com with frame flattening
Summary: Poor rendering on lala.com with frame flattening
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL: http://lala.com
Keywords:
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-04-06 12:22 PDT by Greg Bolsinga
Modified: 2010-04-08 00:53 PDT (History)
10 users (show)

See Also:


Attachments
This patch addresses the problem. (1.71 KB, patch)
2010-04-06 12:58 PDT, Greg Bolsinga
kenneth: review-
Details | Formatted Diff | Diff
iframe offscreen (1.01 KB, text/html)
2010-04-06 13:38 PDT, Greg Bolsinga
no flags Details
intersects the rects as suggested (1.69 KB, patch)
2010-04-06 14:49 PDT, Greg Bolsinga
bolsinga: review-
Details | Formatted Diff | Diff
Potential patch (1.34 KB, patch)
2010-04-06 14:53 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Better solution (1.85 KB, patch)
2010-04-06 16:19 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2010-04-07 10:12 PDT, Kenneth Rohde Christiansen
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Patch fixing issues pointed out by Darin Adler (6.12 KB, patch)
2010-04-07 10:35 PDT, Kenneth Rohde Christiansen
darin: review+
Details | Formatted Diff | Diff
Patch fixing compilation issue and capitalize the comment (6.13 KB, patch)
2010-04-07 10:48 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2010-04-06 12:22:10 PDT
This site does not render properly with iframe/frameset flattening enabled.
Comment 1 Greg Bolsinga 2010-04-06 12:24:01 PDT
There is an offscreen iframe.
Comment 2 Greg Bolsinga 2010-04-06 12:34:30 PDT
I forgot to add that I enabled flat frames for the Mac OS X build to see this problem.
Comment 3 Greg Bolsinga 2010-04-06 12:58:17 PDT
Created attachment 52657 [details]
This patch addresses the problem.
Comment 4 Kenneth Rohde Christiansen 2010-04-06 13:09:55 PDT
What about intersecting node()->document()->view()->visibleContentRect() with node()->renderer()->absoluteClippedOverflowRect()?
Comment 5 Antonio Gomes 2010-04-06 13:13:39 PDT
(In reply to comment #4)
> What about intersecting node()->document()->view()->visibleContentRect() 

kenne, maybe mainFrame's view  not document's view (?)

> with node()->renderer()->absoluteClippedOverflowRect()?
Comment 6 Kenneth Rohde Christiansen 2010-04-06 13:15:19 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > What about intersecting node()->document()->view()->visibleContentRect() 
> 
> kenne, maybe mainFrame's view  not document's view (?)
> 
> > with node()->renderer()->absoluteClippedOverflowRect()?

Yes, you are right.

I can confirm that my suggestion works, and should take care of some additional
cases.

Could you provide a layout test as well?
Comment 7 Greg Bolsinga 2010-04-06 13:38:33 PDT
Created attachment 52664 [details]
iframe offscreen

I wasn't able to build DRT on my Mac at the moment. Can you try this one out?
Comment 8 Darin Adler 2010-04-06 13:53:30 PDT
Comment on attachment 52657 [details]
This patch addresses the problem.

> +    if (isPositioned() && containingBlock() == view() && ((x() + width() <= 0) || (y() + height() <= 0)))
> +        return false;

"x() + width()" is "frameRect().right()" and I think we should use that instead.

"y() + height()" is "frameRect().bottom()" and I think we should use that instead

A much better way to do this would be to call frameRect().intersects(xxx), where xxx is the viewport. Checking specifically for right and bottom that are off the left and top edge is too specific and will miss other valuable cases.

r=me despite these concerns
Comment 9 Darin Adler 2010-04-06 13:54:11 PDT
(In reply to comment #4)
> What about intersecting node()->document()->view()->visibleContentRect() with
> node()->renderer()->absoluteClippedOverflowRect()?

Something like that would be much better.
Comment 10 Darin Adler 2010-04-06 13:54:47 PDT
Comment on attachment 52657 [details]
This patch addresses the problem.

Perhaps someone should clear my review flag since there is no regression test.
Comment 11 Darin Adler 2010-04-06 13:56:13 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > What about intersecting node()->document()->view()->visibleContentRect() 
> > 
> > kenne, maybe mainFrame's view  not document's view (?)
> > 
> > > with node()->renderer()->absoluteClippedOverflowRect()?
> 
> Yes, you are right.
> 
> I can confirm that my suggestion works, and should take care of some additional
> cases.
> 
> Could you provide a layout test as well?

Kenneth, maybe you should do review-. I didn't see your comments when I did a review+.
Comment 12 Kenneth Rohde Christiansen 2010-04-06 13:57:38 PDT
Comment on attachment 52657 [details]
This patch addresses the problem.

r-, due to my comments.
Comment 13 Kenneth Rohde Christiansen 2010-04-06 14:04:49 PDT
(In reply to comment #7)
> Created an attachment (id=52664) [details]
> iframe offscreen
> 
> I wasn't able to build DRT on my Mac at the moment. Can you try this one out?

This one works for me, with my suggestion as well. If you can provide an updated patch with mac results then I will get the Qt results for you.
Comment 14 Darin Adler 2010-04-06 14:10:23 PDT
Since Mac doesn't have frame flattening on, then what are Mac results exactly?
Comment 15 Kenneth Rohde Christiansen 2010-04-06 14:11:41 PDT
(In reply to comment #13)
> (In reply to comment #7)
> > Created an attachment (id=52664) [details] [details]
> > iframe offscreen
> > 
> > I wasn't able to build DRT on my Mac at the moment. Can you try this one out?
> 
> This one works for me, with my suggestion as well. If you can provide an
> updated patch with mac results then I will get the Qt results for you.

Actually, regarding "400px in each direction, disregarding the border". We
cannot verify this as it will not be part of the render tree dump.
Comment 16 Kenneth Rohde Christiansen 2010-04-06 14:12:12 PDT
(In reply to comment #14)
> Since Mac doesn't have frame flattening on, then what are Mac results exactly?

Mac supports enabling frame flattening when used from the DRT.
Comment 17 Greg Bolsinga 2010-04-06 14:15:41 PDT
(In reply to comment #13)
> (In reply to comment #7)
> > Created an attachment (id=52664) [details] [details]
> > iframe offscreen
> > 
> > I wasn't able to build DRT on my Mac at the moment. Can you try this one out?
> 
> This one works for me, with my suggestion as well. If you can provide an
> updated patch with mac results then I will get the Qt results for you.

Can you attach your patch, so we're testing the same? ;)
Comment 18 Greg Bolsinga 2010-04-06 14:49:38 PDT
Created attachment 52671 [details]
intersects the rects as suggested

I'm not sure what to do with DRT and offscreen iframes.
Comment 19 Kenneth Rohde Christiansen 2010-04-06 14:51:14 PDT
(In reply to comment #17)
> (In reply to comment #13)
> > (In reply to comment #7)
> > > Created an attachment (id=52664) [details] [details] [details]
> > > iframe offscreen
> > > 
> > > I wasn't able to build DRT on my Mac at the moment. Can you try this one out?
> > 
> > This one works for me, with my suggestion as well. If you can provide an
> > updated patch with mac results then I will get the Qt results for you.
> 
> Can you attach your patch, so we're testing the same? ;)

Oh yeah of course, but it is not a real patch, just testing code. One sec, let
me add it for you.
Comment 20 Kenneth Rohde Christiansen 2010-04-06 14:51:49 PDT
(In reply to comment #18)
> Created an attachment (id=52671) [details]
> intersects the rects as suggested
> 
> I'm not sure what to do with DRT and offscreen iframes.

We are OK as long as it is offscreen, so I think that is sufficient.
Comment 21 Kenneth Rohde Christiansen 2010-04-06 14:53:39 PDT
Created attachment 52672 [details]
Potential patch
Comment 22 Greg Bolsinga 2010-04-06 14:55:53 PDT
Yours is much safer than mine! I'll obsolete mine. You should nominate yours.
Comment 23 Darin Adler 2010-04-06 14:59:36 PDT
Comment on attachment 52672 [details]
Potential patch

> +    RenderObject* render = node()->renderer();

The thing here called "render" is the same as "this".

> +    FrameView* view = frame->tree()->top()->view();

I would make this go via page()->mainFrame()->view(). But you'd have to null check page().
Comment 24 Kenneth Rohde Christiansen 2010-04-06 14:59:51 PDT
Comment on attachment 52671 [details]
intersects the rects as suggested


> +    if (!node()->document()->topDocument()->frame()->view()->visibleContentRect().intersects(node()->renderer()->absoluteClippedOverflowRect()))
> +        return false;
> +
>      return frame->document()->frame() && frame->document()->frame()->settings()->frameFlatteningEnabled();

I guess that doing the intersection is more costly than checking the setting, so the common case of not having frame flattening so not be made slower. I suggest swapping the tests.

Also, are you sure that all of these will always remain non-null?
Comment 25 Kenneth Rohde Christiansen 2010-04-06 15:00:40 PDT
(In reply to comment #23)
> (From update of attachment 52672 [details])
> > +    RenderObject* render = node()->renderer();
> 
> The thing here called "render" is the same as "this".
> 
> > +    FrameView* view = frame->tree()->top()->view();
> 
> I would make this go via page()->mainFrame()->view(). But you'd have to null
> check page().

Ok, I will fix that and put a real patch up for review.
Comment 26 Kenneth Rohde Christiansen 2010-04-06 15:04:52 PDT
> > +    FrameView* view = frame->tree()->top()->view();
> 
> I would make this go via page()->mainFrame()->view(). But you'd have to null
> check page().

Any particular reason for this, beyond code clarify?
Comment 27 Darin Adler 2010-04-06 15:07:20 PDT
(In reply to comment #26)
> Any particular reason for this, beyond code clarify?

No practical reason, but conceptually: Calling top() on the frame free means walking up the tree in a loop until you hit the top. Calling page()->mainFrame() means going directly to the main frame.
Comment 28 Kenneth Rohde Christiansen 2010-04-06 15:12:41 PDT
(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 52672 [details] [details])
> > > +    RenderObject* render = node()->renderer();
> > 
> > The thing here called "render" is the same as "this".

absoluteClippedOverflowRect() is non const, so it a const_cast or removing const from the method the preferred solution?
Comment 29 Antonio Gomes 2010-04-06 15:14:48 PDT
(In reply to comment #28)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > (From update of attachment 52672 [details] [details] [details])
> > > > +    RenderObject* render = node()->renderer();
> > > 
> > > The thing here called "render" is the same as "this".
> 
> absoluteClippedOverflowRect() is non const, so it a const_cast or removing
> const from the method the preferred solution?

also, please consider [1] while using it


[1] http://lists.macosforge.org/pipermail/webkit-dev/2010-March/012075.html
Comment 30 Darin Adler 2010-04-06 15:41:48 PDT
(In reply to comment #28)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > (From update of attachment 52672 [details] [details] [details])
> > > > +    RenderObject* render = node()->renderer();
> > > 
> > > The thing here called "render" is the same as "this".
> 
> absoluteClippedOverflowRect() is non const, so it a const_cast or removing
> const from the method the preferred solution?

Removing const, I think.
Comment 31 Kenneth Rohde Christiansen 2010-04-06 16:19:07 PDT
Created attachment 52681 [details]
Better solution

I don't have time to finish this today with the layout tests, but the following works on http://www.samisite.com/test-csb2nf/id43.htm, lala.com and with our current layout tests.

I will do an additional test tomorrow where the iframe is within the contents of the main frame, but outside the viewport of the DRT.
Comment 32 Greg Bolsinga 2010-04-06 16:52:36 PDT
FWIW, this works for me too. Is there a DRT example I can crib from for an offscreen iframe?
Comment 33 Kenneth Rohde Christiansen 2010-04-07 10:12:26 PDT
Created attachment 52744 [details]
Patch
Comment 34 Darin Adler 2010-04-07 10:18:55 PDT
Comment on attachment 52744 [details]
Patch

> +    // do not flatten offscreen inner frames during frame flattening.

This comment needs to go closer to the logic, probably on the last paragraph.

> +    IntRect rect(IntPoint(0, 0), view->contentsSize());
> +    return rect.intersects(absoluteBoundingBoxRect());

I don't think we need a local variable here. I would write it in one line like this:

    return absoluteBoundingBoxRect().intersects(IntPoint(), view->contentsSize());

I guess I'll say r=me as is, but I'm not going to set commit-queue+ yet. You can change it back to + if you want to land this patch as-is.
Comment 35 Kenneth Rohde Christiansen 2010-04-07 10:35:09 PDT
Created attachment 52749 [details]
Patch fixing issues pointed out by Darin Adler

Darin, could you please re-set the r+?
Comment 36 Kenneth Rohde Christiansen 2010-04-07 10:38:46 PDT
Comment on attachment 52749 [details]
Patch fixing issues pointed out by Darin Adler

Removing cq, I did a minor mistake.
Comment 37 Darin Adler 2010-04-07 10:39:45 PDT
(In reply to comment #36)
> Removing cq, I did a minor mistake.

You should capitalize the sentence comment if you are doing another patch.
Comment 38 Early Warning System Bot 2010-04-07 10:42:08 PDT
Attachment 52749 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1607288
Comment 39 Darin Adler 2010-04-07 10:43:14 PDT
My bad, I left out an IntRect() in my suggested code.
Comment 40 Kenneth Rohde Christiansen 2010-04-07 10:44:36 PDT
Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ?
Comment 41 Kenneth Rohde Christiansen 2010-04-07 10:45:33 PDT
(In reply to comment #40)
> Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ?

IntPoint(), I mean.
Comment 42 Kenneth Rohde Christiansen 2010-04-07 10:48:46 PDT
Created attachment 52754 [details]
Patch fixing compilation issue and capitalize the comment
Comment 43 Darin Adler 2010-04-07 10:50:40 PDT
(In reply to comment #41)
> (In reply to comment #40)
> > Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ?
> 
> IntPoint(), I mean.

I think IntPoint() is pretty unclear, so I guess IntPoint(0, 0) is slightly better, but I wish there was a zeroPoint of some sort.
Comment 44 Luiz Agostini 2010-04-07 11:16:17 PDT
(In reply to comment #43)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ?
> > 
> > IntPoint(), I mean.
> 
> I think IntPoint() is pretty unclear, so I guess IntPoint(0, 0) is slightly
> better, but I wish there was a zeroPoint of some sort.

implemented in bug 37220. is it what you mean?
Comment 45 WebKit Commit Bot 2010-04-07 13:22:50 PDT
Comment on attachment 52754 [details]
Patch fixing compilation issue and capitalize the comment 

Clearing flags on attachment: 52754

Committed r57225: <http://trac.webkit.org/changeset/57225>
Comment 46 WebKit Commit Bot 2010-04-07 13:22:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Csaba Osztrogonác 2010-04-07 13:45:43 PDT
Platform specific expected files landed in http://trac.webkit.org/changeset/57228
Comment 48 Greg Bolsinga 2010-04-07 14:12:59 PDT
Thanks!
Comment 49 Simon Hausmann 2010-04-08 00:48:09 PDT
Revision r57225 cherry-picked into qtwebkit-2.0 with commit 2705c83cb30eb31addfc7e93a60636ea60d93c38