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

bug: menu stop working in an application that has two main page with each a side menu #18974

Closed
iamkinetic opened this issue Aug 1, 2019 · 41 comments · Fixed by #28269
Closed
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@iamkinetic
Copy link

iamkinetic commented Aug 1, 2019

Bug Report

Ionic version:
[x] 4.7.X
[x] 4.8.X
[x] 4.9.0
[x] 4.10

Current behavior:
Application has two main pages. Each of these pages have a different side menu. When navigating the application this way, the menu stop being displayed.

Expected behavior:
Side menu should not stop being displayed when navigating.

Steps to reproduce:

  1. Open side menu from initial page and click "Page # 2 with menu # 2
  2. Navigation works, and side menu # 2 is displayed.
  3. Open side menu and click "Page # 1 with menu # 1".
  4. Page # 1 is correctly loaded except the menu # 1 is not displayed anymore.
    2b5e74df-4e47-4481-9520-782732a0049d

Related code:

Sample application on GitHub:
https://github.com/iamkinetic/ionic4-multiple-menus-bug

Ionic info:

Ionic:

   Ionic CLI                     : 5.2.1 (C:\Users\iamkinetic\AppData\Roaming\npm\node_modules\ionic)
   Ionic Framework               : @ionic/angular 4.7.1
   @angular-devkit/build-angular : 0.801.3
   @angular-devkit/schematics    : 8.1.3
   @angular/cli                  : 8.1.3
   @ionic/angular-toolkit        : 2.0.0

Utility:

   cordova-res : not installed
   native-run  : 0.2.7

System:

   NodeJS : v11.4.0 (C:\Program Files\nodejs\node.exe)
   npm    : 6.10.2
   OS     : Windows 10
@iamkinetic
Copy link
Author

Does anyone knows of any work around for this bug?

@iamkinetic
Copy link
Author

I believe #18683 may be related to this issue.

@iamkinetic
Copy link
Author

@liamdebeasi Do you think you could take a look at this problem? I've tried fixing the problem for two weeks and this is the only thing preventing me from releasing a new version of our application.

If i need to rethink my navigation I will, but i'd like to be sure this is what causing my problem.

Thanks.

@iamkinetic
Copy link
Author

iamkinetic commented Aug 21, 2019

Bug still present with @ionic/angular 4.8.0 + angular 8.2.3.

@iamkinetic
Copy link
Author

iamkinetic commented Aug 26, 2019

This is my current "workaround" (it's far from perfect):
In my CanActive's guard, I've add some code to enable and make the menu works "correctly". I need to manually activate the menu to make it visible (even though it should already be visible) and then I need to add "menu-pane-visible" to the menu's classes. All of this in a setTimeout.

 setTimeout(async () => {
            await this.menuController.enable(true, 'my-menu');
            const menu = await this.menuController.get('my-menu');
            if (!menu.className.includes('menu-pane-visible')) {
              menu.className = menu.className + ' menu-pane-visible';
            }
          }, 200);

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Sep 5, 2019

Sorry for the delay in response. Which version of Ionic did this issue start happening with?

@iamkinetic
Copy link
Author

I've been porting my application from Ionic 3 to Ionic 4 since last may (only a few hours a month at first), so I've went through a few differents versions (from 4.1 to 4.9). The ported application wasn't really usable before mid-july and I was using Ionic 4.6 back then. I've just tried that version and the menu's bug was already present.

So I guess I can't really say for sure when the issue start happening.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 5, 2019
@iamkinetic
Copy link
Author

Oh and I never have more than one menu visible at once and I also have that issue when testing in a browser or with cordova (ios and android).

@liamdebeasi
Copy link
Contributor

Ok thanks for the additional information. Sounds like this could be related to #18683 as you suggested previously.

@iamkinetic
Copy link
Author

iamkinetic commented Sep 5, 2019

I don't know about that, I've tried with and without Ion-Split-Pane and the menu didn't behave correctly either way. The wrong classes are injected to the menu component when navigating back and forth between my two pages more than twice. It's like it's trying to hide the menu from the previous page, bug it's really hiding the menu from the current page.

@liamdebeasi
Copy link
Contributor

Ok thanks for the additional info. Let me dig into this a bit and see what I can find.

@liamdebeasi
Copy link
Contributor

Related issue: #19280

@iamkinetic
Copy link
Author

@liamdebeasi Any news about this?

@iamkinetic
Copy link
Author

Still broken with 4.10...

@proemke
Copy link

proemke commented Oct 14, 2019

Still broken @ 4.11

My work around: (hope it's a temp fix....)

ionViewWillEnter() {
    const menuid = 'agenda-menu';
    this.menuController.enable(true, menuid);
    this.menuController.get(menuid).then(
      (thisMenu) => {
        if (!thisMenu.className.includes('menu-pane-visible')) {
          thisMenu.className = `${thisMenu.className} menu-pane-visible`;
        }
    });
  }

Using tabs as a base, every tab has it's own menu.

@iamkinetic
Copy link
Author

Yeah, that's basically my workaround. It works ok on a phone, but not so much on a tablet unfortunately.

image

@proemke
Copy link

proemke commented Oct 21, 2019

Still searching for a solution...
Setting the style 'menu-pane-visible' gives the same result as @iamkinetic, visible has to be set directly in the component, when a second menu is loaded all others reset to 0.

@proemke
Copy link

proemke commented Oct 21, 2019

Found en Fixed ... Later today I'll create a pull request.

@iamkinetic
Copy link
Author

Do you have a fork somewhere with that fix? I could help with testing.

@proemke
Copy link

proemke commented Oct 21, 2019

@iamkinetic Check the pull-request 😉

@iamkinetic
Copy link
Author

@proemke What would be the correct way for me to test your change in my repo? Can I install @ionic/core from your PR? How?

@brandyscarney
Copy link
Member

Please see my comment here: #19721 (comment)

Thank you!

@liamdebeasi
Copy link
Contributor

I investigated this issue a bit more and have determined that the root cause of this issue is in the router integration in Ionic Angular, not the menu or split pane.

Starting on the first page with menu A and then navigating to the second page with menu B causes menu A to be disabled; however, navigating back to the first page with menu A does not re-enable the menu. This is why the reported bug is happening.

The workaround @brandyscarney suggested in #19721 (comment) works because upon navigating to the second page, the first page and its menu are completely removed from the DOM. Upon navigating back to the first page, the first page and its menu are added back to the DOM as a new instance of that component. This results in menu A being enabled by default.

I do not think we should merge #19721 as it does not address the root issue. I will investigate more into how we can properly resolve this issue. For now I recommend that you use the workaround Brandy presented.

Thanks!

@iamkinetic
Copy link
Author

@liamdebeasi The workaround seems to be working fine on both the bug repo and my real application repo when testing in browser. I'll test it on a real device tomorrow, but I'm confident it will work correctly.

Thanks for the help to both of you.

Should we keep this issue open until you guys find the root cause of the issue?

@brandyscarney
Copy link
Member

@iamkinetic Yes please keep this open! We'll have to investigate a solution within the router. Let us know how the workaround works, hopefully it doesn't have any negative side effects.

@iamkinetic
Copy link
Author

@brandyscarney @liamdebeasi The workaround is working fine in both iOS and Android devices. There is still some display issues with the menu when testing in Safari, but I don't really care because the bug I had was in a mobile application.

@solenmarketing
Copy link

Hi! any solution? i am working with ionic 6 and angular 11 and i have the same problem

@RaccoonFive
Copy link

We also have this issue in our app and we used the workaround. Would be nice if it could be fixed!

@liamdebeasi
Copy link
Contributor

Thanks for the feedback everyone. I am going to bump this up in priority. I don't have an ETA for a fix, but I will post an update here when I have more to share.

@HthSolid
Copy link

HthSolid commented Nov 10, 2022

Still existing in Ionic 6 and React 18.2
You navigate away from page1, the menu gets deactivated
go back to page1 and menu is still deactivated

Workaround is to pass the current page location from window.location.pathname down to the tabs via state, i.e. make an onclick listener on the navigation button like tab, and use the disabled attribute on the IonMenu to disable the menu if the current page is not the active page.

example of implementation:

//ON app.tsx
const getLocation = ()=>{
    let _l = window.location.pathname.replace('/',''); //works better on mobile
    _l = _l ? _l.split('?')[0] : '';
    return _l;
}
setState({page:getLocation()});

const pageAdjust = (e:any)=>{
  const _p = e.detail.tab;
  setState({page:_p});
 };

<IonTabButton tab="tab1" href="/tab1" onClick={pageAdjust}>
              <IonIcon icon={} />
              <IonLabel></IonLabel>
</IonTabButton>

//ON tab1.tsx
<IonMenu disabled={state.page != 'tab1'}>

@CatalanCabbage
Copy link

Still broken on Ionic+Vue unfortunately,

@eesdil
Copy link

eesdil commented May 3, 2023

if somebody is still struggling with it, this would be an easy workaround in angular using the #menu element:

<ion-menu #menu>
  <ion-header>
    <ion-toolbar>
      <ion-buttons slot="end">
        <ion-button icon-only (click)="menu.close()"
          ><ion-icon name="menu"></ion-icon
        ></ion-button>
      </ion-buttons>
    </ion-toolbar>
  </ion-header>
  <ion-content> ... menu content ... </ion-content>
</ion-menu>

<ion-header>
  <ion-toolbar>
    <ion-buttons slot="start">
      <ion-button icon-only (click)="menu.open()"
        ><ion-icon name="menu"></ion-icon
      ></ion-button>
    </ion-buttons>
  </ion-toolbar>
</ion-header>
<ion-content>
  ... bla bla ...
</ion-content>

@Dalmi200
Copy link

if somebody is still struggling with it, this would be an easy workaround in angular using the #menu element:

<ion-menu #menu>
  <ion-header>
    <ion-toolbar>
      <ion-buttons slot="end">
        <ion-button icon-only (click)="menu.close()"
          ><ion-icon name="menu"></ion-icon
        ></ion-button>
      </ion-buttons>
    </ion-toolbar>
  </ion-header>
  <ion-content> ... menu content ... </ion-content>
</ion-menu>

<ion-header>
  <ion-toolbar>
    <ion-buttons slot="start">
      <ion-button icon-only (click)="menu.open()"
        ><ion-icon name="menu"></ion-icon
      ></ion-button>
    </ion-buttons>
  </ion-toolbar>
</ion-header>
<ion-content>
  ... bla bla ...
</ion-content>

your solution works great, but the previously existing touch slide for opening does not work like this.

@DinosaurDad
Copy link

The issue still exists in Ionic 7.2, at least in Vue. Take a look at this SO post for a solution that may help.

https://stackoverflow.com/questions/76908311/ionic-vue-menu-breaks-after-navigating-to-another-page

@liamdebeasi
Copy link
Contributor

Hi everyone,

Here is a dev build with a proposed fix if anyone would like to test: 7.4.3-dev.11696514206.1c9669de

Install Example:

npm install @ionic/[email protected] @ionic/[email protected]

A couple notes about this bug fix:

  1. If you are using multiple ion-split-pane instances you may still run into a scenario where ion-menu is not visible. This is being tracked in bug: navigating with multiple split panes causes first menu to never be reactivated #18683. I have a partial fix in place, but fixing the first bug revealed a secondary bug which I still need to fix.
  2. Developers using menuController and referencing menus by side should consider switching to using IDs instead. Using menuController.open('start') will potentially result in inconsistent behaviors if there are multiple menus in the DOM on the start side. Doing menuController.open('first-menu') where first-menu is the ID of an ion-menu will avoid this inconsistency.

liamdebeasi added a commit that referenced this issue Oct 10, 2023
Issue number: resolves #18974

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When multiple menus on the same `side` are registered, all but the most
recent menu are disabled. For example, if a user starts on PageA with a
`start` menu and then navigates to PageB which also has a `start` menu,
then the menu on PageA will be disabled. The problem is that if users
navigates back to PageA they will be unable to open the menu on that
view because it is still disabled. This behavior impacts any Ionic
developer trying to open a menu whether by calling the `open` method on
the menu itself or on the `menuController`.

After discussing with the team, we believe the original intent of this
behavior was to prevent users from accidentally opening the wrong menu
when calling `menuController.open('start')`. This API allows developers
to reference a menu by side, and since it's possible to have multiple
menus on the same side it's also possible to open the wrong menu when
referencing by side only.

However, this API starts to break down pretty quickly in a navigation
scenario.

Sample Repo: https://github.com/liamdebeasi/multiple-menu-bug-repro

## Scenario 1: Referencing Menu by Side

1. On the "home" route click "Open 'start' menu". Observe that the home
page menu opens.
2. Close the menu and click "Go to Page Two".
3. On the "page-two" route click "Open 'start' menu". Observe that the
page two menu opens.
4. Go back to "home".
5. Click "Open 'start' menu". Observe that nothing happens.
6. Click "Enable and Open 'start'" Menu". Observe that the home menu
opens.

## Scenario 2: Referencing Menu by ID

1. On the "home" route click "Open '#menu1' menu". Observe that the home
page menu opens.
2. Close the menu and click "Go to Page Two".
3. On the "page-two" route click "Open '#menu2' menu". Observe that the
page two menu opens.
4. Go back to "home".
5. Click "Open '#menu1' menu". Observe that nothing happens.
6. Click "Enable and Open '#menu1'" Menu". Observe that the home menu
opens.

## Scenario 3: Using 3 or more menus even when enabling menus

1. On the "home" route click "Open 'start' menu". Observe that the home
page menu opens.
2. Close the menu and click "Go to Page Two".
3. On the "page-two" route click "Open 'start' menu". Observe that the
page two menu opens.
4. Close the menu and click "Go to Page Three"
5. On the "page-three" route click "Open 'start' menu". Observe that the
page three menu opens.
6. Go back to "page-two".
8. Click "Open 'start' menu". Observe that nothing happens.
9. Click "Enable and Open 'start' Menu". Observe that nothing happens.

The menu controller attempts to find an enabled menu on the specified
side:
https://github.com/ionic-team/ionic-framework/blob/a04a11be3571faa99c751edc034462e94a977e95/core/src/utils/menu-controller/index.ts#L79C12-L79C12

Step 6 is where this breaks down. In this scenario, the menus on "home"
and "page-two" are disabled. This leads menu controller to use its
fallback which tries to get the first menu registered on the specified
side:
https://github.com/ionic-team/ionic-framework/blob/a04a11be3571faa99c751edc034462e94a977e95/core/src/utils/menu-controller/index.ts#L86

This means that the menu controller would attempt to open the "home"
menu even though the user is on "page-two" (because the start menu on
"home" was the first to be registered).

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Menus are no longer automatically disabled when a new menu on the same
side is registered
- Referencing menus by side when multiple menus with that side exist in
the DOM will cause a warning to be logged

This change has a couple implications:

1. Developers no longer need to manually enable a menu as noted in
https://ionicframework.com/docs/api/menu#multiple-menus. Note that
continuing to manually enable the menus will not cause any adverse side
effects and will effectively be a no-op.
2. Developers using the menuController to open a menu based on "side"
may end up having the wrong menu get opened.

Example before to this change:

1. Start on PageA with a `start` menu. Calling
`menuController.open('start')` opens the menu on PageA.
2. Go to PageB with a `start` menu. Calling
`menuController.open('start')` opens the menu on PageB because the menu
on PageA is disabled.

Example after to this change:

1. Start on PageA with a `start` menu. Calling
`menuController.open('start')` opens the menu on PageA.
2. Go to PageB with a `start` menu. Calling
`menuController.open('start')` attempts to opens the menu on PageA
because both menus are enabled. However, since PageA is hidden nothing
will appear to happen.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->

## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

I manually verified that removing the Angular Universal code does not
regress the behavior fixed in
#27814. The menu is
never automatically disabled, so the bug does not happen.

This is a partial fix for
#18683. Properly
fixing this requires another change which is out of scope for this work.
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #28269, and a fix will be available in an upcoming release of Ionic Framework.

This introduces a new behavior for menu, so this will ship in a minor release of Ionic rather than a patch release.

Please feel free to continue testing the dev build, and let me know if you have questions.

Copy link

ionitron-bot bot commented Nov 9, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.