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

[0.71] Web Prop update caused unexpected breaking changes #36229

Closed
Titozzz opened this issue Feb 21, 2023 · 18 comments
Closed

[0.71] Web Prop update caused unexpected breaking changes #36229

Titozzz opened this issue Feb 21, 2023 · 18 comments

Comments

@Titozzz
Copy link
Collaborator

Titozzz commented Feb 21, 2023

Description

Following the work from #34424, there was some (unexpected?) breaking changes to <TextInput />.

Before there were 2 props:

  • autoComplete => android (name-family, name-given, etc)
  • textContentType => iOS (familyName, givenName, etc)

Now due to the changes, there is still 2 props:

  • autoComplete => android & iOS
  • textContentType => iOS

But the new implementation looks like this:

There is a new universal set of keys (some of them identical to the old one, some news)
When you set autoComplete:

  • android: check if name is in the new set of keys, if so map to the android value, else pass it directly to android (compatibility with old keys)
  • iOS: check if the name is in the set of keys, if so map to the iOS value, else if textContentType is defined, use it.

What's breaking is that for example if you used autoComplete = "A" and textContentType="B", on 0.71 it will be treated as textContentType="A" on 0.71

How we could have mitigated this:
-> Deprecate and set warnings on textContentType but still use it first in the implementation so that user migrate without breaking their apps

Version

0.71

Output of npx react-native info

/

Steps to reproduce

Use TextInput with autoComplete and textContentType

Snack, code example, screenshot, or link to a repository

/

@gabrieldonadel
Copy link
Collaborator

cc: @necolas

@necolas
Copy link
Contributor

necolas commented Feb 22, 2023

if you used autoComplete = "A" and textContentType="B", on 0.71 it will be treated as textContentType="A" on 0.71

If you use autoComplete = "A" it should be mapped to whatever the corresponding value of textContentType should be, which isn't necessarily "A". Hard to say anything more without a concrete example of the issue you're experiencing

@NickGerleman
Copy link
Contributor

If both props were here before, and we repurposed one of them without documenting it as a breaking change, I think we messed up here.

I.e. the web prop precedence should only come into effect for someone using a newly introduced prop.

@NickGerleman
Copy link
Contributor

It seems like this change effectively breaks any previous xplat usage of the prop, so we should patch this imo, and make the break later if we want to make it.

@necolas
Copy link
Contributor

necolas commented Feb 22, 2023

We added values to autoComplete and automatically map them to corresponding iOS values, if the autoComplete value is not recognized or not a web value, we use the provided iOS value. The OP needs to explain what specific values are causing issues.

@NickGerleman
Copy link
Contributor

It seems like this is the scenario:

What's breaking is that for example if you used autoComplete = "A" and textContentType="B", on 0.71 it will be treated as textContentType="A" on 0.71

So, if Android and iOS disagreed before. Which is, more scoped, but still technically breaking.

@necolas
Copy link
Contributor

necolas commented Feb 23, 2023

Yes but A is not a specific value. In what scenarios does the same text input need to have completely different autocomplete values across platforms?

@Titozzz
Copy link
Collaborator Author

Titozzz commented Feb 23, 2023

An example with cc-number, if we have an input that can take credit card information or bank account numbers.
On android, setting cc-number will only impact the suggestions but we do not want to use it on iOS and explicitly set it to none because if you use cc-number on iOS, the keyboard changes and restrains the user from using letters, only numbers.

There you have a regression that was unexpected!

@Titozzz
Copy link
Collaborator Author

Titozzz commented Feb 23, 2023

While I understand this is niche and easily fixable dev side, I'm wondering why this approach was chosen, since the non-breaking alternative strategy (using the textContentType first and warn) seemed easier to me (and would also create discovery for the new capabilities)

@necolas
Copy link
Contributor

necolas commented Feb 23, 2023

the keyboard changes and restrains the user from using letter

When do you need to input letters for a credit card number?

I'm wondering why this approach was chosen

It's consistent with what we did for dozens of other props and styles.

@Titozzz
Copy link
Collaborator Author

Titozzz commented Feb 28, 2023

When do you need to input letters for a credit card number?

if we have an input that can take credit card information or bank account numbers.

Bank account have letters in them!

@rickhanlonii
Copy link
Member

rickhanlonii commented Feb 28, 2023

@necolas interesting, can you list all the props we've done this for? I know about the props where the new one takes precedence over the old prop (only impacts new code), but I didn't follow close enough to see the props where we added support for an existing prop on one platform to both platforms (changing the behavior of existing code).

@necolas
Copy link
Contributor

necolas commented Feb 28, 2023

Bank account have letters in them!

Bank account should not use cc-number as that is designed only for credit card numbers, which is why the native keyboard limits it to only numbers.

There is an open HTML issue for adding an autocomplete type for bank accounts, but in the meantime you are expected to use free text entry. On web you can also pattern match to limit to numbers and letters, but React Native doesn't have support for declarative patterns.

whatwg/html#1448

@Titozzz
Copy link
Collaborator Author

Titozzz commented Mar 1, 2023

@necolas I know, but it was a shared input for two data types, that's probably a bad idea but that's what is was. And so have autocomplete for half the data was better than nothing I guess. 🙄

Once again this is just a silly example revealing something that happened. I don't really want to slow down the effort to migrate to universal props nor argue about the pertinence of old code from our codebase.

The only goal was to show some this was a breaking behaviour while offering an alternative 😄

@lunaleaps
Copy link
Contributor

lunaleaps commented Mar 15, 2023

Hey guys for an update, @rickhanlonii, @necolas and I met up to talk about this and the general strategy going forward.

For this case particularly, I am going to:

  • put out a fix that makes textContentType take precedence on iOS and make a patch for 0.71 and ensure its in 0.72
  • Update our documentation for textContentType that it's just an alias for autoComplete but not to use both.

In our discussion we would like to address this one-off and don't see this as a wide-spread issue (as-in, it likely isn't an issue for other prop aliases we've added). In future efforts around conformance we're going to be more intentional in making block changes behind gating flags so we don't get these paper-cuts for un-intentional adoption.

cc @NickGerleman
edit: also thanks @Titozzz for bringing this valuable point up!

facebook-github-bot pushed a commit that referenced this issue Mar 16, 2023
Summary: Changelog: [iOS][Changed] - Give precedence to `textContentType` property for backwards compat as mentioned in #36229 (comment)

Reviewed By: necolas

Differential Revision: D44106291

fbshipit-source-id: 5702d7f171735d1abe6cfbc9ca1ad8f21751d51e
kelset pushed a commit that referenced this issue Mar 28, 2023
Summary: Changelog: [iOS][Changed] - Give precedence to `textContentType` property for backwards compat as mentioned in #36229 (comment)

Reviewed By: necolas

Differential Revision: D44106291

fbshipit-source-id: 5702d7f171735d1abe6cfbc9ca1ad8f21751d51e
@Titozzz
Copy link
Collaborator Author

Titozzz commented Mar 29, 2023

Thanks

@Titozzz Titozzz closed this as completed Mar 29, 2023
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this issue May 22, 2023
Summary: Changelog: [iOS][Changed] - Give precedence to `textContentType` property for backwards compat as mentioned in facebook#36229 (comment)

Reviewed By: necolas

Differential Revision: D44106291

fbshipit-source-id: 5702d7f171735d1abe6cfbc9ca1ad8f21751d51e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants
@necolas @NickGerleman @lunaleaps @rickhanlonii @cortinico @Titozzz @gabrieldonadel and others