1457254 - Bold font not rendering on macOS with WR enabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

On the Apr 26 nightly, mac OS, with gfx.webrender.all enabled. If I go to about:config and type "webrender" in the search bar, the entry for gfx.webrender.all (which is bold because of the non-default value) doesn't render at all. Bolded entries on about:config render fine with WR disabled.

Presumably this is a WR-specific version of bug 1455494.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I didn't confirm, but this is probably a regression from bug 1454598 specific to some WR codepath.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I'm on macOS 10.13.4 (17E199). Lee asked me to check the font of the missing text using the devtools inspector, but the inspector doesn't let me expand the <treechildren> element which seems to contains all the visible rows (including the non-bolded ones). The computed font-family on the <treechildren> is "-apple-system".

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Jonathan, let me know if there's any info I can provide or anything I can to do help debug this. Per IRC discussion in #gfx [1], we take a fallback codepath in WR for about:config because it's XUL, so I believe we'll go down [2] and paint the display item using a BasicLayerManager and then render it as an image with WR.

[1] https://mozilla.logbot.info/gfx/20180426#c14673807 onwards
[2] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/gfx/layers/wr/WebRenderCommandBuilder.cpp#1292

Comment 4

[Aria Beingessner [:Gankra]]

It appears that the bug is in using bold -apple-system *in blob images*. XUL (about:config) and -webkit-text-stroke are the two easiest ways to trigger this.

This example currently shows up as hollow red on macos: 

data:text/html,<span style="font-family: -apple-system, sans-serif;font-weight: bold;-webkit-text-stroke: 1px red;">velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident,</span>

Removing the -webkit-text-stroke has the bold font pop back in.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

The loading of these fonts will be happening around here: https://searchfox.org/mozilla-central/source/gfx/webrender_bindings/Moz2DImageRenderer.cpp#180

Comment 6

[Jonathan Kew [:jfkthame]]

This looks a lot like bug 1455569, presumably happening on a different code path.

Comment 8

[Brunno Pleffken [:bpleffken]]

It doesn't happen only in bold, I'm also experiencing missing texts in tab title (e.g. "New tab") only when the tab is visible. It can be seen here: https://imgur.com/a/Z1BTQeW

Comment 9

[Lee Salzman [:lsalzman]]

Seems to somehow be due to some change in a WR update, though I am still tracking it down...

https://bugzilla.mozilla.org/show_bug.cgi?id=1456114

Comment 10

[Lee Salzman [:lsalzman]]

It appears that bug 1457974 is separate from this, and is rather caused by bug 1456114 wherein WR PR https://github.com/servo/webrender/pull/2677 is the guilty change.

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: copy ScaledFontMac variations with CGFontCopyVariations

As fallout from bug 1454598, we can't apparently safely use CTFontCopyVariation anymore when we don't actually manually install the variation dict in the CTFont, or the results aren't sane. However, using CGFontCopyVariations instead on the CGFont seems to do the work regardless.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8972483 [details] [diff] [review]
copy ScaledFontMac variations with CGFontCopyVariations

Review of attachment 8972483 [details] [diff] [review]:
-----------------------------------------------------------------

No, I'm afraid this doesn't appear to work :\ ... at least according to my testing on a current mozilla-inbound tree. It does make the "missing" text (e.g. bold entries in about:config) reappear, but at the cost of not actually applying the expected variation settings. So the "bold" items render with the regular glyphs, just with slightly increased spacing (because layout was based on the metrics of the bold variant).

More obviously, testing on a site like https://v-fonts.com/, the fonts there no longer respond properly to variations. E.g. if you adjust the Weight slider for the first example (Foreday), the spacing changes as the text is shaped using the variation, but the actual glyphs don't change. Likewise for other example fonts there.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: testcase using font-variation-settings

Here's a testcase that shows the same failure, and also fails on builds -before- bug 1454598 was landed. The correct rendering should show no red (as when webrender is disabled).

The issue here is an underlying failure to handle variation settings on system-installed fonts, and was just exposed more widely by bug 1454598 because that hooked up the font-weight/font-stretch properties to variation axes, so the bug no longer requires explicit use of font-variation-settings to show itself.

I still don't understand why this fails only with text in blob images (per comment 4), while the variations work fine otherwise.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Don't pass true for aIsDataFont when creating an UnscaledFontMac from AddNativeFontHandle, this is for installed fonts

This isn't a problem with the variations we retrieve in ScaledFontMac::GetWRFontInstanceOptions at all. That works fine. I think the real problem here is that we're incorrectly marking an UnscaledFontMac as being a data font when in fact it's a native installed font, and this leads to using the wrong code path in CreateCTFontFromCGFontWithVariations. With the flag fixed here, it seems to be working correctly, at least in all the cases I've tested so far.

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8973018 [details] [diff] [review]
Don't pass true for aIsDataFont when creating an UnscaledFontMac from AddNativeFontHandle, this is for installed fonts

This appears to resolve the issue in my testing as well.

Comment 17

[Lee Salzman [:lsalzman]]

Comment on attachment 8972483 [details] [diff] [review]
copy ScaledFontMac variations with CGFontCopyVariations

># HG changeset patch
># User Lee Salzman <[e-mail]>
># Date 1525226477 14400
>#      Tue May 01 22:01:17 2018 -0400
># Node ID 9f64e53674c59f89966c0d6b6a685f8452b2b7c2
># Parent  258fa1eb49c9a559f0f75bd0a597d14398b01430
>copy ScaledFontMac variations with CGFontCopyVariations
>
>MozReview-Commit-ID: DCsBAD0GekB
>
>diff --git a/gfx/2d/ScaledFontMac.cpp b/gfx/2d/ScaledFontMac.cpp
>--- a/gfx/2d/ScaledFontMac.cpp
>+++ b/gfx/2d/ScaledFontMac.cpp
>@@ -399,53 +399,53 @@ CollectVariationsFromDictionary(const vo
>     if (CFNumberGetValue(static_cast<CFNumberRef>(keyPtr), kCFNumberSInt64Type, &t) &&
>         CFNumberGetValue(static_cast<CFNumberRef>(valuePtr), kCFNumberDoubleType, &v)) {
>       outVariations->push_back(FontVariation{uint32_t(t), float(v)});
>     }
>   }
> }
> 
> static bool
>-GetVariationsForCTFont(CTFontRef aCTFont, std::vector<FontVariation>* aOutVariations)
>+GetVariationsForCGFont(CGFontRef aCGFont, std::vector<FontVariation>* aOutVariations)
> {
>   // Avoid calling potentially buggy variation APIs on pre-Sierra macOS
>   // versions (see bug 1331683)
>   if (!nsCocoaFeatures::OnSierraOrLater()) {
>     return true;
>   }
>-  if (!aCTFont) {
>+  if (!aCGFont) {
>     return true;
>   }
>-  AutoRelease<CFDictionaryRef> dict(CTFontCopyVariation(aCTFont));
>+  AutoRelease<CFDictionaryRef> dict(CGFontCopyVariations(aCGFont));
>   CFIndex count = dict ? CFDictionaryGetCount(dict) : 0;
>   if (count > 0) {
>     aOutVariations->reserve(count);
>     CFDictionaryApplyFunction(dict, CollectVariationsFromDictionary, aOutVariations);
>   }
>   return true;
> }
> 
> bool
> ScaledFontMac::GetFontInstanceData(FontInstanceDataOutput aCb, void* aBaton)
> {
>   // Collect any variation settings that were incorporated into the CTFont.
>   std::vector<FontVariation> variations;
>-  if (!GetVariationsForCTFont(mCTFont, &variations)) {
>+  if (!GetVariationsForCGFont(mFont, &variations)) {
>     return false;
>   }
>   aCb(nullptr, 0, variations.data(), variations.size(), aBaton);
>   return true;
> }
> 
> bool
> ScaledFontMac::GetWRFontInstanceOptions(Maybe<wr::FontInstanceOptions>* aOutOptions,
>                                         Maybe<wr::FontInstancePlatformOptions>* aOutPlatformOptions,
>                                         std::vector<FontVariation>* aOutVariations)
> {
>-  GetVariationsForCTFont(mCTFont, aOutVariations);
>+  GetVariationsForCGFont(mFont, aOutVariations);
> 
>   wr::FontInstanceOptions options;
>   options.render_mode = wr::FontRenderMode::Subpixel;
>   options.subpx_dir = wr::SubpixelDirection::Horizontal;
>   options.flags = 0;
>   if (mUseFontSmoothing) {
>     options.flags |= wr::FontInstanceFlags::FONT_SMOOTHING;
>   }

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66c6d818c89
Don't pass true for aIsDataFont when creating an UnscaledFontMac from AddNativeFontHandle, this is for installed fonts. r=lsalzman

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d66c6d818c89