Bug 103539

Summary: seamless iframes don't inherit styles when srcdoc is used
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, kling, koivisto, mkwst, ojan.autocc, ojan, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1932
Bug Depends on: 105917    
Bug Blocks: 45950    
Attachments:
Description Flags
Patch
none
Patch for landing.
none
Patch
none
Patch for landing none

Description Ojan Vafai 2012-11-28 10:38:58 PST
Not sure what combination of incantations is needed, but see the URL for a case where we don't correctly inherit the CSS from the parent.

Are we possibly treating srcdoc documents as being on a separate domain?
Comment 1 Mike West 2012-11-28 10:50:06 PST
Actually, it's even stranger than your example. Look at http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1933 for instance. `body` styles are inherited in the srcdoc case. It works with `body *` as well, but not `body h1`.
Comment 2 Mike West 2012-12-31 03:24:10 PST
I'm working my way through this, though slowly, as I Have No Idea What I'm Doing™ in rendering code.

I think that simple srcdoc documents aren't triggering a recalcStyle that's necessary to get the rendering right: `srcdoc="<p>Text.</p>"` doesn't inherit style, whereas `srcdoc="<style></style><p>Text.</p>"` does. See http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2052 for a demonstration.

I'll keep printf-debugging my way through the code until I hit something interesting, but if you have a suggestion right off the top of your head as to what might be going on, I'd appreciate it. :)
Comment 3 Eric Seidel (no email) 2012-12-31 06:10:18 PST
Seamless inheritance is done here:
http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L1427
(just search for seamless in that file)

This may be the fixme you're hitting:
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L4580 :)
Comment 4 Mike West 2012-12-31 07:16:04 PST
(In reply to comment #3)
> Seamless inheritance is done here:
> http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L1427
> (just search for seamless in that file)
> 
> This may be the fixme you're hitting:
> http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L4580 :)

It may be, but I don't think it is. Both documents are treated as seamless (e.g. that FIXME returns true as it should), and, so far as I can tell, inheritance is working correctly. It looks like updateStyleIfNeeded is called fairly often on both documents, but it always returns early on the version without style (it bails on http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L1863, as there's not a pending forced layout, nor is a child dirty).

My working theory is that scheduleForcedStyleRecalc needs to be called at some point in the processing of the srcdoc data in order to kickstart the styling process, since there's no stylesheet being added to the simple document (which would call it in updateActiveStyleSheets), but I'm not seeing any relevant difference in how the loader handles substitute data vs "real" data. *shrug* That's where I'm spinning my wheels at the moment. Hopefully it's not a dead end. :)
Comment 5 Eric Seidel (no email) 2012-12-31 07:22:53 PST
When the parser creates the DOM nodes it calls lazyAttach on them, which should mark them (and their ancestors) as needing style resolve.
Comment 6 Mike West 2012-12-31 07:41:46 PST
Created attachment 180976 [details]
Patch
Comment 7 Mike West 2012-12-31 07:43:57 PST
The attached patch solves the problem by calling Document::styleResolverChanged from Document::finishedParsing iff the document is seamless. I'm not sure it's a good solution, but it works. :)

I'm happy to continue to dig through loader bits and pieces to see if there's some better spot to inform the document that it needs to do this sort of thing, but a one-time call to styleResolverChanged seems like a not-too-terrible impact (assuming I haven't missed some terrible side-effects...).

WDYT?
Comment 8 Eric Seidel (no email) 2012-12-31 07:47:35 PST
I suspect that yes, the StyleResolver needs a recalc.  It's probably right to special-case seamless to force that to happen.  But I'm not sure where the right place to do that is.
Comment 9 Eric Seidel (no email) 2012-12-31 07:49:57 PST
Comment on attachment 180976 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4388
> +    if (shouldDisplaySeamlesslyWithParent())
> +        styleResolverChanged(DeferRecalcStyle);

I might add a comment here which says something like:
// Seamless iframes will need a StyleResolver recalc to inherit styles from their parent even
// if they don't have any styling of their own (which would normally trigger this recalc).
Comment 10 Eric Seidel (no email) 2012-12-31 07:50:20 PST
Also ccing the StyleResolver ninjas.
Comment 11 Mike West 2012-12-31 08:18:24 PST
(In reply to comment #10)
> Also ccing the StyleResolver ninjas.

I'll wait a bit for their feedback, and, assuming they're happy, I'll land it in the new year with the suggested comments.

Thanks!
Comment 12 Mike West 2013-01-01 07:20:33 PST
Created attachment 180991 [details]
Patch for landing.
Comment 13 Mike West 2013-01-01 09:15:48 PST
Comment on attachment 180991 [details]
Patch for landing.

Bots are happy and this seems like a low-risk fix. I'll just land this to change the currently broken behavior, and FYI kling and anttik whenever I see them in IRC.
Comment 14 Eric Seidel (no email) 2013-01-01 09:21:38 PST
Comment on attachment 180991 [details]
Patch for landing.

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

> Source/WebCore/dom/Document.cpp:4391
> +    // Seamless iframes require a forced StyleResolver recalc in order to ensure that they
> +    // inherit style from their parent. Without this recalc, frames that don't define any of
> +    // their own styles won't discover that there's still work to be done.
> +    if (shouldDisplaySeamlesslyWithParent())
> +        styleResolverChanged(DeferRecalcStyle);

It's not clear why documents aren't created needing a style resolver recalc.  It's possible that we optimize this away by creating the StyleResolver at creation time.  It's also possible that the style resolver creation happens before we're wired up to the parent document (if that's possible)?  I suspect there are other designs where this is not needed, but with the test and the comment here, it will be easy to remove later if the design changes. :)
Comment 15 WebKit Review Bot 2013-01-01 09:22:07 PST
Comment on attachment 180991 [details]
Patch for landing.

Clearing flags on attachment: 180991

Committed r138601: <http://trac.webkit.org/changeset/138601>
Comment 16 WebKit Review Bot 2013-01-01 09:22:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Mike West 2013-01-01 09:29:24 PST
(In reply to comment #14)
> (From update of attachment 180991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180991&action=review
> 
> > Source/WebCore/dom/Document.cpp:4391
> > +    // Seamless iframes require a forced StyleResolver recalc in order to ensure that they
> > +    // inherit style from their parent. Without this recalc, frames that don't define any of
> > +    // their own styles won't discover that there's still work to be done.
> > +    if (shouldDisplaySeamlesslyWithParent())
> > +        styleResolverChanged(DeferRecalcStyle);
> 
> It's not clear why documents aren't created needing a style resolver recalc.  It's possible that we optimize this away by creating the StyleResolver at creation time.  It's also possible that the style resolver creation happens before we're wired up to the parent document (if that's possible)?  I suspect there are other designs where this is not needed, but with the test and the comment here, it will be easy to remove later if the design changes. :)

Hrm. Ok, I misunderstood your comments, then. I thought you were agreeing that this was the right solution, and wanted to FYI folks that this change was happening.

I'm happy to roll this back and wait for suggestions about a better place to do the work; there's really no rush for this fix, other then my own desire to fiddle with it in Canary. :)
Comment 18 Eric Seidel (no email) 2013-01-01 09:30:50 PST
No, the change is OK.  I just wonder if there is a better change.  The most important part of this (and really any) change is the test.  :)

We can make additional change to make our behavior better.  It's possible that Antti/Kling will provide suggestions there, we'll see.
Comment 19 Eric Seidel (no email) 2013-01-01 09:35:43 PST
(In reply to comment #18)
> No, the change is OK.  I just wonder if there is a better change.  The most important part of this (and really any) change is the test.  :)
> 
> We can make additional change to make our behavior better.  It's possible that Antti/Kling will provide suggestions there, we'll see.

I wonder if:

    StyleResolver* styleResolver()
    { 
        if (!m_styleResolver)
            createStyleResolver();
        return m_styleResolver.get();
    }

Should be changed to call:
styleResolverChanged()

I suspect the case we're hitting here is that we're creating a styleResolver (because someone is accessing it), yet creating it does not cause it to inherit from seamless.  Maybe it should?  Or mabye creating it should just always mark it as invalid. :)

Again, Antti/Kling's thoughts here are useful whenever they're back from vacation.
Comment 20 Antti Koivisto 2013-01-02 03:14:00 PST
This seems like a hack that just hides the bug by forcing a full document style recalc for no obvious reason. It would probably be better to revert this and figure out what actually goes wrong.
Comment 21 Mike West 2013-01-02 04:03:51 PST
(In reply to comment #20)
> This seems like a hack that just hides the bug by forcing a full document style recalc for no obvious reason. It would probably be better to revert this and figure out what actually goes wrong.

Hrm. I think we do need to notify the seamlessly rendered document that it ought to do a full recalc, otherwise it won't inherit properly from its parent. But I'm happy to roll the patch out since you actually know what you're talking about. :)

I'd really appreciate a pointer in the right direction.
Comment 22 WebKit Review Bot 2013-01-02 04:05:35 PST
Re-opened since this is blocked by bug 105917
Comment 23 Mike West 2013-01-02 15:12:08 PST
Created attachment 181076 [details]
Patch
Comment 24 Mike West 2013-01-02 15:18:02 PST
(In reply to comment #21)
> I'd really appreciate a pointer in the right direction.

In short, there's no relevant difference between handing 'src' and 'srcdoc' documents. The reason the former worked was an accident of the particular document I created being categorized into quirks mode, which triggered a style recalculation.

This patch solves the problem by marking seamless documents as needing a recalc in Document::implicitOpen. This should ensure that they'll be properly set up by the time everything's said and done.

Thanks to anttik for walking through bits of this with me on #webkit.
Comment 25 Eric Seidel (no email) 2013-01-02 15:30:59 PST
Its kinda odd to me that all documents don't start out needing style recalc.  Document::createStyleResolver doesn't know how to do the inheritance. Maybe it should. It seems to be an artifact of the fact that style resolver creation is implicit in access of Document::styleResolver and that when a document doesn't have style the default-constructed StyleResolver knows how to do the right thing.
Comment 26 Antti Koivisto 2013-01-02 15:42:11 PST
(In reply to comment #25)
> Its kinda odd to me that all documents don't start out needing style recalc.  Document::createStyleResolver doesn't know how to do the inheritance. Maybe it should. It seems to be an artifact of the fact that style resolver creation is implicit in access of Document::styleResolver and that when a document doesn't have style the default-constructed StyleResolver knows how to do the right thing.

They don't start out needing a style recalc because we currently compute the initial style for elements when they are added to the tree, during attach().

The issue here is not style recalc though, it is that we need to update the stylesheet collection. Normally if the document has no stylesheets the stylesheet collection is empty (ua sheets are handled in StyleResolver). In case of inherited seamless sheets that is not true and we need to trigger an update. The badly named styleResolverChanged() does that.
Comment 27 Antti Koivisto 2013-01-02 15:48:54 PST
Comment on attachment 181076 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2259
> +    // Documents rendered seamlessly should start out requiring a style
> +    // recalculation in order to ensure they inherit all the relevant data
> +    // from their parent.

The comment is wrong. We don't need style recalc, we need stylesheet collection update. The style recalc is just a side effect.
Comment 28 Antti Koivisto 2013-01-02 16:10:27 PST
It is true that the current update mechanism is bug-prone and hard to understand but fixing that is probably outside the scope of this bug.
Comment 29 Eric Seidel (no email) 2013-01-02 22:44:47 PST
(In reply to comment #28)
> It is true that the current update mechanism is bug-prone and hard to understand but fixing that is probably outside the scope of this bug.

If you could (or already have?) file some bugs on the subject, I'm sure there are others who would be happy to fix them. :)
Comment 30 Eric Seidel (no email) 2013-01-02 22:45:37 PST
(In reply to comment #29)
> (In reply to comment #28)
> > It is true that the current update mechanism is bug-prone and hard to understand but fixing that is probably outside the scope of this bug.
> 
> If you could (or already have?) file some bugs on the subject, I'm sure there are others who would be happy to fix them. :)

(Otherwise stated: I suspect the set of folks interested/capable of writing such cleanup patches is larger than the set able/willing to write bugs explaining a better design.)
Comment 31 Mike West 2013-01-03 00:39:39 PST
Created attachment 181154 [details]
Patch for landing
Comment 32 Mike West 2013-01-03 00:40:38 PST
(In reply to comment #27)
> (From update of attachment 181076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181076&action=review
> 
> > Source/WebCore/dom/Document.cpp:2259
> > +    // Documents rendered seamlessly should start out requiring a style
> > +    // recalculation in order to ensure they inherit all the relevant data
> > +    // from their parent.
> 
> The comment is wrong. We don't need style recalc, we need stylesheet collection update. The style recalc is just a side effect.

Fixed, thank you for the explanation.

I'd echo Eric's comments; I'd like to learn more about this area of code. If you have good ideas about improvements that you could point me to, I'd be happy to do some grunt work of refactoring.
Comment 33 WebKit Review Bot 2013-01-03 01:15:09 PST
Comment on attachment 181154 [details]
Patch for landing

Clearing flags on attachment: 181154

Committed r138704: <http://trac.webkit.org/changeset/138704>
Comment 34 WebKit Review Bot 2013-01-03 01:15:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Antti Koivisto 2013-01-03 08:48:59 PST
Bugzilla is not really the forum for discussion like this.