90834 – the body of seamless iframes should default to margin:0

RESOLVED FIXED 90834

the body of seamless iframes should default to margin:0

https://bugs.webkit.org/show_bug.cgi?id=90834

Summary the body of seamless iframes should default to margin:0

Ojan Vafai

Reported 2012-07-09 16:28:23 PDT

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1657 <!DOCTYPE html> ...<style> iframe { border: solid; float: left; } </style> <iframe seamless srcdoc="<div style='background:yellow'>TEST</div>"></iframe>

Attachments
Patch (11.74 KB, patch) 2013-01-01 16:18 PST, Mike West	no flags	Details Formatted Diff Diff
Patch (16.45 KB, patch) 2013-01-01 17:11 PST, Mike West	no flags	Details Formatted Diff Diff
Patch (14.00 KB, patch) 2013-01-02 10:29 PST, Mike West	no flags	Details Formatted Diff Diff
Patch for landing (13.32 KB, patch) 2013-01-03 05:31 PST, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2012-07-11 15:31:12 PDT

Hixie mentioned that body margins were already complicated to calculate so this case shouldn't be hard to add... but I'm not yet sure where to find that code.

Eric Seidel (no email)

Comment 2 2012-07-11 15:34:58 PDT

Yes, a quick grep of our codebase shows margins are ridiculously complicated. :) Still unsure where this code needs to go though....

Mike West

Comment 3 2013-01-01 14:37:49 PST

Is this value actually calculated? My understanding was that default values for elements are loaded via the user agent's stylesheet (in this case, http://trac.webkit.org/browser/trunk/Source/WebCore/css/html.css#L59). It looks like one approach might be to define a 'seamless.css' that we'd pull in via StyleResolver:: ensureDefaultStyleSheetsForElement (http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L547). Assuming that's not a terrible idea, I'll hack that together quickly to see if it works at all.

Mike West

Comment 4 2013-01-01 16:18:02 PST

Created attachment 181000 [details] Patch

Mike West

Comment 5 2013-01-01 16:20:12 PST

After more than a bit of fiddling, StyleResolver::matchUARules looks like the right place to pull in a new stylesheet. I still have no idea what I'm doing here, so I'd very much appreciate a careful eye on this patch. It works*, but it might be the totally wrong way to do things. *on the Chromium port on my local machine. :)

Eric Seidel (no email)

Comment 6 2013-01-01 16:20:58 PST

Ccing the Style ninjas again. :)

Mike West

Comment 7 2013-01-01 16:21:47 PST

(In reply to comment #6) > Ccing the Style ninjas again. :) You're seriously fast. I was looking up the email addresses from the last patch... :)

EFL EWS Bot

Comment 8 2013-01-01 16:21:56 PST

Comment on attachment 181000 [details] Patch Attachment 181000 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15603913

Eric Seidel (no email)

Comment 9 2013-01-01 16:23:14 PST

Comment on attachment 181000 [details] Patch There are a bunch of tests which specifically add this margin: 0 for easier testing. Those tests could be modified now. This patch looks great to me! I would leave Antti/Kling 24hrs to comment, in case I'm missing something.

Eric Seidel (no email)

Comment 10 2013-01-01 16:26:42 PST

Comment on attachment 181000 [details] Patch This all compiles with seamless off, right?

Andreas Kling

Comment 11 2013-01-01 16:27:08 PST

Comment on attachment 181000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181000&action=review > Source/WebCore/css/StyleResolver.cpp:1361 > + if (!defaultSeamlessStyle) > + loadSeamlessStyle(); > + matchUARules(result, defaultSeamlessStyle); This pattern seems overly verbose, it would be nicer if we could just do: matchUARules(result, defaultSeamlessStyle()); (I see that we already have code like this elsewhere in StyleResolver.cpp, but I think we can do better.) > Source/WebCore/css/StyleResolver.cpp:5304 > + if (!defaultSeamlessStyle) > + loadSeamlessStyle(); > + m_features.add(defaultSeamlessStyle->features()); Again, would be nicer with just: m_features.add(defaultSeamlessStyle()->features());

Mike West

Comment 12 2013-01-01 16:31:19 PST

(In reply to comment #10) > (From update of attachment 181000 [details]) > This all compiles with seamless off, right? It might. I'll check. There might be a warning for the empty function. (In reply to comment #11) > (From update of attachment 181000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181000&action=review > > > Source/WebCore/css/StyleResolver.cpp:1361 > > + if (!defaultSeamlessStyle) > > + loadSeamlessStyle(); > > + matchUARules(result, defaultSeamlessStyle); > > This pattern seems overly verbose, it would be nicer if we could just do: > > matchUARules(result, defaultSeamlessStyle()); > > (I see that we already have code like this elsewhere in StyleResolver.cpp, but I think we can do better.) Makes sense. I can update the viewsource loader in this patch as well, if you like, or I can split that refactoring out into a smaller separate patch. Which would you prefer?

Andreas Kling

Comment 13 2013-01-01 16:35:23 PST

(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 181000 [details] [details]) > > This all compiles with seamless off, right? > > It might. I'll check. There might be a warning for the empty function. > > (In reply to comment #11) > > (From update of attachment 181000 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=181000&action=review > > > > > Source/WebCore/css/StyleResolver.cpp:1361 > > > + if (!defaultSeamlessStyle) > > > + loadSeamlessStyle(); > > > + matchUARules(result, defaultSeamlessStyle); > > > > This pattern seems overly verbose, it would be nicer if we could just do: > > > > matchUARules(result, defaultSeamlessStyle()); > > > > (I see that we already have code like this elsewhere in StyleResolver.cpp, but I think we can do better.) > > Makes sense. I can update the viewsource loader in this patch as well, if you like, or I can split that refactoring out into a smaller separate patch. Which would you prefer? I'd do it separately. Atomic patches <3.

Mike West

Comment 14 2013-01-01 16:38:38 PST

(In reply to comment #13) > I'd do it separately. Atomic patches <3. That's what I hoped you'd say. Tiny patches FTW. In that case, I'd just match the current style with this patch (after fixing EFL), and rework both cases in the next patch, since they'll be more or less exactly the same changes.

Mike West

Comment 15 2013-01-01 17:11:13 PST

Created attachment 181002 [details] Patch

WebKit Review Bot

Comment 16 2013-01-02 01:18:59 PST

Comment on attachment 181002 [details] Patch Clearing flags on attachment: 181002 Committed r138611: <http://trac.webkit.org/changeset/138611>

WebKit Review Bot

Comment 17 2013-01-02 01:19:05 PST

All reviewed patches have been landed. Closing bug.

Mike West

Comment 18 2013-01-02 02:10:11 PST

(In reply to comment #14) > (In reply to comment #13) > > I'd do it separately. Atomic patches <3. > > That's what I hoped you'd say. Tiny patches FTW. > > In that case, I'd just match the current style with this patch (after fixing EFL), and rework both cases in the next patch, since they'll be more or less exactly the same changes. I've uploaded a patch for this bit at https://bugs.webkit.org/show_bug.cgi?id=105903.

Antti Koivisto

Comment 19 2013-01-02 03:27:59 PST

Comment on attachment 181002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181002&action=review > Source/WebCore/css/seamless.css:3 > +body { > + margin: 0; > +} It seems crazy to add all this machinery (including separate .css file!) just for matching this one rule. Lets find a lighter weight solution. Also I'm not happy to see all these patches landing during holidays.

Mike West

Comment 20 2013-01-02 03:58:42 PST

(In reply to comment #19) > (From update of attachment 181002 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181002&action=review > > > Source/WebCore/css/seamless.css:3 > > +body { > > + margin: 0; > > +} > > It seems crazy to add all this machinery (including separate .css file!) just for matching this one rule. Lets find a lighter weight solution. What would you suggest? Would hard-coding a string into the StyleResolver (similar to the "simple" stylesheet) be appropriate, or is using the UA style machinery not the right answer at all? > Also I'm not happy to see all these patches landing during holidays. I'll roll both of the patches out. The intention certainly wasn't to surprise you; the holidays just gave me a some solid chunks of free time to ignore my day job and work through both of these bugs. :)

WebKit Review Bot

Comment 21 2013-01-02 04:00:45 PST

Re-opened since this is blocked by bug 105916

Antti Koivisto

Comment 22 2013-01-02 07:33:05 PST

> What would you suggest? Would hard-coding a string into the StyleResolver (similar to the "simple" stylesheet) be appropriate, or is using the UA style machinery not the right answer at all? A pseudo class might be the nicest solution. Then could just have body:-webkit-seamless-document { margin: 0 } or similar on the default sheet and all the machinery will work magically. If you grep for "PseudoFullScreenDocument" you pretty much learn everything about implementing it.

Mike West

Comment 23 2013-01-02 10:29:31 PST

Created attachment 181035 [details] Patch

Mike West

Comment 24 2013-01-02 10:31:14 PST

(In reply to comment #22) > > What would you suggest? Would hard-coding a string into the StyleResolver (similar to the "simple" stylesheet) be appropriate, or is using the UA style machinery not the right answer at all? > > A pseudo class might be the nicest solution. Then could just have > > body:-webkit-seamless-document { margin: 0 } > > or similar on the default sheet and all the machinery will work magically. If you grep for "PseudoFullScreenDocument" you pretty much learn everything about implementing it. The attached patch is a stab at that approach. Would you mind taking a look? I've modeled it after the full-screen-document pseudoclass, in that it matches for all element types. That might be useful in the future if the spec changes to involve more overrides in the future. What do you think?

WebKit Review Bot

Comment 25 2013-01-02 10:33:14 PST

Attachment 181035 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyleConstants.h:79: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Eric Seidel (no email)

Comment 26 2013-01-02 12:12:01 PST

Comment on attachment 181035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181035&action=review Seems reasonable to me. But so did the last one. I guess you should look for an antti in #webkit. :) > LayoutTests/fast/frames/seamless/seamless-inherited-origin.html:14 > + // Initially about:blank has no content, thus no height. > + shouldBeEqualToString("window.getComputedStyle(iframe).height", "0px"); This is only seamless documents end up with this magic 0px of course. :)

Antti Koivisto

Comment 27 2013-01-02 13:37:54 PST

Comment on attachment 181035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181035&action=review r=me with the changes > Source/WebCore/css/CSSSelector.cpp:169 > +#if ENABLE(IFRAME_SEAMLESS) > + case PseudoSeamlessDocument: > + return SEAMLESS_DOCUMENT; > +#endif You should't need this (you are not using it anywhere). FULL_SCREEN_DOCUMENT and pals look equally pointless, I think someone was just cargo-culting. >> Source/WebCore/rendering/style/RenderStyleConstants.h:79 >> FULL_SCREEN, FULL_SCREEN_DOCUMENT, FULL_SCREEN_ANCESTOR, ANIMATING_FULL_SCREEN_TRANSITION, >> + SEAMLESS_DOCUMENT, > > enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] It would be good to remove FULL_SCREEN ones too in another patch if they are as useless as they look. I believe the confusingly named PseudoId enum is only relevant for pseudo elements never pseudo classes.

Mike West

Comment 28 2013-01-03 05:31:19 PST

Created attachment 181166 [details] Patch for landing

WebKit Review Bot

Comment 29 2013-01-03 06:31:40 PST

Comment on attachment 181166 [details] Patch for landing Clearing flags on attachment: 181166 Committed r138709: <http://trac.webkit.org/changeset/138709>

WebKit Review Bot

Comment 30 2013-01-03 06:31:47 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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Mike West

Reported

2012-07-09 16:28 PDT

Modified

2013-01-03 06:31 PST History

CC List

14 users Show

URL

Keywords WebExposed

Depends on

105916

Blocks

45950 105903

Dependencies

tree graph