37164 – Poor rendering on lala.com with frame flattening

CLOSED FIXED 37164

Poor rendering on lala.com with frame flattening

https://bugs.webkit.org/show_bug.cgi?id=37164

Summary Poor rendering on lala.com with frame flattening

Greg Bolsinga

Reported 2010-04-06 12:22:10 PDT

This site does not render properly with iframe/frameset flattening enabled.

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Greg Bolsinga

Comment 1 2010-04-06 12:24:01 PDT

There is an offscreen iframe.

Greg Bolsinga

Comment 2 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.

Greg Bolsinga

Comment 3 2010-04-06 12:58:17 PDT

Created attachment 52657 [details] This patch addresses the problem.

Kenneth Rohde Christiansen

Comment 4 2010-04-06 13:09:55 PDT

What about intersecting node()->document()->view()->visibleContentRect() with node()->renderer()->absoluteClippedOverflowRect()?

Antonio Gomes

Comment 5 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()?

Kenneth Rohde Christiansen

Comment 6 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?

Greg Bolsinga

Comment 7 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?

Darin Adler

Comment 8 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

Darin Adler

Comment 9 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.

Darin Adler

Comment 10 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.

Darin Adler

Comment 11 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+.

Kenneth Rohde Christiansen

Comment 12 2010-04-06 13:57:38 PDT

Comment on attachment 52657 [details] This patch addresses the problem. r-, due to my comments.

Kenneth Rohde Christiansen

Comment 13 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.

Darin Adler

Comment 14 2010-04-06 14:10:23 PDT

Since Mac doesn't have frame flattening on, then what are Mac results exactly?

Kenneth Rohde Christiansen

Comment 15 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.

Kenneth Rohde Christiansen

Comment 16 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.

Greg Bolsinga

Comment 17 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? ;)

Greg Bolsinga

Comment 18 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.

Kenneth Rohde Christiansen

Comment 19 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.

Kenneth Rohde Christiansen

Comment 20 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.

Kenneth Rohde Christiansen

Comment 21 2010-04-06 14:53:39 PDT

Created attachment 52672 [details] Potential patch

Greg Bolsinga

Comment 22 2010-04-06 14:55:53 PDT

Yours is much safer than mine! I'll obsolete mine. You should nominate yours.

Darin Adler

Comment 23 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().

Kenneth Rohde Christiansen

Comment 24 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?

Kenneth Rohde Christiansen

Comment 25 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.

Kenneth Rohde Christiansen

Comment 26 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?

Darin Adler

Comment 27 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.

Kenneth Rohde Christiansen

Comment 28 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?

Antonio Gomes

Comment 29 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

Darin Adler

Comment 30 2010-04-06 15:41:48 PDT

Kenneth Rohde Christiansen

Comment 31 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.

Greg Bolsinga

Comment 32 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?

Kenneth Rohde Christiansen

Comment 33 2010-04-07 10:12:26 PDT

Created attachment 52744 [details] Patch

Darin Adler

Comment 34 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.

Kenneth Rohde Christiansen

Comment 35 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+?

Kenneth Rohde Christiansen

Comment 36 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.

Darin Adler

Comment 37 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.

Early Warning System Bot

Comment 38 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

Darin Adler

Comment 39 2010-04-07 10:43:14 PDT

My bad, I left out an IntRect() in my suggested code.

Kenneth Rohde Christiansen

Comment 40 2010-04-07 10:44:36 PDT

Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ?

Kenneth Rohde Christiansen

Comment 41 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.

Kenneth Rohde Christiansen

Comment 42 2010-04-07 10:48:46 PDT

Created attachment 52754 [details] Patch fixing compilation issue and capitalize the comment

Darin Adler

Comment 43 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.

Luiz Agostini

Comment 44 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?

WebKit Commit Bot

Comment 45 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>

WebKit Commit Bot

Comment 46 2010-04-07 13:22:58 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 47 2010-04-07 13:45:43 PDT

Platform specific expected files landed in http://trac.webkit.org/changeset/57228

Greg Bolsinga

Comment 48 2010-04-07 14:12:59 PDT

Thanks!

Simon Hausmann

Comment 49 2010-04-08 00:48:09 PDT

Revision r57225 cherry-picked into qtwebkit-2.0 with commit 2705c83cb30eb31addfc7e93a60636ea60d93c38

Note You need to log in before you can comment on or make changes to this bug.

Status CLOSED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS OS X 10.5

Product WebKit

Component Layout and Rendering

Assignee

Kenneth Rohde Christiansen

Reported

2010-04-06 12:22 PDT

Modified

2010-04-08 00:53 PDT History

CC List

10 users Show

URL

http://lala.com

Keywords

Depends on

Blocks

35784

Dependencies

tree graph