1714754 - Noto Sans JP variable font is rendered with narrower letter-spacing

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[6k64x4ma]

Attachment: index.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:90.0) Gecko/20100101 Firefox/90.0

Steps to reproduce:

1. Install NotoSansJP-VF.otf from https://github.com/googlefonts/noto-cjk/tree/main/Sans/Variable/OTF/Subset
2. Create a new profile and open attached file.

Actual results:

Characters are renderd with narrower letter-spacing in Firefox 90 and Firefox 91.
Rendered correctly in Firefox 89.

Standard fonts https://github.com/googlefonts/noto-cjk/tree/main/Sans/SubsetOTF/JP are rendered correctly in Fx89, Fx90 and Fx91.

Expected results:

Same rendering as before.

Comment 1

[6k64x4ma]

Attachment: Fx90-NotoSansJP-VF.png

Comment 2

[6k64x4ma]

Attachment: Fx89-NotoSansJP-VF.png

Comment 3

[6k64x4ma]

Attachment: Fx90-NotoSansJP.png

Comment 4

[6k64x4ma]

Attachment: Fx89-NotoSansJP.png

Comment 5

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

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Text and Fonts' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 6

[Jonathan Kew [:jfkthame]]

It seems to render with correct spacing on macOS using Nightly (91), so this may be specific to the DirectWrite backend.

Though one thing I notice is that when varying the font-weight, I'm only seeing four distinct weights, whereas Font Book exposes seven (and TextEdit.app can happily use them all). So it seems like something isn't quite right there, too. FWIW, Chrome seems to be able to use five weights. Odd.

Comment 7

[Alice0775 White]

Regression Window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=815e96ee029296a3ade7f577228d9266b87d252b&tochange=f6589db7849e5246a4982c6e7e2fa0d8e5a92822

Comment 8

[Jonathan Kew [:jfkthame]]

Oh, interesting! Thanks. (I wonder why that broke things on Windows but not Mac. Will investigate...)

Comment 9

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

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

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Jonathan Kew (:jfkthame) from comment #8)

Oh, interesting! Thanks. (I wonder why that broke things on Windows but not Mac. Will investigate...)

Did you have a chance to investigate? Any news?

Comment 11

[Jonathan Kew [:jfkthame]]

Sorry, I haven't had cycles to look into this yet. I'm just setting up a new Windows machine this week, though, so will try to reproduce this there. (Leaving needinfo flag for now...)

Comment 12

[Jonathan Kew [:jfkthame]]

OK, I see what broke here. Now that the 'opsz' value is not unconditionally added to the font style, we end up returning false from gfxDWriteFont::ProvidesGlyphWidths, so we read advances directly from the 'hmtx' table instead of calling DirectWrite, and this doesn't respect variation settings. So the text gets rendered with the font's default advances, regardless of the weight chosen.

Some things that follow from this: (a) if you set font-weight to 100 (which matches the un-varied glyph shapes) the spacing will look OK, but at other weights it's wrong because we just keep using that same spacing; (b) if you explicitly add a font-variation-settings value to the style (even a "random" axis that isn't actually present in the font), that will trigger the use of DirectWrite glyph widths, and it'll look correct; (c) the same bug actually occurs with the current release if you set font-optical-sizing:none, because then we no longer realize we need to use the DW-widths code path.

Fortunately, there's a trivial fix: we should just always use the DirectWrite glyph widths API with variation fonts, regardless of whether there are explicit variation settings in the style.

(An alternative fix would be to move from our "raw" table-reading implementation to the hb-ot font functions, which implement variation support; we'll probably do that at some point, but it's a more invasive change and will need some care. For right now I want to take the simple and safe fix.)

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1714754 - Always use DirectWrite to get glyph widths for variable fonts. r=lsalzman

This ensures we get the correct advances for the variation settings that are in use,
not just the font's "default" advances from the hmtx table.

(I'm sad there wasn't a testcase that detected this regression, so added one here.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1c8f4779cac
Always use DirectWrite to get glyph widths for variable fonts. r=lsalzman

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29462 for changes under testing/web-platform/tests

Comment 16

[Dorel Luca [:dluca]]

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

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 9228492 [details]
Bug 1714754 - Always use DirectWrite to get glyph widths for variable fonts. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: OpenType variable fonts render with bad spacing on Windows
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minimal change to Windows-only code to ensure we always call the DirectWrite APIs for these fonts. (The "good" code path already exists, we just don't always use it when we should.)
• String changes made/needed:

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9228492 [details]
Bug 1714754 - Always use DirectWrite to get glyph widths for variable fonts. r=lsalzman

approved for 90.0b12

Comment 19

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8151cfce181

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot