We need to clone the Shadow DOM and relevant styling too to support printing.

(As far as I see, there isn't anything Custom Elements specific here, only Shadow DOM.)

This turns out to tbe the cause of the printing failure reported in bug 1421462 comment 3. That is, failure to print the contents of this page: https://www.xfinity.com/support/articles/forward-calls-using-xfinity-mobile-app Presumably we're not doing the right thing under nsIDocument::CreateStaticClone.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #2) > https://www.xfinity.com/support/articles/forward-calls-using-xfinity-mobile-app (And yes, xfinity are generating the entire contents of their support articles as shadow DOM content. <facepalm/> )

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #2) > This turns out to tbe the cause of the printing failure reported in bug > 1421462 comment 3. That is, failure to print the contents of this page: > > https://www.xfinity.com/support/articles/forward-calls-using-xfinity-mobile- > app > > Presumably we're not doing the right thing under > nsIDocument::CreateStaticClone. Hmm, webcomponents isn't enabled in 57 though.

Sure, you need Nightly for the page to render at all. It still doesn't print the shadow dom content in Nightly though, which is this bug.

So with: dom.webcomponents.enabled = false and: dom.webcomponents.customelements.enabled = true The page renders fine, but there are no shadow roots on the page, so it cannot be this bug.

I'm in process to write some reasonable tests for this. Printing can't be tested automatically, but print preview can be, to some extent.

Comment on attachment 8954945 [details] [diff] [review] shadow_printing.diff Review of attachment 8954945 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ShadowRoot.cpp @@ +99,5 @@ > +{ > + size_t sheetsCount = aOther->SheetCount(); > + for (size_t i = 0; i < sheetsCount; ++i) { > + RefPtr<StyleSheet> sheet = aOther->SheetAt(i); > + if (sheet && sheet->IsApplicable()) { No need for null-check here, the sheets are guaranteed to be non-null. Similarly no need to take a strong ref, cloning a sheet can never make it go away @@ +101,5 @@ > + for (size_t i = 0; i < sheetsCount; ++i) { > + RefPtr<StyleSheet> sheet = aOther->SheetAt(i); > + if (sheet && sheet->IsApplicable()) { > + RefPtr<StyleSheet> clonedSheet = > + sheet->Clone(nullptr, nullptr, nullptr, nullptr); The null owner node thingie is somewhat fishy, but it's no different from what we do for Doc stylesheets, so I guess it's fine. @@ +103,5 @@ > + if (sheet && sheet->IsApplicable()) { > + RefPtr<StyleSheet> clonedSheet = > + sheet->Clone(nullptr, nullptr, nullptr, nullptr); > + if (clonedSheet) { > + AppendStyleSheet(*(clonedSheet.get())); The parenthesis and .get() aren't needed, right? @@ +104,5 @@ > + RefPtr<StyleSheet> clonedSheet = > + sheet->Clone(nullptr, nullptr, nullptr, nullptr); > + if (clonedSheet) { > + AppendStyleSheet(*(clonedSheet.get())); > + Servo_AuthorStyles_AppendStyleSheet(mServoStyles.get(), Maybe add a ShadowRoot::AppendStyleSheet method and make that call the DocumentOrShadowRoot version and the Servo function, then use it here? I didn't do that in bug 1425759 because it had only one caller... It's not a big deal either way. ::: dom/base/ShadowRoot.h @@ +88,5 @@ > { > return &DocumentOrShadowRoot::EnsureDOMStyleSheets(); > } > > + void CloneInternalDataFrom(ShadowRoot* aOther); Please document this? We don't have any similarly-named function. Also I'd make it take a reference but... :P ::: dom/base/nsNodeUtils.cpp @@ +680,5 @@ > + if (aClone) { > + if (clone->OwnerDoc()->IsStaticDocument()) { > + ShadowRoot* originalShadowRoot = aNode->AsElement()->GetShadowRoot(); > + if (originalShadowRoot) { > + ShadowRootInit init; Hmm, I wonder if we should move this code (from init to the CloneInternalDataFrom call) into ShadowRoot.cpp so it has a bit less risk of getting out of sync.

I've actually started to not like references.

not sure what exactly would go out of sync in that init case, and how it would go less sync elsewhere.

(In reply to Olli Pettay [:smaug] from comment #11) > not sure what exactly would go out of sync in that init case, and how it > would go less sync elsewhere. Fair enough I guess. I was thinking of adding more stuff to ShadowRootInit that could affect something, but as you mentioned that probably already means auditing a bunch of code.

Comment on attachment 8954967 [details] [diff] [review] some nits fixed Review of attachment 8954967 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ShadowRoot.cpp @@ +96,5 @@ > > void > +ShadowRoot::CloneInternalDataFrom(ShadowRoot* aOther) > +{ > + size_t sheetsCount = aOther->SheetCount(); Uber-nit (feel free to ignore): `sheetCount` feels much more natural as a variable name to me.

indeed it does.

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc75e6db0ee Ensure printing documents which have ShadowDOM works, r=mrbkap,emilio

Just double-checking, am I right that we can't add tests for this because reftest-paged doesn't do the static-clone thing? Could we extend https://searchfox.org/mozilla-central/source/layout/base/tests/chrome/printpreview_helper.xul to cover this? I can try to write the test if you want.

I'm going to write print preview tests. And yes, by adding more stuff to printpreview_helper.xul. reftests-paged wouldn't indeed test the right code paths.

But if you have some spare time to write the tests, feel free to :) I'll be busy with the web components meeting for two days or so.