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

RESOLVED FIXED 87707

https://bugs.webkit.org/show_bug.cgi?id=87707

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

Eric Seidel (no email)

Reported 2012-05-29 01:12:43 PDT

Attachments
Patch (8.15 KB, patch) 2013-01-08 06:19 PST, Mike West	no flags	Details Formatted Diff Diff
Patch (10.03 KB, patch) 2013-01-09 05:02 PST, Mike West	no flags	Details Formatted Diff Diff
Patch (5.58 KB, patch) 2013-01-23 07:17 PST, Mike West	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Mike West

Reported

2012-05-29 01:12 PDT

Modified

2013-02-07 23:37 PST History

CC List

6 users Show

URL

Keywords

Duplicates (1)

90836 View as bug list

Depends on

Blocks

45950 90836

Dependencies

tree graph