-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Conversation
Changed category to "iOS" from "Catalyst" per @pull-bot. |
Base commit: 79f305e |
Base commit: b7d6b32 |
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 :).
If you define |
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 I was able to make some progress by making the changes:
But even after these patches, the React-graphics pod appears to have some misconfiguration issues. It fails to build with this error:
… which led me to notice that the React-graphics Pod's Xcode project is missing most of its source files, including the 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. |
Hello @andymatuschak, Re Fabric: Thank you |
There was a problem hiding this 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.
This pull request was successfully merged by @andymatuschak in 694e22d. When will my fix make it into a release? | Upcoming Releases |
Thank you for testing and landing this! |
@andymatuschak thank you for working on this :) |
…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
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 usesNSLayoutManager
to rasterize text (rather than relying on the system render server viaCATextLayer
), 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
RNTester
target's configuration, and check the "Mac" checkbox under "Deployment Info.use_flipper!
andflipper_post_install
in the Podfile, then runningpod install
; and b) removing theFB_SONARKIT_ENABLED
preprocessor flags in the Xcode project.RNTester
target configuration pane. Unfortunately, you must also do this in thePods
project for theReact-Core-AccessibilityResources
target (this is an issue which CocoaPods must fix).