1425864 - Ensure printing documents which have ShadowDOM works

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Jonathan Watt [:jwatt]]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: manual test

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_printing.diff

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 9

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I've actually started to not like references.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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

[Olli Pettay [:smaug][[e-mail]]]

Attachment: some nits fixed

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

indeed it does.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shadow_printing_3.diff

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc75e6db0ee
Ensure printing documents which have ShadowDOM works, r=mrbkap,emilio

Comment 18

[Emilio Cobos Álvarez (: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.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7cc75e6db0ee