1798036 - Font-Weight get's messed up when using background-clip: text on Pixel 6/7

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

From github: https://github.com/mozilla-mobile/fenix/issues/27595.

Steps to reproduce

Visit this codepen on Firefox Mobile (looks alright on desktop or responsive design mode inside firefox desktop):

https://codepen.io/bengin_cetindere/pen/PoaqQZb

Chrome and Chrome Mobile are consistent with the firefox desktop behavior, only Firefox Android shows issue.

Expected behaviour

Both divs should render the text with the same font weight.

Actual behaviour

Observed in Firefox Android Only: The first div renders the text, dismissing the font-weight attribute (text does not have increased font-weight like specified) but making up for it by ... I think increasing the tracking / letter-spacing or similar.

Device name

Pixel 7 Pro

Android version

Android 13

Firefox release type

Firefox

Firefox version

106.1.0

Device logs

No response

Additional information

I tried to lookup why that could be (in the source code) but I got lost in the stuff that's partly here on github but gecko itself not being there... If someone more experienced could point me into the right direcition I'd also be glad to contribute myself :)

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Galaxy Note screenshot

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Pixel 6 Pro screenshot

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

There is a behavior change on the Pixel 6 around March 8/9th where the Pixel goes from both skinny to the black text being bolder.

Comment 4

[Jonathan Kew [:jfkthame]]

It'd be interesting to know exactly what the sans-serif font on the device is. I'm guessing that it may be using font with a variable weight axis, rather than having separate regular and bold faces, and for some reason we're failing to apply the appropriate variation when doing the background-clip thing. The pushlog linked in comment 3 includes bug 1758315, which sounds suspiciously like it might be relevant.

Presumably this is the device's Roboto we're seeing? If you can pull the font file(s) from there and attach a copy, that'd be great.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Pixel6Roboto.zip

Here are the Roboto fonts on my Pixel 6 Pro. I confirmed with about:debugging that the font is Roboto.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot_20221101-153304.png

Using "Source Code Pro" looks correct.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Pixel6SourceSans.zip

Comment 8

[Brad Werth [:bradwerth]]

Jamie, this seems like it may be a styling thing, but it may be due to some decision in our Android graphics path. Can you reproduce it?

Comment 9

[Jamie Nicol [:jnicol]]

I used mozregression and got a narrower range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2badefb067b2950db2eec1d502559d141fcf8764&tochange=f6a476e4ef2d993565be04a81fb87d3ea7ba56c4

Which seems even more likely that it's bug 1758315.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1758315

Comment 11

[Jonathan Kew [:jfkthame]]

The same version of Roboto installed locally on Linux seems to work as expected, so I think this is specific to the "FT2" font backend that we use on Android. (Note that on desktop Linux we also use FreeType, but via a separate fontconfig-based codepath.)

I'd be surprised if this is a bug on the styling/font-selection side of things, because resolving the CSS properties to a gfxFont instance shouldn't be affected by whether we're using background-clip or just normal glyph painting. Seems like the problem must be on the gfx rendering side: when we get glyph paths in order to use them for clipping (as opposed to just rendering the glyphs directly), we're failing to apply the appropriate variation settings to the face.

Unfortunately, I can't reproduce this in the emulator, as the android version there doesn't have the variable Roboto (it has a collection of individual faces instead), and doesn't behave the same.

Lee, do you have a device where you can reproduce this? It seems like the glyph-paths code must be using an FT_Face that isn't set up with the proper variations. There's a reduced example at https://jfkthame.github.io/test/roboto.html that should be sufficient to show the issue; the key question is whether the second blue "abc" appears properly bolded, or just shows the regular glyphs.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

We are in 107 RC week, wontfix 106.

Comment 13

[Jonathan Kew [:jfkthame]]

Ok, I was able to get an Android emulator set up where I can reproduce this.

The failure doesn't only happen with background-clip: text; it also happens with normal filled text above a certain font size. I modified the example at https://jfkthame.github.io/test/roboto.html to show several sizes. In the Pixel 6 emulator, the bold background-clip text, which is the second blue "abc" at each size never works (always uses the regular glyphs). But also, the simple bold text (black) fails at the largest size, and just renders the regular glyphs.

I assume this is because there's a font size threshold above which we no longer use the usual freetype rasterizer but instead retrieve glyph outlines and fill them as paths. So then we're hitting the problem of the glyph path API returning outlines without the variation setting, just like happens with background-clip:text at all sizes.

Comment 14

[Jonathan Kew [:jfkthame]]

The size threshold where normal glyph painting (without background-clip) fails to honor the variation setting appears to be 320 device pixels, which corresponds to around 122.7 css px on the Pixel 6 emulator. This corresponds to the FONT_SIZE_LIMIT here and kWebRenderFontSizeLimit here.

Comment 15

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew [:jfkthame] from comment #14)

The size threshold where normal glyph painting (without background-clip) fails to honor the variation setting appears to be 320 device pixels, which corresponds to around 122.7 css px on the Pixel 6 emulator. This corresponds to the FONT_SIZE_LIMIT here and kWebRenderFontSizeLimit here.

So the kWebRenderFontSizeLimit there takes us to a blob image which will rasterize the font with Skia instead of WR. So the question is whether or not the issue is something we're doing in supplying stuff to Skia, or if Skia itself fails at this? If you artificially lower kWebRenderFontSizeLimit even further so that we hit the Skia path before Skia hits its own internal threshold above which it goes to a path/outline, does the issue still happen? That might be one way to discern whether its specifically the path outlines that cause this, or any glyph rasterization at all with Skia?

Comment 16

[Jonathan Kew [:jfkthame]]

The most immediate cause of the problem here is that when we punt to the blob rendering path to paint the text, we call UnscaledFontFreeType::CreateScaledFontFromWRFont to create the ScaledFont to use. This calls through to CreateScaledFont, and at this point we have the correct variation settings in hand. But when CreateScaledFont wants to apply these settings to the font, it can't, because there is no FTUserFontData attached to the FreeType face, and so we can't CloneFace() it.[1]

Because of this, variations will only be successfully handled on this codepath if we're using a webfont (which has FTUserFontData to track the backing data), but not for installed fonts. That wasn't much of a problem at first.... but now that new Android devices are using a variable-font Roboto instead of individual faces, we're running into this issue.

To fix this, I think we can extend FTUserFontData to also work with installed fonts; instead of holding on to the associated font data, in this case it'll need to store the file path so that a new FT_Face can be "cloned" when required. (This will only be needed for fonts that have variations; we can skip attaching the extra data for normal static fonts.)

[1] Aside: I wonder why these UnscaledFontFreeType methods are in ScaledFontFreeType.cpp rather than UnscaledFontFreeType.cpp? Historical accident, I suspect; I propose to move them to the more logical location.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1798036 - Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

This is needed so that UnscaledFontFreeType::CreateScaledFont can later call CloneFace
if it needs to apply variation settings.

Tested locally with an emulator running Android 13; not testable in CI as the older Android
versions we have there don't use the new variable-font version of Roboto.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aea86168d3c
Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 9302798 [details]
Bug 1798036 - Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: In some cases on recent Android versions, text using the device's standard Roboto font fails to respect Bold (or other weights such as Thin or Black) and just renders at Regular weight.
  See bug 1799942 for a screenshot of an example.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Using an affected device (e.g. Pixel 6 or 7, running Android 13), visit the testcases mentioned here (https://codepen.io/bengin_cetindere/pen/PoaqQZb) and in bug 1799942 (https://nitter.snopyta.org/firefox), and check if all the Bold text renders correctly.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changed behavior is restricted to variable fonts, Android backend; no other content or platform should be affected.
• String changes made/needed:
• Is Android affected?: Yes

Comment 20

[Jonathan Kew [:jfkthame]]

Note that while this was a pre-existing bug, the introduction of COLRv1 font support (shipping in 107) has led to it showing up in additional contexts (as it can now be triggered by the presence of emoji in the text). As such, it'll become more noticeable, and would be good to get the fix out quickly.

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9302798 [details]
Bug 1798036 - Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

107 is in RC. Changing the beta uplift request to release uplift request, for consideration as a ride-along in a dot release.

Comment 22

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/3aea86168d3c

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Donal Meehan [:dmeehan] from comment #21)

Comment on attachment 9302798 [details]
Bug 1798036 - Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

107 is in RC. Changing the beta uplift request to release uplift request, for consideration as a ride-along in a dot release.

Note that if we do take this as a dot-release ride-align, we'll need to take the (minimal) fix from bug 1800367 along with it.

Comment 25

[Delia Pop]

Tested on the latest Nightly 108.0a1 from 11/13 with Google Pixel 4 (Android 13) and Google Pixel 6 (Android 13). The text is now properly displayed on the mentioned websites. Please observe the latest file attached.

Also verified on Beta 108.0b1 with Motorola Moto G30 (Android 12).
Leaving the qe-verify+ label in case this needs to be checked in a dot release.

Comment 26

[Delia Pop]

Attachment: Pixel6_Text.png

Comment 27

[Donal Meehan [:dmeehan]]

Comment on attachment 9302798 [details]
Bug 1798036 - Attach a SharedFTFaceData (recording the filename) when instantiating SharedFTFace for an installed variation font. r=lsalzman

Approved for 107.0.1

Comment 28

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/ab57e0e07e6f

Comment 29

[Andrei Mocirean]

Hello,
Verified as fixed on the latest RC (107.2.0). The text is displayed correctly.
Used the Google Pixel 6 running Android 13 for testing.
Thanks!