Closed Bug 1473742 Opened 3 years ago Closed 3 years ago

Fonts loaded with CSS font loading API not printable

Categories

(Core :: Printing: Output, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(2 files)

Attached file 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.
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 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?
Attachment #8991368 - Flags: review?(jfkthame) → review+
Comment on attachment 8991368 [details]
Bug 1473742 - Fix printing fonts loaded with JS API.

Tests added...
Attachment #8991368 - Flags: review+ → review?(jfkthame)
Assignee: nobody → bdahl
Blocks: 1196991
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?
Attachment #8991368 - Flags: review?(jfkthame) → review+
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13f6f213eecd
Fix printing fonts loaded with JS API. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/13f6f213eecd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.