Bug 106167

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.

seamless and frameflattening share a lot of code. frame-flattening is a mobile-only feature.

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!

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.

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.

Created attachment 181441 [details] Patch

(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.

Created attachment 181454 [details] Patch

Hrm. It apparently hit the bots anyway. Uploading the same patch again for EWS now that the previous patch landed.

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?

(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?

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?

Tab knows the answers to these questions. :)

(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. :)

Comment on attachment 181454 [details] Patch If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't!

(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.

Comment on attachment 181454 [details] Patch Clearing flags on attachment: 181454 Committed r138917: <http://trac.webkit.org/changeset/138917>

All reviewed patches have been landed. Closing bug.

(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

(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.