WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
106167
Seamless: IFrame's padding isn't taken into account when calculating its height.
https://bugs.webkit.org/show_bug.cgi?id=106167
Summary
Seamless: IFrame's padding isn't taken into account when calculating its height.
Mike West
Reported
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.
Attachments
Patch
(4.14 KB, patch)
2013-01-05 11:37 PST
,
Mike West
no flags
Details
Formatted Diff
Diff
Patch
(4.14 KB, patch)
2013-01-06 00:17 PST
,
Mike West
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
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.
Eric Seidel (no email)
Comment 2
2013-01-04 23:18:53 PST
seamless and frameflattening share a lot of code. frame-flattening is a mobile-only feature.
Mike West
Comment 3
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!
Eric Seidel (no email)
Comment 4
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.
Mike West
Comment 5
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
.
Mike West
Comment 6
2013-01-05 11:37:05 PST
Created
attachment 181441
[details]
Patch
Mike West
Comment 7
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.
Mike West
Comment 8
2013-01-06 00:17:32 PST
Created
attachment 181454
[details]
Patch
Mike West
Comment 9
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.
Eric Seidel (no email)
Comment 10
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?
Mike West
Comment 11
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?
Eric Seidel (no email)
Comment 12
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?
Eric Seidel (no email)
Comment 13
2013-01-06 00:48:52 PST
Tab knows the answers to these questions. :)
Mike West
Comment 14
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. :)
Eric Seidel (no email)
Comment 15
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!
Mike West
Comment 16
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.
WebKit Review Bot
Comment 17
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
>
WebKit Review Bot
Comment 18
2013-01-06 01:44:37 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 19
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
Mike West
Comment 20
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug