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 Rendering

Assignee:

Mike West <mkwst>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, ian, mkwst, ojan.autocc, webkit.review.bot

Priority:

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

Ojan Vafai

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

Attachments
Patch (4.92 KB, patch) 2013-01-04 14:58 PST, Mike West	no flags	Details Formatted Diff Diff
Patch for landing (6.27 KB, patch) 2013-01-04 23:19 PST, Mike West	no flags	Details Formatted Diff Diff
Patch for landing (6.27 KB, patch) 2013-01-04 23:22 PST, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 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...

Eric Seidel (no email)

Comment 2 2012-07-09 15:32:18 PDT

At least the first one looks easy to fix.

Ojan Vafai

Comment 3 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>

Eric Seidel (no email)

Comment 4 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.

Mike West

Comment 5 2013-01-04 13:43:18 PST

Since this just popped up on #webkit, I guess I'll take it next.

Eric Seidel (no email)

Comment 6 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. :)

Mike West

Comment 7 2013-01-04 14:58:01 PST

Created attachment 181381 [details] Patch

Mike West

Comment 8 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!

Ojan Vafai

Comment 9 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. :)

Eric Seidel (no email)

Comment 10 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.

Mike West

Comment 11 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

Eric Seidel (no email)

Comment 12 2013-01-04 23:16:01 PST

Fantastic!

Mike West

Comment 13 2013-01-04 23:19:33 PST

Created attachment 181428 [details] Patch for landing

Eric Seidel (no email)

Comment 14 2013-01-04 23:20:30 PST

Once again, I really appreciate you taking this on Mike.

Mike West

Comment 15 2013-01-04 23:21:07 PST

Comment on attachment 181428 [details] Patch for landing Ugh. Typo.

Mike West

Comment 16 2013-01-04 23:22:32 PST

Created attachment 181429 [details] Patch for landing

WebKit Review Bot

Comment 17 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>

WebKit Review Bot

Comment 18 2013-01-05 12:52:09 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.