Bug 90827

Summary: seamless iframes don't take border into account properly and make the iframe too small.
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ian, mkwst, ojan.autocc, 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 for landing
none
Patch for landing none

Description Ojan Vafai 2012-07-09 15:25:57 PDT
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1656
Doesn't take border into account properly and makes the iframe too small.

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1655
Puts scrollbars.
Comment 1 Eric Seidel (no email) 2012-07-09 15:31:38 PDT
Yup.  I'm aware there are a couple minor shrink-wrap issues.  Working on security issues with seamless atm.

It's probably most helpful to me if you were to attach the reductions to the bug (and that each issue be its own bug).  I guess we can trust that Hixie's site will be around a while...
Comment 2 Eric Seidel (no email) 2012-07-09 15:32:18 PDT
At least the first one looks easy to fix.
Comment 3 Ojan Vafai 2012-07-09 16:30:57 PDT
Moved the second to bug 90836.

This bug is now just about http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1656:
<!DOCTYPE html>
...<style>
 iframe { border: solid;  float: left; }
</style>
<iframe seamless srcdoc="<style>*{margin:0;padding:0;overflow:hidden}</style><div style='background:yellow'>TEST</div>"></iframe>
Comment 4 Eric Seidel (no email) 2012-07-11 15:27:18 PDT
It looks like the vertical border is being incorporated correctly, but the horrizontal border isn't.  RenderFrame::computeLogicalWidth() just calls up to RenderBox::computeLogicalWidth in the seamless case, being careful to pretend to have the correct replaced/inline/etc flags.  I'm not sure what's going wrong in this case.
Comment 5 Mike West 2013-01-04 13:43:18 PST
Since this just popped up on #webkit, I guess I'll take it next.
Comment 6 Eric Seidel (no email) 2013-01-04 14:22:20 PST
Hyatt seemed to think this would be easy to fix.  It's not obvious to me which line of code is missing here, but this is probably a one-liner. :)
Comment 7 Mike West 2013-01-04 14:58:01 PST
Created attachment 181381 [details]
Patch
Comment 8 Mike West 2013-01-04 14:59:27 PST
(In reply to comment #6)
> Hyatt seemed to think this would be easy to fix.  It's not obvious to me which line of code is missing here, but this is probably a one-liner. :)

Two-liner, plus a patch. But you both were right, it was a fairly trivial fix (assuming that I properly converted your instructions into code).

Thanks!
Comment 9 Ojan Vafai 2013-01-04 16:28:17 PST
Comment on attachment 181381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181381&action=review

> LayoutTests/fast/frames/seamless/seamless-border.html:7
> +        #hasborder { border: 10px solid black; }

Mind adding padding to this test case since your change adds in both border and padding? Or you can do it in a separate patch. I'm not sure we have test coverage that padding on seamless iframes works at all. :)
Comment 10 Eric Seidel (no email) 2013-01-04 16:46:37 PST
Comment on attachment 181381 [details]
Patch

Yes, please do test padding. :)  Not required to do so in this patch, but it makes sense to, since it will be easy for you to verify pre-patch if it's broken or not.
Comment 11 Mike West 2013-01-04 23:14:43 PST
You'll be shocked, shocked!, to learn that padding doesn't completely work.

This patch fixes padding for the width of the IFrame, but the height isn't picked up. I've added test cases to the patch, and I'll land it with the height failures. I'll tackle that bug in http://wkbug.com/106167
Comment 12 Eric Seidel (no email) 2013-01-04 23:16:01 PST
Fantastic!
Comment 13 Mike West 2013-01-04 23:19:33 PST
Created attachment 181428 [details]
Patch for landing
Comment 14 Eric Seidel (no email) 2013-01-04 23:20:30 PST
Once again, I really appreciate you taking this on Mike.
Comment 15 Mike West 2013-01-04 23:21:07 PST
Comment on attachment 181428 [details]
Patch for landing

Ugh. Typo.
Comment 16 Mike West 2013-01-04 23:22:32 PST
Created attachment 181429 [details]
Patch for landing
Comment 17 WebKit Review Bot 2013-01-05 12:52:05 PST
Comment on attachment 181429 [details]
Patch for landing

Clearing flags on attachment: 181429

Committed r138904: <http://trac.webkit.org/changeset/138904>
Comment 18 WebKit Review Bot 2013-01-05 12:52:09 PST
All reviewed patches have been landed.  Closing bug.