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

[V3] options:sideMenu:enabled: false overwritten to true on pan gesture #5444

Closed
Royce opened this issue Sep 2, 2019 · 8 comments
Closed

Comments

@Royce
Copy link
Contributor

Royce commented Sep 2, 2019

sideMenu.enabled flag overwritten when opening/closing menu.

On iOS, the flags to control whether the side menus are enabled or disabled is reset to true when a pan gesture (to open the menu) occurs.

Steps to Reproduce / Code Snippets / Screenshots

With the following setup. Both menu's disabled...

      Navigation.setRoot({
        root: {
          sideMenu: {
            id: 'RootLayoutId',
            options: {
              // The options are optional, but makes the bug more obvious.
              sideMenu: {
                left: { enabled: false },
                right: { enabled: false },
              },
            },
            left: { component: { name: 'MenuScreen' } },
            right: { component: { name: 'MenuScreen' } },
            center: { component: { name:'MainScreen' } },
          },
        },
      });

Then, triggered off a button or something, we explicitly enable one (and disable the other) of the two menu's:

      Navigation.mergeOptions("RootLayoutId", {
        sideMenu: {
          left: { enabled: true }, 
          right: { enabled: false }  // ** Need to set this too to get the bug.
        }
      });
  1. Swipe left (should open menu). Now close it again.
  2. Swipe right (should not open menu)... but it does.

The problem.

The function SideMenuPresenter.applyOptions contains the following code, and is called when we Navigation.setRoot, but also when SideMenuController.panGestureCallback is triggered.

[sideMenuController side:MMDrawerSideLeft enabled:[withDefault.sideMenu.left.enabled getWithDefaultValue:YES]];
[sideMenuController side:MMDrawerSideRight enabled:[withDefault.sideMenu.right.enabled getWithDefaultValue:YES]];

The getWithDefaultValue:YES part means that enabled keeps getting set to true. No good!

Workaround / hack.

Wrap an if around these lines of code.
Royce@8d2b20c

But this may have side effects that I am not considering.


Environment

  • React Native Navigation version: 3.1.2 (also 3.0.0)
  • React Native version: 0.60.5
  • Platform(s) (iOS, Android, or both?): both, but iOS fixed by 3.1.2-snapshot.490
@Royce Royce changed the title [V3] iOS options: sideMenu: enabled: false overwritten on menu pan gesture [V3] iOS options:sideMenu:enabled: false overwritten to true on pan gesture Sep 2, 2019
@guyca guyca self-assigned this Sep 3, 2019
guyca added a commit that referenced this issue Sep 4, 2019
* Enabled option was wrongly consumed after it was merged. That meant that the next time SideMenu options were applied, the wrong enabled value was applied
* supportedInterfaceOrientations didn't take default orientation value into account
* mergeOptions didn't save the new options in the ViewControllers current options
* SideMenu always returned the centre ViewController as the current child and didn't take open SideMenu into account

Fixes #5444
@guyca guyca closed this as completed in 5816fb8 Sep 4, 2019
@guyca
Copy link
Collaborator

guyca commented Sep 4, 2019

@Royce Could you please try version 3.1.2-snapshot.490 and let me know if there are any issues?

guyca added a commit that referenced this issue Sep 4, 2019
* Enabled option was wrongly consumed after it was merged. That meant that the next time SideMenu options were applied, the wrong enabled value was applied
* supportedInterfaceOrientations didn't take default orientation value into account
* mergeOptions didn't save the new options in the ViewControllers current options
* SideMenu always returned the centre ViewController as the current child and didn't take open SideMenu into account

Fixes #5444
@Royce
Copy link
Contributor Author

Royce commented Sep 4, 2019

@guyca I have updated to 3.1.2-snapshot.490 and can confirm the problem has gone away on iOS. Thanks!

@guyca guyca reopened this Sep 5, 2019
@guyca
Copy link
Collaborator

guyca commented Sep 5, 2019

Reopened as this seems to be a problem on Android now 😕

@Royce Royce changed the title [V3] iOS options:sideMenu:enabled: false overwritten to true on pan gesture [V3] options:sideMenu:enabled: false overwritten to true on pan gesture Sep 6, 2019
@Royce
Copy link
Contributor Author

Royce commented Sep 6, 2019

In SideMenuPresenter applyLockMode treats enable being undefined as true, So we have to be very careful that it's not undefined when it shouldn't be.

    private void applyLockMode(SideMenuRootOptions options) {
        int leftLockMode = options.left.enabled.isTrueOrUndefined() ? DrawerLayout.LOCK_MODE_UNLOCKED : DrawerLayout.LOCK_MODE_LOCKED_CLOSED;
        sideMenu.setDrawerLockMode(leftLockMode, Gravity.LEFT);

        int rightLockMode = options.right.enabled.isTrueOrUndefined() ? DrawerLayout.LOCK_MODE_UNLOCKED : DrawerLayout.LOCK_MODE_LOCKED_CLOSED;
        sideMenu.setDrawerLockMode(rightLockMode, Gravity.RIGHT);
    }

applyLockMode is called by SideMenuPresenter.applyChildOptions which is called by SideMenuController.applyChldOptions

    @Override
    public void applyChildOptions(Options options, ViewController child) {
        super.applyChildOptions(options, child);
        presenter.applyChildOptions(resolveCurrentOptions());
        performOnParentController(parentController ->
                ((ParentController) parentController).applyChildOptions(this.options, child)
        );
    }

Looking at resolveCurrentOptions leads us to ParentController.resolveCurrentOptions. Setting a breakpoint here (during a pan gesture, perhaps) show that the initialOptions, which contain sidemenu.left.enable = false etc., are not being transferred to the copy of the child options (i.e. the left menu view) which has enable = null etc.

    public Options resolveCurrentOptions() {
	    if (CollectionUtils.isNullOrEmpty(getChildControllers())) return initialOptions;
        return getCurrentChild()
                .resolveCurrentOptions()
                .copy()
                .withDefaultOptions(initialOptions);
    }

And thus the side menu / drawers are unlocked when they shouldn't be.

@Royce
Copy link
Contributor Author

Royce commented Sep 6, 2019

Hey @guyca I figured out where the problem occurs (see above comment) and have two possible solutions (see #5459, #5460), both of these solutions solved the problem for me but may not line up with the intention of the existing code.

@guyca guyca closed this as completed in 46ca0f3 Sep 6, 2019
@guyca
Copy link
Collaborator

guyca commented Sep 6, 2019

Thanks for looking into this one @Royce !
I merged one of the PRs and added another minor fix, available in 3.1.2-snapshot.493

@Royce
Copy link
Contributor Author

Royce commented Sep 7, 2019

3.1.2-snapshot.493 has introduced a new problem. Demo:

function start() {
  registerScreens();
  Navigation.events().registerAppLaunchedListener(async () => {
    setDefaultOptions();

    Navigation.setRoot({
      root: {
        sideMenu: {
          id: "RootLayoutId",
            options: {
              // The options are the implicit defaults. So, not required.
              sideMenu: {
                left: { enabled: true },
                right: { enabled: true },
              },
            },
          left: { component: { name: Screens.SideMenuLeft, id: "LEFT" } },
          right: { component: { name: Screens.SideMenuRight, id: "RIGHT" } },
          center: {
            stack: {
              children: [{ component: { name: Screens.SideMenuCenter } }]
            }
          }
        }
      }
    });
  }
}

Then programatically open one menu (and close it), then open the other....

Navigation.mergeOptions("RootLayoutId", { sideMenu: { left: { visible: true } } });

Swipe it close, or whatever.... then:

Navigation.mergeOptions("RootLayoutId", { sideMenu: { right: { visible: true } } });

The problem:

Both menu's become visible! (Only the right should become visible.)

@Royce
Copy link
Contributor Author

Royce commented Sep 7, 2019

The above demo can be played with in this branch of the playground app: https://github.com/Royce/react-native-navigation/tree/demo-sticky-sidemenu-visible

And this tweak to SideMenuOptions.java makes the problem go away.

public void mergeWithDefault(SideMenuOptions defaultOptions) {
     // Just need to forget the old visible flag. Like this, maybe.
    //if (!visible.hasValue()) visible = defaultOptions.visible;
    if (!animate.hasValue()) animate = defaultOptions.animate;
    if (!enabled.hasValue()) enabled = defaultOptions.enabled;
    if (!height.hasValue()) height = defaultOptions.height;
    if (!width.hasValue()) width = defaultOptions.width;
}

vshkl pushed a commit to vshkl/react-native-navigation that referenced this issue Feb 5, 2020
* Enabled option was wrongly consumed after it was merged. That meant that the next time SideMenu options were applied, the wrong enabled value was applied
* supportedInterfaceOrientations didn't take default orientation value into account
* mergeOptions didn't save the new options in the ViewControllers current options
* SideMenu always returned the centre ViewController as the current child and didn't take open SideMenu into account

Fixes wix#5444
vshkl pushed a commit to vshkl/react-native-navigation that referenced this issue Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants