Bug 36798 – iframe flattening doesn't flatten

• Component: Layout and Rendering
• Status: CLOSED FIXED

[Greg Bolsinga]

I built Mac OS X with WebKitFrameFlatteningEnabledPreferenceKey with the value YES.

The page at the URL doesn't expand the iframes.

This went in with http://trac.webkit.org/changeset/56718

[Kenneth Rohde Christiansen]

Are these flattened on the iPhone? I'm currently not flattening fixed sized iframes, but I guess we should do that.

[Kenneth Rohde Christiansen]

OK, bug found :-) Thanks Greg for the bug report!

I will add some more tests and upload a new patch.

[Kenneth Rohde Christiansen]

OK, I noticed something really strange that I might need some help to fix.

If the iframe that is supposed to be flattened, would be partly visible when
flattened, it will only be flattened every second reload.

It will also show scrollbars (though they are suppressed)

[Kenneth Rohde Christiansen]

Created attachment 52045 [details]
Test case

[Kenneth Rohde Christiansen]

Created attachment 52046 [details]
First load

[Kenneth Rohde Christiansen]

Created attachment 52047 [details]
Second load

[Kenneth Rohde Christiansen]

s/partly visible/partly visible or not visible within the viewport/

[Kenneth Rohde Christiansen]

Created attachment 52054 [details]
Patch for wrong logic

[Greg Bolsinga]

With the patch applied, I do not see the frames expanded. In addition, the "FRAME6" has scrollbars with this patch, but not without it.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> With the patch applied, I do not see the frames expanded. In addition, the
> "FRAME6" has scrollbars with this patch, but not without it.

Nope, it only makes the check equal to the one used by the iPhone, and thus fixes the tests that are supplied with the patch which weren't working before.

It still doesn't solve all issues, just some of them.

[Greg Bolsinga]

OK. I tried your patch applied to iPhone as well, and saw the same issues there.

[Kenneth Rohde Christiansen]

I think the problem is that it isn't doing relayout of the owner frame when the
contents of the iframes have loaded.

[Kenneth Rohde Christiansen]

The showing of scrollbars is an issue.

The iframe when flattened needs to be the size of the contents plus the border (default 2 pixels inset), but if I do not add additional two pixels with width and height I get scrollbars.

Calling suppressScrollbars as I currently do, doesn't work and a updateWidgetPositions() reshows the scrollbars. Dave Hyatt tells me that as we cannot turn scrollbars off (as we base our policy for doing iframe flattening using that) we might need to add a virtual method to ScrollView for using zero sized scrollbars for subframes when using frame flattening.

[Antti Koivisto]

Comment on attachment 52054 [details]
Patch for wrong logic

Progression + nice tests. r=me even if there is need for followups.

[Antti Koivisto]

I think there should be a real way to turn scrollbars of for frames. No scrollbar objects created etc. Currently there is no good way to do this. Even invisible or zero width scrollbars still end up running lots of hairy code that can affect layout, generate repaints etc.

[WebKit Commit Bot]

Comment on attachment 52054 [details]
Patch for wrong logic

Rejecting patch 52054 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Last 500 characters of output:

Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12591 test cases.
fast/frames/flattening/iframe-flattening-fixed-height.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/platform/mac/fast/frames/flattening)

Exiting early after 1 failures. 7115 tests run.
122.70s total testing time

7114 test cases (99%) succeeded
1 test case (<1%) was new
2 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1591105

[Kenneth Rohde Christiansen]

I will land manually and update the results afterward. Ossy has already generated new results for the Qt bots which I have validated.

[Kenneth Rohde Christiansen]

Created attachment 52172 [details]
Scrollbar related patch

[Eric Seidel (no email)]

Attachment 52172 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1605143

[Antti Koivisto]

Comment on attachment 52172 [details]
Scrollbar related patch

r=me

> +bool FrameView::avoidScrollbarCreation()
> +{
> +    // with frame flattening no subframe can have scrollbars
> +    // but we also cannot turn scrollbars of as we determine
> +    // our flattening policy using that.
> +
> +    if (m_frame->page()->mainFrame() == m_frame)
> +        false;
> +
> +    if (!m_frame->settings() || m_frame->settings()->frameFlatteningEnabled())
> +        return true;

I suspect there no need to null test m_frame->setting().

[Antonio Gomes]

(In reply to comment #20)
> (From update of attachment 52172 [details])
> r=me
> 
> > +bool FrameView::avoidScrollbarCreation()
> > +{
> > +    // with frame flattening no subframe can have scrollbars
> > +    // but we also cannot turn scrollbars of as we determine
> > +    // our flattening policy using that.
> > +
> > +    if (m_frame->page()->mainFrame() == m_frame)
> > +        false;

you want a 'return' here. pls fix that before landing.

[Kenneth Rohde Christiansen]

> > > +    if (m_frame->page()->mainFrame() == m_frame)
> > > +        false;
> 
> you want a 'return' here. pls fix that before landing.

Good catch! I just came to the same conclusion after tracing a bug :-)

[Kenneth Rohde Christiansen]

Comment on attachment 52054 [details]
Patch for wrong logic

Landed in r56852

[Kenneth Rohde Christiansen]

Comment on attachment 52172 [details]
Scrollbar related patch

Fixed patch landed in r56854

[Csaba Osztrogonác]

(In reply to comment #24)
> (From update of attachment 52172 [details])
> Fixed patch landed in r56854

Followup patch landed in http://trac.webkit.org/changeset/56855

[Greg Bolsinga]

See  Bug 36894

[Greg Bolsinga]

Comment 9 still applies with this patch applied and with the crasher removed.

[Kenneth Rohde Christiansen]

(In reply to comment #27)
> Comment 9 still applies with this patch applied and with the crasher removed.

Yes, I'm still trying to figure out what is going wrong. Your comment #9 says that you seen scrollbars. Do you still see that?

[Adam Barth]

http://trac.webkit.org/changeset/56854 appears to have broken Leopard Intel Debug (Tests)

[Kenneth Rohde Christiansen]

(In reply to comment #29)
> http://trac.webkit.org/changeset/56854 appears to have broken Leopard Intel
> Debug (Tests)

Potential fix has been committed in r56856

[Greg Bolsinga]

(In reply to comment #28)
> Yes, I'm still trying to figure out what is going wrong. Your comment #9 says
> that you seen scrollbars. Do you still see that?

Yes I do, in "Static IFrame" and "Static 2". However when I just went to screen shot it to attach it to the bug report, I resized the window, and now "Static 2" is full size. However "Static IFrame" is still showing scrollbars (and the frame would not appear to be flattened).

[Greg Bolsinga]

Created attachment 52201 [details]
screen shot _after_ a resize of the window.

[Greg Bolsinga]

Created attachment 52202 [details]
screen shot _before_ a resize of the window.

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> Created an attachment (id=52202) [details]
> screen shot _before_ a resize of the window.

Ah, I recall that mac uses different code for scrollbars due to the fact that Widget is implemented using a platform widget.

Antti, do you have any input on how to turn off subframe scrollbars on mac?

[Kenneth Rohde Christiansen]

I have been debugging the issue with the iframe outside of the viewport not being flattened.

Apparently the contentsSize is wrong when not inside the viewport, meaning that

setContentsSize(IntSize(root->rightLayoutOverflow(), root->bottomLayoutOverflow()));

sets the wrong value.

This also happens to partly visible iframes, but only every second reload.

Any help appreciated.

[Kenneth Rohde Christiansen]

Hyatt, I have a question.

RenderPartObject:;layout() calls m_overflow.clear(); addShadowOverflow();

Is it right calling this for flattened iframes?

Also, why is it calling m_overflow.clear and not clearLayoutOverflow() defined in RenderBox?

[Antti Koivisto]

(In reply to comment #34)
> Antti, do you have any input on how to turn off subframe scrollbars on mac?

Hyatt knows about the scrollbars, I'm not sure if anyone else does.

[Kenneth Rohde Christiansen]

The case of a partly visible iframe:

LOAD:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

[Kenneth Rohde Christiansen]

If not visible at all, every load gives:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 204/19
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/19
FrameView::adjustViewSize(): layout overflow : 200/19
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400


If full visible, every load gives:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

[Kenneth Rohde Christiansen]

Created attachment 52324 [details]
Patch (should make the flattening work on the site in question)

[Kenneth Rohde Christiansen]

Greg, could you please test?

[Greg Bolsinga]

Great! With this "Static IFrame" and "STATIC 2" look great.

Static "FRAME5" little frame doesn't float with the oval image however.

[Greg Bolsinga]

<rdar://problem/7674554>

[Kenneth Rohde Christiansen]

(In reply to comment #42)
> Great! With this "Static IFrame" and "STATIC 2" look great.
> 
> Static "FRAME5" little frame doesn't float with the oval image however.

I didn't quite understand the issue. If you can provide a reduced test, I will happily look into fixing it.

[Dave Hyatt]

Comment on attachment 52324 [details]
Patch (should make the flattening work on the site in question)

r=me

[Kenneth Rohde Christiansen]

(In reply to comment #44)
> (In reply to comment #42)
> > Great! With this "Static IFrame" and "STATIC 2" look great.
> > 
> > Static "FRAME5" little frame doesn't float with the oval image however.
> 
> I didn't quite understand the issue. If you can provide a reduced test, I will
> happily look into fixing it.

Do you mean the thing hovering on top of STATIC 2? That also gives me a similar
result on the iPhone OS 2.0

[Greg Bolsinga]

(In reply to comment #46)
> (In reply to comment #44)
> > (In reply to comment #42)
> > > Great! With this "Static IFrame" and "STATIC 2" look great.
> > > 
> > > Static "FRAME5" little frame doesn't float with the oval image however.
> > 
> > I didn't quite understand the issue. If you can provide a reduced test, I will
> > happily look into fixing it.
> 
> Do you mean the thing hovering on top of STATIC 2? That also gives me a similar
> result on the iPhone OS 2.0

Yeah, that is the part, and it is also not centered properly on iPhone. I can file a separate bug if you'd like.

[Kenneth Rohde Christiansen]

Please do.

[Greg Bolsinga]

Bug 37095. Thanks Kenneth!

[Kenneth Rohde Christiansen]

Comment on attachment 52324 [details]
Patch (should make the flattening work on the site in question)

Landed in 57080

[Kenneth Rohde Christiansen]

Updated Qt iframe results committed in 57083

[Adam Barth]

This looks to have regressed a number of iframe flattening tests:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57081%20(12596)/results.html

[Kenneth Rohde Christiansen]

(In reply to comment #52)
> This looks to have regressed a number of iframe flattening tests:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57081%20(12596)/results.html

I will have a look at it. I was expecting this and watching the bots :-)

[Adam Barth]

Would you be willing to come on #webkit to coordinate?  If you expected this to light the tree on fire, why did you land it manually?  The commit-queue could have cannaried it for you without disrupting everyone else.

[Greg Bolsinga]

Also r57086

[Kenneth Rohde Christiansen]

(In reply to comment #54)
> Would you be willing to come on #webkit to coordinate?  If you expected this to
> light the tree on fire, why did you land it manually?  The commit-queue could
> have cannaried it for you without disrupting everyone else.

As discussed on irc, I needed the new results generated by the mac and Qt bot. The Qt bot generate slightly different results that I get locally due to font differences (sucks!).

Having EWS to run the tests and give back the results, would solve the problem.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57080 might have broken Tiger Intel Release

[Eric Seidel (no email)]

Looking at the various commits in the range, this appears to be the one that broke Tiger.

[Kenneth Rohde Christiansen]

(In reply to comment #58)
> Looking at the various commits in the range, this appears to be the one that
> broke Tiger.

Adam Barth is building and testing. The code shouldn't be affecting the non-flattening code path.

[Kenneth Rohde Christiansen]

(In reply to comment #59)
> (In reply to comment #58)
> > Looking at the various commits in the range, this appears to be the one that
> > broke Tiger.
> 
> Adam Barth is building and testing. The code shouldn't be affecting the
> non-flattening code path.

A clean rebuild on Tiger make the failing test work again.

[Simon Hausmann]

<cherry-pick-for-backport: 56855>

[Simon Hausmann]

<cherry-pick-for-backport: 57083>

[Simon Hausmann]

<cherry-pick-for-backport: 57086>

[Simon Hausmann]

Revision r56852 cherry-picked into qtwebkit-2.0 with commit 85def4da93f5509994f4349d4b415263498d1e1d
Revision r56854 cherry-picked into qtwebkit-2.0 with commit 7cbfd1fda76f72bc21d39f5a6a326cb2fff6f05d
Revision r56855 cherry-picked into qtwebkit-2.0 with commit fb15164cd65dd69200199c005a06b9bec6374069
Revision r56856 cherry-picked into qtwebkit-2.0 with commit 59daec93fc7ad1f4c5dbeb88b67aca17d6f4cc3b
Revision r57080 cherry-picked into qtwebkit-2.0 with commit 37453501b2151c33c89a7002fef63e1a3103c7df
Revision r57083 cherry-picked into qtwebkit-2.0 with commit a1eed0c2297d458e938ce8491142045c431301e2
Revision r57086 cherry-picked into qtwebkit-2.0 with commit 732efe4699b61e7e4b71773ff9272eceae28fe8b