Skip to content

feat: Manage keyboard shortcuts visibility of TextInput #47671

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

Closed

Conversation

rezkiy37
Copy link
Contributor

@rezkiy37 rezkiy37 commented Nov 18, 2024

Summary:

iOS does offer a native property for UITextField called inputAssistantItem. According to the documentation, we can hide the "shortcuts" by setting the leadingBarButtonGroups and trailingBarButtonGroups properties to empty arrays.

I propose adding a new property for TextInput in React Native, which would set these native properties to empty arrays. This new property could be called disableInputAssistant or disableKeyboardShortcuts and would be a boolean.

Developers can manage this behavior (the redo & undo buttons and suggestions pop-up hiding) after applying these native props.

react-native-community/discussions-and-proposals#830

Changelog:

[IOS] [ADDED] - [TextInput] Integrate a new property - disableKeyboardShortcuts. It can disable the keyboard shortcuts on iPads.

Test Plan:

Manual

  1. Open TextInput examples.
  2. Scroll down and reach the "Keyboard shortcuts" section.
  3. Test each case.

Note: TextInput behaves the same as now when the new prop is not passed or is false.

Example.mp4

@cipolleschi
Copy link
Contributor

Sorry for the delay. I'm about to log off for a week of PTO.

I'll have a look at this when I'm back, unless some of my colleagues take care of that before.

@cortinico
Copy link
Contributor

unless some of my colleagues take care of that before.

I've done a quick pass and overall the code looks good to me. However we'll have to wait for @cipolleschi to be back next week for a final pass as iOS expert

@cipolleschi
Copy link
Contributor

@rezkiy37 Thanks for the proposal and for the PR. Can I ask you what's your use case, specifically?
I read through the proposal and the PR, but this seems a very niche API that:

  1. we don't use internally
  2. we would have to maintain
  3. we can't ensure it won't break in the future (due to 1.)
  4. is very iOS specific, what happens on Android is the property is set? What if Android will present a similar API in the future and people will start relying on the behavior that it is a no-op for Android?

@rezkiy37
Copy link
Contributor Author

rezkiy37 commented Dec 5, 2024

@rezkiy37 Thanks for the proposal and for the PR. Can I ask you what's your use case, specifically? I read through the proposal and the PR, but this seems a very niche API that:

  1. we don't use internally
  2. we would have to maintain
  3. we can't ensure it won't break in the future (due to 1.)
  4. is very iOS specific, what happens on Android is the property is set? What if Android will present a similar API in the future and people will start relying on the behavior that it is a no-op for Android?

Hello @cipolleschi!
Thank you for reviewing the proposal and PR. Our use case is well demonstrated in the video below:

372854999-dbd0e4ef-066c-462a-8ca1-828d212a5808.mp4

As shown, the user cannot effectively interact with buttons on the iPad when the input is focused, but the native keyboard is hidden. This is why I propose introducing this new API to address such scenarios.

Regarding your third point, I understand the concern about maintainability. However, this PR introduces the new property with a default value of false, leaving it to developers to enable it only when necessary.

As for point 4, I agree that this is an iOS-specific edge case, but it is one that still needs to be handled. To clarify, this issue does not occur on Android, as shown in the video below:

Video

Android.mp4

What if Android will present a similar API in the future...

In that case, we could extend support for Android, enhancing the UX for both platforms.

...people will start relying on the behavior that it is a no-op for Android?

This property would be clearly marked as iOS-specific, similar to existing properties like clearButtonMode and enablesReturnKeyAutomatically.

Thank you again for your time and consideration!

@cipolleschi
Copy link
Contributor

Hi there, CI is red. Could you please rebase on main and update the pr to fix the test_js jobs? 🙏

@rezkiy37
Copy link
Contributor Author

@cipolleschi, please approve the workflow 🙂

@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.

@cipolleschi
Copy link
Contributor

JS tests are red. :(

@cipolleschi
Copy link
Contributor

the public API file must be updated

@rezkiy37
Copy link
Contributor Author

@cipolleschi, I've updated the public API snapshot - 9e22ee0.

@rezkiy37
Copy link
Contributor Author

rezkiy37 commented Dec 19, 2024

test and test-ci have been completed successfully for me locally.

Screenshot 2024-12-19 at 20 41 34 Screenshot 2024-12-19 at 20 47 56

@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.

@rezkiy37
Copy link
Contributor Author

Hi!
I am writing to let you know about my upcoming vacation until the 7th Tuesday of January. I will resume my work on the issue when I get back.

Wishing you a Happy New Year!

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 6, 2025
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 0154372.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @rezkiy37 in 0154372

When will my fix make it into a release? | How to file a pick request?

@rezkiy37
Copy link
Contributor Author

rezkiy37 commented Jan 9, 2025

I've opened a PR to update the doc - facebook/react-native-website#4434.

@cipolleschi
Copy link
Contributor

@rezkiy37 that's great, thank you for following up on this and improving our docs!

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. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants