WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61491
Frame flattening is broken with nested frames
https://bugs.webkit.org/show_bug.cgi?id=61491
Summary
Frame flattening is broken with nested frames
Simon Fraser (smfr)
Reported
2011-05-25 17:36:40 PDT
Frame flattening is totally broken if there are nested frames. The inner frame ends up with size 0x0. There are no tests for this.
Attachments
Testcase
(3.00 KB, application/octet-stream)
2011-05-25 17:37 PDT
,
Simon Fraser (smfr)
no flags
Details
Patch.
(29.56 KB, patch)
2011-05-29 15:17 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(29.56 KB, patch)
2011-05-31 06:09 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(52.77 KB, patch)
2011-05-31 18:49 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(52.77 KB, patch)
2011-05-31 18:56 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(81.84 KB, patch)
2011-06-01 15:37 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(111.07 KB, patch)
2011-06-02 06:27 PDT
,
Yael
kenneth
: review+
Details
Formatted Diff
Diff
Patch
(111.09 KB, patch)
2011-06-03 06:17 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2011-05-25 17:37:18 PDT
Created
attachment 94888
[details]
Testcase
Simon Fraser (smfr)
Comment 2
2011-05-25 17:56:22 PDT
Seems to be an issue when the intermediate iframe has "scrolling=no"
Simon Fraser (smfr)
Comment 3
2011-05-25 18:16:10 PDT
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.
Kenneth Rohde Christiansen
Comment 4
2011-05-26 07:20:05 PDT
(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?
Simon Fraser (smfr)
Comment 5
2011-05-26 08:08:38 PDT
I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong.
Kenneth Rohde Christiansen
Comment 6
2011-05-26 08:09:57 PDT
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;
Kenneth Rohde Christiansen
Comment 7
2011-05-26 08:10:34 PDT
(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 :-)
Simon Fraser (smfr)
Comment 8
2011-05-26 08:13:52 PDT
Right, I think the isScrollable check is the real source of the bug.
Kenneth Rohde Christiansen
Comment 9
2011-05-26 08:31:55 PDT
(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.
Kenneth Rohde Christiansen
Comment 10
2011-05-26 08:49:26 PDT
@@ -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.
Simon Fraser (smfr)
Comment 11
2011-05-26 09:10:33 PDT
Don't we call updateWidgetPositions() after layout anyway? I'm not sure why frame flattening has to call it explicitly.
Simon Fraser (smfr)
Comment 12
2011-05-26 10:42:06 PDT
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?
Simon Fraser (smfr)
Comment 13
2011-05-26 10:46:59 PDT
performPostLayoutTasks() is being skipped because inSubframeLayoutWithFrameFlattening is true. So we never update the widget inside the flattened iframe.
Simon Fraser (smfr)
Comment 14
2011-05-26 10:54:36 PDT
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.
Yael
Comment 15
2011-05-26 18:14:10 PDT
(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
.
Yael
Comment 16
2011-05-29 15:17:55 PDT
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.
Kenneth Rohde Christiansen
Comment 17
2011-05-30 01:11:21 PDT
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?
Yael
Comment 18
2011-05-31 06:01:02 PDT
(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.
Yael
Comment 19
2011-05-31 06:09:15 PDT
Created
attachment 95425
[details]
Patch. Put Simon Fraser's name in the LayoutTests Changelog.
Kenneth Rohde Christiansen
Comment 20
2011-05-31 06:12:34 PDT
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?
Yael
Comment 21
2011-05-31 06:49:31 PDT
(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().
Antonio Gomes
Comment 22
2011-05-31 07:02:53 PDT
> > > 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
WebKit Commit Bot
Comment 23
2011-05-31 07:20:19 PDT
Comment on
attachment 95425
[details]
Patch. Clearing flags on attachment: 95425 Committed
r87726
: <
http://trac.webkit.org/changeset/87726
>
WebKit Commit Bot
Comment 24
2011-05-31 07:20:26 PDT
All reviewed patches have been landed. Closing bug.
Yael
Comment 25
2011-05-31 08:39:18 PDT
We should also fix the issue described in
comment #5
.
Ademar Reis
Comment 26
2011-05-31 12:18:50 PDT
Revision
r87726
cherry-picked into qtwebkit-2.2 with commit de5d05b <
http://gitorious.org/webkit/qtwebkit/commit/de5d05b
>
Yael
Comment 27
2011-05-31 18:49:18 PDT
Created
attachment 95530
[details]
Patch. Do not flatten offscreen iframes during frame flattening, if flattening will make them visible.
Yael
Comment 28
2011-05-31 18:56:39 PDT
Created
attachment 95532
[details]
Patch. One more try. Patches with images are hard to manage :)
Simon Fraser (smfr)
Comment 29
2011-05-31 20:04:54 PDT
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,
Yael
Comment 30
2011-06-01 15:37:18 PDT
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.
Kenneth Rohde Christiansen
Comment 31
2011-06-01 15:50:32 PDT
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?
Yael
Comment 32
2011-06-01 16:37:03 PDT
(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.
Kenneth Rohde Christiansen
Comment 33
2011-06-01 17:08:30 PDT
(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
Yael
Comment 34
2011-06-01 17:20:35 PDT
(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.
Kenneth Rohde Christiansen
Comment 35
2011-06-01 17:22:59 PDT
(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)
Yael
Comment 36
2011-06-01 17:36:00 PDT
(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 :)
Yael
Comment 37
2011-06-02 06:27:56 PDT
Created
attachment 95757
[details]
Patch. Additional test that was requested by
comment #35
.
Kenneth Rohde Christiansen
Comment 38
2011-06-03 01:10:22 PDT
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
Yael
Comment 39
2011-06-03 06:17:31 PDT
Created
attachment 95906
[details]
Patch Updated comments per Kenneth's request.
WebKit Commit Bot
Comment 40
2011-06-03 06:47:18 PDT
Comment on
attachment 95906
[details]
Patch Clearing flags on attachment: 95906 Committed
r88011
: <
http://trac.webkit.org/changeset/88011
>
WebKit Commit Bot
Comment 41
2011-06-03 06:47:27 PDT
All reviewed patches have been landed. Closing bug.
Oleg Romashin (:romaxa)
Comment 42
2011-07-12 14:25:38 PDT
- // 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?
Oleg Romashin (:romaxa)
Comment 43
2011-07-12 14:32:42 PDT
Simple testcase: <iframe frameborder="0" style="width: 0px; height: 0px;" src="data:text/html"</iframe>
Simon Fraser (smfr)
Comment 44
2011-07-12 14:38:43 PDT
Please file a new bug if you think there's an outstanding issue.
Yael
Comment 45
2011-07-12 14:51:48 PDT
http://trac.webkit.org/changeset/87853
Fixed the issue of 0 size iframes. Can you try that?
Oleg Romashin (:romaxa)
Comment 46
2011-07-12 15:41:35 PDT
(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
Yael
Comment 47
2011-07-12 18:52:27 PDT
(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.
Ademar Reis
Comment 48
2011-07-13 06:09:27 PDT
Revision
r88011
cherry-picked into qtwebkit-2.2 with commit 3b8d474 <
http://gitorious.org/webkit/qtwebkit/commit/3b8d474
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug