Bug 87707

Summary: <iframe seamless> using display: inline or float, (shrink-wrapping) are too tall
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Layout and RenderingAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, jamesr, jchaffraix, mkwst, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 45950, 90836    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Eric Seidel (no email)
Reported 2012-05-29 01:12:43 PDT
<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.
Attachments
Patch (8.15 KB, patch)
2013-01-08 06:19 PST, Mike West 		no flags
DetailsFormatted DiffDiff
Patch (10.03 KB, patch)
2013-01-09 05:02 PST, Mike West 		no flags
DetailsFormatted DiffDiff
Patch (5.58 KB, patch)
2013-01-23 07:17 PST, Mike West 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mike West
Comment 1 2013-01-06 13:00:34 PST
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)
Comment 2 2013-01-06 13:17:59 PST
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
Comment 3 2013-01-08 05:48:01 PST
(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
Comment 4 2013-01-08 06:19:12 PST
Created attachment 181687 [details] Patch
Build Bot
Comment 5 2013-01-08 08:03:22 PST
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
Comment 6 2013-01-08 08:56:50 PST
(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)
Comment 7 2013-01-08 10:45:49 PST
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
Comment 8 2013-01-09 05:02:24 PST
Created attachment 181899 [details] Patch
Mike West
Comment 9 2013-01-09 05:08:51 PST
(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)
Comment 10 2013-01-09 11:34:22 PST
Hyatt is in central time and tends to be on in the early afternoon PST.
Eric Seidel (no email)
Comment 11 2013-01-09 11:35:52 PST
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
Comment 12 2013-01-09 11:39:57 PST
(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
Comment 13 2013-01-09 11:48:39 PST
(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 14 2013-01-14 11:12:59 PST
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 15 2013-01-14 11:42:27 PST
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
Comment 16 2013-01-23 07:17:26 PST
Created attachment 184225 [details] Patch
Mike West
Comment 17 2013-01-23 07:21:44 PST
(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
Comment 18 2013-01-28 02:06:56 PST
*** Bug 90836 has been marked as a duplicate of this bug. ***
Mike West
Comment 19 2013-02-01 00:05:03 PST
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
Comment 20 2013-02-07 23:07:13 PST
(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 21 2013-02-07 23:08:44 PST
Comment on attachment 184225 [details] Patch This looks totally reasonable to me.
WebKit Review Bot
Comment 22 2013-02-07 23:37:24 PST
Comment on attachment 184225 [details] Patch Clearing flags on attachment: 184225 Committed r142236: <http://trac.webkit.org/changeset/142236>
WebKit Review Bot
Comment 23 2013-02-07 23:37:29 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.