Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in new architecture #47614

RickardZrinski · 2024-11-14T17:20:59Z

Summary:

The maxFontSizeMultiplier prop for Text and TextInput was not handled in Fabric / New Architecture as documented in #47499.

Changelog:

[GENERAL] [FIXED] - Fix maxFontSizeMultiplier prop on Text and TextInput components in Fabric / New Architecture

Test Plan:

I have not added any automated tests for this change but try to do so if requested. I have however added examples to RN Tester for both the Text and TextInput components, as well as compared the behaviour with Paper / Old Architecture. Both on version 0.76.

Noticed now I didn't do exactly the same steps in both videos, oops! Be aware that reapplying changes made in the Settings are currently half-broken on the new architecture, thus I'm restarting the app on Android and iOS. But this issue is unrelated to my changes. I've tested on main branch and it has the same issue.

Here are comparison videos between Paper and Fabric on iOS after I've made my fix.

Text

Paper	Fabric
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.19.39.32.mov	Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.20.10.51.mov

TextInput

Paper	Fabric
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.19.41.58.mov	Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.20.12.15.mov

…ook#47499)

RickardZrinski · 2024-11-14T17:22:42Z

...eact-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextAttributeProps.java

@@ -61,6 +61,7 @@ public class TextAttributeProps {
  public static final short TA_KEY_LINE_BREAK_STRATEGY = 25;
  public static final short TA_KEY_ROLE = 26;
  public static final short TA_KEY_TEXT_TRANSFORM = 27;
+  public static final short TA_KEY_MAX_FONT_SIZE_MULTIPLIER = 29;


Note that I've skipped 28 here. It's reserved by TA_KEY_ALIGNMENT_VERTICAL in attributedstring/conversions.h, but is missing its equivalent in this file. Might be another bug.

cipolleschi

Amazing job! Thanks for working on this!

facebook-github-bot · 2024-11-14T17:51:58Z

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

…eMultiplier

cipolleschi

Sorry for the late reply, I added a feedback that has arrived internally that should be addressed.

...er/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm

…namicTypeRamp

…eMultiplier

cipolleschi · 2024-12-09T16:07:52Z

Thanks for working on this and for applying the provided feedback!

facebook-github-bot · 2024-12-09T16:08:07Z

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

RickardZrinski · 2024-12-09T17:16:27Z

Thanks for working on this and for applying the provided feedback!

No problem, my pleasure!

I see that the internal linter failed. For future reference, any way to run that linter, or any other native code linter/autoformatter locally?

And while I have you: how do I run all of the iOS tests through Xcode? For RN tester I know I can just open RNTesterPods.xcworkspace and run them but there's obviously a bunch of tests outside of the RN Tester package, and those don't seem to be available to run in the RN Tester project. There's ./scripts/objc-test.sh test but I had some trouble with it, and not very practical when I want to debug with breakpoints etc.

Sorry for the off topic, but it'll help me contribute in the future 😊

cipolleschi · 2024-12-10T12:26:34Z

I see that the internal linter failed. For future reference, any way to run that linter, or any other native code linter/autoformatter locally?

The JS linters are aligned, but the iOS ones no. This is a step we have to do ourselves from the internal repository.
Different people (including myself) tried to align clang-format between internal and OSS, but we all failed because internally we used a modified version of Clang.. :/

And while I have you: how do I run all of the iOS tests through Xcode? For RN tester I know I can just open RNTesterPods.xcworkspace and run them but there's obviously a bunch of tests outside of the RN Tester package, and those don't seem to be available to run in the RN Tester project. There's ./scripts/objc-test.sh test but I had some trouble with it, and not very practical when I want to debug with breakpoints etc.

Yeah, that has been on my plate for a while, since... I need to get back to them but I didn't have time yet.
The way to run all of these tests is indeed ./scripts/objc-test.sh test.

The script does nothing magical:

starts metro
starts a websocket server (that echoes back messages)
build RNTester and runs unit and integration tests.

You can do the first two steps manually (the websocket server can be launched running ./packages/rn-tester/IntegrationTests/launchWebSocketServer.sh) and then run ⌘+u from Xcode. If you need breakpoints, that's the way to go.

cipolleschi · 2024-12-17T14:33:39Z

Brief update here: it is taking a little bit of time to land this because we need to add E2E internally and it is not as straightforward as we expected.

RickardZrinski · 2024-12-17T15:25:44Z

Brief update here: it is taking a little bit of time to land this because we need to add E2E internally and it is not as straightforward as we expected.

Thanks for the update! I understand. Let me know if there's anything I can assist with.

mysport12 · 2025-01-22T19:24:12Z

Curious if there is any update on this?

cipolleschi · 2025-01-29T13:17:42Z

Sorry for the delay... our internal E2E infrastructure is much more complex than what I was expecting and to enable the system font scaling at device level requires much more effort than what I thought.

I'll try to land this, splitting the tests in a separate change to unblock the situation.

facebook-github-bot · 2025-01-29T17:32:45Z

@cipolleschi merged this pull request in 97cf42f.

react-native-bot · 2025-01-29T17:32:48Z

This pull request was successfully merged by @RickardZrinski in 97cf42f

^{When will my fix make it into a release? | How to file a pick request?}

rei-shaholli · 2025-01-29T21:47:25Z

I am curious: will this fix be included in react native .78?

cipolleschi · 2025-02-03T14:03:56Z

Yep: reactwg/react-native-releases#765
I create pick requests also for 0.77 and 0.76

… in new architecture (#47614) Summary: The `maxFontSizeMultiplier` prop for `Text` and `TextInput` was not handled in Fabric / New Architecture as documented in #47499. bypass-github-export-checks ## Changelog: [GENERAL] [FIXED] - Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in Fabric / New Architecture Pull Request resolved: #47614 Test Plan: I have not added any automated tests for this change but try to do so if requested. I have however added examples to RN Tester for both the Text and TextInput components, as well as compared the behaviour with Paper / Old Architecture. Both on version 0.76. Noticed now I didn't do exactly the same steps in both videos, oops! Be aware that reapplying changes made in the Settings are currently half-broken on the new architecture, thus I'm restarting the app on Android and iOS. But this issue is unrelated to my changes. I've tested on main branch and it has the same issue. Here are comparison videos between Paper and Fabric on iOS *after* I've made my fix. ### Text | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/f4fd009f-aa6d-41ab-92fa-8dcf1e351ba1" /> | <video src="https://github.com/user-attachments/assets/fda42cc6-34c2-42a7-a6e2-028e7c866075" /> | ### TextInput | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/59b59f7b-25d2-4b5b-a8e2-d2054cc6390b" /> | <video src="https://github.com/user-attachments/assets/72068566-8f2a-4463-874c-45a6f5b63b0d" /> | Reviewed By: Abbondanzo Differential Revision: D65953019 Pulled By: cipolleschi fbshipit-source-id: 90c3c7e236229e9ad9bd346941fafe4af8a9d9fc

… in new architecture (#47614) Summary: The `maxFontSizeMultiplier` prop for `Text` and `TextInput` was not handled in Fabric / New Architecture as documented in #47499. bypass-github-export-checks [GENERAL] [FIXED] - Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in Fabric / New Architecture Pull Request resolved: #47614 Test Plan: I have not added any automated tests for this change but try to do so if requested. I have however added examples to RN Tester for both the Text and TextInput components, as well as compared the behaviour with Paper / Old Architecture. Both on version 0.76. Noticed now I didn't do exactly the same steps in both videos, oops! Be aware that reapplying changes made in the Settings are currently half-broken on the new architecture, thus I'm restarting the app on Android and iOS. But this issue is unrelated to my changes. I've tested on main branch and it has the same issue. Here are comparison videos between Paper and Fabric on iOS *after* I've made my fix. | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/f4fd009f-aa6d-41ab-92fa-8dcf1e351ba1" /> | <video src="https://github.com/user-attachments/assets/fda42cc6-34c2-42a7-a6e2-028e7c866075" /> | | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/59b59f7b-25d2-4b5b-a8e2-d2054cc6390b" /> | <video src="https://github.com/user-attachments/assets/72068566-8f2a-4463-874c-45a6f5b63b0d" /> | Reviewed By: Abbondanzo Differential Revision: D65953019 Pulled By: cipolleschi fbshipit-source-id: 90c3c7e236229e9ad9bd346941fafe4af8a9d9fc

react-native-bot · 2025-02-05T14:33:03Z

This pull request was successfully merged by @RickardZrinski in c0e6523

^{When will my fix make it into a release? | How to file a pick request?}

… in new architecture (#47614) Summary: The `maxFontSizeMultiplier` prop for `Text` and `TextInput` was not handled in Fabric / New Architecture as documented in #47499. bypass-github-export-checks [GENERAL] [FIXED] - Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in Fabric / New Architecture Pull Request resolved: #47614 Test Plan: I have not added any automated tests for this change but try to do so if requested. I have however added examples to RN Tester for both the Text and TextInput components, as well as compared the behaviour with Paper / Old Architecture. Both on version 0.76. Noticed now I didn't do exactly the same steps in both videos, oops! Be aware that reapplying changes made in the Settings are currently half-broken on the new architecture, thus I'm restarting the app on Android and iOS. But this issue is unrelated to my changes. I've tested on main branch and it has the same issue. Here are comparison videos between Paper and Fabric on iOS *after* I've made my fix. | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/f4fd009f-aa6d-41ab-92fa-8dcf1e351ba1" /> | <video src="https://github.com/user-attachments/assets/fda42cc6-34c2-42a7-a6e2-028e7c866075" /> | | Paper | Fabric | | ------------- | ------------- | | <video src="https://github.com/user-attachments/assets/59b59f7b-25d2-4b5b-a8e2-d2054cc6390b" /> | <video src="https://github.com/user-attachments/assets/72068566-8f2a-4463-874c-45a6f5b63b0d" /> | Reviewed By: Abbondanzo Differential Revision: D65953019 Pulled By: cipolleschi fbshipit-source-id: 90c3c7e236229e9ad9bd346941fafe4af8a9d9fc

RickardZrinski added 3 commits November 12, 2024 22:00

Fix support for maxFontSizeMultiplier prop in new architecture (faceb…

b726c7c

…ook#47499)

Merge branch 'facebook:main' into new-arch-maxFontSizeMultiplier

19b6e55

Merge branch 'facebook:main' into new-arch-maxFontSizeMultiplier

f489311

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 14, 2024

RickardZrinski commented Nov 14, 2024

View reviewed changes

cipolleschi approved these changes Nov 14, 2024

View reviewed changes

RickardZrinski added 2 commits November 14, 2024 19:35

remove unused TextInput import

b7b5b62

Merge remote-tracking branch 'upstream/main' into new-arch-maxFontSiz…

c8ce637

…eMultiplier

cipolleschi requested changes Dec 4, 2024

View reviewed changes

...er/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm Outdated Show resolved Hide resolved

RickardZrinski added 3 commits December 7, 2024 10:55

fix: maxFontSizeMultiplier not being taken into account when using dy…

b912ffc

…namicTypeRamp

Merge remote-tracking branch 'upstream/main' into new-arch-maxFontSiz…

ff630fa

…eMultiplier

be consistent with conditionals

6c97419

RickardZrinski requested a review from cipolleschi December 7, 2024 16:46

cipolleschi approved these changes Dec 9, 2024

View reviewed changes

ehsemloh mentioned this pull request Jan 11, 2025

iOS support of maxFontSizeMultiplier #48480

Closed

customputing mentioned this pull request Jan 24, 2025

[Bug]: maxFontSizeMultiplier is not being respected #47499

Closed

facebook-github-bot closed this in 97cf42f Jan 29, 2025

facebook-github-bot added the Merged This PR has been merged. label Jan 29, 2025

cipolleschi mentioned this pull request Feb 10, 2025

[RN][iOS][0.77] Properly handle null values in the TMInterop layer #49291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in new architecture #47614

Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in new architecture #47614

RickardZrinski commented Nov 14, 2024 •

edited

Loading

RickardZrinski Nov 14, 2024

cipolleschi left a comment

facebook-github-bot commented Nov 14, 2024

cipolleschi left a comment

cipolleschi commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

RickardZrinski commented Dec 9, 2024

cipolleschi commented Dec 10, 2024

cipolleschi commented Dec 17, 2024

RickardZrinski commented Dec 17, 2024

mysport12 commented Jan 22, 2025

cipolleschi commented Jan 29, 2025

facebook-github-bot commented Jan 29, 2025

react-native-bot commented Jan 29, 2025

rei-shaholli commented Jan 29, 2025

cipolleschi commented Feb 3, 2025

react-native-bot commented Feb 5, 2025

Fix maxFontSizeMultiplier prop on Text and TextInput components in new architecture #47614

Fix maxFontSizeMultiplier prop on Text and TextInput components in new architecture #47614

Conversation

RickardZrinski commented Nov 14, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

Text

TextInput

RickardZrinski Nov 14, 2024

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 14, 2024

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi commented Dec 9, 2024

facebook-github-bot commented Dec 9, 2024

RickardZrinski commented Dec 9, 2024

cipolleschi commented Dec 10, 2024

cipolleschi commented Dec 17, 2024

RickardZrinski commented Dec 17, 2024

mysport12 commented Jan 22, 2025

cipolleschi commented Jan 29, 2025

facebook-github-bot commented Jan 29, 2025

react-native-bot commented Jan 29, 2025

rei-shaholli commented Jan 29, 2025

cipolleschi commented Feb 3, 2025

react-native-bot commented Feb 5, 2025

Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in new architecture #47614

Fix `maxFontSizeMultiplier` prop on `Text` and `TextInput` components in new architecture #47614

RickardZrinski commented Nov 14, 2024 •

edited

Loading