[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it #10354

litherum · 2023-02-20T05:32:46Z

`b2e8181`

[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it
https://bugs.webkit.org/show_bug.cgi?id=252552
rdar://105662167

Reviewed by Alan Baradlay.

This is a follow-up from 260447@main. That previous patch went almost all the way toward fixing the
problem, but couldn't actually apply optical sizing to all text that requested it. The reason for
this is that the trigger we were using to enable optical sizing, `kCTFontOpticalSizeAttribute`,
enables the `opsz` variation axis, but also enables the `trak` table. Unfortunately, we have a lot
of tests which use fonts that don't support the optical sizing variation axis, but do have a `trak`
table. So, if we just naively started enabling `kCTFontOpticalSizeAttribute` for all text that
requests it, we'll get tons of test failures. It's not really great if we make tons of text on the
web look different, without it being a deliberate decision. So, the previous patch stopped just
short of enabling `kCTFontOpticalSizeAttribute` everywhere.

However, all fonts which *do* support the `opsz` variation axis should have it set automatically,
which is what 260447@main was aiming to address. We have the ability to *just* set the variation
axis without also setting `trak`, so that's what we need to do for fonts that request optical
sizing but for which we can't use `kCTFontOpticalSizeAttribute` because of compatibility.

So, we need 3 buckets:
- Disable optical sizing. This is triggered by font-optical-sizing:none. This maps to telling Core
Text to use `kCTFontOpticalSizeAttribute`: `none`.
- Enable full optical sizing, including `trak`. This is triggered by
text-rendering:optimizeLegibility. This maps to telling Core Text to use
`kCTFontOpticalSizeAttribute`: `auto`.
- Enable optical sizing, but just the `opsz` variation, and not `trak`, for compatibility. This
is what all situations not captured by the above use. This maps to telling Core Text to use
`kCTFontVariationAttribute`: {`opsz`: font size}.

This patch implements the third bucket, which wasn't implemented in the previous patch. The third
bucket doesn't actually work correctly on older OSes, so it's guarded behind a OS version check.

https://bugs.webkit.org/show_bug.cgi?id=252592 is about deleting the compatibility third bucket in
favor of the second bucket: actually enabling `trak` for all text that should have optical sizing
enabled.

Just like 260447@main, the tests for this adds a megabyte of fonts (because we have to keep the
whole release of an OFL font together) so I'm not including the test with this PR, and will upload
another PR later with a custom font that I create from scratch for testing this.

* Source/WTF/wtf/PlatformUse.h:
* Source/WebCore/platform/graphics/cocoa/UnrealizedCoreTextFont.cpp:
(WebCore::UnrealizedCoreTextFont::addAttributesForOpticalSizing):
(WebCore::UnrealizedCoreTextFont::applyVariations):
(WebCore::UnrealizedCoreTextFont::modifyFromContext):
(WebCore::UnrealizedCoreTextFont::realize const):
(WebCore::applyVariations): Deleted.
(WebCore::modifyFromContext): Deleted.
* Source/WebCore/platform/graphics/cocoa/UnrealizedCoreTextFont.h:

Canonical link: https://commits.webkit.org/260781@main

47febba

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	~~🛠 watch~~	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
✅ 🛠 🧪 merge	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-02-20T05:36:31Z

EWS run on previous version of this PR (hash 9922549)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

litherum · 2023-02-20T19:55:27Z

All these tests pass on a new Mac operating system, so I think we need a version check.

webkit-early-warning-system · 2023-02-20T20:28:49Z

EWS run on previous version of this PR (hash 9619f04)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	❌ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	❌ 🧪 mac-wk1	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	❌ 🧪 mac-wk2	⏳ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch	❌ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-02-20T20:28:51Z

EWS run on previous version of this PR (hash 91751c1)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
⏳ 🧪 bindings	⏳ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk
~~🧪 webkitperl~~	⏳ 🧪 ios-wk2	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2	⏳ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 jsc-armv7-tests
⏳ 🧪 services	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress	⏳ 🛠 jsc-mips
	⏳ 🛠 watch-sim		⏳ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-02-20T22:51:01Z

EWS run on previous version of this PR (hash 34cbb7d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	❌ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	❌ 🧪 mac-wk1	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	❌ 🧪 mac-wk2	⏳ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch	❌ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-02-21T03:00:19Z

EWS run on previous version of this PR (hash 8c3872d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

litherum · 2023-02-22T21:32:28Z

Source/WTF/wtf/PlatformUse.h

@@ -391,3 +391,10 @@
 // The header says "Before macOS 13.0 and iOS 16.0 this attribute is not accurate"
 #define USE_KCTFONTVARIATIONAXESATTRIBUTE 1
 #endif
+
+#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 130000) \


Update copyright

litherum · 2023-02-22T21:32:59Z

Verified tests pass on Ventura

litherum · 2023-02-22T23:45:16Z

This patch should be disabled on Monterey.

webkit-early-warning-system · 2023-02-23T06:15:03Z

EWS run on previous version of this PR (hash e0eb411)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	❌ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-02-23T20:52:37Z

EWS run on current version of this PR (hash 47febba)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	~~🛠 watch~~	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
✅ 🛠 🧪 merge	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

…for all fonts that need it https://bugs.webkit.org/show_bug.cgi?id=252552 rdar://105662167 Reviewed by Alan Baradlay. This is a follow-up from 260447@main. That previous patch went almost all the way toward fixing the problem, but couldn't actually apply optical sizing to all text that requested it. The reason for this is that the trigger we were using to enable optical sizing, `kCTFontOpticalSizeAttribute`, enables the `opsz` variation axis, but also enables the `trak` table. Unfortunately, we have a lot of tests which use fonts that don't support the optical sizing variation axis, but do have a `trak` table. So, if we just naively started enabling `kCTFontOpticalSizeAttribute` for all text that requests it, we'll get tons of test failures. It's not really great if we make tons of text on the web look different, without it being a deliberate decision. So, the previous patch stopped just short of enabling `kCTFontOpticalSizeAttribute` everywhere. However, all fonts which *do* support the `opsz` variation axis should have it set automatically, which is what 260447@main was aiming to address. We have the ability to *just* set the variation axis without also setting `trak`, so that's what we need to do for fonts that request optical sizing but for which we can't use `kCTFontOpticalSizeAttribute` because of compatibility. So, we need 3 buckets: - Disable optical sizing. This is triggered by font-optical-sizing:none. This maps to telling Core Text to use `kCTFontOpticalSizeAttribute`: `none`. - Enable full optical sizing, including `trak`. This is triggered by text-rendering:optimizeLegibility. This maps to telling Core Text to use `kCTFontOpticalSizeAttribute`: `auto`. - Enable optical sizing, but just the `opsz` variation, and not `trak`, for compatibility. This is what all situations not captured by the above use. This maps to telling Core Text to use `kCTFontVariationAttribute`: {`opsz`: font size}. This patch implements the third bucket, which wasn't implemented in the previous patch. The third bucket doesn't actually work correctly on older OSes, so it's guarded behind a OS version check. https://bugs.webkit.org/show_bug.cgi?id=252592 is about deleting the compatibility third bucket in favor of the second bucket: actually enabling `trak` for all text that should have optical sizing enabled. Just like 260447@main, the tests for this adds a megabyte of fonts (because we have to keep the whole release of an OFL font together) so I'm not including the test with this PR, and will upload another PR later with a custom font that I create from scratch for testing this. * Source/WTF/wtf/PlatformUse.h: * Source/WebCore/platform/graphics/cocoa/UnrealizedCoreTextFont.cpp: (WebCore::UnrealizedCoreTextFont::addAttributesForOpticalSizing): (WebCore::UnrealizedCoreTextFont::applyVariations): (WebCore::UnrealizedCoreTextFont::modifyFromContext): (WebCore::UnrealizedCoreTextFont::realize const): (WebCore::applyVariations): Deleted. (WebCore::modifyFromContext): Deleted. * Source/WebCore/platform/graphics/cocoa/UnrealizedCoreTextFont.h: Canonical link: https://commits.webkit.org/260781@main

webkit-commit-queue · 2023-02-24T04:29:44Z

Committed 260781@main (b2e8181): https://commits.webkit.org/260781@main

Reviewed commits have been landed. Closing PR #10354 and removing active labels.

litherum self-assigned this Feb 20, 2023

litherum added the Text For bugs in text layout and rendering, including international text support. label Feb 20, 2023

litherum force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch 2 times, most recently from 91751c1 to 9619f04 Compare February 20, 2023 20:07

litherum force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch from 9619f04 to 34cbb7d Compare February 20, 2023 22:50

litherum force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch from 34cbb7d to 8c3872d Compare February 21, 2023 02:58

litherum commented Feb 22, 2023

View reviewed changes

litherum force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch from 8c3872d to e0eb411 Compare February 23, 2023 06:14

litherum marked this pull request as ready for review February 23, 2023 06:16

litherum requested review from alanbaradlay and heycam February 23, 2023 06:16

litherum force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch from e0eb411 to 47febba Compare February 23, 2023 20:52

alanbaradlay approved these changes Feb 24, 2023

View reviewed changes

litherum added the merge-queue Applied to send a pull request to merge-queue label Feb 24, 2023

webkit-early-warning-system force-pushed the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch from 47febba to b2e8181 Compare February 24, 2023 04:29

webkit-early-warning-system merged commit b2e8181 into WebKit:main Feb 24, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 24, 2023

litherum deleted the eng/Cocoa-font-optical-sizingauto-doesnt-set-the-opsz-variation-axis-for-all-fonts-that-need-it branch February 24, 2023 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it #10354

[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it #10354

litherum commented Feb 20, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 20, 2023 •

edited

litherum commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 21, 2023 •

edited

litherum Feb 22, 2023

litherum commented Feb 22, 2023

litherum commented Feb 22, 2023

webkit-early-warning-system commented Feb 23, 2023 •

edited

webkit-early-warning-system commented Feb 23, 2023 •

edited

webkit-commit-queue commented Feb 24, 2023

[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it #10354

[Cocoa] font-optical-sizing:auto doesn't set the opsz variation axis for all fonts that need it #10354

Conversation

litherum commented Feb 20, 2023 • edited by webkit-early-warning-system

b2e8181

webkit-early-warning-system commented Feb 20, 2023 • edited

litherum commented Feb 20, 2023 • edited

webkit-early-warning-system commented Feb 20, 2023 • edited

webkit-early-warning-system commented Feb 20, 2023 • edited

webkit-early-warning-system commented Feb 20, 2023 • edited

webkit-early-warning-system commented Feb 21, 2023 • edited

litherum Feb 22, 2023

Choose a reason for hiding this comment

litherum commented Feb 22, 2023

litherum commented Feb 22, 2023

webkit-early-warning-system commented Feb 23, 2023 • edited

webkit-early-warning-system commented Feb 23, 2023 • edited

webkit-commit-queue commented Feb 24, 2023

litherum commented Feb 20, 2023 •

edited by webkit-early-warning-system

`b2e8181`

webkit-early-warning-system commented Feb 20, 2023 •

edited

litherum commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 20, 2023 •

edited

webkit-early-warning-system commented Feb 21, 2023 •

edited

webkit-early-warning-system commented Feb 23, 2023 •

edited

webkit-early-warning-system commented Feb 23, 2023 •

edited