Skip to content

feat: fix show soft input on focus behavior (#35244)#52

Closed
zzz0108 wants to merge 0 commit intoExpensify:ExpensifyRC1-0.72.0-alpha.0from
zzz0108:cherry-pick/fix/show-soft-input-on-focus-behavior-ios
Closed

feat: fix show soft input on focus behavior (#35244)#52
zzz0108 wants to merge 0 commit intoExpensify:ExpensifyRC1-0.72.0-alpha.0from
zzz0108:cherry-pick/fix/show-soft-input-on-focus-behavior-ios

Conversation

@zzz0108
Copy link

@zzz0108 zzz0108 commented Apr 14, 2023

Upstream PR Link

facebook#35244

I'm cherry-picking this commit from the React Native upstream repo, main branch. This is tagged for the 0.72.0 release so should be out soon.

This is needed so we can move forward with this issue.

Summary

The showSoftInputOnFocus of TextInput is not working properly. When we set showSoftInputOnFocus to false and subsequently to true, it fails to open the keyboard, we have to re-focus on the input in order for the keyboard to show. It's expected that the keyboard will be opened as soon as showSoftInputOnFocus becomes true. This happens on iOS only.
Reference in React Native doc: https://reactnative.dev/docs/textinput#showsoftinputonfocus.

Changelog

[iOS] [Fixed] - Fix issue where keyboard does not open when TextInput showSoftInputOnFocus changes from false to true

Test Plan

I've made a clean create-react-native-app repo to demonstrate this https://github.com/christianwen/reduced-rn-app.
Here's the steps:

  1. Clone https://github.com/christianwen/reduced-rn-app
  2. Run yarn install
  3. Run cd ios, then pod install, then cd ..
  4. Run yarn ios
  5. See the text input on the top of the screen, it can be observed that the keyboard does not open because showSoftInputOnFocus is false, 3 seconds later it becomes true due to a setTimeout that is used

Screenshot 2022-11-07 at 23 39 50

Screen.Recording.2022-11-07.at.23.42.11.mov

However, it can be seen that the keyboard does not open even though showSoftInputOnFocus becomes true
6. Now add the change in this PR to Libraries/Text/TextInput/RCTBaseTextInputView.m
7. Run yarn ios again
8. Now verify the step 5 again, the keyboard will open automatically when showSoftInputOnFocus becomes true

Simulator.Screen.Recording.-.iPhone.14.-.2022-11-07.at.23.32.01.mp4

The reason why I created a fresh RN repo instead of using rn-tester app is because the showSoftInputOnFocus example is not working in rn-tester app for some reasons (it shows the keyboard even though showSoftInputOnFocus is false in the example).

Regarding the code, it's similar to the fix for keyboardType in facebook@8baaacb.

Screenshot 2022-11-07 at 23 36 41

@Santhosh-Sellavel
Copy link

@christianwen Can you follow the PR tempate and update it.

@zzz0108
Copy link
Author

zzz0108 commented Apr 15, 2023

@christianwen Can you follow the PR template and update it.

@Santhosh-Sellavel done! thanks for highlighting

@Santhosh-Sellavel
Copy link

cc: @iwiznia

Copy link

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

LGTM, all yours @iwiznia.

@Santhosh-Sellavel
Copy link

@iwiznia bump again!

@iwiznia
Copy link

iwiznia commented Apr 17, 2023

Yes yes, I am on it, I just don't work on weekends 😄

iwiznia
iwiznia previously approved these changes Apr 17, 2023
@Santhosh-Sellavel
Copy link

Ah, it was on weekend right 😅 !

@iwiznia
Copy link

iwiznia commented Apr 17, 2023

This failed
image

@Santhosh-Sellavel
Copy link

cc: @christianwen

@Santhosh-Sellavel
Copy link

@iwiznia Reinitiate the workflow, also changes didn't touch anything specific to android

@iwiznia
Copy link

iwiznia commented Apr 17, 2023

ok, re-running. If it fails, let's try merging main to this branch

@iwiznia
Copy link

iwiznia commented Apr 17, 2023

It failed again... @christianwen can you try merging main into this branch please?

@zzz0108
Copy link
Author

zzz0108 commented Apr 18, 2023

@iwiznia it says "Already up to date." when I try to merge main into my branch. Also the latest update on main was from Jan 19, while I branched out from main only a few days ago so I think my branch is up to date.

@iwiznia
Copy link

iwiznia commented Apr 18, 2023

Ah then no idea, can you investigate the failure?

@zzz0108
Copy link
Author

zzz0108 commented Apr 19, 2023

@iwiznia hmm I'm not familiar to the CI setup here and also I don't think we're touching any Android part in this PR.

Just confirming is 'main' the right branch to merge to? I see some recent merged PRs are merged into release branches, not main. For example this PR #46.

@iwiznia
Copy link

iwiznia commented Apr 19, 2023

hmmm good question, maybe not? I am not sure... @roryabraham I see you on that other PR, is [Expensify:Expensify-0.71.2-alpha.2](https://github.com/Expensify/react-native/tree/Expensify-0.71.2-alpha.2) f the right branch to target?

@roryabraham
Copy link

@iwiznia Now the correct branch to target should be ExpensifyRC1-0.72.0-alpha.0

@Santhosh-Sellavel
Copy link

@christianwen can you change the target branch then?

@zzz0108 zzz0108 changed the base branch from main to ExpensifyRC1-0.72.0-alpha.0 April 21, 2023 02:46
@zzz0108 zzz0108 changed the base branch from ExpensifyRC1-0.72.0-alpha.0 to main April 21, 2023 02:46
@zzz0108 zzz0108 dismissed stale reviews from iwiznia and Santhosh-Sellavel April 21, 2023 02:46

The base branch was changed.

@zzz0108 zzz0108 changed the base branch from main to ExpensifyRC1-0.72.0-alpha.0 April 21, 2023 14:32
@zzz0108 zzz0108 closed this Apr 21, 2023
@zzz0108 zzz0108 force-pushed the cherry-pick/fix/show-soft-input-on-focus-behavior-ios branch from faadd01 to 33ce1f4 Compare April 21, 2023 14:34
@zzz0108
Copy link
Author

zzz0108 commented Apr 21, 2023

If ExpensifyRC1-0.72.0-alpha.0 is the correct branch then the commit I intended to cherry-pick here is already included in that branch. But I see we're not using the 0.72.0 version in our Expensify/App, we're using 0.71.2, so I doubt ExpensifyRC1-0.72.0-alpha.0 is the right branch if we want the commit to be used by the app. @roryabraham Am I missing something?

@roryabraham
Copy link

We are working to upgrade E/App to 0.72.0 as soon as we can.

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

Successfully merging this pull request may close these issues.

4 participants