Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Catalyst][Text] Fix Catalyst text rendering by disabling inappropriate subpixel-antialiasing #29609

Closed
wants to merge 3 commits into from

Conversation

andymatuschak
Copy link
Contributor

@andymatuschak andymatuschak commented Aug 10, 2020

Summary

I noticed when porting my iOS app to macOS via Catalyst that the text rendering was somewhat different on the two platforms. Text looked blurry and over-weight on macOS, even when disabling the Catalyst scaling transform.

I hazily remembered that I'd seen this problem before in my old Cocoa development days: this kind of blurring occurs when rendering text with sub-pixel anti-aliasing into an offscreen buffer which will then be traditionally composited, because when the SPAA algorithm attempts to blend with the underlying content (i.e. in the offscreen buffer), there isn't any. SPAA is disabled on iOS, so the issue wouldn't appear there. On macOS, typical approachs to displaying text (e.g. CATextLayer) normally disable SPAA, since it's been incompatible with the platform's compositing strategy since the transition to layer-backed views some years ago. But React Native uses NSLayoutManager to rasterize text (rather than relying on the system render server via CATextLayer), and that class doesn't touch the context's font smoothing bit before drawing.

This change makes macOS/Catalyst text rendering consistent with iOS text rendering by disabling SPAA.

It appears that the code I've modified is in the process of being refactored (for Fabric?). It looks like this is the corresponding place in the new code (@sammy-SC, is that right?). I'm happy to include a change to the new renderer in this patch if someone can point me at how to test that change.

Changelog

[iOS] [Fixed] - Improved text rendering on macOS Catalyst

Test Plan

  1. Prepare RNTester for running on macOS (or apply this patch to handle parts 1 and 2, but you'll still need to do part 3):
    1. Open the workspace, navigate to the RNTester target's configuration, and check the "Mac" checkbox under "Deployment Info.
    2. Flipper doesn't yet compile for Catalyst ([0.62.0-rc.3] Catalyst macOS build issue typedef redefinition with different types #27845), so you must disable it by: a) commenting out use_flipper! and flipper_post_install in the Podfile, then running pod install; and b) removing the FB_SONARKIT_ENABLED preprocessor flags in the Xcode project.
    3. macOS has different signing rules from iOS; you must set a development team in the "Signing & Capabilities" tab of the RNTester target configuration pane. Unfortunately, you must also do this in the Pods project for the React-Core-AccessibilityResources target (this is an issue which CocoaPods must fix).
  2. Run RNTester with and without the patch. You'll see that the font hinting is overweight without the patch; see screenshots below (incorrect rendering above, correct rendering below; note that fonts still remain slightly blurred because of Catalyst's window scaling transform, but that's removed on Big Sur).

Screen Shot 2020-08-12 at 10 03 50 AM
Screen Shot 2020-08-12 at 10 03 15 AM

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 10, 2020
@andymatuschak
Copy link
Contributor Author

Changed category to "iOS" from "Catalyst" per @pull-bot.

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Aug 10, 2020
@analysis-bot
Copy link

analysis-bot commented Aug 10, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 79f305e

@analysis-bot
Copy link

analysis-bot commented Aug 10, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,840,195 -1,908
android hermes armeabi-v7a 7,450,193 -5,687
android hermes x86 8,298,482 -2,112
android hermes x86_64 8,191,573 -2,047
android jsc arm64-v8a 9,999,729 -1,896
android jsc armeabi-v7a 9,601,845 -5,704
android jsc x86 9,886,114 -2,104
android jsc x86_64 10,465,398 -2,038

Base commit: b7d6b32

@sammy-SC
Copy link
Contributor

It appears that the code I've modified is in the process of being refactored (for Fabric?). It looks like this is the corresponding place in the new code (@sammy-SC, is that right?).

Hello @andymatuschak,

the code you have refactored will stay in place until Fabric is completely rolled out. Fabric on iOS is a complete rewrite.

You have found the corresponding place in Fabric :).

I'm happy to include a change to the new renderer in this patch if someone can point me at how to test that change.

If you define RN_FABRIC_ENABLED, RNTester will start with the new renderer.

@andymatuschak
Copy link
Contributor Author

andymatuschak commented Aug 12, 2020

Thanks for that, @sammy-SC! I've just spent a while trying to get Fabric up and running (with iOS in general, not even on Catalyst). I faced immediate configuration issues which suggested to me that master may have some out of date Fabric components: several configuration files contain what appear to be outdated paths, and a dependency on flow-node was introduced without being added to the package.

I was able to make some progress by making the changes:

diff --git a/ReactCommon/react/renderer/graphics/React-graphics.podspec b/ReactCommon/react/renderer/graphics/React-graphics.podspec
index ddd519790e..07d54f808f 100644
--- a/ReactCommon/react/renderer/graphics/React-graphics.podspec
+++ b/ReactCommon/react/renderer/graphics/React-graphics.podspec
@@ -5,7 +5,7 @@
 
 require "json"
 
-package = JSON.parse(File.read(File.join(__dir__, "..", "..", "..", "package.json")))
+package = JSON.parse(File.read(File.join(__dir__, "..", "..", "..", "..", "package.json")))
 version = package['version']
 
 source = { :git => 'https://github.com/facebook/react-native.git' }
diff --git a/package.json b/package.json
index b9d85cdd85..99f3510d0f 100644
--- a/package.json
+++ b/package.json
@@ -142,6 +142,7 @@
     "eslint-plugin-react-native": "3.8.1",
     "eslint-plugin-relay": "1.7.1",
     "flow-bin": "^0.131.0",
+    "flow-remove-types": "^2.131.0",
     "jest": "^26.0.1",
     "jest-junit": "^10.0.0",
     "jscodeshift": "^0.9.0",
diff --git a/scripts/react_native_pods.rb b/scripts/react_native_pods.rb
index 73bb9ad55a..534dd6d298 100644
--- a/scripts/react_native_pods.rb
+++ b/scripts/react_native_pods.rb
@@ -52,7 +52,7 @@ def use_react_native! (options={})
 
   if fabric_enabled
     pod 'React-Fabric', :path => "#{prefix}/ReactCommon"
-    pod 'React-graphics', :path => "#{prefix}/ReactCommon/fabric/graphics"
+    pod 'React-graphics', :path => "#{prefix}/ReactCommon/react/renderer/graphics"
     pod 'React-jsi/Fabric', :path => "#{prefix}/ReactCommon/jsi"
     pod 'React-RCTFabric', :path => "#{prefix}/React"
     pod 'Folly/Fabric', :podspec => "#{prefix}/third-party-podspecs/Folly.podspec"

But even after these patches, the React-graphics pod appears to have some misconfiguration issues. It fails to build with this error:

/Users/andym/Development/External/react-native/ReactCommon/react/renderer/graphics/platform/ios/Color.h:13:10: 'react/renderer/graphics/ColorComponents.h' file not found

… which led me to notice that the React-graphics Pod's Xcode project is missing most of its source files, including the RCTTextLayoutManager.mm file I've modified. This screenshot shows the source listing for React-graphics:

Screen Shot 2020-08-12 at 9 57 25 AM

At this point, I stopped, because these problems don't appear to be local to my machine—it looks to me more like I'm simply not running known-good sources for Fabric.

I've updated the test plan in the pull request description to include the steps for running RNTester on macOS/Catalyst, and I've pushed a commit to my branch which implements my change for Fabric. If you'd be willing to test the change yourself or to point me to where I've gone wrong in my attempt above, I'd be grateful.

@sammy-SC
Copy link
Contributor

Hello @andymatuschak,

Re Fabric:
Apparently it isn't ready to be run in open source yet. I will import this PR into our systems and take it for a spin in Fabric to make sure it works.

Thank you

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sammy-SC has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andymatuschak in 694e22d.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 27, 2020
@andymatuschak
Copy link
Contributor Author

Thank you for testing and landing this!

@sammy-SC
Copy link
Contributor

@andymatuschak thank you for working on this :)

philippeauriach pushed a commit to philippeauriach/react-native that referenced this pull request May 5, 2021
…liasing (facebook#29609)

Summary:
I noticed when porting my iOS app to macOS via Catalyst that the text rendering was somewhat different on the two platforms. Text looked blurry and over-weight on macOS, even when disabling the Catalyst scaling transform.

I hazily remembered that I'd seen this problem before in my old Cocoa development days: this kind of blurring occurs when rendering text with sub-pixel anti-aliasing into an offscreen buffer which will then be traditionally composited, because when the SPAA algorithm attempts to blend with the underlying content (i.e. in the offscreen buffer), there isn't any. SPAA is disabled on iOS, so the issue wouldn't appear there. On macOS, typical approachs to displaying text (e.g. `CATextLayer`) normally disable SPAA, since it's been incompatible with the platform's compositing strategy since the transition to layer-backed views some years ago. But React Native uses `NSLayoutManager` to rasterize text (rather than relying on the system render server via `CATextLayer`), and that class doesn't touch the context's font smoothing bit before drawing.

This change makes macOS/Catalyst text rendering consistent with iOS text rendering by disabling SPAA.

It appears that the code I've modified is in the process of being refactored (for Fabric?). It looks like [this](https://github.com/facebook/react-native/blob/8d6b41e9bcede07fb627d57cf6c11050ae590d57/ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTTextLayoutManager.mm#L111) is the corresponding place in the new code (sammy-SC, is that right?). I'm happy to include a change to the new renderer in this patch if someone can point me at how to test that change.

## Changelog

[iOS] [Fixed] - Improved text rendering on macOS Catalyst

Pull Request resolved: facebook#29609

Test Plan:
1. Prepare RNTester for running on macOS (or apply [this patch](https://gist.github.com/andymatuschak/d0f5b4fc1a28efc4f860801aa1deddcd) to handle parts 1 and 2, but you'll still need to do part 3):
   1. Open the workspace, navigate to the `RNTester` target's configuration, and check the "Mac" checkbox under "Deployment Info.
   2. Flipper doesn't yet compile for Catalyst (facebook#27845), so you must disable it by: a) commenting out `use_flipper!` and `flipper_post_install` in the Podfile, then running `pod install`; and b) removing the `FB_SONARKIT_ENABLED` preprocessor flags in the Xcode project.
   3. macOS has different signing rules from iOS; you must set a development team in the "Signing & Capabilities" tab of the `RNTester` target configuration pane. Unfortunately, you must also do this in the `Pods` project for the `React-Core-AccessibilityResources` target ([this is an issue which CocoaPods must fix](CocoaPods/CocoaPods#8891)).
2. Run RNTester with and without the patch. You'll see that the font hinting is overweight without the patch; see screenshots below (incorrect rendering above, correct rendering below; note that fonts still remain slightly blurred because of Catalyst's window scaling transform, but that's removed on Big Sur).

![Screen Shot 2020-08-12 at 10 03 50 AM](https://user-images.githubusercontent.com/2771/90045523-0374c700-dc84-11ea-8945-2d9830bccbd1.png)
![Screen Shot 2020-08-12 at 10 03 15 AM](https://user-images.githubusercontent.com/2771/90045547-0bcd0200-dc84-11ea-88af-37a8879b4efd.png)

Reviewed By: PeteTheHeat

Differential Revision: D23344751

Pulled By: sammy-SC

fbshipit-source-id: 1bbf682b681e381a8a90e152245d9b0df8ec7697
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants