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] Going back to main list after keyboard call won't close keypad #2150

Closed
isagoico opened this issue Mar 30, 2021 · 19 comments · Fixed by #2221
Closed

[HOLD] Going back to main list after keyboard call won't close keypad #2150

isagoico opened this issue Mar 30, 2021 · 19 comments · Fixed by #2221
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

Keyboard will be dismissed after user quits conversation

Actual Result:

Keyboard keeps on top even after going back to chats list; using it will actually send message to latest chat

Action Performed:

  1. Open app and login
  2. Open any chat and tap message field in order to open keyboard
  3. Go back to chats list
  4. Keep using the keyboard
  5. Go back to previous conversation

Workaround:

Unknown

Platform:

Where is this issue confirmed?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.7-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
Device: iPhone SE 2020 / iOS 14.4

Video

image

Expensify/Expensify Issue URL:

@aliabbasmalik8
Copy link
Contributor

The solution is just call Keyboard.dismiss(); onPress backArrow in HeaderView component

@marcaaron marcaaron added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @NicMendonca (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Mar 31, 2021
@NicMendonca NicMendonca changed the title Going back to main list after keyboard call won't close keypad [Chat] Going back to main list after keyboard call won't close keypad Mar 31, 2021
@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Mar 31, 2021
@NicMendonca NicMendonca removed their assignment Mar 31, 2021
@NicMendonca NicMendonca added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Mar 31, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/7703 for more details.

@kevinksullivan
Copy link
Contributor

Posted to upwork:

https://www.upwork.com/jobs/~016e7e7cc383ab17f6

Switching this to the expensify repo per a conversation I had with @NicMendonca

@SameeraMadushan
Copy link
Contributor

Hi,

Proposal

  • Dismissing the keyboard when go back will fix the issue.
  • In the <HeaderView> component back button I will add the keyboard dismiss as following.
                    <Pressable
                        onPress={() => {
                            props.onNavigationMenuButtonClicked();
                            Keyboard.dismiss();
                        }}
                        style={[styles.LHNToggle]}
                    >

Thanks.

@MelvinBot
Copy link

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mallenexpensify
Copy link
Contributor

@chiragsalian you were auto-assigned, can you please review proposals and follow steps here? https://stackoverflow.com/c/expensify/questions/7676

@kidroca
Copy link
Contributor

kidroca commented Apr 3, 2021

Hey, @mallenexpensify I've commented on this in slack

My current PR is covering this issue (check out the video if you'd like)

I'm not using Keyboard.dismiss(); though - the compose input component is not mounted at all when it is covered by the drawer. So when the drawer opens the Keyboard would disappear as there is no longer a focused field

@marcaaron
Copy link
Contributor

Just a heads up I'm actually going to close this issue so we can handle all issues related to the keyboard in a single PR. There is a lot of discussion and a lot of confusion about which PRs will fix what so rather than have yet another person addressing this behavior we should close this. Thanks!

@marcaaron
Copy link
Contributor

Actually just going to put this on HOLD instead. My main concern is having another contributor look into the same issue as we have plenty of eyes on it already.

@marcaaron marcaaron reopened this Apr 6, 2021
@marcaaron marcaaron changed the title [Chat] Going back to main list after keyboard call won't close keypad [HOLD] Going back to main list after keyboard call won't close keypad Apr 6, 2021
@marcaaron
Copy link
Contributor

We can close this once the issue is resolved.

@marcaaron
Copy link
Contributor

Going to reopen this as @kidroca's PR did not fix it.

@marcaaron marcaaron reopened this Apr 8, 2021
@kidroca
Copy link
Contributor

kidroca commented Apr 8, 2021

Sorry should have removed the hash from the PR

It's covered for everything but Android though . The keyboard would close on iOS and on mWeb browsers. It just something with Android native that won't let go ⌨️. This is captured by my sample videos (mWeb and iOS)
I can mention it in the PR tests steps.

@aliabbasmalik8
Copy link
Contributor

@kidroca this issue not produced on any platform. can you please share a video in which this issue produced?
Thanks

@kidroca
Copy link
Contributor

kidroca commented Apr 12, 2021

@aliabbasmalik8

this issue not produced on any platform. can you please share a video in which this issue produced?
Thanks

I've posted a thorough description here: #2221 (comment)

The code have been modified since then, but I'm still successfully recreating the issue from the current code in master (6abe2f2)

Android.Emulator.-.Pixel_2_API_29_5554.2021-04-12.18-52-17.mp4

As in the original comment - I'm just tapping near the left side, shortly after the keyboard pops up. Since the back button in the header is also on the left side tapping there seems to produce the same effect, but after you return to the LHN

@kidroca
Copy link
Contributor

kidroca commented Apr 12, 2021

@marcaaron
I think I've tracked where the issue might be coming from after all.

  1. I can no longer recreate the issue after the changes here: Replace the existing US-located numbers, date and amounts with locale-aware components/ methods  #2349
  2. But switching to master would immediately bring the issue for me
    • tried clearing the cache
    • rebuilding the app
    • many other things I've posted in the original comment
  3. Going back to Replace the existing US-located numbers, date and amounts with locale-aware components/ methods  #2349 immediately fixes the issue for me, clearing cache, rebuilding does not bring the issue back

I think it might actually be related to the code here: #2346 (comment)

https://github.com/Expensify/Expensify.cash/blob/d3f1d28478357e2a66b155e660def191d6ed79a8/src/libs/Navigation/NavigationRoot.js#L75-L78

Adding the Home state twice. Might interfere with react native navigation. There are keyboard related props, which means the Keyboard is involved in the navigation in some way:

Other than that I don't know why I'm not getting the Keyboard issue inside that PR

@aliabbasmalik8
Copy link
Contributor

@marcaaron
Home route added twice in the state so I updated this code https://github.com/Expensify/Expensify.cash/blob/d3f1d28478357e2a66b155e660def191d6ed79a8/src/libs/Navigation/NavigationRoot.js#L75-L78
like this way and it's resolved the issue on android as well worked properly on other platforms

initialState.routes = [
    ...initialState.routes,
];

@marcaaron
Copy link
Contributor

@kidroca Great detective work! I think we should maybe keep this one on HOLD while we look into that PR. If we can confirm that fixes it we can close this issue.

@marcaaron
Copy link
Contributor

Verified this issue is no longer happening on the main branch for iOS or Android so I think this issue can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants