Bug 106167 – Seamless: IFrame's padding isn't taken into account when calculating its height.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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.

[Eric Seidel (no email)]

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)]

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

[Mike West]

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)]

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]

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]

Created attachment 181441 [details]
Patch

[Mike West]

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

Created attachment 181454 [details]
Patch

[Mike West]

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 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]

(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 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)]

Tab knows the answers to these questions. :)

[Mike West]

(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 on attachment 181454 [details]
Patch

If non-seamless iframes respect padding, than it's definitely a bug that seamless ones don't!

[Mike West]

(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 on attachment 181454 [details]
Patch

Clearing flags on attachment: 181454

Committed r138917: <http://trac.webkit.org/changeset/138917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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

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