1473742 - Fonts loaded with CSS font loading API not printable

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Brendan Dahl [:bdahl]]

Attachment: fontface_load.html

STR:
1) Open attachment
2 [review]) Print page to PDF

Expected: the font of 'lmnop' is the 'test' font loaded with the CSS font loading API
Actual: the font is the fallback of 'monopsace'

I'm guessing during creating the static clone of the document we don't copy over these fonts.

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: Bug 1473742 - Fix printing fonts loaded with JS API.

Copy over non-rule font faces to the static clone document that is used
during printing.

Review commit: https://reviewboard.mozilla.org/r/256266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256266/

Comment 2

[Brendan Dahl [:bdahl]]

Jonathan,
I'm not sure this solution is the proper one. I could also see a different approach where we move the non-rule font faces to the style sheet so we don't have to perform this copy.

Also, I have some prototype tests for this, but unfortunately there's not really any tests that I can use that trigger the nsIDocument::CreateStaticClone on MacOS. When I get a chance I'll try to get something working on Linux or Windows where the print preview tests are run.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

https://reviewboard.mozilla.org/r/256266/#review263674

This looks reasonable to me. Moving the fonts to a stylesheet (which stylesheet?) sounds more questionable, as they really don't belong to one.

Having tests would be awesome. :)

::: layout/style/FontFaceSet.h:180
(Diff revision 1)
>      return set ? set->GetPresContext() : nullptr;
>    }
>  
>    void RefreshStandardFontLoadPrincipal();
>  
> +  void CopyNonRuleFacesTo(FontFaceSet* aFontFaceSet);

Can we make this a const method, and use a `const FontFaceSet*` pointer in CreateStaticClone above?

Comment 4

[Brendan Dahl [:bdahl]]

Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256266/diff/1-2/

Comment 5

[Brendan Dahl [:bdahl]]

Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

Tests added...

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

https://reviewboard.mozilla.org/r/256266/#review267716

Looks good, thanks for adding the test.

::: layout/base/tests/chrome/printpreview_font_api_ref.html:16
(Diff revision 2)
> +      /*
> +       * Intentionally use a different fallback font than the test file so that
> +       * if the font fails to load the test and reference will still be
> +       * different.
> +       */
> +      font-family: test, Arial;

There's no guarantee "Arial" is present; I'd suggest using `font-family: test, sans-serif;` here.

::: layout/style/FontFaceSet.cpp:1945
(Diff revision 2)
> +FontFaceSet::CopyNonRuleFacesTo(FontFaceSet* aFontFaceSet) const
> +{
> +  for (const FontFaceRecord& rec : mNonRuleFaces) {
> +    ErrorResult rv;
> +    RefPtr<FontFace> f = rec.mFontFace;
> +    aFontFaceSet->Add(*f, rv);

Hmm, seems like we should check the ErrorResult here rather than just ignore it, at least in debug builds. Perhaps MOZ_ASSERT() that it didn't fail?

Comment 7

[Brendan Dahl [:bdahl]]

Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256266/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13f6f213eecd
Fix printing fonts loaded with JS API. r=jfkthame

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/13f6f213eecd