[HOLD for payment 2021-12-06] The Android hardware back button does not minimise the app when pressed on the LHN/home screen

kp17211 commented 3 years ago

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

Action Performed:

Click on an a chat from the LHN
Once you're in the chat, press the back button to navigate back to the LHN/home screen
From the LHN/home screen, press the back button again

Expected Result:

When pressing the back button on the LHN/home screen, the app should be minimised (exited).

Actual Result:

When pressing the back button on the LHN/home screen, we navigate back to the previous chat that users was in.

Workaround:

None

Platform:

Where is this issue occurring?

[ ] Web
[ ] iOS
[x] Android
[ ] Desktop App
[ ] Mobile Web

Version Number: Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Upwork URL: https://www.upwork.com/jobs/~017e60e720f6f9aca8

View all open jobs on Upwork

MelvinBot commented 3 years ago

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

mallenexpensify commented 3 years ago

I don't have Android, trying to find someone on the CM team to triage

mallenexpensify commented 3 years ago

Assigning to @laurenreidexpensify for review since she has Android and I don't.

parasharrajat commented 3 years ago

To me this is reproducible.

laurenreidexpensify commented 3 years ago

@kp17211 @parasharrajat I'm trying to reproduce this and a bit confused.

Am on my Chats page
tap into a chat
within chat, tap top left hand < icon
taken back to chat page
swipe left as the generic Android "back" action
am taken to the chat I was just in

Am I missing something? Isn't this the expected behaviour?

parasharrajat commented 3 years ago

am taken to the chat I was just in

This is an issue.

the flow should be

You click on any chat from LHN.
It takes you to the chat.
You click back button on android phone.
It takes you back to chat list / LHN.
You press the hardward back button again.
App should minimize.
But it will take you back to the recent open chat.

laurenreidexpensify commented 3 years ago

Ah got it - okay yes I can confirm this is the behaviour then. Will send this to our engineering team for next review.

MelvinBot commented 3 years ago

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

flodnv commented 3 years ago

I could've sworn this was a feature and not a bug (granted, that I found strange as well). What's the behavior on iOS?

Beamanator commented 3 years ago

What's the behavior on iOS?

I don't believe iOS has a "back" hardware button (at least most of their devices don't) so I think this only applies in Android.

I agree this should be fixed eventually, it's how most (if not all) Android apps work, so I'll take this over and making it External

Looks like these resources could be helpful:

https://reactnative.dev/docs/backhandler
https://reactnavigation.org/docs/custom-android-back-button-handling/
Conversation in #expensify-open-source about using hooks here

MelvinBot commented 3 years ago

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

trjExpensify commented 3 years ago

The title of this issue is a bit generic and the steps in the OP are a little confusing, so it would be good to spruce this up before posting to Upwork. Before I make the updates, am I right in thinking the issue here is isolated to: The Android hardware back button does not minimise the app when pressed on the LHN/home screen?

Then the Action Performed reproduction steps are:

Click on an a chat from the LHN
Once you're in the chat, press the back button to navigate back to the LHN/home screen
From the LHN/home screen, press the back button again

Expected Results

When pressing the back button on the LHN/home screen, the app should be minimised (exited).

Actual Results

When pressing the back button on the LHN/home screen, we navigate back to the previous chat that users was in.

Let me know if that's correct and I'll update the issue. 👍 Also, not sure if it's helpful to reference for the implementation approach, but we had some wider inconsistencies with the Android back button a little while back here.

parasharrajat commented 3 years ago

We should first look into React-navigation. It was previously fixed but after the upgrade of react-navigation to v6 broke that fix.

Beamanator commented 3 years ago

Nice @trjExpensify that looks like a great summary! 👍

trjExpensify commented 3 years ago

Cool, updated. Upwork job is here. @kp17211 are you interested in applying seeing as though you raised this issue?

kp17211 commented 3 years ago

Hello @trjExpensify yes I want to work on this issue

Beamanator commented 3 years ago

@kp17211 I think what @trjExpensify was asking was if you want to submit a proposal for the fix :D

Beamanator commented 3 years ago

Not overdue, just waiting for proposals

Beamanator commented 3 years ago

Changing priority to weekly as we wait for proposals

Aamer-Rasheed commented 3 years ago

@Beamanator I'm interested to work on the job. Submitted proposal at Upwork.

Beamanator commented 3 years ago

Thanks for your interested @Aamer-Rasheed ! Please make sure to follow these steps too: https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#propose-a-solution-for-the-job

trjExpensify commented 3 years ago

I've accepted @kp17211 proposal in Upwork, as he created the issue. Over to you! 👍

Beamanator commented 3 years ago

@kp17211 before you submit your PR could you please post your proposal here so I can review? 👍

trjExpensify commented 3 years ago

Okay, @kp17211 has reached out to me on Upwork to say he has accepted another offer so won't have time for this. Unassigning! @kp17211 in the future, could you just drop us a note here please? We're far more active and on top of comms in the GH repo than we are the Upwork messaging tool. Thanks!

trjExpensify commented 3 years ago

@Aamer-Rasheed are you still interested in this job? Please let us know how you plan to approach the solution and then we can proceed to hire and moving to the PR.

Aamer-Rasheed commented 3 years ago

@trjExpensify Thanks for the query. Got another project with a deadline of 03 weeks so would be unavailable during this time.

mallenexpensify commented 3 years ago

Doubled price to $500 in Upwork

parasharrajat commented 3 years ago

After debugging I found out this.

Drawer is handling the hardware back button when the Drawer is open. It triggers DrawerAction.CloseDrawer() action which opens it. I am not sure how that is supposed to work but it is the main cause of the issue.
And from some digging, App close behaviour is not handled for the Drawer Router. So we have to customise the hardware back button action with Backhandler.
But there is a problem that when the drawer is open hardwareBackPress event is consumed by the Drawer BankHandler which block our attempt to override it.
@marcaaron opened a PR days back to disable this behind a flag. https://github.com/react-navigation/react-navigation/issues/9511 but it was closed.

To fix this issue we have to do the following but I am not how to run this piece of code when the drawer is open as Backhandler'hardwareBackPress'never fires.

Run it on Backhandler'hardwareBackPress'
if(drawerisOpen) {
 navigation.dispatch(StackAction.pop());
 }

Other things I am exploring:

Listen for Drawer close event and then block the navigation and fire StackAction.pop() action to close the drawer.

maniecodes commented 3 years ago

I got an invite from Upwork regarding this task. Is it still available? @trjExpensify @parasharrajat

parasharrajat commented 3 years ago

Yes @Dev-Manny

Beamanator commented 3 years ago

@parasharrajat thanks for linking the previous issue opened by Marc! It looks like it was closed b/c we didn't respond back. In the issue, someone linked https://reactnavigation.org/docs/preventing-going-back/ - have you tried testing the code on that page?

parasharrajat commented 3 years ago

reactnavigation.org/docs/preventing-going-back . This does not work for drawers.

we have a Stack screen that holds the drawer and now even will fire when we are navigating away from that screen but this never happens as the drawer captures the back button and toggle its status instead of going back.
We want to go back.
Best is what I explained above but I have not found a good way to do it.

But what I found so far is this.

useFocusEffect(
        React.useCallback(() => {
            let backSub = {remove: () => {}};
            const handler = _.throttle((e) => {
                const state = getActiveState(e.data.state);
                const history = _.last(state.history);
                console.debug(e.data.state);
                if (!history) {
                    return;
                }
                console.debug(state.type, history);
                if (state.type === 'drawer' && history.type === 'drawer' && history.status === 'open') {
                    console.debug('open');
                    backSub.remove();
                    backSub = BackHandler.addEventListener('hardwareBackPress', () => {
                        if (navigation.canGoBack()) {
                            Navigation.dismissModal();
                        } else {
                            BackHandler.exitApp();
                        }
                        backSub.remove();
                        return true;
                    });
                } else {
                    console.debug('not open');
                    backSub.remove();
                }
            });
            const sub = navigation.addListener('state', handler);
            return () => {
                sub();
                backSub.remove();
            };
        }, [navigation]),
    );

It works great and handles both of our drawers but it does not work for the first time when you navigate to the drawer screen as the drawer is opened by default which does not create a history entry.

If we can disable the drawer back handler from the Marc issue, things will become easier.

Julesssss commented 3 years ago

Apologies if I am repeating something that is no longer true, but @Maftalion suggested the following solution a while ago, which sounds like a better fix in my opinion:

@Maftalion

the complexity stems from the way the current navigation is set up, technically the reportview is the active page and the list of chats is a open drawer on top of that page

if that gets restructured as two separate pages, then the issue with the reportview would go away

He's referring to the point that the reportView page used to be the top-level page in the app, with the NavDrawer|LHN being a view that overlayed the content, instead of being its own separate page. When we made the NavDrawer|LHN the top-level page we did not modify the RN page hierarchy to represent the true navigational hierarchy.

marcaaron commented 3 years ago

It looks like it was closed b/c we didn't respond back

Oh hmm the react-navigation issue was closed (maybe we can reopen it with a reproduction?). That suggestion was not from a maintainer which is why I didn't respond (it also doesn't work).

the reportView page used to be the top-level page in the app, with the NavDrawer|LHN being a view that overlayed the content, instead of being its own separate page

It sounds like we are suggesting to make the LHN another screen in the main stack navigator but I'm not too sure. We can achieve the drawer navigator "overlay" style with drawerType: 'front' if that's a design we want to go with and it fixes the back handler issue.

we did not modify the RN page hierarchy to represent the true navigational hierarchy

What makes something a true navigational hierarchy?

marcaaron commented 3 years ago

Does everyone agree the proper fix should be in react-navigation?

parasharrajat commented 3 years ago

I agree that there should be an option to disable the back handler controlling the drawer as we want that. This causes issue when the drawer is nested in a screen.

Julesssss commented 3 years ago

It sounds like we are suggesting to make the LHN another screen in the main stack navigator but I'm not too sure.

Yeah, that's the suggestion here. We are only facing this issue because we are trying to force a NavigationDrawer to behave like a Page for our convenience. I believe this is also the reason why the component does not support App close behaviour -- it is not supposed to handle app close, because that should always be the responsibility of the page it sits on top of.

Scenario A: Original version of our app

User opens app to chat page
User opens drawer to see LHN
User presses back [Drawer closes]
User presses back [App closes]

A NavDrawer should open on top of the current page and is a part of that page.

A few months ago, we decided to switch to the WhatsApp pattern. The important distinction is that the LHN is now its own Page, rather than an overlay NavDrawer. We now (correctly) animate between the chat page and LHN as if they are parent and child pages, instead of the LHN simply being an overlay.

Scenario B: Our current app

User opens app to see LHN
User selects list item to open chat page
User presses back [Chat page closes]
User presses back [App closes]

https://material.io/archive/guidelines/assets/0B6Okdz75tqQsWjgyVU9kMWN0V2s/patterns-navigational-transitions-parent-to-child-list-02-xhdpi-019.webm

I agree that we definitely could continue to use the NavigationDrawer component for the LHN page and it will look mostly look fine, but I fear we'll run into lots of minor bugs/odd behaviors if we continue with this. One thing that jumps out to me is that we have to maintain two different ways of animating pages: 1) react-navigation handles page to page animation for us, 2) we use the NavigationDrawer component options to choose from 4 animation types (this is the only page handled in this way).

The reason I care (probably too much) about this, is because I would like us to have a truly mobile-first user experience and I fear that we'll end up with mostly correct, but buggy-looking UX.

What makes something a true navigational hierarchy?

I mean, if we can make the NavigationDrawer behave in the right way then there is no problem. But why not make LHN a page? It makes our lives easier because we can trust the library to handle navigation, rather than having to manually fix this specific case with a workaround.

Sorry for the wall of text 😬

marcaaron commented 3 years ago

The reason I care (probably too much) about this, is because I would like us to have a truly mobile-first user experience and I fear that we'll end up with mostly correct, but buggy-looking UX.

Thanks for the thoughts @Julesssss! It might be worth creating a separate issue to discuss all this and we can loop some more folks in. It sounds like even if we fix this back button behavior there will remain an issue that the UX is still not ideal. And in that case, I would definitely encourage you to do the research and build a case for it if you feel strongly about it.

why not make LHN a page?

Might need to do some digging to produce a more thorough answer, but the reasons I can think of are...

No one has proposed a fully fleshed out alternative (like a proof of concept or something) and how it would work on the web side
IIRC we wanted a swipe left on the LHN to reveal chat and a swipe right on the chat to reveal the LHN. I'm unsure whether stack navigation alone can do this. Having the sidebar float above would either require us to use drawer navigator with drawerType: front or do a custom implementation where we have the drawer sit above everything in a fixed way and not include it in the react-navigation stack. The latter sounds complicated to me, but might worth doing.
It's unclear how we could create the wide screen UI (fixed sidebar mostly used on web) with stack navigator alone. Figuring out how this layout will work might be a challenge if we want to abandon the drawer navigator entirely.

I'm not against ditching the drawer navigator. It might actually be better for performance since we can render the LHN without incurring a big cost to render the react-navigation tree. But it does feel like a big undertaking if our main concern is that the drawer navigator doesn't let us close the app on Android.

Maftalion commented 3 years ago

Yeah, if I remember correctly the biggest issue was getting the desired UI/UX across all platforms (mobile, web, etc) which led to the drawer implementation.

As you mentioned @Julesssss a lot of the issues stem from trying to use the drawer as a screen instead of a drawer but at the same time want some functionality of the drawer (as Marc mentioned above with the gesture swiping).

Has there been any thought on keeping it as a drawer but not making the LHN the "initial screen"? Basically if you implemented something similar to Slack's UI where the chat is the home screen and the LHN is treated purely as a drawer? I believe that would fix the back button since back will just close the drawer and back from the chat would close the app.

mallenexpensify commented 3 years ago

Doubled price to $1000 https://www.upwork.com/jobs/~017e60e720f6f9aca8

parasharrajat commented 3 years ago

I am awaiting @marcaaron 's decision on approaching the issue.

Either we should reopen the issue https://github.com/react-navigation/react-navigation/issues/9511 and add code to hide the drawer. The React-navigation repo is like a snail. I found it hard that someone will help us there.
Jules has a good suggestion https://github.com/Expensify/App/issues/4211#issuecomment-902155340 but I am not sure if that is possible. It requires rethinking the app architecture for few major parts. I don't think it's a good idea at this stage of the APP.
Last option is we hack our way through it and apply the fix in the code.

Here is a way to do it, Adding this handler in the Stack screen which contains the drawer. App already has a common BaseDrawerNavigator component which makes it the best target. This code handles both Main LHN and Workspace Drawer.

   const navigation = useNavigation();

    useFocusEffect(
        React.useCallback(() => {
            let backSub = {remove: () => {}};
            const handler = _.throttle((e) => {
                // latest active state by custom function
                const state = getActiveState(e.data.state);
                const history = _.last(state.history);
                if (state.type !== 'drawer' || (history && history.type === 'drawer' && history.status === 'open')) {
                    console.debug('open');
                    backSub.remove();
                    backSub = BackHandler.addEventListener('hardwareBackPress', () => {
                        if (navigation.canGoBack()) {
                            Navigation.dismissModal();
                        } else {
                            BackHandler.exitApp();
                        }
                        backSub.remove();
                        return true;
                    });
                } else {
                    console.debug('not open');
                    backSub.remove();
                }
            });
            const sub = navigation.addListener('state', handler);
            return () => {
                sub();
                backSub.remove();
            };
        }, [navigation]),
    );

Beamanator commented 3 years ago

What if we go with this plan:

Implement Rajat's code above (assuming we test and don't find any issues)
Rajat tries to follow up with React Navigation to get the change approved (maybe also submit a PR to them so they can test it on their end?)
If React Navigation merges the change, Rajat refactors the code above with the simpler fix

marcaaron commented 3 years ago

Implement Rajat's code above (assuming we test and don't find any issues)

The code looks a bit complicated to me personally - which makes me concerned it will be difficult to maintain if we end up not doing 2. I think we should first try to re-open the issue, provide a reproduction, and attempt a fix in react-navigation.

I can't think of any reason why this issue needs to be fixed urgently - there are other ways to exit the app.

Julesssss commented 3 years ago

Just to follow up on my post above, I created a separate issue. It's probably not a high priority, but I think this improvement is worth implementing if we can figure out how to handle web.

Thanks for your comments!

Beamanator commented 3 years ago

Ok so let's go with this:

Try to re-open the React-Navigation issue: https://github.com/react-navigation/react-navigation/issues/9511
- Post in the issue why https://reactnavigation.org/docs/preventing-going-back/ doesn't work for our case
- Maybe submit a PR with our recommended change, as this could speed things along
If the above issue gets merged into their main repo, use the change to allow the Android app to close when clicking the back button

I think this would count as 2 jobs, so the reward would be $500

These are both relatively small changes, so @parasharrajat are you willing to work on those while we consider replacing the LHN with a Page? (@Julesssss 's suggestion)

parasharrajat commented 3 years ago

I am on vacation. back on 8.

Beamanator commented 3 years ago

Enjoy! Take your time responding :)

Beamanator commented 3 years ago

Bump @parasharrajat :D

parasharrajat commented 3 years ago

Ok. I am not yet sure what are we trying to fix in the R-navigation repo. @Beamanator. The issue I linked was only trying to disable the functionality to control the open-closed state of the drawer behind a flag.

So before we move, a few questions:

What is the problem, we are trying to fix on R-navigation repo?
Do we need to reopen the linked issue? If So, it only disables the back hardware key functionality for the drawer and we will still need to add the navigation in our app but the code would be much simpler than what I proposed.
Or do we want to add the back hardware button functionality to navigate back or close the app with drawer Navigator in r-navigation lib? If so, I didn't find any piece of code that does that in the codebase for r-navigation. I think is managed by react-native-screens.
Or, I think if we disable the back hardware key functionality for the drawer, we may not need the custom logic to handle it as It will be handled via Stack Navigator and thus should work fine.

Beamanator commented 3 years ago

@parasharrajat

You're right, we're technically not asking for a "fix", it's an enhancement, exactly what Mark reported in the issue
I thought it would be easier to just continue from where Marc left off, but it's possible only maintainers can re-open issues, so you may have to start from scratch - whatever you think is easier / most effective.

As for where to make the changes, I would go with the smallest change in React-Navigation (as this is probably the easiest to get approved), then the rest in our app. So 2 or 4 - but honestly I can't really tell the difference between those proposals

Expensify / App