Add Dynamic Type support for iOS (Paper and Fabric) #35017
Conversation
facebook-github-bot
added
the
CLA Signed
react-native-bot
added
Platform: iOS
Base commit: d71d0db |
Base commit: 7964d48 |
|
Hey there. When we talked about this PR a bit internally, there was a little bit of concern on accepting new capabilities which are paper-only. Having functionality that works on Paper, but not Fabric, means we would be taking new features away when the new Architecture is rolled out/default in OSS. I see that the Fabric implementation is blocked by a separate bug in #34990. I will make sure this is surfaced internally. |
|
@NickGerleman Now that e9b89b5 is a thing, I've added a Fabric implementation as well. |
NickGerleman
changed the title
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
PR build artifact for 355c7d2 is ready. |
ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTAttributedTextUtils.mm
Show resolved
Hide resolved
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| #import <UIKit/UIKit.h> | |||
|
|
|||
| #import <React/RCTDynamicTypeRamp.h> | |||
There was a problem hiding this comment.
Seeing this error on an internal build script:
packages/rn-tester/Pods/Headers/Public/React-Core/React/RCTTextAttributes.h:10:9: fatal error: 'React/RCTDynamicTypeRamp.h' file not found
#import <React/RCTDynamicTypeRamp.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
RCTTextDecorationLineType lives in React/Views. Wondering if this header should go there as well, vs if there is some internal bits we need to update.
There was a problem hiding this comment.
Not sure what RCTTextDecorationLineType has to do with any of this. Considering that RCTDynamicTypeRamp.h is a newly created file with this PR, I'm guessing 0254c3a should help here.
There was a problem hiding this comment.
@NickGerleman Can you confirm that this fix works?
|
PR build artifact for 0254c3a is ready. |
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Seeing the below when we are invoking the Cocoapods build internally via
|
Summary: Somwhere between importing facebook#35017 and running `js1 oss update-pods`, the entire Podfile was deleted without me noticing. This change runs `js1 oss prepare-pods` to restore it. Differential Revision: D41343786 fbshipit-source-id: 96e370d12f31a7d17ce31c0540a55b63a02161ef
Summary: Pull Request resolved: facebook#35369 Somwhere between importing facebook#35017 and running `js1 oss update-pods`, the entire Podfile was deleted without me noticing. This change runs `js1 oss prepare-pods` to restore it. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D41343786 fbshipit-source-id: 4bff39a8c22a6271c1fc2b4caa1d4a287e1c7021
Summary: Pull Request resolved: #35369 Somwhere between importing #35017 and running `js1 oss update-pods`, the entire Podfile lock was deleted without me noticing. This change runs `js1 oss prepare-pods` to restore it. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D41343786 fbshipit-source-id: 79abe41b18b8e13b9fba70906b8301b002d0e3db
Summary: This adds Dynamic Type support in iOS as described [here](react-native-community/discussions-and-proposals#519). `Text` elements have a new prop, `dynamicTypeRamp`, that allows users to specify which font ramp a particular `Text` element should take on as the OS's accessibility setting for text size. The different types line up with different values of `UIFontTextStyle`. If not specified, we default to the current behavior. ~~For the moment, this change is only for Paper. I tried applying a corresponding change to Fabric by adding an additional field to [`facebook::react::TextAttributes`](https://github.com/facebook/react-native/blob/main/ReactCommon/react/renderer/attributedstring/TextAttributes.h) and changing [`RCTEffectiveFontSizeMultiplierFromTextAttributes`](https://github.com/facebook/react-native/blob/afb124dcf0cdf0db525acc7cfd2cea2742c64068/ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTAttributedTextUtils.mm#L79-L84) to use that new field, but in the process I discovered that this function doesn't seem to ever get called, hence [this bug](#34990 ## Changelog [iOS] [Added] - Dynamic Type support Pull Request resolved: #35017 Test Plan: Validated with a test page in RNTester. Screenshots follow: A) Default text size B) Largest non-accessibility text size C) Largest accessibility text size, split across two screenshots due to size | A | B | C | |-|-|-| |  |  |  | |||  | Reviewed By: sammy-SC Differential Revision: D40779346 Pulled By: NickGerleman fbshipit-source-id: efc7a8e9810a93afc82c5def97af15a2e8453d90 # Conflicts: # packages/rn-tester/Podfile.lock
Summary: This adds Dynamic Type support in iOS as described [here](react-native-community/discussions-and-proposals#519). `Text` elements have a new prop, `dynamicTypeRamp`, that allows users to specify which font ramp a particular `Text` element should take on as the OS's accessibility setting for text size. The different types line up with different values of `UIFontTextStyle`. If not specified, we default to the current behavior. ~~For the moment, this change is only for Paper. I tried applying a corresponding change to Fabric by adding an additional field to [`facebook::react::TextAttributes`](https://github.com/facebook/react-native/blob/main/ReactCommon/react/renderer/attributedstring/TextAttributes.h) and changing [`RCTEffectiveFontSizeMultiplierFromTextAttributes`](https://github.com/facebook/react-native/blob/afb124dcf0cdf0db525acc7cfd2cea2742c64068/ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTAttributedTextUtils.mm#L79-L84) to use that new field, but in the process I discovered that this function doesn't seem to ever get called, hence [this bug](facebook#34990 ## Changelog [iOS] [Added] - Dynamic Type support Pull Request resolved: facebook#35017 Test Plan: Validated with a test page in RNTester. Screenshots follow: A) Default text size B) Largest non-accessibility text size C) Largest accessibility text size, split across two screenshots due to size | A | B | C | |-|-|-| |  |  |  | |||  | Reviewed By: sammy-SC Differential Revision: D40779346 Pulled By: NickGerleman fbshipit-source-id: efc7a8e9810a93afc82c5def97af15a2e8453d90
Summary: Pull Request resolved: facebook#35369 Somwhere between importing facebook#35017 and running `js1 oss update-pods`, the entire Podfile lock was deleted without me noticing. This change runs `js1 oss prepare-pods` to restore it. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D41343786 fbshipit-source-id: 79abe41b18b8e13b9fba70906b8301b002d0e3db
Summary: It seems this method is not referenced by anything anymore. I think #35017 made it redundant. Let's remove it? I can also do the whole "Deprecate for one version, remove in the next" since this was publicly exported. ## Changelog: [iOS] [REMOVED] - Remove RCTGetMultiplierForContentSizeCategory Pull Request resolved: #39617 Test Plan: CI should pass Reviewed By: dmytrorykun Differential Revision: D49618166 Pulled By: cipolleschi fbshipit-source-id: fb72e961d2a1eb9977944fea3ee4deeab46b946b
Summary
This adds Dynamic Type support in iOS as described here.
Textelements have a new prop,dynamicTypeRamp, that allows users to specify which font ramp a particularTextelement should take on as the OS's accessibility setting for text size. The different types line up with different values ofUIFontTextStyle. If not specified, we default to the current behavior.For the moment, this change is only for Paper. I tried applying a corresponding change to Fabric by adding an additional field tofacebook::react::TextAttributesand changingRCTEffectiveFontSizeMultiplierFromTextAttributesto use that new field, but in the process I discovered that this function doesn't seem to ever get called, hence this bug.Changelog
[iOS] [Added] - Dynamic Type support
Test Plan
Validated with a test page in RNTester. Screenshots follow:
A) Default text size
B) Largest non-accessibility text size
C) Largest accessibility text size, split across two screenshots due to size