Bug 103539

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

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

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 :)

(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. :)

When the parser creates the DOM nodes it calls lazyAttach on them, which should mark them (and their ancestors) as needing style resolve.

Created attachment 180976 [details] Patch

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?

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

Also ccing the StyleResolver ninjas.

(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!

Created attachment 180991 [details] Patch for landing.

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 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 on attachment 180991 [details] Patch for landing. Clearing flags on attachment: 180991 Committed r138601: <http://trac.webkit.org/changeset/138601>

All reviewed patches have been landed. Closing bug.

(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. :)

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.

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

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.

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

Re-opened since this is blocked by bug 105917

Created attachment 181076 [details] Patch

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

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.

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

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.

(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. :)

(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.)

Created attachment 181154 [details] Patch for landing

(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 on attachment 181154 [details] Patch for landing Clearing flags on attachment: 181154 Committed r138704: <http://trac.webkit.org/changeset/138704>

All reviewed patches have been landed. Closing bug.

Bugzilla is not really the forum for discussion like this.