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

fix default drawer status issue #9076

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,11 @@ const CONST = {

// There's a limit of 60k characters in Auth - https://github.com/Expensify/Auth/blob/198d59547f71fdee8121325e8bc9241fc9c3236a/auth/lib/Request.h#L28
MAX_COMMENT_LENGTH: 60000,

DRAWER_STATUS: {
OPEN: 'open',
CLOSED: 'closed',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition moving these to CONST, thanks!

};

export default CONST;
3 changes: 3 additions & 0 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,7 @@ export default {

// Validating Email?
USER_SIGN_UP: 'userSignUp',

// Store default drawer status
DEFAULT_DRAWER_STATUS: 'defaultDrawerStatus',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: unsure whether we need to store this in Onyx or not... seems like something that could just be local to the navigation lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why we use Onyx is because we need a place that can keep the value even after refresh the page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why do we need to keep the value after a refresh of the page?

What is the expected behavior if someone directly navigates to a /r/<reportID> route? Would we show the report or the sidebar?

@parasharrajat do you know the expected behavior and can you point me to where we decided how it should work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked this #8126 (comment) with Expected output #8126 (comment).
It should show the report screen.

But the main issue was that If the user is viewing LHN, refreshing it should land back on LHN and vice-versa.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I guess my question could be re-phrased as... do we need this feature to track the drawer state at all?

What is the issue with this logic?

  • When / -> Show drawer open
  • When /r/123 -> Show report page (drawer closed)

?

Switching to the sidebar and refreshing the page e.g. will remember that the drawer is open when we are on the report page. Does this mean in other contexts when I want to navigate directly to a report page it will instead show me the sidebar if it was last set as 'open'? That sounds undesirable.

Copy link
Contributor Author

@mollfpr mollfpr May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right.

I have another approach for that case, but it's not 100% working as expected.

On my Chrome Version 101.0.4951.64 (Official Build) (arm64) there is a case if we paste the same URL as the current URL it will read as reload type, other than that can be returned as navigate type. On Safari, it works as expected.

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming/type

        if ((window.performance.navigation && window.performance.navigation.type === 1) || _.map(window.performance.getEntriesByType('navigation'), nav => nav.type).includes('reload')) {
            return defaultDrawerStatus;
        }

        const path = getPathFromState(navigationRef.current.getState(), linkingConfig.config);

        return path === '/' ? CONST.DRAWER_STATUS.OPEN : CONST.DRAWER_STATUS.CLOSED;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, if I'm understanding the proposal... you are saying that we should differentiate between a "reload" and a "navigate" - but I'm not sure why.

Seems like determining the default state of the drawer is only necessary when a new app window opens or is reloaded.

Overall, it feels like the drawer status is something that the linkingConfig in react-navigation should handle and be based not on history - but on the url.

I'm going to bring this to the Slack channel just to get some clarification.

};
14 changes: 14 additions & 0 deletions src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {View} from 'react-native';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import styles from '../../../styles/styles';
import * as StyleUtils from '../../../styles/StyleUtils';
import * as App from '../../actions/App';

import Navigation from '../Navigation';

Expand Down Expand Up @@ -66,6 +67,19 @@ class BaseDrawerNavigator extends Component {
),
swipeEdgeWidth: 500,
}}
screenListeners={{
state: (e) => {
const state = e.data.state;

// Get the history that has drawer type to get the status.
const hasDrawerHistory = _.find(state.history || [], h => h.type === 'drawer');

// hasDrawerHistory will has undefined value if the route drawer is equal to initial route drawer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this. What does "if the route drawer is equal to initial route drawer" mean exactly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, hasDrawerHistory is not a boolean so this variable name should communicate that this is a "first history item that is type drawer"? (If I am understanding this correctly - not really getting any of this code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we first open the page and shows up the LHN, then click on any report item, when on the report screen the drawer has the history. But when we go back again to LHN, the drawer history will return undefined.

Also, hasDrawerHistory is not a boolean so this variable name should communicate that this is a "first history item that is type drawer"? (If I am understanding this correctly - not getting any of this code)

Yeah, you're right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional thoughts! I'm trying to better understand what "history" is...

Looking at the shape of a navigation state object in the react-navigation docs and found this:

https://reactnavigation.org/docs/navigation-state

history - A list of visited items. This is an optional property and not present in all navigators. For example, it's only present in tab and drawer navigators in the core. The shape of the items in the history array can vary depending on the navigator. There should be at least one item present in this array.

So it seems like there should be at least something in the history at all times...

when we go back again to LHN, the drawer history will return undefined.

Does this seem correct? How can it happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not making it clear, what I mean by drawer history is history that has the type drawer.

Screen Shot 2022-05-20 at 13 12 23

For example on this screenshot, when I first open the app it's showing a report screen. Then I go to LHN, in this step the history will have a history that has the type drawer which has open value. After that, I open the report screen again and there's no history for type drawer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the history always has a value - it's just not always a type "drawer" history item. That would be good to clarify in the comment.

But maybe let's determine if this is the correct solution first...

// Using the default status if hasDrawerHistory is undefined and get the status from the current route
// if the value provided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't understand this comment... where does the default status coming from ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status comes from the route state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not sure if I'm following that...

Going to try to explain in my own words and you let me know if I'm on the right track...

The item that we are getting from the history is a route. That route has a status that refers to the state of the drawer.

The e.data.state has a default property to indicate the drawer status as well.

Are these drawer states documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not well documented on the docs https://reactnavigation.org/docs/navigation-events/#state.

Here are the example from data.state

{
    "stale": false,
    "type": "drawer",
    "key": "drawer-o_B0nHlaCG0jGCWESf5PA",
    "index": 0,
    "routeNames": [
        "Report"
    ],
    "history": [
        {
            "type": "route",
            "name": "Report"
        },
        {
            "type": "route",
            "name": "Report"
        },
        {
            "type": "route",
            "name": "Report"
        }
    ],
    "routes": [
        {
            "name": "Report",
            "params": {
                "reportID": "90420013"
            },
            "key": "Report-oiQ55dmXeZgHMcLsx0rmh"
        }
    ],
    "default": "closed"
}
{
    "stale": false,
    "type": "drawer",
    "key": "drawer-o_B0nHlaCG0jGCWESf5PA",
    "index": 0,
    "routeNames": [
        "Report"
    ],
    "history": [
        {
            "type": "route",
            "name": "Report"
        },
        {
            "type": "route",
            "name": "Report"
        },
        {
            "type": "drawer",
            "status": "open"
        }
    ],
    "routes": [
        {
            "name": "Report",
            "params": {
                "reportID": "92055915"
            },
            "key": "Report-YVI0AsQPNmKDO-oKjc-Sl"
        }
    ],
    "default": "closed"
}

The default value is what we set from defaultStatus props of Drawer.Navigator.

App.setDefaultDrawerStatus(hasDrawerHistory ? hasDrawerHistory.status : state.default);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we need this to run exactly? It seems like it will happen on every state change...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug seems like it just affects the initial state of the drawer when the app first inits so do we need to set this any other time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We save the drawer status when it changes.

For example when we are on the report screen, so the drawer has the status closed, and then we save the status to Onyx. When we refresh the page, it will still be showing the report screen, not the LHN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so to summarize, this code is saving the "last state of the drawer" so that when we open the app again we can make sure to use that drawer state?

First question I have is whether it would be better to use the useDrawerStatus() hook to track this?

https://reactnavigation.org/docs/drawer-navigator#checking-if-the-drawer-is-open

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it can be placed inside one of our main components like Expensify and update the status there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the hook is a good alternative, but in our cases, we can't use it on main components like Expensify because the component is not the child of the Drawer.Navigator. If we still want to use the hook, we need to add it to the screens component that uses the drawer navigation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok well wherever we can put it and it will work seems more straightforward than this screenListeners thing that is looking at a bunch of undocumented parameters that could change in the future?

Let's maybe pause this briefly while we figure out if a drawer status tracker is necessary.

},
}}
>
{_.map(this.props.screens, screen => (
<Drawer.Screen
Expand Down
20 changes: 18 additions & 2 deletions src/libs/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ import CustomActions from './CustomActions';
import ONYXKEYS from '../../ONYXKEYS';
import linkingConfig from './linkingConfig';
import navigationRef from './navigationRef';
import CONST from '../../CONST';

let isLoggedIn = false;
Onyx.connect({
key: ONYXKEYS.SESSION,
callback: val => isLoggedIn = Boolean(val && val.authToken),
});

let defaultDrawerStatus = CONST.DRAWER_STATUS.OPEN;
Onyx.connect({
key: ONYXKEYS.DEFAULT_DRAWER_STATUS,
callback: val => defaultDrawerStatus = val,
});

// This flag indicates that we're trying to deeplink to a report when react-navigation is not fully loaded yet.
// If true, this flag will cause the drawer to start in a closed state (which is not the default for small screens)
// so it doesn't cover the report we're trying to link to.
Expand Down Expand Up @@ -74,9 +81,18 @@ function closeDrawer() {
*/
function getDefaultDrawerState(isSmallScreenWidth) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really getting this method or why we are using it...

Found this...

// Calculate the defaultStatus only once on mount to prevent breaking the navigation internal state.
// Directly passing the dynamically calculated defaultStatus to drawer Navigator breaks the internal state
// And prevents the drawer actions from reaching to active Drawer Navigator while screen is resized on from Web to mobile Web.
defaultStatus: Navigation.getDefaultDrawerState(props.isSmallScreenWidth),

Which led me here:

#8067

The comment there makes no sense at all to me and vaguely references the "navigation internal state". @parasharrajat @thienlnam what exactly is this code doing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know the real reason for how this breaks it internally so I used the term navigation internal state but I found the following:

This component uses a key prop to remount the navigator when the browser resizes. This hack was done to prevent the layout from breaking on the drawer. Now due to that, it is somehow breaking the internal navigation state on change of defaultStatus. So instead of passing the prop directly to the navigator, I calculated it once per mount and passed. It seems to work at that time. I moved forward with this change as we can't remove the key hack. It breaks the UI for Drawer.

So in short, we can remove the key prop hack, we can get rid of this.

So as we are trying to move away from custom Logic. I think an audit should be done of our navigation implementation and bad patterns or hacks should be removed. I can take a look at that but not sure if I will be able to suggest a better design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know the real reason

Ok haha well, if you don't know the reason as the author who will? 😅

This component uses a key prop to remount the navigator when the browser resizes. This hack was done to prevent the layout from breaking on the drawer.

Ok and we are not sure why the layout breaking on the drawer in the first place? Or did we discover the root cause?

in short, we can remove the key prop hack, we can get rid of this.

Is there any open issue to figure out what the root cause is? Maybe would be good to create one issue that:

  • Documents the workaround we took and why with as much information as possible
  • Asks to investigate the root cause
  • Try to fix it in react-navigation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok haha well, if you don't know the reason as the author who will?

😄 I debugged the internal state during analyzing that issue and found that it was causing the wrong status ('open' | 'closed') on the history. The drawer state (closed or open) depends on status history entries. And the right value ('open' | 'closed') in the history entry depends on how you configure the defaultStatus prop. During my refactoring, I found that changing is dynamically set the wrong value (But don't know how). It was a workaround.

Is there any open issue to figure out what the root cause is? Maybe would be good to create one issue that:

I will find the ticket and details. Don't remember it correctly but it was either withWindowDimensions issue or Reanimated 1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out the main issue which talks about the layout issue #5591. I used the key prop hack to fix that issue (sorry for using the hack). I didn't find any other solution at that time.

But I think that issue is somehow linked with #2727. wrong dimensions may be causing bad style calculations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can create a new issue to get to the root cause of #5591 ? Temporary solution you have there seems fine for now, but could have been documented better.

if (didTapNotificationBeforeReady) {
return 'closed';
return CONST.DRAWER_STATUS.CLOSED;
}
return isSmallScreenWidth ? 'open' : 'closed';

if (isSmallScreenWidth) {
const path = getPathFromState(navigationRef.current.getState(), linkingConfig.config);

// If the initial route path is HOME SCREEN,
// return open for default status drawer instead of using value from Onyx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This comment should explain why the "home" route will always return a status of "open"

But that kind of begs the question of why defaultDrawerStatus is not already the correct value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This comment should explain why the "home" route will always return a status of "open"

Yeah, I will update the comment to make it more make sense.

But that kind of begs the question of why defaultDrawerStatus is not already the correct value?

When the user opens from URL https://staging.new.expensify.com I make it will open the LHN even if the latest drawer status is closed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user opens from URL https://staging.new.expensify.com I make it will open the LHN even if the latest drawer status is closed.

That makes sense. But wouldn't the opposite be true as well? If we are directly navigating to a "report route" then the drawer should always be closed.

I'm not sure I understand the case where we need to track and save the "last drawer status"?

Seems like if we did the following there would be no issue:

When the url is /r/<reportID> drawer should init closed
When the url is / drawer should init open

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have suggestion in here.

return path === '/' ? CONST.DRAWER_STATUS.OPEN : defaultDrawerStatus;
}

return CONST.DRAWER_STATUS.CLOSED;
}

/**
Expand Down
9 changes: 9 additions & 0 deletions src/libs/actions/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,22 @@ function fixAccountAndReloadData() {
});
}

/**
* Store drawer status to Onyx
* @param {String} defaultDrawerStatus
*/
function setDefaultDrawerStatus(defaultDrawerStatus) {
Onyx.set(ONYXKEYS.DEFAULT_DRAWER_STATUS, defaultDrawerStatus);
}

// When the app reconnects from being offline, fetch all initialization data
NetworkConnection.onReconnect(() => getAppData(true, false));

export {
setCurrentURL,
setLocale,
setSidebarLoaded,
setDefaultDrawerStatus,
getLocale,
getAppData,
fixAccountAndReloadData,
Expand Down