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>
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.
Yes, a quick grep of our codebase shows margins are ridiculously complicated. :) Still unsure where this code needs to go though....
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.
Created attachment 181000 [details] Patch
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. :)
Ccing the Style ninjas again. :)
(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... :)
Comment on attachment 181000 [details] Patch Attachment 181000 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15603913
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.
Comment on attachment 181000 [details] Patch This all compiles with seamless off, right?
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());
(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?
(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.
(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.
Created attachment 181002 [details] Patch
Comment on attachment 181002 [details] Patch Clearing flags on attachment: 181002 Committed r138611: <http://trac.webkit.org/changeset/138611>
All reviewed patches have been landed. Closing bug.
(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.
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.
(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. :)
Re-opened since this is blocked by bug 105916
> 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.
Created attachment 181035 [details] Patch
(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?
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.
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. :)
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.
Created attachment 181166 [details] Patch for landing
Comment on attachment 181166 [details] Patch for landing Clearing flags on attachment: 181166 Committed r138709: <http://trac.webkit.org/changeset/138709>