Bug 106167

Summary: Seamless: IFrame's padding isn't taken into account when calculating its height.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan.autocc, ojan, robert, tabatkins, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 45950    
Attachments:
Description Flags
Patch
none
Patch none

Mike West
Reported Saturday, January 5, 2013 7:14:06 AM UTC
fast/frames/seamless/seamless-border.html contains two failures, both demonstrating that seamless IFrames don't correctly calculate their height when padding is applied to the frame. Width is correctly calculated, but the padding is ignored when calculating height.
Attachments
Patch (4.14 KB, patch)
2013-01-05 11:37 PST, Mike West
no flags
Patch (4.14 KB, patch)
2013-01-06 00:17 PST, Mike West
no flags
Eric Seidel (no email)
Comment 1 Saturday, January 5, 2013 7:18:37 AM UTC
Presumably this code is layoutSeamlessly is what you need to change: FrameView* childFrameView = static_cast<FrameView*>(widget()); if (childFrameView) // Widget should never be null during layout(), but just in case. setLogicalHeight(childFrameView->contentsHeight() + borderTop() + borderBottom()); void RenderFrameBase::layoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight) likely also gets it wrong. There are frame flattening tests in fast/frames/flattening if you feel so inclined.
Eric Seidel (no email)
Comment 2 Saturday, January 5, 2013 7:18:53 AM UTC
seamless and frameflattening share a lot of code. frame-flattening is a mobile-only feature.
Mike West
Comment 3 Saturday, January 5, 2013 7:20:22 AM UTC
I'll certainly take a look at the flattening code as well. Makes sense to fix both in the same patch, if they're both incorrect. Thanks for the pointer!
Eric Seidel (no email)
Comment 4 Saturday, January 5, 2013 7:22:46 AM UTC
Flattening's behavior is slightly different wrt scrollbars. Flattening is not so much about displaying the content seamlessly, as expanding the iframe enough to that the user never has to see nested scrollbars. Dealing with nested frames on mobile is a pain. :) Speaking of which... I suspect that seamless may have some scrolling bugs as well if we go looking.
Mike West
Comment 5 Saturday, January 5, 2013 12:43:15 PM UTC
Fixing RenderIFrame::layoutSeamlessly is trivial; I'll put up a patch as soon as the patch for https://bugs.webkit.org/show_bug.cgi?id=90827 lands (looks like the CQ is wedged), as this fix depends on that one. Flattening seems disabled on the Chromium port, but I'll see if I can poke at it on the mac port in a separate patch. Neither vertical nor horizontal padding is accounted for in RenderFrameBase::layoutWithFlattening; It seems like the right thing to do, though, so I'll take care of both in https://bugs.webkit.org/show_bug.cgi?id=106174.
Mike West
Comment 6 Saturday, January 5, 2013 7:37:05 PM UTC
Mike West
Comment 7 Saturday, January 5, 2013 7:38:27 PM UTC
(In reply to comment #5) > Fixing RenderIFrame::layoutSeamlessly is trivial; I'll put up a patch as soon as the patch for https://bugs.webkit.org/show_bug.cgi?id=90827 lands (looks like the CQ is wedged), as this fix depends on that one. webkit-patch is, happily, clever enough to upload a patch even though the previous patch hasn't landed yet(!). I'm not going to throw this to the bots until it's in, but the change is pretty clear regardless.
Mike West
Comment 8 Sunday, January 6, 2013 8:17:32 AM UTC
Mike West
Comment 9 Sunday, January 6, 2013 8:18:08 AM UTC
Hrm. It apparently hit the bots anyway. Uploading the same patch again for EWS now that the previous patch landed.
Eric Seidel (no email)
Comment 10 Sunday, January 6, 2013 8:25:09 AM UTC
Comment on attachment 181454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181454&action=review > Source/WebCore/rendering/RenderIFrame.cpp:134 > + // Replaced elements normally do not respect padding, but seamless elements should: we'll add > + // both padding and border to the child's logical height here. Is this true? I don't know what the expected behavior is?
Mike West
Comment 11 Sunday, January 6, 2013 8:32:45 AM UTC
(In reply to comment #10) > (From update of attachment 181454 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181454&action=review > > > Source/WebCore/rendering/RenderIFrame.cpp:134 > > + // Replaced elements normally do not respect padding, but seamless elements should: we'll add > > + // both padding and border to the child's logical height here. > > Is this true? I don't know what the expected behavior is? Hrm. Which part of this are you questioning? :) I thought that dropping the padding-related FIXME here was the point of the patch; did I misunderstand your comments?
Eric Seidel (no email)
Comment 12 Sunday, January 6, 2013 8:47:08 AM UTC
Comment on attachment 181454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181454&action=review >>> Source/WebCore/rendering/RenderIFrame.cpp:134 >>> + // both padding and border to the child's logical height here. >> >> Is this true? I don't know what the expected behavior is? > > Hrm. Which part of this are you questioning? :) > > I thought that dropping the padding-related FIXME here was the point of the patch; did I misunderstand your comments? Yeah, I guess I'm just re-asking the FIXME. :) I don't know if they should or not?
Eric Seidel (no email)
Comment 13 Sunday, January 6, 2013 8:48:52 AM UTC
Tab knows the answers to these questions. :)
Mike West
Comment 14 Sunday, January 6, 2013 8:55:50 AM UTC
(In reply to comment #12) > (From update of attachment 181454 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181454&action=review > > >>> Source/WebCore/rendering/RenderIFrame.cpp:134 > >>> + // both padding and border to the child's logical height here. > >> > >> Is this true? I don't know what the expected behavior is? > > > > Hrm. Which part of this are you questioning? :) > > > > I thought that dropping the padding-related FIXME here was the point of the patch; did I misunderstand your comments? > > Yeah, I guess I'm just re-asking the FIXME. :) I don't know if they should or not? The spec only talks about setting the "intrinsic height" of the frame via the child's height. I'm pretty sure that doesn't include padding or borders. Given that non-seamless frames respect padding (http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2058), it seems like the right thing to do for seamless frames as well. That said, yes, Tab will know better than I. :)
Eric Seidel (no email)
Comment 15 Sunday, January 6, 2013 9:14:05 AM UTC
Comment on attachment 181454 [details] Patch If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't!
Mike West
Comment 16 Sunday, January 6, 2013 9:39:32 AM UTC
(In reply to comment #15) > (From update of attachment 181454 [details]) > If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't! Or the non-seamless behavior is a bug. :) I think this patch is correct. If it turns out that we're doing the wrong thing with IFrames in general, I'll poke at it in a separate patch.
WebKit Review Bot
Comment 17 Sunday, January 6, 2013 9:44:32 AM UTC
Comment on attachment 181454 [details] Patch Clearing flags on attachment: 181454 Committed r138917: <http://trac.webkit.org/changeset/138917>
WebKit Review Bot
Comment 18 Sunday, January 6, 2013 9:44:37 AM UTC
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 19 Sunday, January 6, 2013 6:52:11 PM UTC
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 181454 [details] [details]) > > If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't! > > Or the non-seamless behavior is a bug. :) > > I think this patch is correct. If it turns out that we're doing the wrong thing with IFrames in general, I'll poke at it in a separate patch. The non seamless behavior is easy to confirm in other browsers
Mike West
Comment 20 Sunday, January 6, 2013 7:20:11 PM UTC
(In reply to comment #19) > (In reply to comment #16) > > (In reply to comment #15) > > > (From update of attachment 181454 [details] [details] [details]) > > > If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't! > > > > Or the non-seamless behavior is a bug. :) > > > > I think this patch is correct. If it turns out that we're doing the wrong thing with IFrames in general, I'll poke at it in a separate patch. > > The non seamless behavior is easy to confirm in other browsers Works in Firefox.
Note You need to log in before you can comment on or make changes to this bug.