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

[HOLD] Improve soft keyboard padding fix with native solution #10648

Closed
wants to merge 10 commits into from

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Aug 29, 2022

Details

To resolve a fire and ensure we could deploy mobile while in Santiago, we merged a fix for a keyboard padding issue that depends on a Platform conditional.

This PR reverts the original changes and resolves the issue with an improved native fix:

The idea behind this PR is that the underlying cause of the original issue was that both RN and the Android OS were trying to handle the showing/hiding of the soft keyboard. This solution prevents Android from doing this in every single case, ensuring the keyboard control is delegated to RN and the KeyboardAvoidingView.

The native Android windowSoftInputMode attribute is explained here. We were using adjustResize, which would always attempt to resize the Activities View and to make space for the keyboard, whereas adjustUnspecified is the default behavior that uses the context of the underlying view. It's important to note that the Android OS will still reduce the Activities View height to make space for the keyboard when RN descides to show/hide it (this is desired, as the alternative is that the entire view moves up to make room for the keyboard). But now, the OS is not forcing the extra padding.

Fixed Issues

$ #10539

Tests

Please test Android and iOS native apps and mWeb with a physical mobile device

I have tested on:

  • Physical Nexus 5, Android 8.1
  • Physical Pixel 6a, Android 13
  • Emulated Pixel 2, Android 8.1
  • Simulated iPhone 13, iOS 16.0
  • Physical iPhone 7, 13.3

1) Ensure the original issue is fixed

  • Create a chat with a user you have not chatted with before
  • Tap the input field
  • The keyboard should be displayed correctly
  • Close the keyboard
  • The keyboard should hide correctly

2) Ensure the keyboard works in other inputs throughout the app

  • Test the app features (registration/sign in/settings/workspace/new chats/ect) ad ensure the soft keyboard works as expected
  • Applause should run the full regression suite instead

3) Verify that no errors appear in the JS console

Phsyical Device screencapture
Click to view -- not sure why it's not embedded
https://user-images.githubusercontent.com/10736861/193265622-fc8eb721-5db1-450b-a4aa-3287e5c62980.mp4

image2

Emulator Screenshots
Screenshot_1661437808 Screenshot_1661437843
Screenshot_1661437855

@Julesssss
Copy link
Contributor Author

Next step is to compile a build and attach it here for Applause to test.

@Julesssss
Copy link
Contributor Author

Reminder to self that this change might actually fix this other issue

@tgolen
Copy link
Contributor

tgolen commented Sep 19, 2022

What's the status of this PR? Can we get it merged and deployed?

@Julesssss
Copy link
Contributor Author

This is low priority at the moment. But I will return to this. Next step is to compile release builds and test a few of the attributes manually, but this takes a lot of time.

@tgolen
Copy link
Contributor

tgolen commented Sep 29, 2022

@Julesssss this is actually turning out to be pretty high priority for me since I'm looking into simplifying/improving our KeyboardAvoidingView behaviors. Is there anything I can do to help? I've not ever compiled release builds before. Are these the SOs?

https://stackoverflow.com/c/expensify/questions/7311
https://stackoverflow.com/c/expensify/questions/8290

What do you mean by "test a few of the attributes manually"?

@Julesssss Julesssss force-pushed the jules-nativeFixForKeyboardPaddingIssue branch from 2476398 to f9a00dc Compare September 30, 2022 10:30
@Julesssss
Copy link
Contributor Author

I found some time this morning to finish up this PR and I tested it on three Android devices:

  • Physical Nexus 5, Android 8.1
  • Physical Pixel 6a, Android 13
  • Simulator Pixel 2, Android 8.1

Is there anything I can do to help? I've not ever compiled release builds before. Are these the SOs?

It would be great if you could test a native iOS and Android build, and yeah, those are the correct stack overflow posts. Please let me know if you have any further questions here!

What do you mean by "test a few of the attributes manually"?

The idea behind this fix is that we tell the OS not to automatically show/hide the soft-keyboard, but this solution requires that we use one of these attributes on the MainActivity and I wasn't 100% sure that I had made the correct choice. However, after testing I'm happy that adjustUnspecified` is correct.

@Julesssss
Copy link
Contributor Author

CC-ing a few people who have context about the original changes that this native fix reverts: @roryabraham @marcaaron @Luke9389

@Julesssss Julesssss marked this pull request as ready for review September 30, 2022 12:05
@Julesssss Julesssss requested a review from a team as a code owner September 30, 2022 12:05
@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team September 30, 2022 12:05
@Julesssss Julesssss requested review from tgolen and a team September 30, 2022 12:05
@melvin-bot melvin-bot bot requested review from sketchydroide and removed request for a team September 30, 2022 12:05
@tgolen
Copy link
Contributor

tgolen commented Sep 30, 2022

That worked! Thank you

Luke9389
Luke9389 previously approved these changes Sep 30, 2022
@Julesssss
Copy link
Contributor Author

@tgolen I should have realized this on Friday, but it looks like you tested the release build on a simulator -- in which case it's expected that the keyboard won't display 😅 Given the context of the PR it was a good thing to raise though.

Anyway, after testing I can confirm the keyboard works as expected on a physical device. Added iPhone 7 to the list of devices I have tested.

I think this is ready for further testing, or to be merged now.

# Conflicts:
#	src/pages/home/ReportScreen.js
@Julesssss Julesssss dismissed stale reviews from Luke9389 and sketchydroide via 5166d5c October 3, 2022 11:49
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I think that's some pretty good testing. I'll go ahead and merge this

@tgolen
Copy link
Contributor

tgolen commented Oct 3, 2022

Actually, holding off to get a C+ review

@Julesssss
Copy link
Contributor Author

Actually, holding off to get a C+ review

Requested a C+ review here. Awaiting the review/confirmation.

@thesahindia
Copy link
Member

@Julesssss, I was able to repro the blank space issue after applying changes.

Steps:-

  1. Search new user
  2. Keep the keyboard open
  3. Tap on the contact
video_20221004_043659_edit.mp4

@Julesssss
Copy link
Contributor Author

@Julesssss, I was able to repro the blank space issue after applying changes.

Damn it. Thank you for testing 😞

I recall this initially being inconsistently reproducible, but I'll try to see if I can find the cause. I have a feeling it's related to the type of component.

@tgolen
Copy link
Contributor

tgolen commented Oct 4, 2022

Is that reproducible on any Android? I haven't had the time yet, but I'd be really curious to see if my changes in #11586 will fix that or not.

@thesahindia
Copy link
Member

Is that reproducible on any Android?

Yeah, it was reproducible for iqoo z5. (haven't tested on other android devices)

but I'd be really curious to see if my changes in #11586 will fix that or not.

I will try testing it again with your changes.

@Julesssss
Copy link
Contributor Author

I'd be really curious to see if my changes in #11586 will fix that or not.

Oh good point. I'll also test this PR against your changes

@tgolen
Copy link
Contributor

tgolen commented Oct 10, 2022

What were the results of the testing? Any luck? What's the next step here for this PR?

@thesahindia
Copy link
Member

What were the results of the testing? Any luck? What's the next step here for this PR?

Same results. It's still happening
And as mentioned here, we are seeing one new issue at login page.

@tgolen
Copy link
Contributor

tgolen commented Oct 10, 2022

Ah sorry, I got the comments mixed up. Bummer that my PR doesn't fix the empty space issue :(

@Julesssss
Copy link
Contributor Author

Ah sorry, I got the comments mixed up. Bummer that my PR doesn't fix the empty space issue :(

Yeah... I feel like we're not too far off the underlying issue here, but I'm just not able to spend any further time on this at the moment.

@Julesssss Julesssss changed the title Improve soft keyboard padding fix with native solution [HOLD] Improve soft keyboard padding fix with native solution Oct 11, 2022
@PauloGasparSv
Copy link
Contributor

I could not reproduce this on iOS but it looks similar to this closed issue #11858 so I'm linking it here!

@tgolen
Copy link
Contributor

tgolen commented Oct 24, 2022

Last time I spoke with Jules about this, the solution here sounded pretty hopeless. We decided to go full-steam-ahead with this PR instead: #11586

I'm going to close this PR. We can reopen it later if we want to.

@tgolen tgolen closed this Oct 24, 2022
@roryabraham roryabraham deleted the jules-nativeFixForKeyboardPaddingIssue branch November 2, 2022 23:08
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.

8 participants