1721612 - Excessive letter spacing on San Francisco font (-apple-system) on macOS M1

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Dan Burzo]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:90.0) Gecko/20100101 Firefox/90.0

Steps to reproduce:

Any website that uses the -apple-system font will display excessive letter spacing in Firefox 90 on macOS Big Sur (11.4) on Apple Silicon (M1). The text renders correctly on Firefox 90 on macOS Big Sur (11.4) on Intel.

Recent versions of Chrome and Safari on either CPU architecture don't exhibit the problem.

Comment 1

[Dan Burzo]

Attachment: Screenshot in FF90 / macOS 11.4 Intel

Attached 'before' image in Firefox 90 / macOS 11.4 Intel

Comment 2

[Dan Burzo]

Attachment: Firefox 90 / macOS 11.4 M1

Attached screenshot in Firefox 90 / macOS 11.4 M1

Comment 3

[BugBot [:suhaib / :marco]]

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

Comment 4

[Yestin Harrison]

Popping in to confirm this; the following are screenshots of example.com, which just so happens to use:

    body {
        [...]
        font-family: -apple-system, system-ui, BlinkMacSystemFont, [...]; 
    }

Comment 5

[Yestin Harrison]

Attachment: 91.0esr, example.com, new profile, running under Rosetta

Comment 6

[Yestin Harrison]

Attachment: 91.0esr, example.com, new profile, running natively on M1

Comment 7

[Brad Werth [:bradwerth]]

Jonathan, can you examine this please?

Comment 8

[Jonathan Kew [:jfkthame]]

This seems likely related to optical sizing in some way. But the difference in behavior between native vs rosetta is weird -- this seems to suggest Core Text might be behaving differently in the two environments.

What macOS SDK versions do we use to build each of the binaries? If they're not the same, that might be a source of differing behavior.

Comment 9

[Sam Johnson]

This is something I noticed pretty much immediately after upgrading to an M1-based system. It also seems to occur on pages not using the system font, that fall back to serif, such as this one if you do not have the Segoe UI font installed. Letter spacing is different between M1 and Intel, and line spacing is also different between the two, so I could see this negatively affecting layout of some sites.

Comment 10

[Markus Stange [:mstange]]

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

What macOS SDK versions do we use to build each of the binaries? If they're not the same, that might be a source of differing behavior.

That's right, they're not the same. We use the macOS 10.12 SDK for Intel builds (bug 1696504) and the macOS 11 SDK for Apple Silicon builds.

Comment 11

[Sam Johnson]

If it would help narrow things down, I'm willing to try an Intel build built with the macOS 11 SDK and compare results, if one is available.

Comment 12

[Sam Johnson]

My understanding is that Nightly build 20220526213638 should be built with the macOS 11 SDK for Intel builds (bug 1696504), however this issue still does not reproduce when opening with Rosetta, or on an Intel machine--the issue is still only occurring on Apple Silicon builds.

Comment 13

[Stephen A Pohl]

I didn't mean to add bug 1696504 as regressing bug just yet. Could you try again with today's Nightly to confirm that you're still not seeing this issue on Intel, even when Firefox is built with the macOS 11 SDK?

Comment 14

[Sam Johnson]

I have confirmed with Nightly build 20220527092603 that I am still not seeing this issue on Intel, only Apple Silicon.

Comment 15

[Jack Roberts]

It appears someone wrote a script to fix a bug very similar to this one some years ago, and it was apparently fixed in v58.

I can also confirm that I do not experience this issue on Intel Macs or when running under Rosetta.

Comment 16

[Isaac Marovitz]

This is happening for me on Firefox 115.0.3 on macOS Sonoma.

Chrome and Firefox under Rosetta render the font spacing correctly, under ARM Firefox, it is too wide.

Comment 17

[Sam Johnson]

I can confirm that the issue now affects Intel Macs when running on macOS Sonoma beta 4, using a 2018 MacBook Air.

Comment 18

[Markus Stange [:mstange]]

Jonathan, could you take a look?

Comment 19

[Jonathan Kew [:jfkthame]]

I can try to take a look after All Hands; not yet running Sonoma on my MBPro, so it'll take a bit of setup to be able to investigate here...

(leaving ni? for now)

Comment 20

[Sam Johnson]

@jfkthame: I have been doing some testing today and I believe the Sonoma issue is actually separate from the issue reported in this bug, and seems to be exclusive to Retina displays. I will open a separate bug for that.

I am attaching a better screenshot illustrating the issue in this bug (and also illustrating that Sonoma does not affect this specific bug). I have overlaid a screenshot of this GitHub repo's readme on an Intel Mac running Sonoma (correct letter spacing) with an M1 Mac running Ventura (too-wide letter spacing). By the end of each line the difference is extreme.

Comment 21

[Sam Johnson]

Attachment: intel-m1-comparison.png

Comment 22

[Sam Johnson]

I have opened bug 1851033 for the Sonoma issue that also affects Intel.

Comment 23

[Sam Johnson]

It looks like this is fixed by the patch in bug 1856035. Nice!

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Sam Johnson from comment #23)

It looks like this is fixed by the patch in bug 1856035. Nice!

Thanks for confirming this! Dup'ing forward to the bug that has the patch.

Comment 25

[Jonathan Kew [:jfkthame]]

According to bug 1856035 comment 46 and following, it looks like this may not be fully resolved by the patch there. Re-opening this for further checking, once the dust settles in bug 1856035.

Comment 26

[Sam Johnson]

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

According to bug 1856035 comment 46 and following, it looks like this may not be fully resolved by the patch there. Re-opening this for further checking, once the dust settles in bug 1856035.

Apologies, I can confirm --- I didn't realize the try build I was testing was an Intel build!

Comment 27

[Jack Roberts]

So I downloaded the SF Pro font family from Apple's developer site, and I'm noticing there's actually a few different variants of San Francisco, including one called "SF Text" that has extra spacing between the letters that seems to match what Firefox is rendering.

See this image for illustration: https://i.imgur.com/0sV1tSf.png

Perhaps Chrome and Safari are rendering SF Pro, while Firefox is for some reason using the SF Pro Text variant?

Comment 28

[Jack Roberts]

So I downloaded the SF Pro font family from Apple's developer site, and I'm noticing there's actually a few different variants of San Francisco, including one called "SF Text" that has extra spacing between the letters that seems to match what Firefox is rendering.

See this image for illustration: https://i.imgur.com/0sV1tSf.png

Perhaps Chrome and Safari are rendering SF Pro, while Firefox is for some reason using the SF Pro Text variant?

Upon further testing, I'm also finding that when changing the default sans-serif font in Firefox's settings (while also disallowing pages from setting their own font), selecting SF Pro still does not actually render SF Pro: it's still using the SF Pro Text variant.

However, if you select the SF Pro Display variant, Firefox will properly display it, although this isn't a solution since that variant of San Francisco is too narrow.

Furthermore when selecting SF Pro Text, there seems to be no change at all from the default setting, indicating Firefox is indeed failing to render the standard SF Pro variant at all.

Comment 29

[Brunno Pleffken [:bpleffken]]

(In reply to Jack Roberts from comment #28)

Furthermore when selecting SF Pro Text, there seems to be no change at all from the default setting, indicating Firefox is indeed failing to render the standard SF Pro variant at all.

Nice point! Confirming this. Seems like Firefox is unable to define the correct font family for -apple-system.

According to their guidelines (https://developer.apple.com/design/human-interface-guidelines/typography), SF Pro is the font for UIs and SF Text should not be used for it. Pro Text is a variant font, just as SF Condensed, SF Rounded and SF Display.

It's worth mentioning that SF Pro Text has a discrete optical size, while SF Pro is a variable font. I don't know if this could be interfering in any way.

Comment 30

[Laura Hausmann]

As far as I can tell, this affects all fonts with optical sizing, e.g. the default font on Gmail (Google Sans), as well.

Firefox in Rosetta: https://img.zotan.services/i/e7eef3a3cd5d03085aba9a750bf19670bb162f4c2d892725036825763d81c32b.png
Firefox running natively: https://img.zotan.services/i/0af01fa861837d7920feba0a98c7dedf71645822b506a30f50f3c5c117b92a3a.png

(Difference is especially noticeable between the first two letters)

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to Laura Hausmann from comment #30)

As far as I can tell, this affects all fonts with optical sizing, e.g. the default font on Gmail (Google Sans), as well.

Firefox in Rosetta: https://img.zotan.services/i/e7eef3a3cd5d03085aba9a750bf19670bb162f4c2d892725036825763d81c32b.png
Firefox running natively: https://img.zotan.services/i/0af01fa861837d7920feba0a98c7dedf71645822b506a30f50f3c5c117b92a3a.png

(Difference is especially noticeable between the first two letters)

Yeah; another visible difference in those screenshots occurs in the word "Thank", where in one case the "T" is noticeably shorter than the "h", while in the other image they're the same height.

But what version of Firefox are you using? I could reproduce that issue with Google Sans using FF118, but since updating to 119, it's looking better here.

Comment 32

[Laura Hausmann]

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

(In reply to Laura Hausmann from comment #30)

As far as I can tell, this affects all fonts with optical sizing, e.g. the default font on Gmail (Google Sans), as well.

Firefox in Rosetta: https://img.zotan.services/i/e7eef3a3cd5d03085aba9a750bf19670bb162f4c2d892725036825763d81c32b.png
Firefox running natively: https://img.zotan.services/i/0af01fa861837d7920feba0a98c7dedf71645822b506a30f50f3c5c117b92a3a.png

(Difference is especially noticeable between the first two letters)

Yeah; another visible difference in those screenshots occurs in the word "Thank", where in one case the "T" is noticeably shorter than the "h", while in the other image they're the same height.

But what version of Firefox are you using? I could reproduce that issue with Google Sans using FF118, but since updating to 119, it's looking better here.

Rosetta firefox is 119, native is 118. After retesting with 119 native, the Google Sane font does indeed appear to be rendered almost if not fully identically as 119 in Rosetta renders it. Strange, as the system-ui font issue definitely affects 119 native as well.

Example:
Native (119.0): https://img.zotan.services/i/3614851cb06f7dc18fddb2d8eb8e7073ba457a1d2f8d63b1cc27039b63ec3db9.png
Rosetta (119.0): https://img.zotan.services/i/3166e0a702fbee1bba283edf13588951a0a64391cc7aef44d52ba70bd7537c22.png

Comment 33

[Laura Hausmann]

Spent some time debugging this and I'm fairly sure the bug (well, technically this is an Apple bug causing inconsistent behavior between platforms) is in either CreateCGFontWithVariations or more likely in CreateCTFontFromCGFontWithVariations, as the bug does not occur when overriding the system font to SF Pro Display in gfxMacPlatformFontList::InitSystemFontNames (NSFont* sys = [NSFont fontWithName:@"SF Pro Display" size:16.0];). My guess is that macOS-arm64 implements the conversion from CoreGraphics to CoreTexts fonts ever so slightly differently. I have no experience with any of these components but will continue to investigate and learn as I go. Definitely want to get to the bottom of this.

Comment 34

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1721612 - Fix instantiation of optically-sized system font in Skia on recent macOS versions. r=#gfx-reviewers

Comment 35

[Jonathan Kew [:jfkthame]]

As far as I can tell, the (remaining) issue here occurs specifically when the system font is used in situations where we render the text using Skia rather than WebRender.

The SkCTFontCreateExactCopy function fails to properly transfer the variation settings to the new font instance, and so we end up with glyphs that were positioned (during shaping) based on the correct optical size, but then rasterized at painting time using the wrong optical size value, leading to the poor spacing (and slightly-too-small glyphs).

This patch, which fixes SkCTFontCreateExactCopy in a similar way to CreateCTFontFromCGFontWithVariations in ScaledFontMac.cpp, appears to fix things in my testing.

Comment 36

[Sam Johnson]

@jfkthame, did you intend to put the patch on bug 1858347 instead? The issue described on this bug happens all the way back to Big Sur, while your patch seems to affect only Ventura and later.

Comment 37

[Jonathan Kew [:jfkthame]]

(In reply to Sam Johnson from comment #36)

@jfkthame, did you intend to put the patch on bug 1858347 instead? The issue described on this bug happens all the way back to Big Sur, while your patch seems to affect only Ventura and later.

Huh, maybe we should put it there, and then see what remains once that's fixed. I was under the impression this was at least partially resolved by bug 1856035, but I don't have a full collection of macOS versions/CPUs on hand locally for testing.

I'll move the patch as suggested, as that bug seems more clearly scoped, and once it's in Nightly, see if we can get more extensive testing again.

Comment 38

[Phabricator Automation]

Comment on attachment 9362445 [details]
Bug 1721612 - Fix instantiation of optically-sized system font in Skia on recent macOS versions. r=#gfx-reviewers

Revision D192989 was moved to bug 1858347. Setting attachment 9362445 [details] to obsolete.

Comment 39

[Sam Johnson]

Attachment: new_nightly_comparison.png

I've tested build 20231108211203, which should have the patch from bug 1858347. I am unfortunately still seeing the different text spacing between Intel and Apple Silicon.

Attached is another overlay of this GitHub repo readme on two machines, both running macOS 14.1, one is Intel and the other is Apple Silicon. Note that on one of the lines, the difference is enough to cause different wrapping.

Comment 40

[Laura Hausmann]

I can confirm that the mentioned patch does not fix this bug.

Comment 41

[Jonathan Kew [:jfkthame]]

(In reply to Sam Johnson from comment #39)

Created attachment 9362749 [details]
new_nightly_comparison.png

I've tested build 20231108211203, which should have the patch from bug 1858347. I am unfortunately still seeing the different text spacing between Intel and Apple Silicon.

Attached is another overlay of this GitHub repo readme on two machines, both running macOS 14.1, one is Intel and the other is Apple Silicon. Note that on one of the lines, the difference is enough to cause different wrapping.

Sigh .... ok, thanks for testing. We'll have to see if we can get to the bottom of that one. Can you confirm that bug 1858347 did resolve some issues, in places like the Library window's Import menu shown in https://bugzilla.mozilla.org/attachment.cgi?id=9357758?

Comment 42

[Jonathan Kew [:jfkthame]]

Attachment: Current Nightly on an M2 MBPro, macOS 14.1, running natively vs under Rosetta

OK, I can confirm I'm seeing the same discrepancy, depending on whether I'm running the Arm64 or Intel code. This screenshot compares rendering in the exact same Nightly build (20231108211203) running on the same machine; the top is the "native" rendering, and below is what I see after checking the Open using Rosetta option.

To my eye, the "bad" (top) version, though it has slightly wider spacing, doesn't look as bad as the menu example from bug 1858347, but there's clearly still something weird going on.

Comment 43

[Jack Roberts]

The fix applied in the most recent version of Nightly fixes the issue of poor rendering on Sonoma in general that was observed in bug 1858347, but like others I'm still observing the longstanding issue with letter spacing on the system font on Apple silicon.

Comment 44

[Jonathan Kew [:jfkthame]]

Attachment: Comparison of macOS 14.1 system font on Apple Silicon (left) and Intel (right) builds of current mozilla-central

One more comparison, using mozilla-central as of today, built for aarch64 (left) and for x86_64 (right).

This shows that at small font sizes (<=12px), the two builds match; and at large sizes (>=24px), they also match. But there's a range of intermediate font sizes, from 13px to 23px, where the Apple Silicon build gives us significantly looser spacing, which can clearly be seen as a "bulge" in the slope of the waterfall here.

I haven't yet figured out exactly where this discrepancy arises. :-(

Comment 45

[Sam Johnson]

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

Can you confirm that bug 1858347 did resolve some issues, in places like the Library window's Import menu shown in https://bugzilla.mozilla.org/attachment.cgi?id=9357758?

Yes! That bug is indeed resolved now :)

Comment 46

[Jonathan Kew [:jfkthame]]

OK, I finally found where this is happening. This isn't actually about the optical-sizing going wrong (which was the case in bug 1858347, etc), but about the tracking (OpenType 'trak' table) in the font. The Apple Silicon build simply fails to apply negative tracking values (which SFNS uses between 13px and 23px), but does apply them when positive (which is the case up to 12px, and from 24px upwards).

Turns out this is because of the incorrect use of an unsigned (uint32_t) variable. Assigning a negative floating-point value to this is undefined behavior. It "worked" on x86_64 via modulo arithmetic, but arm64 instead just treats it as zero. We need to use a signed variable for a value that might be negative!

Comment 47

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1721612 - Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=#gfx-reviewers

This method (as its name suggests) was originally created to handle synthetic-bold,
which only ever increases the advance, and so its use of an unsigned value for the
adjustment worked ok.

But when applying tracking, the adjustment may be negative, and assigning this to
a uint32_t value takes us into undefined-behavior territory. It seems this worked
"as expected" on x86_64 (using modulo arithmetic), but on arm64 the value just
clamped to zero, and the intended negative tracking doesn't get applied.

Making it an int32_t results in consistent behavior across both architectures.

Comment 48

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1721612 - (followup) Rename AdjustAdvancesForSyntheticBold to ApplyTrackingToClusters, and clamp the adjusted advance to avoid becoming negative. r=#gfx-reviewers

This is not necessary to fix the observed bug, but as a precaution against excessive
negative tracking, let's clamp the adjusted advance so that it can't become negative
(which seems unlikely to end well).

Also rename the method, given that it is not only used for synthetic bold adjustments
any longer.

(No change in behavior, except in the edge-case of a font that has such small
advances and extreme tracking that it tries to go backwards...)

Depends on D193288

Comment 49

[Laura Hausmann]

I can confirm that applying D193288 fixes this issue on my M1 MBA. Thank you so much!

Comment 50

[Jonathan Kew [:jfkthame]]

That's good to hear - thanks for confirming!

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1721612 - (more followup) Try to make clang-tidy happier, and cache the tracking value for a given px size. r=#gfx-reviewers

No change in behavior, but should be a minor (imperceptible?) perf win for fonts
with a 'trak' table.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5826f3cfadf
Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/665f815ddf6f
(followup) Rename AdjustAdvancesForSyntheticBold to ApplyTrackingToClusters, and clamp the adjusted advance to avoid becoming negative. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/a19116887ffa
(more followup) Try to make clang-tidy happier, and cache the tracking value for a given px size. r=gfx-reviewers,lsalzman

Comment 53

[Sam Johnson]

I can also confirm the issue is resolved on my machine with the patch. Thank you for digging into this!

Comment 54

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1721612) for causing failures in transform-input-002.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Li8dZt3FSEeJRj_kkAQY7Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=os%2Cx%2C10.15%2Cwebrender%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-macosx1015-64-qr%2Fopt-web-platform-tests-reftest%2Cwr3&revision=a19116887ffadfac6649a0d8a784264f01db143b

Log: https://treeherder.mozilla.org/logviewer?job_id=435872762&repo=autoland&lineNumber=21524

Backout: https://hg.mozilla.org/integration/autoland/rev/b5c67816c05d7e23a7e8869c5528c5bb43a42dde

Comment 55

[Jonathan Kew [:jfkthame]]

(In reply to Noemi Erli[:noemi_erli] from comment #54)

Backed out 3 changesets (Bug 1721612) for causing failures in transform-input-002.html CLOSED TREE

Just an "unexpected" pass, thanks to improved floating-point precision in the tracking adjustment. I'll update the expectation.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c28af806e64
Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/1870e54c72b0
(followup) Rename AdjustAdvancesForSyntheticBold to ApplyTrackingToClusters, and clamp the adjusted advance to avoid becoming negative. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/c03353fd1b03
(more followup) Try to make clang-tidy happier, and cache the tracking value for a given px size. r=gfx-reviewers,lsalzman

Comment 57

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0c28af806e64
https://hg.mozilla.org/mozilla-central/rev/1870e54c72b0
https://hg.mozilla.org/mozilla-central/rev/c03353fd1b03

Comment 58

[Jonathan Kew [:jfkthame]]

Comment on attachment 9362991 [details]
Bug 1721612 - Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Spacing of macOS system font is incorrect at sizes from 13-23px for users on Apple Silicon systems
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Trivial patch to avoid risk of undefined behavior (assigning a negative value to a uint32_t variable), which happened to work on x86_64 systems but fails on Arm64.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Users on (increasingly-common) Apple Silicon machines get poor rendering of the system font at common sizes; the fix is trivial/safe, so we should uplift it to improve the user experience on these machines.
• User impact if declined: Spacing of macOS system font is incorrect at sizes from 13-23px for users on Apple Silicon systems
• Fix Landed on Version: 121
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial patch declaring variable as signed, because its value may be negative at runtime.
  (The additional followup patches are not required to fix the behavior.)

Comment 59

[Dianna Smith [:diannaS]]

Comment on attachment 9362991 [details]
Bug 1721612 - Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=#gfx-reviewers

Switching flag to release, since we are out of betas. We will consider this for a RC respin or a Planned dot release

Comment 60

[Dianna Smith [:diannaS]]

Comment on attachment 9362991 [details]
Bug 1721612 - Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=#gfx-reviewers

Approved for 120.0rc2

Comment 61

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/8669734fe6af

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9362991 [details]
Bug 1721612 - Fix AdjustAdvancesForSyntheticBold to reliably handle negative adjustments. r=#gfx-reviewers

Approved for 115.6esr.

Comment 63

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/b12746b922ba