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

Description Mike West 2013-01-04 23:14:06 PST
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.
Comment 1 Eric Seidel (no email) 2013-01-04 23:18:37 PST
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.
Comment 2 Eric Seidel (no email) 2013-01-04 23:18:53 PST
seamless and frameflattening share a lot of code.  frame-flattening is a mobile-only feature.
Comment 3 Mike West 2013-01-04 23:20:22 PST
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!
Comment 4 Eric Seidel (no email) 2013-01-04 23:22:46 PST
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.
Comment 5 Mike West 2013-01-05 04:43:15 PST
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.
Comment 6 Mike West 2013-01-05 11:37:05 PST
Created attachment 181441 [details]
Patch
Comment 7 Mike West 2013-01-05 11:38:27 PST
(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.
Comment 8 Mike West 2013-01-06 00:17:32 PST
Created attachment 181454 [details]
Patch
Comment 9 Mike West 2013-01-06 00:18:08 PST
Hrm. It apparently hit the bots anyway. Uploading the same patch again for EWS now that the previous patch landed.
Comment 10 Eric Seidel (no email) 2013-01-06 00:25:09 PST
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?
Comment 11 Mike West 2013-01-06 00:32:45 PST
(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 12 Eric Seidel (no email) 2013-01-06 00:47:08 PST
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?
Comment 13 Eric Seidel (no email) 2013-01-06 00:48:52 PST
Tab knows the answers to these questions. :)
Comment 14 Mike West 2013-01-06 00:55:50 PST
(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 15 Eric Seidel (no email) 2013-01-06 01:14:05 PST
Comment on attachment 181454 [details]
Patch

If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't!
Comment 16 Mike West 2013-01-06 01:39:32 PST
(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 17 WebKit Review Bot 2013-01-06 01:44:32 PST
Comment on attachment 181454 [details]
Patch

Clearing flags on attachment: 181454

Committed r138917: <http://trac.webkit.org/changeset/138917>
Comment 18 WebKit Review Bot 2013-01-06 01:44:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Eric Seidel (no email) 2013-01-06 10:52:11 PST
(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
Comment 20 Mike West 2013-01-06 11:20:11 PST
(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.