[IOS] Text replacement by dot after 2 spaces

[gedu]

Summary:

Pre-requisite:

• Ensure that the option "." Shortcut" is enabled in Settings->Keyboard.

This PR addresses several issues:

When using TextInput in an uncontrolled manner, without using the value prop, after entering a letter or word and double-tapping the spacebar, it should now correctly add a "." at the end of the letter or word.
When using TextInput in an uncontrolled manner, without using the value prop, after entering certain letters for text replacement, such as "omw" or "@@" (previously created), it now produces the expected replacements.

Motivation:

From Expensify app: Expensify/App#17153
From discussion on: #27693

Changelog:

[IOS][FIXED] - For uncontrolled TextInput: Double tapping space should add "." and Text Replacement for special characters

Test Plan:

TextInput:

"." shortcut

Pre-requisite:

• Ensure that the option "." Shortcut" is enabled in Settings->Keyboard.

• Using the onChangeText and the value props to set new values:

  • Writing a letter: like E then tapping the space 2 times, you should see E.
  • Writing a word: like Test then tapping the space 2 times, you should see Test.

Text Replacement

Pre-requisite:

• Create a new Text Replacement inside of Settings -> Keyboard, using symbols: like @@ -> [email protected]

• Using the onChangeText and the value props to set new values:

  • Writing the symbol created: @@ should be selected and clicking space, the @@ should be replaced by [email protected]

input_space_to_dot_working.mp4

[facebook-github-bot]

Hi @gedu!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[javache]

Could you provide an integration test that demonstrates how this interacts with UIKit? This piece of code is quite complex, and already has to do quite a bit of complex state management (eg see _comingFromJS, _ignoreNextTextInputCall)

Not updating the attributed string may cause issues if the text completion causes the text input to have to grow over multiple lines.

[gedu]

@javache Hey, yes I can, is there something already in place for TextInput?

[javache]

Unfortunately, I don't think we have any in place right now. You could look at adding them here: https://github.com/facebook/react-native/tree/main/packages/rn-tester/RNTesterIntegrationTests (but this may need some work to support the new architecture). You could also try an rn-tester-e2e test.

[gedu]

I will try to work on this beginning next week

[gedu]

I ran into some issues when updating main, next week I will start adding the test under rn-tester-e2e

[gedu]

Ok could set the first test, and is working, will keep adding more, and will try to add as much as I can.

Testing RewriteExample

input: test space replace
output: test_space_replace

[gedu]

During my testing. I observed that whenever I attempted to set a text, the second and third characters were getting cropped.

textInput_wrong_text_no_fix_in_place.mp4

So then I applied my fix on the Native realm (I wanted to make sure that all was working before my fix, but I couldn't make it work)

textInput_correct_with_fix.mp4

And it seems that it is working after running many times the tests

[gedu]

I updated my local and seems there were many changes and I need to find a new way to fix this issue, given that seems that isn't working now

[gedu]

Ok I could find the fix, seems that wasn't a big change after all, but only will work for fabric, but I'm double checking. Now I'm working on the testings

[gedu]

The integration tests are working on Android, but on iOS getting the text from the Input isn't working. I can add text but later I can't get it back from the Input. Looking into that

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	16,574,182	+4
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	19,955,218	-1
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e4708d6
Branch: main

[gedu]

@javache I added some tests. What do you think?

[gedu]

@javache friendly ping

[gedu]

@javache did you have a chance to look at this?

[mhoran]

In addition to fixing #27693, @gedu has confirmed this will also fix #29572 and the duplicate issue #42792. Thanks for fixing these long standing issues!

[zwilderrr]

hey team - can you merge? would love to get this in my app

[gedu]

@javache can you take a look at this or ping someone that can help us to review this PR?

[gedu]

I will fix the conflict soon

[zwilderrr]

Thanks!!!

…

On Fri, Jun 7, 2024 at 6:11 AM edug ***@***.***> wrote: I will fix the conflict soon — Reply to this email directly, view it on GitHub <#39385 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFJCPNV4IOVRCIFZSEO3FW3ZGGBO3AVCNFSM6AAAAAA4TKBPT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJUGUZDSMRVGM> . You are receiving this because you commented.Message ID: ***@***.***>

[javache]

Apologies for the delay here, and thank you for adding the test!!

Synchronizing state between JS and native is a really complex part of React Native and introducing new variables like _stateUpdated add additional complexity there. I'd love to see a more scoped fix with detailed explanation of how updateState ends-up becoming re-entrant.

The right fix is probably not in bailing out early inside updateState, but in whatever callback gets fired that triggers the re-entrant state update. This is already what _comingFromJS means ("delegate methods textInputDidChange and textInputDidChangeSelection will exit early"). Can we leverage that instead?

[gedu]

@javache I found the e2e tests were removed, I will remove them from here too: #45032

I'd love to see a more scoped fix with detailed explanation of how updateState ends-up becoming re-entrant.

Yeah, I would have to dig again to remember the exact flow.
The main issue is multiple calls and flow are happen almost at the same time and sometimes in different orders (depending if the change/update is for a controlled or uncontrolled TextInput)
I will look into so I can refresh my memory

[gedu]

@javache Looking a bit deeper, I found something in the _setAttributedString when using _textOf with value and onChangeText (controlled component). The if condition was false, so it was processing the text in another way. I saw that:
newText was

A{
    NSBackgroundColor = "UIExtendedSRGBColorSpace 0 0 0 0";
    NSColor = "UIExtendedSRGBColorSpace 0 0 0 1";
    NSFont = "<UICTFont: 0x165124130> font-family: \".SFUI-Regular\"; font-weight: normal; font-style: normal; font-size: 13.00pt";
}

oldText

A{
    NSBackgroundColor = "UIExtendedSRGBColorSpace 0 0 0 0";
    NSColor = "UIExtendedSRGBColorSpace 0 0 0 1";
    NSFont = "<UICTFont: 0x165124130> font-family: \".SFUI-Regular\"; font-weight: normal; font-style: normal; font-size: 13.00pt";
    NSParagraphStyle = "Alignment 4, LineSpacing 0, ParagraphSpacing 0, ParagraphSpacingBefore 0, HeadIndent 0, TailIndent 0, FirstLineHeadIndent 0, LineHeight 0/0, LineHeightMultiple 0, LineBreakMode 4, Tabs (\n    28L,\n    56L,\n    84L,\n    112L,\n    140L,\n    168L,\n    196L,\n    224L,\n    252L,\n    280L,\n    308L,\n    336L\n), DefaultTabInterval 0, Blocks (null), Lists (null), BaseWritingDirection 0, HyphenationFactor 0, TighteningForTruncation NO, HeaderLevel 0 LineBreakStrategy 65535 PresentationIntents (\n) ListIntentOrdinal 0 CodeBlockIntentLanguageHint '(null)'";
    NSShadow = "NSShadow {0, -1} color = {(null)}";
}

When not using value and onChangeText (uncontrolled component), both newText and oldText were the same.
I noticed that RCTNSAttributedStringFromAttributedStringBox was making a copy of the attributes, done by RCTNSTextAttributesFromTextAttributes. It checks if the attributes exist; if they do, it copies them. In the case of the controlled component, those attributes were missing, so nothing was copied, and when _textOf checked, it failed.
Here's what I did: After checking, all the attributes are set to default values before returning the copy:

 NSMutableAttributedString *mutableAttributedString = [[NSMutableAttributedString alloc]
        initWithString:string
            attributes:RCTNSTextAttributesFromTextAttributes(fragment.textAttributes)];
      // Apply default attributes if they're missing
    NSDictionary<NSAttributedStringKey, id> *defaultAttributes = RCTDefaultTextAttributes();
    for (NSAttributedStringKey key in defaultAttributes) {
      if (![mutableAttributedString attribute:key atIndex:0 effectiveRange:NULL]) {
        [mutableAttributedString addAttribute:key value:defaultAttributes[key] range:NSMakeRange(0, mutableAttributedString.length)];
      }
    }   
    return mutableAttributedString; 

That way it works. This way I don't have to add the new flag

What do you think?

spaceToReplace.mp4

[gedu]

@javache did you have time to look into the new proposal?

[gedu]

@javache friendly ping

[gedu]

Does the new proposal makes sense? Looks good?

[gedu]

@javache can you take a look at my new proposal, I'm not using a new flag

[gedu]

I'll dedicate time to fixing the conflicts and getting everything sorted

[javache]

Thanks for the ping. I'll try to find an owner to help review this internally, as it may be related to other auto-correct issues we've been seeing.

[gedu]

@javache Awesome, thanks, I pushed the code without the flag and just adding the missing fields, I think I need to clean up a bit more the code

[NickGerleman]

I suspect #46970 will fix this

In that change, I tried to avoid filling in specific attributes like this, since their presence is an implementation detail of UITextField that may change in the future (and I'm not sure that UITextView and others would have the same). Text (non-input) we probably don't want these either. Comparison insensitive to default typing attributes helps make this more robust for the future (or... other inputs), but was something we are still planning to add UI tests for internally to catch in future iOS updates if they break things.

NSParagraphStyle after auto-correction in backing input will also change to non-default version, and alignment and writing direction will resolve from natural to either left or right depending on locale, so this fix would not work as user continues to type.

When I was looking at this, we also had different EventEmitter attributes often, which we handle now in RCTTextInputComponentView, but is part of the same class of issue.

[gedu]

Closing this PR in favor of #46970