Summary: | Poor rendering on lala.com with frame flattening | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Bolsinga <bolsinga> | ||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||||||
Status: | CLOSED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, hausmann, hyatt, kenneth, koivisto, luiz, ossy, tonikitoo, webkit-ews | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
URL: | http://lala.com | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 35784 | ||||||||||||||||||||
Attachments: |
|
Description
Greg Bolsinga
2010-04-06 12:22:10 PDT
There is an offscreen iframe. I forgot to add that I enabled flat frames for the Mac OS X build to see this problem. Created attachment 52657 [details]
This patch addresses the problem.
What about intersecting node()->document()->view()->visibleContentRect() with node()->renderer()->absoluteClippedOverflowRect()? (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()? (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? 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 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 (In reply to comment #4) > What about intersecting node()->document()->view()->visibleContentRect() with > node()->renderer()->absoluteClippedOverflowRect()? Something like that would be much better. Comment on attachment 52657 [details]
This patch addresses the problem.
Perhaps someone should clear my review flag since there is no regression test.
(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 on attachment 52657 [details]
This patch addresses the problem.
r-, due to my comments.
(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. Since Mac doesn't have frame flattening on, then what are Mac results exactly? (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. (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. (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? ;) Created attachment 52671 [details]
intersects the rects as suggested
I'm not sure what to do with DRT and offscreen iframes.
(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. (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. Created attachment 52672 [details]
Potential patch
Yours is much safer than mine! I'll obsolete mine. You should nominate yours. 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 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? (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. > > + 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?
(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. (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? (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 (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. 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. FWIW, this works for me too. Is there a DRT example I can crib from for an offscreen iframe? Created attachment 52744 [details]
Patch
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. Created attachment 52749 [details]
Patch fixing issues pointed out by Darin Adler
Darin, could you please re-set the r+?
Comment on attachment 52749 [details]
Patch fixing issues pointed out by Darin Adler
Removing cq, I did a minor mistake.
(In reply to comment #36) > Removing cq, I did a minor mistake. You should capitalize the sentence comment if you are doing another patch. Attachment 52749 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1607288 My bad, I left out an IntRect() in my suggested code. Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ? (In reply to comment #40) > Yes, that is what I'm fixing, btw, do you prefer IntRect() over IntRect(0, 0) ? IntPoint(), I mean. Created attachment 52754 [details]
Patch fixing compilation issue and capitalize the comment
(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. (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 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> All reviewed patches have been landed. Closing bug. Platform specific expected files landed in http://trac.webkit.org/changeset/57228 Thanks! Revision r57225 cherry-picked into qtwebkit-2.0 with commit 2705c83cb30eb31addfc7e93a60636ea60d93c38 |