Fix capitalize Text style on IOS by MaeIg · Pull Request #32774 · react/react-native

MaeIg · 2021-12-16T17:58:21Z

Summary

On my project, I realized that capitalize style doesn't work with dates on IOS. I found this issue and tried to solve it.

(code example: https://snack.expo.dev/@maelg/capitalize-demo)

Android	IOS	Web

As we can see, the behavior is different on IOS, Android and web:

Android: Capitalize the first letter of each word, unless it begins with a number. And put the rest in lowercase.
IOS: Capitalize the first letter of each word, ~~unless it begins with a number~~. And put the rest in lowercase.
Web: Capitalize the first letter of each word, unless it begins with a number. ~~And put the rest in lowercase.~~

This PR aims to unify behavior on Android and Ios. I am not changing the behavior which differs from the web because I don't know if it is desirable to align with the web.

Changelog

[IOS] [Changed] - Don't capitalize the first letter of a word that is starting by a number

Test Plan

I manually tested my changes on a POC app (same code: https://snack.expo.dev/@maelg/capitalize-demo) on react-native v0.66.4 and react-native main branch.

You can see the result here:

Android	IOS

I tried to use rn-tester but it was not taking my code. I think it is because fabric was enabled so it was using other code.
I tried to disable fabric but I was not able to build the app on my IOS simulator anymore:

On rn-tester:

analysis-bot · 2021-12-16T18:31:36Z

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

Base commit: 3bd1159
Branch: main

analysis-bot · 2021-12-16T18:33:51Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,293,521	+5,943
android	hermes	armeabi-v7a	7,624,024	+4,342
android	hermes	x86	8,764,166	+6,221
android	hermes	x86_64	8,703,118	+6,758
android	jsc	arm64-v8a	9,681,919	+5,893
android	jsc	armeabi-v7a	8,669,110	+4,292
android	jsc	x86	9,638,068	+6,157
android	jsc	x86_64	10,234,605	+6,681

Base commit: 3bd1159
Branch: main

yungsters · 2021-12-16T18:45:32Z

Thanks for the contribution! It is interesting that web preserves pre-existing capitalized letters. I agree that we do not need to consider that as part of this PR, but it is interesting to consider which behavior is desirable longer term.

facebook-github-bot · 2021-12-16T18:47:06Z

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

sota000 · 2021-12-16T21:18:32Z

Hi there! For testing it with RN-tester, can you please try removing Pods folder, Podfile.lock, and build folder before running pod install with fabric_enabled off?

MaeIg · 2021-12-16T22:15:01Z

Thanks for your quick answers!

I managed to launch rn-tester without fabric thanks to you @sota000! I'll edit my first comment to add screenshots.
I found a crash on rn-tester when there are several spaces in a row. I just pushed a fix!

philIip · 2021-12-17T16:22:54Z

we prefer to use new if there's no initializer params

Oh I didn't know this syntax. Done!

philIip · 2021-12-17T16:30:39Z

let's add one for 2 digit dates as well

Done!
It still works:

philIip · 2021-12-17T16:33:52Z

why aren't we just joining on newWords? NSString's description is just the string itself

Yes you're right. Fixed!

philIip · 2021-12-17T16:42:00Z

can you make this a C function? i.e.

static NSString *CapitalizeText(NSString *text) { ... }

this is more lightweight and the function does not require any state from this class.

I think this is good. Tell me if it isn't!

As you can see with the code I wrote, I'm not familiar with Objective-C. I learned while I was making this PR 😁 So, thanks a lot for taking the time to review this PR, I learned a lot!

It would crash the app if there were several spaces in a row

Also rename the function to be clearer

facebook-github-bot · 2022-01-24T22:46:09Z

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

react-native-bot · 2022-01-25T17:15:05Z

This pull request was successfully merged by @MaeIg in 8b5a5d4.

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

Summary:  Few month ago, I created a [pull request](#32774) to unify the behavior of capitalize style between Android and IOS. But, I found out that it doesn't work when fabric is enabled. We have the old behavior : | Android | IOS | | ------------- | ------------- | | <img width="458" alt="capitalize_android_fabric" src="https://user-images.githubusercontent.com/40902940/163182082-4061003c-230b-46f7-9e93-c34b66dbf3d2.png"> | <img width="476" alt="capitalize_ios_fabric" src="https://user-images.githubusercontent.com/40902940/163182124-b6dee450-46e3-41a3-b5bb-553d7c2662e6.png"> | (source: rn-tester, last commit: dac56ce) As fabric is now live since v0.68, we should fix this behavior for fabric aswell. I don't know why there is so much duplicated code between `ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTAttributedTextUtils.mm` and `Libraries/Text/RCTTextAttributes.m` because I don't know the architecture of the project very well. But if you see missing tests or some refacto to do I'm open to suggestions! ## Changelog  [iOS] [Fixed] - Don't capitalize the first letter of a word that is starting by a number (Fabric renderer) Pull Request resolved: #33629 Test Plan: I manually tested these changes on rn-tester (react-native `main` branch): | Fabric ? | Android | IOS | | ------------- | ------------- | ------------- | | YES | <img width="458" alt="capitalize_android_fabric" src="https://user-images.githubusercontent.com/40902940/163182082-4061003c-230b-46f7-9e93-c34b66dbf3d2.png"> | <img width="476" alt="capitalize_ios_fabric_after" src="https://user-images.githubusercontent.com/40902940/163184277-6c58c117-7144-4f6b-98ea-0c1db654f27b.png"> | | NO | <img width="458" alt="android_nf_after" src="https://user-images.githubusercontent.com/40902940/163190263-9d801f6a-09c2-4ec6-a841-3dca115a5ef7.png"> | <img width="476" alt="ios_nf_after" src="https://user-images.githubusercontent.com/40902940/163190333-7e9eac6a-3f28-4826-8ef9-bcf45bf870a9.png"> | Reviewed By: cortinico Differential Revision: D35611086 Pulled By: GijsWeterings fbshipit-source-id: 0c43807dcddb30e65921eb1525c0fe440162ec32

…33629) Summary:  Few month ago, I created a [pull request](react#32774) to unify the behavior of capitalize style between Android and IOS. But, I found out that it doesn't work when fabric is enabled. We have the old behavior : | Android | IOS | | ------------- | ------------- | | <img width="458" alt="capitalize_android_fabric" src="https://user-images.githubusercontent.com/40902940/163182082-4061003c-230b-46f7-9e93-c34b66dbf3d2.png"> | <img width="476" alt="capitalize_ios_fabric" src="https://user-images.githubusercontent.com/40902940/163182124-b6dee450-46e3-41a3-b5bb-553d7c2662e6.png"> | (source: rn-tester, last commit: dac56ce) As fabric is now live since v0.68, we should fix this behavior for fabric aswell. I don't know why there is so much duplicated code between `ReactCommon/react/renderer/textlayoutmanager/platform/ios/RCTAttributedTextUtils.mm` and `Libraries/Text/RCTTextAttributes.m` because I don't know the architecture of the project very well. But if you see missing tests or some refacto to do I'm open to suggestions! ## Changelog  [iOS] [Fixed] - Don't capitalize the first letter of a word that is starting by a number (Fabric renderer) Pull Request resolved: react#33629 Test Plan: I manually tested these changes on rn-tester (react-native `main` branch): | Fabric ? | Android | IOS | | ------------- | ------------- | ------------- | | YES | <img width="458" alt="capitalize_android_fabric" src="https://user-images.githubusercontent.com/40902940/163182082-4061003c-230b-46f7-9e93-c34b66dbf3d2.png"> | <img width="476" alt="capitalize_ios_fabric_after" src="https://user-images.githubusercontent.com/40902940/163184277-6c58c117-7144-4f6b-98ea-0c1db654f27b.png"> | | NO | <img width="458" alt="android_nf_after" src="https://user-images.githubusercontent.com/40902940/163190263-9d801f6a-09c2-4ec6-a841-3dca115a5ef7.png"> | <img width="476" alt="ios_nf_after" src="https://user-images.githubusercontent.com/40902940/163190333-7e9eac6a-3f28-4826-8ef9-bcf45bf870a9.png"> | Reviewed By: cortinico Differential Revision: D35611086 Pulled By: GijsWeterings fbshipit-source-id: 0c43807dcddb30e65921eb1525c0fe440162ec32

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 Dec 16, 2021

MaeIg changed the title ~~Fix capitalize on IOS~~ Fix capitalize Text style on IOS Dec 16, 2021

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Dec 16, 2021

react-native-bot added the Platform: iOS iOS applications. label Dec 16, 2021

MaeIg force-pushed the fix-capitalize-on-ios branch from 118e09e to 2a7e0ef Compare December 16, 2021 22:11

philIip suggested changes Dec 17, 2021

View reviewed changes

MaeIg added 4 commits December 19, 2021 18:17

add examples in rn-tester

0356f65

don't capitalize words starting by a number

6022d82

fix: check if the word can be empty

66605d6

It would crash the app if there were several spaces in a row

make capitalizeText a C-function

3fb165a

Also rename the function to be clearer

MaeIg force-pushed the fix-capitalize-on-ios branch from 2a7e0ef to 3fb165a Compare December 19, 2021 17:32

MaeIg requested a review from philIip December 19, 2021 17:40

facebook-github-bot closed this in 8b5a5d4 Jan 25, 2022

react-native-bot added the Merged This PR has been merged. label Jan 25, 2022

MaeIg deleted the fix-capitalize-on-ios branch January 27, 2022 09:40

This was referenced Apr 13, 2022

Don't capitalize words starting by a number (fabric renderer) MaeIg/react-native#2

Closed

Don't capitalize words starting by a number (fabric renderer) #33629

Closed

amgleitman mentioned this pull request Jun 14, 2022

Merge from upstream up to the 0.68-stable branch point microsoft/react-native-macos#1197

Merged

4 tasks

Conversation

MaeIg commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yungsters commented Dec 16, 2021

Uh oh!

facebook-github-bot commented Dec 16, 2021

Uh oh!

sota000 commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaeIg commented Dec 16, 2021

Uh oh!

philIip Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

MaeIg Dec 19, 2021

Choose a reason for hiding this comment

Uh oh!

philIip Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

MaeIg Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philIip Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

MaeIg Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philIip Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

MaeIg Dec 19, 2021

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 24, 2022

Uh oh!

react-native-bot commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MaeIg commented Dec 16, 2021 •

edited

Loading

analysis-bot commented Dec 16, 2021 •

edited

Loading

analysis-bot commented Dec 16, 2021 •

edited

Loading

sota000 commented Dec 16, 2021 •

edited

Loading

MaeIg Dec 19, 2021 •

edited

Loading

MaeIg Dec 19, 2021 •

edited

Loading