<iframe seamless> using display: inline or float, (shrink-wrapping) are too tall

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

<iframe seamless> using display: inline or float, (shrink-wrapping) are too tall

This is causing two layout test failures:
http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/seamless/seamless-float-expected.txt#L3
http://trac.webkit.org/browser/trunk/LayoutTests/fast/frames/seamless/seamless-inline-expected.txt#L5

This is due to this code:
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderIFrame.cpp#L166

We set the height to 0, and then ask the child document to layout.  This confuses FrameView, which expects to be at its final size when doing scrollbar negotiation, so it adds a vertical scrollbar during layout, when we ask it then what it's content height is, that height is assuming there is a vertical scrollbar.

RenderBlock has no problem with this.  scrollbars are handled separately in the RenderLayer layout logic:
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayer.cpp#L2508

However, FrameView (which is a Widget, not a RenderObject, and holds the RenderView -- a RenderBlock subclass and root of the rendering tree) has its own scrollbar logic.

Ideally we'd find a way to skip over the scrollbar logic in FrameView, and have it use RenderBlock's logic instead.  I'm not exactly sure how that would look though.

Currently seamless piggy-backs on the frame-flattening code which causes the child FrameView to abort layout, and instead lays out from the root:
http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L929
http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L2926
http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L2948

Flattening gets around this by always disabling scrollbars.

However seamless does not (and should not) disable scrollbars, so they will affect layout:
http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L1039

I welcome any suggestions.

[Mike West]

Grabbing this. I'll spend some time reading through the relevant code tomorrow, Eric, then I'll ping you with hopefully intelligent questions. :)

[Eric Seidel (no email)]

One of the ways we might solve this is by moving away from RenderIframe to a separate renderer, which was just a RenderReplaced and didn't have any of the Frame/Widget overhead.

It would be tricky, as you'd have to re-invent some of that for this new renderer, and I'm not sure it's the right soluiton.

But Widgets are really a drag, and a big source of bugs for us.  seamless iframes don't ever want their OS-provided view abstractions (which is the point of the Widget tree), they always want to be seamlessly part of the document.

I guess one question si what happens when you put a plugin in a seamless iframe.  Since there you would need an OS-provided HWND, etc. to hand to the plugin.

It's very possible that we can make FrameView smarter here for the seamless case, and let seamless do the scrollbar negotation instead of FrameView.

[Mike West]

(In reply to comment #2)
> It's very possible that we can make FrameView smarter here for the seamless case, and let seamless do the scrollbar negotation instead of FrameView.

This is the route I'd like to try first. The major distinction between scrollbar negotiation in RenderBlock and FrameView/ScrollView is the first pass. RenderBlock waits until an initial pass of layout is finished before applying scrollbars to the content. FrameView sets vertical scrollbars to AlwaysOn in the first pass. One approach would be to simply special-case this first pass to avoid auto-application of scrollbars if the document that's being laid out is a) seamless, and b) has 0 height.

I have a patch which implements these checks that I'll upload in a moment, but I'll honestly be shocked if it's acceptable. :)

Eric, Ojan: I'll poke at one of you next time I see you on IRC to talk about where we go from here.

[Mike West]

Created attachment 181687 [details]
Patch

[Build Bot]

Comment on attachment 181687 [details]
Patch

Attachment 181687 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15771061

New failing tests:
fast/frames/seamless/seamless-inline.html
fast/frames/seamless/seamless-float.html

[Mike West]

(In reply to comment #5)
> (From update of attachment 181687 [details])
> Attachment 181687 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15771061
> 
> New failing tests:
> fast/frames/seamless/seamless-inline.html
> fast/frames/seamless/seamless-float.html

Mac apparently doesn't use updateScrollbars, branching into platformSetScrollbarModes instead[1]. Not sure if I can do anything there... :(

[1]: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ScrollView.cpp?rev=139063#L154

[Eric Seidel (no email)]

Hyatt would be the person to ask.  You might try to catch him on #webkit.  Both for review, and suggestion as to what one might do on Mac.  This patch seems reasonable to me, but again, Hyatt is the only person I've seen in the scrollbar code in years. :(

[Mike West]

Created attachment 181899 [details]
Patch

[Mike West]

(In reply to comment #7)
> Hyatt would be the person to ask.  You might try to catch him on #webkit.  Both for review, and suggestion as to what one might do on Mac.  This patch seems reasonable to me, but again, Hyatt is the only person I've seen in the scrollbar code in years. :(

I didn't see Hyatt on IRC last night, but I didn't stay up very late. I'll try again tonight.

In the meantime, I've expanded the patch slightly, adding a similar check to ScrollView::setContentsSize in order to bypass the platform-specific scrollbar-setting code completely on the first layout pass for seamless documents.

This feels even hackier than the initial patch, but it works locally, which is a step in the right direction. :)

[Eric Seidel (no email)]

Hyatt is in central time and tends to be on in the early afternoon PST.

[Eric Seidel (no email)]

Comment on attachment 181899 [details]
Patch

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

> Source/WebCore/platform/ScrollView.cpp:310
> +    if (isSeamlessDocument() && !fullVisibleSize.height())
> +        return;

Do we know if any badness can happen if we early-return here?  I don't know what those later calls do.

[Mike West]

(In reply to comment #11)
> (From update of attachment 181899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181899&action=review
> 
> > Source/WebCore/platform/ScrollView.cpp:310
> > +    if (isSeamlessDocument() && !fullVisibleSize.height())
> > +        return;
> 
> Do we know if any badness can happen if we early-return here?  I don't know what those later calls do.

We do not! I'm fairly confident that nothing terrible happens (I mean, otherwise a test would break, right? :P ), but I wouldn't be at all surprised to find that there are subtleties here that I'm completely missing.

[Mike West]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 181899 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181899&action=review
> > 
> > > Source/WebCore/platform/ScrollView.cpp:310
> > > +    if (isSeamlessDocument() && !fullVisibleSize.height())
> > > +        return;
> > 
> > Do we know if any badness can happen if we early-return here?  I don't know what those later calls do.
> 
> We do not! I'm fairly confident that nothing terrible happens (I mean, otherwise a test would break, right? :P ), but I wouldn't be at all surprised to find that there are subtleties here that I'm completely missing.

Also note that not knowing what that platform-specific call does is why I'm skipping the branch entirely. Otherwise I'd have dug in a bit deeper to find a smaller fix, similar to what this patch contains for ScrollView::updateScrollbars. The scrollbar code, so far as I can tell, is just headers.

[Dave Hyatt]

Comment on attachment 181899 [details]
Patch

I would take a different approach here and just turn the vertical scrollbar off if you're seamless (as though scrolling=no was set on the frame).

[Dave Hyatt]

Comment on attachment 181899 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1169
> -                    if (vMode == ScrollbarAuto)
> +                    if (vMode == ScrollbarAuto && !isSeamlessDocument())
>                          setVerticalScrollbarMode(ScrollbarAlwaysOn); // This causes a vertical scrollbar to appear.

To clarify, I would do the opposite here:

if (vMode == ScrollbarAuto)
    setVerticalScrollbarMode(!isSeamlessDocument() ? ScrollbarAlwaysOn : ScrollbarAlwaysOff);

Basically we should default to assuming that a scrollbar is not desired from a seamless document.

Not convinced any of the ScrollView changes will be necessary if you turn the scrollbar off on first layout.

[Mike West]

Created attachment 184225 [details]
Patch

[Mike West]

(In reply to comment #15)
> (From update of attachment 181899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181899&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1169
> > -                    if (vMode == ScrollbarAuto)
> > +                    if (vMode == ScrollbarAuto && !isSeamlessDocument())
> >                          setVerticalScrollbarMode(ScrollbarAlwaysOn); // This causes a vertical scrollbar to appear.
> 
> To clarify, I would do the opposite here:
> 
> if (vMode == ScrollbarAuto)
>     setVerticalScrollbarMode(!isSeamlessDocument() ? ScrollbarAlwaysOn : ScrollbarAlwaysOff);
> 
> Basically we should default to assuming that a scrollbar is not desired from a seamless document.
> 
> Not convinced any of the ScrollView changes will be necessary if you turn the scrollbar off on first layout.

Thanks for the feedback.

This wasn't enough on its own; in fact, it looks like the first pass of layout is irrelevant. The height doesn't change until the second pass. I'd love to tell you that I understand exactly why: I don't.

The current patch addresses the problem by moving the special-casing into FrameView::calculateScrollbarModesForLayout. If that method would set the vertical mode to ScrollbarAuto, it now checks whether the document is a) seamless, and b) has a full visible height of 0. In that case, it sets the vertical mode to ScrollbarAlwaysOff.

This takes care of the initial passes of layout where the height is 0, and seems to do the right thing in later passes, once the height has been set to the contentHeight of the framed document.

Assuming that the bots are happy, would you mind taking another look?

[Mike West]

*** Bug 90836 has been marked as a duplicate of this bug. ***

[Mike West]

Friendly ping, dhyatt. Would you mind taking a look at this patch?

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 181899 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181899&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:1169
> > > -                    if (vMode == ScrollbarAuto)
> > > +                    if (vMode == ScrollbarAuto && !isSeamlessDocument())
> > >                          setVerticalScrollbarMode(ScrollbarAlwaysOn); // This causes a vertical scrollbar to appear.
> > 
> > To clarify, I would do the opposite here:
> > 
> > if (vMode == ScrollbarAuto)
> >     setVerticalScrollbarMode(!isSeamlessDocument() ? ScrollbarAlwaysOn : ScrollbarAlwaysOff);
> > 
> > Basically we should default to assuming that a scrollbar is not desired from a seamless document.
> > 
> > Not convinced any of the ScrollView changes will be necessary if you turn the scrollbar off on first layout.
> 
> Thanks for the feedback.
> 
> This wasn't enough on its own; in fact, it looks like the first pass of layout is irrelevant. The height doesn't change until the second pass. I'd love to tell you that I understand exactly why: I don't.
> 
> The current patch addresses the problem by moving the special-casing into FrameView::calculateScrollbarModesForLayout. If that method would set the vertical mode to ScrollbarAuto, it now checks whether the document is a) seamless, and b) has a full visible height of 0. In that case, it sets the vertical mode to ScrollbarAlwaysOff.
> 
> This takes care of the initial passes of layout where the height is 0, and seems to do the right thing in later passes, once the height has been set to the contentHeight of the framed document.
> 
> Assuming that the bots are happy, would you mind taking another look?

[Mike West]

(In reply to comment #19)
> Friendly ping, dhyatt. Would you mind taking a look at this patch?

Looks like dhyatt is pretty busy. Eric, Ojan, would you mind taking a stab at reviewing this?

[Eric Seidel (no email)]

Comment on attachment 184225 [details]
Patch

This looks totally reasonable to me.

[WebKit Review Bot]

Comment on attachment 184225 [details]
Patch

Clearing flags on attachment: 184225

Committed r142236: <http://trac.webkit.org/changeset/142236>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.