Skip to content

Add Dynamic Type support for iOS (Paper and Fabric) - #35017

Closed
amgleitman wants to merge 12 commits into
react:mainfrom
amgleitman:core-ios-dynamic-type
Closed

Add Dynamic Type support for iOS (Paper and Fabric)#35017
amgleitman wants to merge 12 commits into
react:mainfrom
amgleitman:core-ios-dynamic-type

Conversation

@amgleitman

@amgleitmanamgleitman commented Oct 18, 2022

Copy link
Copy Markdown

Summary

This adds Dynamic Type support in iOS as described here.

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 and changing RCTEffectiveFontSizeMultiplierFromTextAttributes to 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

ABC
Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 17 08Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 17 55Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 18 33
Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 18 42

@facebook-github-botfacebook-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 Oct 18, 2022
@react-native-botreact-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Oct 18, 2022
@analysis-bot

analysis-bot commented Oct 19, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a6,993,434+1,351
androidhermesarmeabi-v7a6,369,583+965
androidhermesx867,406,626+1,852
androidhermesx86_647,270,277+1,485
androidjscarm64-v8a8,857,216+1,259
androidjscarmeabi-v7a7,595,335+865
androidjscx868,915,461+1,751
androidjscx86_649,398,459+1,399

Base commit: d71d0db
Branch: main

@analysis-bot

analysis-bot commented Oct 19, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 7964d48
Branch: main

@NickGerleman

Copy link
Copy Markdown
Contributor

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.

@amgleitman

Copy link
Copy Markdown
Author

@NickGerleman Now that e9b89b5 is a thing, I've added a Fabric implementation as well.

@NickGerlemanNickGerleman changed the title Add Dynamic Type support for iOS (Paper only for now)Add Dynamic Type support for iOS (Paper and Fabric)Oct 27, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pull-bot

Copy link
Copy Markdown

PR build artifact for 355c7d2 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.


#import <UIKit/UIKit.h>

#import <React/RCTDynamicTypeRamp.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@NickGerleman Can you confirm that this fix works?

@pull-bot

Copy link
Copy Markdown

PR build artifact for 0254c3a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NickGerleman

Copy link
Copy Markdown
Contributor

Seeing the below when we are invoking the Cocoapods build internally via react-native-oss-ios-jsc, but not coverage in CircleCI. @cipolleschi@hramos any guesses?

The following build commands failed:

CompileC <>/Library/Developer/Xcode/DerivedData/RNTesterPods-avhcctdswdycllfjzyasqkbrubnn/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/React-RCTText.build/Objects-normal/x86_64/RCTUITextView.o <>/Libraries/Text/TextInput/Multiline/RCTUITextView.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler (in target 'React-RCTText' from project 'Pods')
(1 failure)

@NickGerlemanNickGerleman mentioned this pull request Nov 16, 2022
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Nov 16, 2022
Summary:
Pull Request resolved: react#35369
Somwhere between importing react#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
facebook-github-bot pushed a commit that referenced this pull request Nov 16, 2022
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
kelset pushed a commit that referenced this pull request Nov 22, 2022
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 |
|-|-|-|
| ![Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 17 08](https://user-images.githubusercontent.com/717674/196562746-c8bbf53d-3c70-4e55-8600-0cfed8aacf5d.png) | ![Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 17 55](https://user-images.githubusercontent.com/717674/196563051-68bb0e34-c573-47ed-8c19-58ae45a7ce2b.png) | ![Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 18 33](https://user-images.githubusercontent.com/717674/196563185-61ede5ee-e79e-4af5-84a7-8f1e230a25f8.png) |
||| ![Simulator Screen Shot - iPad (9th generation) - 2022-10-18 at 16 18 42](https://user-images.githubusercontent.com/717674/196563208-2242efa2-5f24-466d-80f5-eb57a7678a67.png) |
Reviewed By: sammy-SC
Differential Revision: D40779346
Pulled By: NickGerleman
fbshipit-source-id: efc7a8e9810a93afc82c5def97af15a2e8453d90
# Conflicts:
#	packages/rn-tester/Podfile.lock
facebook-github-bot pushed a commit that referenced this pull request Oct 9, 2023
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
@cipolleschicipolleschi mentioned this pull request Oct 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.p: MicrosoftPartner: MicrosoftPartnerPlatform: iOSiOS applications.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@amgleitman@analysis-bot@NickGerleman@facebook-github-bot@pull-bot@kelset@react-native-bot