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

Android Hardware Back press required twice #2098

Closed
HarshulSharma000 opened this issue Mar 20, 2018 · 15 comments
Closed

Android Hardware Back press required twice #2098

HarshulSharma000 opened this issue Mar 20, 2018 · 15 comments

Comments

@HarshulSharma000
Copy link
Collaborator

HarshulSharma000 commented Mar 20, 2018

Description-
Inside any of the streams to go to previous screen by hardware back press, it is required to be pressed twice. Thus it seems like first back key press is being ignored. And after that app navigates to first screen(home) of main tab navigator. Instead if it should keep the progress of previous tab navigator.

Platform: Android 6
Steps to Reproduce:-

  1. Go to the streams tab of main tab navigator.
  2. Enter into any of the streams
  3. Press Android hardware back key.
@HarshulSharma000
Copy link
Collaborator Author

I would like to work over this @borisyankov @gnprice @jainkuniya

@borisyankov
Copy link
Contributor

Sure you can work on that. In fact, this is an important bug to fix fast.
This is likely an issue with React Navigation (though some chance it is with our code). I noticed it too.
@jainkuniya did you fix the previous similar bug? If so, can you remind me and @HarshulSharma000 what was the issue?

@jainkuniya
Copy link
Member

Yup! I fixed the previous similar bug.
My previous commit 2a3e821

Actually each navigation in react-navigation (drawer, tabs, stack navigation) have their own implementation of handling back button press. Now the question is: when you press button it should be captured by which component.

On Current master if you observe carefully you will notice that:
for e.g.:
if you are on setting tab and open lets say Switch Account page, and then press button, you requires two back button press. First one is captured by MainTabs (HomeTabs) and 2nd one by stack navigation (which is responsible for going back). (Another version of this one: If you are on All Stream tab and then switch to Setting tabs and then Switch Account. In this case 3 press are required. One for MainTabs, one for StreamTabs and one for StackNavigation)

react-navigation/react-navigation#3200
I proposed react-navigation to add optional prop so that we can handle Back button as they want. But the better solution which I got from them is to override their implementation, the way I have done in my commit.

@HarshulSharma000 think logically at which level we need to override it. Feel free to reach me if you face any problem. Also claim issue by commenting @zulipbot claim.

@HarshulSharma000
Copy link
Collaborator Author

@zulipbot claim

@zulipbot
Copy link
Member

Hello @HarshulSharma000!

Thanks for your interest in Zulip! You have attempted to claim an issue without the label "help wanted". Since you're a new contributor, you can only claim and submit pull requests for issues with the help wanted label.

If this is your first time here, we recommend reading our guide for new contributors before getting started.

@jainkuniya
Copy link
Member

@zulipbot label "help wanted"

@jainkuniya
Copy link
Member

@HarshulSharma000 Now you can claim

@HarshulSharma000
Copy link
Collaborator Author

@zulipbot claim

@zulipbot
Copy link
Member

Welcome to Zulip, @HarshulSharma000! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip-mobile/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

@HarshulSharma000
Copy link
Collaborator Author

@jainkuniya We can also try screen wise custom back behaviour as described in- https://reactnavigation.org/docs/custom-android-back-button-handling.html

@pulkonet
Copy link
Collaborator

It is not an issue on Android 8.0.0. Can't reproduce it.

@borisyankov
Copy link
Contributor

It likely is not dependent on platform version so you probably just missed it 😄

@HarshulSharma000
Copy link
Collaborator Author

@borisyankov I think I have found the problem please check mobile channel.

@pulkonet
Copy link
Collaborator

@borisyankov Checked it again. It goes back in a single press. Not sure why this might be happening.

jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Mar 26, 2018
Complete fix for multiple back button are required to navigate back.

Fix: zulip#2098
jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Mar 26, 2018
Fix: multiple back button press are required if narrowed
from tab other home i.e steams and conversation.
Because one press was captured by main tab to navigate to
home tab that is the first tab.

Fix: zulip#2098
@borisyankov
Copy link
Contributor

Fixed by #2180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants