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

feat: Add inputMode prop to TextInput component #34460

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Aug 19, 2022

Summary

This adds the inputMode prop to the TextInput component as requested on #34424, mapping web inputMode types to equivalent keyboardType values. This PR also updates RNTester TextInputExample in order to facilitate the manual QA.

Caveats

This only adds support to text, decimal, numeric, tel, search, email, and url types.

inputMode="none"

Currently mapped to default keyboard type.

The main problem with this input mode is that it's not supported natively neither on Android or iOS. Android TextView does accept none as android:inputType but that makes the text not editable, which wouldn't really solve our problem. UITextInput on iOS on the other hand doesn't even have something similar to avoid displaying the virtual keyboard.

If we really want to add the support for inputMode="none" one interesting approach we could take is to do something similar to what WebKit has done (WebKit/WebKit@3b5f0c8). In order to achieve this behavior, they had to return a UIView with a bounds of CGRectZero as the inputView of the WKContentView when inputmode=none is present.
I guess the real question here should be, do we really want to add this? Or perhaps should we just map inputMode="none" to keyboardType="default"

Android docs: https://developer.android.com/reference/android/widget/TextView#attr_android:inputType
iOS docs: https://developer.apple.com/documentation/uikit/uikeyboardtype?language=objc

inputMode="search" on Android

Currently mapped to default keyboard type.

Android TextView does not offers any options like UIKeyboardTypeWebSearch on iOS to be used as search with android:inputType and that's probably the reason why keyboardType="web-search" is iOS only. I checked how this is handled on the browser on my Android device and it seems that Chrome just uses the default keyboard, maybe we should do the same?

Open questions

  • What should be done about inputMode="none"? Add it and map it to default keyboard type.
  • Which keyboard should we show on Android when inputMode="search"? Use the default keyboard the same way Chrome does

Changelog

[General] [Added] - Add inputMode prop to TextInput component

Test Plan

  1. Open the RNTester app and navigate to the TextInput page
  2. Test the TextInput component through the Input modes section
Screen.Recording.2022-08-19.at.16.16.10.mov

@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 Aug 19, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 19, 2022
@facebook-github-bot 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 Aug 19, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 19, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,618,315 +445
android hermes armeabi-v7a 7,032,564 +440
android hermes x86 7,918,371 +441
android hermes x86_64 7,891,987 +436
android jsc arm64-v8a 9,495,611 +161
android jsc armeabi-v7a 8,273,233 +170
android jsc x86 9,433,404 +174
android jsc x86_64 10,026,379 +161

Base commit: 8c882b4
Branch: main

@analysis-bot
Copy link

analysis-bot commented Aug 19, 2022

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

Base commit: 8c882b4
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@necolas
Copy link
Contributor

necolas commented Aug 22, 2022

'none' => 'default' (or noop) sounds right.

| 'tel'
| 'search'
| 'email'
| 'url';
Copy link
Contributor

Choose a reason for hiding this comment

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

'none' should be added to the allowed values even if we do nothing with it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, I've just added none there and mapped it to default

@gabrieldonadel gabrieldonadel force-pushed the feat/add-input-mode-to-text-input branch from 11fb0b7 to e5f7e1a Compare August 24, 2022 01:59
@yungsters
Copy link
Contributor

Which keyboard should we show on Android when inputMode="search"?

Doing the same thing as Chrome seems reasonable. So inputMode="search" should display the default keyboard.

@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel gabrieldonadel force-pushed the feat/add-input-mode-to-text-input branch from e5f7e1a to 69f4b33 Compare August 24, 2022 18:10
@gabrieldonadel
Copy link
Collaborator Author

@cipolleschi I've just rebased this now that CI has been fixed

@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel gabrieldonadel force-pushed the feat/add-input-mode-to-text-input branch from 69f4b33 to 20fe307 Compare August 30, 2022 01:35
@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel gabrieldonadel deleted the feat/add-input-mode-to-text-input branch August 30, 2022 16:11
@gabrieldonadel
Copy link
Collaborator Author

@necolas @cipolleschi any thoughts on why this didn't get labeled as merged?

@necolas
Copy link
Contributor

necolas commented Nov 6, 2022

Apparently inputMode=none could be mapped to showSoftInputOnFocus

See necolas/react-native-web#2421

@gabrieldonadel
Copy link
Collaborator Author

Apparently inputMode=none could be mapped to showSoftInputOnFocus

See necolas/react-native-web#2421

Sounds good to me, gonna open a follow-up PR updating this

@gabrieldonadel
Copy link
Collaborator Author

Apparently inputMode=none could be mapped to showSoftInputOnFocus
See necolas/react-native-web#2421

Sounds good to me, gonna open a follow-up PR updating this

Here it goes @necolas #35228

facebook-github-bot pushed a commit that referenced this pull request Nov 8, 2022
#35228)

Summary:
This PR updates `inputMode` prop from the `TextInput` component to map the `none` option to `showSoftInputOnFocus={false}`  as suggested by necolas here -> #34460 (comment). This change makes the inputMode API behaves a bit more similarly across platforms.

Related to necolas/react-native-web#2421

## Changelog

[General] [Changed] -  Update TextInput inputMode to map "none" to showSoftInputOnFocus

## Test Plan

1. Open the RNTester app and navigate to the TextInput page
2. Test the `TextInput` component through the `Input modes` section

https://user-images.githubusercontent.com/11707729/200218435-6a33b319-e989-4086-aac3-506546982b38.mov

Pull Request resolved: #35228

Reviewed By: lunaleaps, necolas

Differential Revision: D41081876

Pulled By: jacdebug

fbshipit-source-id: cc634c3723647d8950bf2cfe67be70d0fbd488a6
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
facebook#35228)

Summary:
This PR updates `inputMode` prop from the `TextInput` component to map the `none` option to `showSoftInputOnFocus={false}`  as suggested by necolas here -> facebook#34460 (comment). This change makes the inputMode API behaves a bit more similarly across platforms.

Related to necolas/react-native-web#2421

## Changelog

[General] [Changed] -  Update TextInput inputMode to map "none" to showSoftInputOnFocus

## Test Plan

1. Open the RNTester app and navigate to the TextInput page
2. Test the `TextInput` component through the `Input modes` section

https://user-images.githubusercontent.com/11707729/200218435-6a33b319-e989-4086-aac3-506546982b38.mov

Pull Request resolved: facebook#35228

Reviewed By: lunaleaps, necolas

Differential Revision: D41081876

Pulled By: jacdebug

fbshipit-source-id: cc634c3723647d8950bf2cfe67be70d0fbd488a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants