Bug 61491

Created attachment 94888 [details] Testcase

Seems to be an issue when the intermediate iframe has "scrolling=no"

Seems like RenderIFrame::flattenFrame() decides not to flatten the first time around because the absoluteBoundingBoxRect().intersects() tests fails, because view->contentsSize() is 0,0 at this point (we haven't finished the first layout). That test is bogus, and nothing later comes back to cause us to re-evaluate that decision.

(In reply to comment #3) > Seems like RenderIFrame::flattenFrame() decides not to flatten the first time around because the absoluteBoundingBoxRect().intersects() tests fails, because view->contentsSize() is 0,0 at this point (we haven't finished the first layout). That test is bogus, and nothing later comes back to cause us to re-evaluate that decision. So you suggest that we reevaluate whether we should flatten or not?

I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong.

I think the bug is elsewhere. The nested iframe has a fixed size and thus the flattenFrame() method is returning false: 92 if (!isScrollable && style()->width().isFixed() 93 && style()->height().isFixed()) return false; Regarding the absoluteBoundingBoxRect().intersects() test. Basically to check if something is offscreen, either of the left values of absoluteBoundingBoxRect() needs to be negative. The code could probably be rewritten to something like: // Do not flatten offscreen inner frames during frame flattening. IntRect rect = absoluteBoundingBoxRect(); int rightX = rect.x() + rect.width(); int rightY = rect.y() + rect.height(); return rightX > 0 && rightY > 0;

(In reply to comment #5) > I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong. I totally agree with that :-)

Right, I think the isScrollable check is the real source of the bug.

(In reply to comment #8) > Right, I think the isScrollable check is the real source of the bug. Well, make it return true to flattenFrame makes it work, but I don't see why it becomes 0x0 size if flattenFrame() is false. Why should it be laid out differently than without frame flattening enabled. The first iframe IS flattened.

@@ -115,7 +122,8 @@ void RenderIFrame::layout() if (flattenFrame()) { layoutWithFlattening(style()->width().isFixed(), style()->height().isFixed()); return; - } + } else + updateWidgetPosition(); The above makes it work, so apparently updateWidgetPosition() needs to be called when frame flattening is on, but maybe this is not the right place.

Don't we call updateWidgetPositions() after layout anyway? I'm not sure why frame flattening has to call it explicitly.

Oh, updateWidgetPositions() doesn't go into frames/iframes. But doing layout on a FrameView should update the positions of contained widgets, so why does that not happen in this case?

performPostLayoutTasks() is being skipped because inSubframeLayoutWithFrameFlattening is true. So we never update the widget inside the flattened iframe.

inSubframeLayoutWithFrameFlattening checks were added in http://trac.webkit.org/changeset/77988 to fix bug 52449, but I don't see how performPostLayoutTasks() gets called at all on nested, flattened frames now.

(In reply to comment #14) > inSubframeLayoutWithFrameFlattening checks were added in http://trac.webkit.org/changeset/77988 to fix bug 52449, but I don't see how performPostLayoutTasks() gets called at all on nested, flattened frames now. performPostLayoutTasks() is called on a timer only for flattened iframes. That is becuase it calls resizeEvent() and sites like gmail.com would use that event to relayout themselves. That caused the bug 52449.

Created attachment 95303 [details] Patch. After r77988, when frame flattening is enabled, performPostLayoutTasks() is called on a timer for iframes. This causes layout of nested iframes to sometimes happen asynchronously, but WebCore expects layout to always finish synchronously. Added a call to updateWidgetPosition() for cases that performPostLayoutTasks() is now happening asynchronously.

Comment on attachment 95303 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=95303&action=review > Source/WebCore/page/FrameView.cpp:1041 > if (!m_hasPendingPostLayoutTasks) { > - if (!m_inSynchronousPostLayout && !inSubframeLayoutWithFrameFlattening) { > - m_inSynchronousPostLayout = true; > - // Calls resumeScheduledEvents() > - performPostLayoutTasks(); > - m_inSynchronousPostLayout = false; > + if (!m_inSynchronousPostLayout) { who not do if (!m_hasPendingPostLayoutTasks && !m_inSynchronousPostLayout) ? > LayoutTests/ChangeLog:7 > +2011-05-29 Yael Aharon <yael.aharon@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Frame flattening is broken with nested frames > + https://bugs.webkit.org/show_bug.cgi?id=61491 > + Shouldn't you give some credit to Simon Fraser for the test case?

(In reply to comment #17) > (From update of attachment 95303 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95303&action=review > > > Source/WebCore/page/FrameView.cpp:1041 > > if (!m_hasPendingPostLayoutTasks) { > > - if (!m_inSynchronousPostLayout && !inSubframeLayoutWithFrameFlattening) { > > - m_inSynchronousPostLayout = true; > > - // Calls resumeScheduledEvents() > > - performPostLayoutTasks(); > > - m_inSynchronousPostLayout = false; > > + if (!m_inSynchronousPostLayout) { > > who not do > > if (!m_hasPendingPostLayoutTasks && !m_inSynchronousPostLayout) ? > The above condition is not correct for the second half of the if statement.

Created attachment 95425 [details] Patch. Put Simon Fraser's name in the LayoutTests Changelog.

Comment on attachment 95425 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=95425&action=review > Source/WebCore/page/FrameView.cpp:1043 > + m_frame->contentRenderer()->updateWidgetPositions(); Are we ensured to always have a contentRenderer?

(In reply to comment #20) > (From update of attachment 95425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95425&action=review > > > Source/WebCore/page/FrameView.cpp:1043 > > + m_frame->contentRenderer()->updateWidgetPositions(); > > Are we ensured to always have a contentRenderer? Yes, we are. When layout starts, we check for a layout root and return if there is no root. If m_layoutRoot exists, that is out root, otherwise, the root is the same as contentRenderer().

> > > Source/WebCore/page/FrameView.cpp:1043 > > > + m_frame->contentRenderer()->updateWidgetPositions(); > > > > Are we ensured to always have a contentRenderer? > > Yes, we are. > When layout starts, we check for a layout root and return if there is no root. > If m_layoutRoot exists, that is out root, otherwise, the root is the same as contentRenderer(). An assert is highly recommended in such cases, then

Comment on attachment 95425 [details] Patch. Clearing flags on attachment: 95425 Committed r87726: <http://trac.webkit.org/changeset/87726>

All reviewed patches have been landed. Closing bug.

We should also fix the issue described in comment #5.

Revision r87726 cherry-picked into qtwebkit-2.2 with commit de5d05b <http://gitorious.org/webkit/qtwebkit/commit/de5d05b>

Created attachment 95530 [details] Patch. Do not flatten offscreen iframes during frame flattening, if flattening will make them visible.

Created attachment 95532 [details] Patch. One more try. Patches with images are hard to manage :)

Comment on attachment 95532 [details] Patch. I think you should test what happens when the iframes are moved after the first layout, so the invisible one becomes visible and vice versa. A good test would also include nested iframes,

Created attachment 95675 [details] Patch. Added a second test that has a nested iframe and scrolls the main frame so that the iframes become visible.

Comment on attachment 95675 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > + scrollTo(1200, 0); shouldnt you do something to invalidate the layout and force relayout?

(In reply to comment #31) > (From update of attachment 95675 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > > + scrollTo(1200, 0); > > shouldnt you do something to invalidate the layout and force relayout? Not sure what you mean? As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.

(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 95675 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > > > + scrollTo(1200, 0); > > > > shouldnt you do something to invalidate the layout and force relayout? > > Not sure what you mean? > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened. But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests

(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #31) > > > (From update of attachment 95675 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > > > > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > > > > + scrollTo(1200, 0); > > > > > > shouldnt you do something to invalidate the layout and force relayout? > > > > Not sure what you mean? > > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened. > > But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests The idea is that it was flattened while off screen, and not re-laid out after scrolling into view.

(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #32) > > > (In reply to comment #31) > > > > (From update of attachment 95675 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > > > > > > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > > > > > + scrollTo(1200, 0); > > > > > > > > shouldnt you do something to invalidate the layout and force relayout? > > > > > > Not sure what you mean? > > > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened. > > > > But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests > > The idea is that it was flattened while off screen, and not re-laid out after scrolling into view. But if a relayout happens now that it is visible, you need to make sure it is still flattened right. So maybe that is yet another test. You can force a relayout by doing the following: var ignoreMe = document.body.offsetWidth; Just accessing that value should force a relayout (grep in other tests for more examples)

(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > > (In reply to comment #32) > > > > (In reply to comment #31) > > > > > (From update of attachment 95675 [details] [details] [details] [details] [details]) > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review > > > > > > > > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12 > > > > > > + scrollTo(1200, 0); > > > > > > > > > > shouldnt you do something to invalidate the layout and force relayout? > > > > > > > > Not sure what you mean? > > > > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened. > > > > > > But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests > > > > The idea is that it was flattened while off screen, and not re-laid out after scrolling into view. > > But if a relayout happens now that it is visible, you need to make sure it is still flattened right. So maybe that is yet another test. > > You can force a relayout by doing the following: > > var ignoreMe = document.body.offsetWidth; > > Just accessing that value should force a relayout (grep in other tests for more examples) I can add another test to do that :)

Created attachment 95757 [details] Patch. Additional test that was requested by comment #35.

Comment on attachment 95757 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=95757&action=review > Source/WebCore/ChangeLog:8 > + Do not flatten offscreen iframes during frame flattening, if flattening will make them visible. as flattening might make them visible > Source/WebCore/rendering/RenderIFrame.cpp:104 > + // Do not flatten offscreen inner frames during frame flattening, if flattening will make them visible. same here

Created attachment 95906 [details] Patch Updated comments per Kenneth's request.

Comment on attachment 95906 [details] Patch Clearing flags on attachment: 95906 Committed r88011: <http://trac.webkit.org/changeset/88011>

All reviewed patches have been landed. Closing bug.

- // Do not flatten offscreen inner frames during frame flattening. - return absoluteBoundingBoxRect().intersects(IntRect(IntPoint(0, 0), view->contentsSize())); + // Do not flatten offscreen inner frames during frame flattening, as flattening might make them visible. + IntRect boundingRect = absoluteBoundingBoxRect(); + return boundingRect.maxX() > 0 && boundingRect.maxY() > 0; this change seems making flattenFrame() return true for 0 size frames with x && y position != 0... is it expected?

Simple testcase: <iframe frameborder="0" style="width: 0px; height: 0px;" src="data:text/html"</iframe>

Please file a new bug if you think there's an outstanding issue.

http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes. Can you try that?

(In reply to comment #45) > http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes. > Can you try that? yep, that works fine, thanks... probably it make sense to set proper dependency between these two bugs... and make this bug depends from 61831

(In reply to comment #46) > (In reply to comment #45) > > http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes. > > Can you try that? > > yep, that works fine, thanks... probably it make sense to set proper dependency between these two bugs... > and make this bug depends from 61831 It was a mere coincidence that these 2 bugs were fixed at the same time. I did not know about the other issue until later. (No, I did not introduce the problem :) Anyways, I am glad it works now.

Revision r88011 cherry-picked into qtwebkit-2.2 with commit 3b8d474 <http://gitorious.org/webkit/qtwebkit/commit/3b8d474>