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

Settings - App doesn't navigate to another App by clicking physical back button #20389

Closed
2 of 6 tasks
lanitochka17 opened this issue Jun 7, 2023 · 31 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 7, 2023

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


Isue found when executing PR #19263

Action Performed:

  1. Open the App
  2. Login with any account
  3. In another app, write a link to the Settings page: https://staging.new.expensify.com/settings
  4. Click on the link and verify you have been navigated to the Setting page
  5. Click the back button

Expected Result:

You have been navigated to another app (from step 3)

Actual Result:

You redirected to LHN

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.25. 2

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6083487_Record_2023-06-07-10-41-58.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c3e2fc9f9ecf8085
  • Upwork Job ID: 1666775441912328192
  • Last Price Increase: 2023-06-08
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 7, 2023

Triggered auto assignment to @conorpendergrast (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Jun 7, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@conorpendergrast
Copy link
Contributor

I'm not sure of the expectations for Android, so checking that in Slack

@conorpendergrast
Copy link
Contributor

Confirmed in Slack and in the PR that the expectation is that the back button will bring you to the previous app!

@mountiny
Copy link
Contributor

mountiny commented Jun 8, 2023

This is a bug and I will add it to the navigation polish tracking project. Seems like the root cause is that with the deeplink we first go to the LHN and then to the settings page which add the screen artifically to the stack hence clicking back goes to LHN

@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@mountiny mountiny self-assigned this Jun 8, 2023
@mountiny mountiny added the Internal Requires API changes or must be handled by Expensify staff label Jun 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01c3e2fc9f9ecf8085

@melvin-bot
Copy link

melvin-bot bot commented Jun 8, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @sobitneupane (Internal)

@mountiny
Copy link
Contributor

mountiny commented Jun 8, 2023

I will wait for SWM engineers to come back from ooo next week to look at this

@conorpendergrast
Copy link
Contributor

@mountiny Do you have a moment to reproduce? I am really struggling with the setup on BrowserStack!

@rushatgabhane
Copy link
Member

rushatgabhane commented Jun 8, 2023

Was able to reproduce. Built the release variant and opened the settings page using npx uri-scheme

screen-20230608-184320.mp4

@mountiny
Copy link
Contributor

mountiny commented Jun 8, 2023

Thank you Rushat. Handling this as a normal bug as this is not serious issue.

@melvin-bot melvin-bot bot added the Overdue label Jun 12, 2023
@conorpendergrast
Copy link
Contributor

@mountiny Do you think this should be Daily, or reproduce the urgency to Weekly?

@melvin-bot melvin-bot bot removed the Overdue label Jun 12, 2023
@mountiny
Copy link
Contributor

I think daily as any other bug, asking SWM developers to get this one on so we can clean up all the navigation refactor follow ups.

@WoLewicki
Copy link
Contributor

Commenting

1 similar comment
@adamgrzybowski
Copy link
Contributor

Commenting

@WoLewicki
Copy link
Contributor

Are we 100% sure we want the behavior of going out of the app after clicking the back button? Does it happen in other similar apps when we deeplink into some nested navigation flows? And did it work correctly before the refactor? cc @mountiny react-navigation intercepts the physical back button and it might be hard to properly replicate that behavior without some dirty tricks.

@mountiny
Copy link
Contributor

And did it work correctly before the refactor?

This is irrelevant question, there were a lot of flows which worked incorrectly in the previous app navigation.

Does it happen in other similar apps when we deeplink into some nested navigation flows?

I do not have android to test this, but in general, yes it should be like that.

@rushatgabhane @Julesssss Can you help us on this one? What happens if you deeplink to WhatsApp chat, Discord channel, Facebook page, Instagram profile on Android and you click the native back? I guess if the react-navigation makes it hard we can double chat what users might expect from other apps but I believe from the predesign the conclussion was to move out of the app

@rushatgabhane
Copy link
Member

@mountiny whatsapp takes me to LHN, and instagram takes me back to the feed

Steps

  1. click the link
  2. press back

https://api.whatsapp.com/send/?phone=14437877678&text=HELLO

https://instagram.com/expensify

@mountiny
Copy link
Contributor

Interesting, thanks! What about Discord, that on is built with react native.

@rushatgabhane
Copy link
Member

@mountiny same thing for discord. It opens the LHN. i.e. two back press required to exit the app.

@Julesssss
Copy link
Contributor

This is a choice that is left to the developers, Android allows us to either highjack or exit the app immidiately. Twitter also highjacks the back button and keeps you in the app and this sucks because they are showing you completely unrelated content form that which I clicked to enter the app.

Also, I'm not able to reproduce currently on v1.3.27-2, do we still need to do anything here?

@WoLewicki
Copy link
Contributor

@Julesssss what do you mean by not reproducible?

I believe that showing the list of chats after clicking back button seems natural.

@Julesssss
Copy link
Contributor

Expected Result:
You have been navigated to another app (from step 3)

Okay, weird. Earlier I didn't get taken to the LHN, but now I am (with the same version). It seems inconsistent.

Here was the latest discussion in slack. The decision was that taking the user back to their app was preferable. Users still have the option of remaining in-app by simply using the < toolbar button.

@mountiny
Copy link
Contributor

I think if other major apps do it this way tis a strong argument to keep it that way especially if it might be tricky to implement so we leave the app on a first click.

@WoLewicki could you elaborate on the tricks?

@Julesssss
Copy link
Contributor

If it's complicated then maybe leaving as-is is fine.

One outstanding worry though is that we'll settle on this pattern, then later on will introduce deep linking to a page that is 3/4 levels deep in the nav hierarchy -- in that case, it is very annoying to have to press back 3/4 times to get back to the previous app. One of the main Android guidelines was that user flows can be made up of multiple different apps -- hijacking the back button breaks this and in the case of Twitter is so damn annoying, but I do agree that limiting this to the LHN for now is okay.

@WoLewicki
Copy link
Contributor

Yeah, since clicking hardware back button calls goBack, it should pop the RHP even if we are deep in the RHP flow, but it would be nice to test it on many scenarios to be sure.

@conorpendergrast
Copy link
Contributor

So, the consensus in Slack was that back should navigate you to the original app. Am I reading right that the thinking here is that we should keep the current behaviour?

@melvin-bot melvin-bot bot added the Overdue label Jun 19, 2023
@conorpendergrast
Copy link
Contributor

conorpendergrast commented Jun 19, 2023

Cross-posted to Slack that the discussion is going in the opposite direction here. It seems like we should close this out as the desired behaviour on Android to open the RHP when you tap the back button. I'll do that tomorrow unless I hear otherwise!

@melvin-bot melvin-bot bot removed the Overdue label Jun 19, 2023
@mountiny
Copy link
Contributor

I believe this one is tough to decide, as the sentiment is that the navigation should take you directly out of the app, but testing other popular Apps like Discord, Instagram or Whatsapp and a similar chat deeplink for them, we can see they all navigate to LHN and after that they take you out of the app. I believe in this case the consistency with other apps is a better UX as most likely that is what users are familiar with and that might be their expectation. Additionally this is a default react-navigation behaviour hence any changes to this would introduce more complex code which might have some worse unintented sideeffects. I believe its better to close this issue out and keep the current behaviour and focus on other issues as we got a bigger fish to fry.

We can always bring this discussion up if we will see some real world problem stemming from the current behaviour.

Thanks everyone!

@mountiny mountiny moved this from Todo to Done in Navigation Refactor Follow-ups Jun 19, 2023
@conorpendergrast
Copy link
Contributor

Yep, thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
No open projects
Development

No branches or pull requests

8 participants