Skip to content
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

[iOS] Fixed maxLength of TextInput based on glyph count #24109

Closed
wants to merge 6 commits into from

Conversation

zhongwuzw
Copy link
Contributor

Summary

Fixes #10964 #22463 .
We need changed maxLength to count glyph instead. Because like emoji, some language,it represents using multiple bytes, so if we limit using characters, would leads some weird bugs, like clear the line or glyph not show correctly.

cc. @cpojer .

Changelog

[iOS] [Fixed] - Fixed maxLength of TextInput based on glyph count

Test Plan

If we input one emoji, it needs to show correctly.

<TextInput maxLength={1} />

@facebook-github-bot 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 Mar 23, 2019
@react-native-bot react-native-bot added Component: TextInput Related to the TextInput component. Platform: iOS iOS applications. Includes Changelog labels Mar 23, 2019
Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

Are there any chances that we can restructure the code moving complexity of this particular feature to separate thing/class/function?

TextInput is very complex and fragile component. We have to try very hard to maintain the manageability of this complexity (yes it is already pretty poor but it was even worse).

Now, with this implementation, we have a hard dilemma: what's more important to us - this feature or regress of manageability?
This is hard choice.

@@ -20,6 +20,25 @@
#import "RCTTextAttributes.h"
#import "RCTTextSelection.h"

@interface NSString (RCTUtility)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to decouple that into separate file(s).

@zhongwuzw
Copy link
Contributor Author

@shegin Is this what we want? 🤔 I restructure the code and consolidate max length handler into separate function.

Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

I believe to land this feature we need to reduce complexity instead of increasing it.

Yes, it's challenging but that's the only sustainable way to progress.
I don't know how it can be done, but I believe it's possible having that e.g. - (BOOL)textInputShouldChangeTextInRange:(NSRange)range replacementText:(NSString *)text is already pretty complex and clearly needs more love.

@@ -619,6 +610,38 @@ - (void)handleInputAccessoryDoneButton

#pragma mark - Helpers

- (void)trimInputTextInRange:(NSRange)range
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly.
This separate function is still a method (it mutates self) which does not solve the complexity problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shergin Yeah, actually, Swift already solved this glyphs issue, but OC, we seems need to do these heavy things by ourself. Have no idea how to move on. 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, Swift's strings are character-aware but normal Objective-C are not. Yeah... but that's only String's properties, that is not related to UITextField and UITextView implementation, right? So, we already have two subclasses for those classes that enhance the behaviors and fix some issue. Maybe we can encapsulate that character-aware logic there?

Or, what if we move that maxLength feature implementation into those classes and try to make it totally transparent for all external logic?

Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

❤️
Back to your queue.
I would love to land this feature, but we need to design it in a way that reduces complexity first.

@zhongwuzw
Copy link
Contributor Author

@shergin
Hi :) I may need your some inputs, is this the right direction you wanted? 🤔

@cpojer
Copy link
Contributor

cpojer commented Jun 5, 2019

I thought about this some more and we have a ton of code that on the backend expects maxLength to adhere to the real length and not the human readable length. Changing the default now and only for iOS will likely cause a lot of issues for us at Facebook and others.

If we want to solve this I think we may want to add another prop that explicitly specifies the max length is based on the glyph count instead of the number of bytes and make people opt-in to use that prop in the cases where the human readable length matters to them.

@cpojer cpojer closed this Jun 5, 2019
@zhongwuzw
Copy link
Contributor Author

@cpojer 👌 We already improved TextInput so much, let's wait to see wether any user wants it, then we can add it. :)

facebook-github-bot pushed a commit that referenced this pull request Mar 24, 2022
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextInput maxlength cuts Unicode caracters (Emojis)
6 participants