Bug 53546 – When frame flattening is on iframe outside viewport is not flattened

• Component: Frames
• Status: NEW

[Misha]

When frame flattening is on and page has an iframe which is partially visible or not visible at all this iframe is not flattened and contentsSize() reports incorrect value.

To reproduce enable the frame flattening, make the window small enough and load attached example. Depending on the size of your window iframe will be partially visible or not visible at all. When you scroll you can see that it's not flattened.

[Misha]

Created attachment 80844 [details]
test case

[Misha]

It's the same issue Kenneth has seen in https://bugs.webkit.org/show_bug.cgi?id=36798#c35

What I found is that in bool RenderIFrame::flattenFrame() in the bottom we do 
return absoluteBoundingBoxRect().intersects(IntRect(IntPoint(0, 0), view->contentsSize()));
the comment says: 
// Do not flatten offscreen inner frames during frame flattening.
Firts of all during first layout content size is not set yet, so the result will be false.
Second, what was the intent? Why we don't want to flatten iframe which are outside viewport?
Should I just remove this line and return true?

[Kenneth Rohde Christiansen]

This was done so on purpose as pages uses offscreen iframes for some things. Not doing this broke lala.com (im not sure it still does). I am pretty sure there is a layout test that tests this.

[Misha]

Created attachment 80954 [details]
patch

I'm proposing the fix for the issue. If contents size is empty we cannot check if the iframe is off screen so let's do flattening.

[Misha]

It looks like it will be always the situation when some sites are broken when frame flattening is on. I think it's just the nature of this feature - we just forcefully changing layout of the page. So why we don't want to flatten offscreen iframes? (with submitted patch we still don't, but I'm wondering)
Another question is how frame flattening can and should co-exist with overflow:hidden of the parent.

[Yael]

(In reply to comment #4)
> Created an attachment (id=80954) [details]
> patch
> 
> I'm proposing the fix for the issue. If contents size is empty we cannot check if the iframe is off screen so let's do flattening.

I am not sure this is really fixing a bug. The next round of layout will pick up the content and will flatten it.

[Misha]

(In reply to comment #6)
> I am not sure this is really fixing a bug. The next round of layout will pick up the content and will flatten it.

Well, next round of layout will go through the frame flattening but for some reasons it doesn't affect contents size. I'm looking now why contents size are not getting changed. 
In the same time I still don't understand why we want to "break" original layout for "normal" iframes, but don't want to do it for off screen ones.

[Misha]

I noticed that when load is happening for the first time iframe is getting flattened correctly. But if I do page reload after that it doesn't go through frame flattening. 
So __IF__ layoutWithFlattening() is invoked everything is ok. The issues is that  layoutWithFlattening() is not invoked all the time because flattenFrame() returns false because contentsSize is not set yet. It looks like during initial page load we go through extra round of layout. I think it happens because iframe content arrives later (through posted network reply event) and when it happens another layout is triggered which goes through layoutWithFlattening().
When we do page reload the code path is different and we have one layout less. 

Antti, Kenneth,  any idea what would be the correct approach to fix the issue?

[Ademar Reis]

Is this targeted at 2.1/2.1.x? (please add it to the meta-bug if that's the case).

[Suresh Voruganti]

(In reply to comment #9)
> Is this targeted at 2.1/2.1.x? (please add it to the meta-bug if that's the case).

This is targeted for 2.1.x

[Misha]

I noticed that the issue is not reproducible on the trunk anymore but still is repro on QtWebkit 2.1.x (with Yeal fix for https://bugs.webkit.org/show_bug.cgi?id=52449). Looking now what change in trunk makes the difference.

[Eric Seidel (no email)]

Comment on attachment 80954 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80954&action=review

How do we test this?

> Source/WebCore/ChangeLog:6
> +During initial layout when frame flattening is on contents size is not set yet. Becuase of this the check that the frame is not offscreen is failing and flattenFrame() returns false which cause layout go without frame flattening. 

Indent?  and maybe a little wrapping :)

[Misha]

Created attachment 83654 [details]
test case 

removed absolute positioning ov the div. This test case works on the trunk but failing on QtWebkit 2.1.x

[Misha]

(In reply to comment #12)
> (From update of attachment 80954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80954&action=review
> 
> How do we test this?
> 
> > Source/WebCore/ChangeLog:6
> > +During initial layout when frame flattening is on contents size is not set yet. Becuase of this the check that the frame is not offscreen is failing and flattenFrame() returns false which cause layout go without frame flattening. 
> 
> Indent?  and maybe a little wrapping :)

The test case is attached. The blue div should be shown 1000px by 1400px.

The patch doesn't make sense now as it work on the trunk, but we need to identify the change that need to be pulled from trunk to QtWebkit 2.1.x

[Caio Marcelo de Oliveira Filho]

(In reply to comment #14)
> The patch doesn't make sense now as it work on the trunk, but we need to identify the change that need to be pulled from trunk to QtWebkit 2.1.x

Misha, any progress on this?

[Misha]

(In reply to comment #15)
> (In reply to comment #14)
> > The patch doesn't make sense now as it work on the trunk, but we need to identify the change that need to be pulled from trunk to QtWebkit 2.1.x
> 
> Misha, any progress on this?

Unfortunately not. I was busy doing other things. All I know that on trunk we have at least one layout more but it were quite a few changes in the layout logic since qtwebkit 2.1.x has been branched.I'll try to spend some time nex week on it. On the other hand I'm not sure if it makes sense now.

[Joel Parks]

no longer blocking 50925 per Misha email

[Simon Fraser (smfr)]

Bug 61491 is related.

[Ahmad Saleem]

Attached testcase behave similar in all browsers (Chrome Canary 115, Firefox Nightly 115 & STP169).

1) Shows scrollbar (horizontal & vertical)
2) Scrollable in both directions

If I missed anything, please highlight else I think we can close it. Thanks!