[HOLD #11768] [$8000] The Back ⌘[ and Forward ⌘] shortcuts keys are not working as expected when navigating via RHN (Right Hand Navigation)

[mallenexpensify]

[BELOW IS THE PROCESS OF BEING EDITED/UPDATED AND IS ON HOLD]

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

---

Breaking this off from #4612, that issue will be focused solely on LeftHandNav (LHN) and this one is for the navigation in RHN (or RHP - Pane)

Action Performed:

• Open the app on the Web or desktop.
• Open Search/Chat Switch (⌘+k shortcut)
• Navigate to some chat P.
• Open the Search Page again and navigate to some chat Q.
• Open the Search Page again and navigate to some chat R.
• Open the Search Page again and navigate to some chat Q.
• Use the back ⌘[ and Forward ⌘] shortcuts to go to previous and forwards chats respectively.
• You can navigate to any number to chats.

Expected Result:

When we open reports from RHN, Back ⌘[ and Forward ⌘] shortcuts should work and navigate through the previously navigated conversations in the main/focused chat view. ie. if I visit chats with Joanie, Alex then Daniel, I expect ⌘[ to take me from my chat with Daniel back to Alex then ⌘[ again back to Joanie. Most-everywhere else the back/forward actions follow a standard flow.

RHN contains navigating via Search Page, New Chat Page, or any other modal.

Back ⌘[ and Forward ⌘] shortcuts should work and navigate through the previously navigated conversations in the main/focused chat view. ie. if I visit chats with Joanie, Alex then Daniel, I expect ⌘[ to take me from my chat with Daniel back to Alex then ⌘[ again back to Joanie. Most-everywhere else the back/forward actions follow a standard flow.

Actual Result:

Shortcuts are not working as expected.

Workaround:

User has to manually navigate through the conversations. ie. If using keyboard navigation you always have to ⌘k, start typing a name, then return to navigate

Platform:

Where is this issue occurring?

• Desktop App

Version Number: Version 1.1.57-0

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

Notes/Photos/Videos: Any additional supporting documentation
Unable to get a video atm. Will update with video later.
Other job for LHN navigation issue #4612

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1628722621224600

The Back ⌘[ and Forward ⌘] shortcuts keys don't work properly on Desktop Version 1.0.82-7

[melvin-bot]

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

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~01ce192fdce6f46806

[melvin-bot]

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

[melvin-bot]

Current assignee @NikkiWines is eligible for the Exported assigner, not assigning anyone new.

[Salehmrcp]

It is js coding issue
i can fix it easy

[parasharrajat]

@Salehmrcp Can you post a proposal? Please look at CONTRIBUTING.md for more.

[Salehmrcp]

I have submitted a proposal in upwork- under name hwaida mohamed I looked also at the file

…

On Mon, 18 Apr 2022, 9:31 am Rajat Parashar, ***@***.***> wrote: @Salehmrcp <https://github.com/Salehmrcp> Can you post a proposal? Please look at CONTRIBUTING.md for me. — Reply to this email directly, view it on GitHub <#8657 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYNXU5ELOQ7UUOTJRB34MLDVFUFTHANCNFSM5TRSIXKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[NikkiWines]

@Salehmrcp, per the guidelines here you need to do the following:

After you reproduce the issue, make a proposal for your solution and post it as a comment in the corresponding GitHub issue (linked in the Upwork job). Your solution proposal should include a brief technical explanation of the changes you will make.

[Salehmrcp]

Thanks nikki But I can not even write the solution before I am hired My upwork link is here https://www.upwork.com/freelancers/~010501fb0f15f0e713?viewMode=1 Kindly hire me first to start

…

On Mon, 18 Apr 2022, 6:35 pm Nikki Wines, ***@***.***> wrote: @Salehmrcp <https://github.com/Salehmrcp>, per the guidelines here <https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#propose-a-solution-for-the-job> you need to do the following: After you reproduce the issue, make a proposal for your solution and post it as a comment in the corresponding GitHub issue (linked in the Upwork job). Your solution proposal should include a brief technical explanation of the changes you will make. — Reply to this email directly, view it on GitHub <#8657 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYNXU5GDKBOE3NC2RKMRAELVFWFNJANCNFSM5TRSIXKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mdneyazahmad]

Proposal

DrawerNavigtor has history prop that is used to store the routes visited and it also stores the drawer status (open or closed). There is a catch here history can contain drawer status object or not. It is based on the defaultStatus of the DrawerNavigator.

1. It does not contain this object if the current status of the drawer is same as defaultStatus.
2. It contains an object { type: 'drawer, status: 'open' | 'closed' } if current status is not same as defaultStatus.

App/src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

Line 63 in ad637d3

drawerType: StyleUtils.getNavigationDrawerType(this.props.isSmallScreenWidth),

StyleUtils .getNavigationDrawerType returns either slide or permanent. When drawerType is set to permanent it always displays the drawer irrespective of the status of drawer in state object.

We can safely provide defaultStatus = "open" as this is the value for small screen and it does not have any effects on large screen. In this case history will contain { type: 'drawer, status: 'closed' } for closed state of the drawer and it will be removed from history if the status is open.

App/src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

Line 57 in ad637d3

defaultStatus={this.state.defaultStatus}

-                defaultStatus={this.state.defaultStatus}
+                defaultStatus="open"

pushDrawerRoute is only called when the path is r/:reportId. We are sure at this point that we should close the drawer as it goes to the report. As the default status of the drawer is open, history must contain an object {type: 'drawer', status: 'closed'}. Again it does not have any effects on large screen that will always be opened as the drawerType is set to permanent on large screen.

App/src/libs/Navigation/CustomActions.js

Lines 106 to 113 in ad637d3

	if (!hasDrawerhistory || currentState.type !== 'drawer') {
	history.push({
	type: 'drawer',
	// If current state is not from drawer navigator then always use closed status to close the drawer
	status: currentState.type !== 'drawer' || currentState.default === 'open' ? 'closed' : 'open',
	});
	}

-        const hasDrawerhistory = _.find(state.history || [], h => h.type === 'drawer');
+        const isDrawerInHistory = _.some(history || [], h => h.type === 'drawer');

+        if (!isDrawerInHistory) {
+            history.push({
+                type: 'drawer',
+                status: 'closed',
+            });
+         }

Result:

navigation.mp4

[mdneyazahmad]

@parasharrajat I may not be correct here but the original issue has problem with shortcut key when we navigate with search page. But later on @rushatgabhane has identified another bug (this one) that was working previously #4612 (comment)

If you are not satisfied with my proposal let me know I will add more context to it and explain the root cause. Thank you

[metehanozyurt]

Proposal

We mapping history state this line but not use it for drawer check. So it can be skip 1 person when click from list and not push to history list.

App/src/libs/Navigation/CustomActions.js

Line 101 in ad637d3

const history = _.map(state.history ? [...state.history] : [screenRoute], () => screenRoute);

we don't use the mapping we did before

Solution

if we change like this this can be fix problem

App/src/libs/Navigation/CustomActions.js

Line 105 in ad637d3

const hasDrawerhistory = _.find(state.history || [], h => h.type === 'drawer');

     const hasDrawerhistory = _.find(history || [], h => h.type === 'drawer');

Screen-Recording-2022-04-24-at-13.19.35.mp4

[mdneyazahmad]

Root cause:

Let's add console.log('hsitory', history) here and navigate to multiple reports.

App/src/libs/Navigation/CustomActions.js

Line 114 in ad637d3

As we can see that drawer object is alternatively added and hence history.length does not change for every two navigation.

react-navigation calculates history delta here which comes out to be 0 for every two navigation and hence it does not push to to browser history.
https://github.com/react-navigation/react-navigation/blob/4c2b69f2e339e21714a841f4e8e9e3d5c7120269/packages/native/src/useLinking.tsx#L569

The drawer status does not change in these navigation. It should check against history and not state.history

App/src/libs/Navigation/CustomActions.js

Line 105 in ad637d3

const hasDrawerhistory = _.find(state.history || [], h => h.type === 'drawer');

[mdneyazahmad]

@metehanozyurt Could you please not edit? You can create a new comment or else it won't be fair for other contributer you keep editing and claim to be the first to propose a better solution. Thanks

[metehanozyurt]

ofcourse @mdneyazahmad it can be visible first proposal and edited version("video upload"). both version before your last message. When i see your commit after upload image i didnt touch anything. Sorry for the time conflict. I was also looking. Thank you

[mdneyazahmad]

I am sorry @metehanozyurt I was unaware about the this feature (we can see edited history).

[mdneyazahmad]

The reason I added a static defaultState:

1. It does not matter for large screen as drawerType is permanent.
2. When user starts with browser resized it has default status of closed on large screen and when user starts with full width it has defaultStatus of open on large screen. It has two different value on web for the same drawer on large screen.

defaultstatus.mp4

[JmillsExpensify]

Snagging this now that I'm in the tracking issue.

[JmillsExpensify]

See the tracking issue for the latest.

[JmillsExpensify]

Still holding.

[JmillsExpensify]

Still held as this initiative goes through the design process.

[JmillsExpensify]

Still holding on the larger react navigation project.

[melvin-bot]

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.

[JmillsExpensify]

Still on hold for the larger navigation re-write.

[JmillsExpensify]

Same as above.

[JmillsExpensify]

Still on hold for navigation. No ETA.

[JmillsExpensify]

Same same

[JmillsExpensify]

Still on hold.

[JmillsExpensify]

Still on hold

[JmillsExpensify]

Coming off hold very soon!

[JmillsExpensify]

Ok I'm coming back to this one now but we don't officially support this type of keyboard navigation so I'm closing. Keyboard navigation is a new feature and one day will be an initiative.

[JmillsExpensify]

Quick clarification: Based on the Contributor+ work @parasharrajat did to review and manage this issue, he's eligible for a 25% payment as a result.

[JmillsExpensify]

Additionally, this issue is eligible and approved and payment on NewDot.