fix(menu-bar): z-index not restored on menu close

[oliversalzburg]

When an mdMenuBar contains a menu and that menu is opened, the menu bar will receives a CSS class that ultimately affects its z-order. When the menu is closed, the CSS class is supposed to be removed from the menu bar.

When the menu has multiple levels, the menu hierarchy can not be correctly resolved upon menu closure. This results in the CSS class not being removed from the menu bar.

I assumed that this failure is mostly due to the attempt to detect menu structure through DOM traversal, which will not work in this case as the menu hierarchy is not directly reflected in the DOM structure (menus are attached to the body instead).

Fixes #11235

PR Checklist

Please check that your PR fulfills the following requirements:

• The commit message follows our guidelines
• Tests for the changes have been added or this is not a bug fix / enhancement
• Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #11235

What is the new behavior?

I store a reference to the original menu bar controller to establish the hierarchy when handling the menu closure. This way the CSS class is properly removed from the menu bar and Z order is restored.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

[Splaktar]

Thanks for this PR. I'm happy with the approach, but it would be great to get 1-2 basic tests written to verify this.

Oh and in the commit message and PR title, did you mean z-index?

[oliversalzburg]

Thanks for the quick review! I'll get back to it later tonight.

[oliversalzburg]

I did mean z-index, but I thought I had to upper-case it. I will fix it in the next change.

I will look into the unit tests.

[oliversalzburg]

I can't get ChromeHeadless to work in the DevContainer I'm using. I can't properly write the tests like this. I might try a couple of pushes and see what I get out of CircleCI, but I might have to temporarily downgrade some machine to Node 10 to finish this later.

[oliversalzburg]

Alright, those were though to write, but I think I got something acceptable.

In the should close when clicking on a nested menu item test, I also wanted to spy on the close() of the nested menu controller, but I have no idea how to retrieve it. I opted to skip that check. If you feel like it should be added, please help me out with that controller reference.

In general, I didn't add tests specifically for the existence of the class, but focused on the general closing behavior that was already under test by existing tests. These tests included assertions for the removal of the md-has-open-menu class. So it seemed to make most sense to just extend this area of the test suite to cover more deeply nested menus.

[Splaktar]

I might have to temporarily downgrade some machine to Node 10 to finish this later.

I'm not sure if you're on Windows or not, but NVM is one of a few options for quickly switching your NodeJS installation. The most mature one is macOS, but there is a separate version for Windows as well (plus another option or two).

[oliversalzburg]

Yeah, I worked with nvm/nvs/... in the past (and I'm on Windows using bash). It always ended up causing more problems than it solved.

I opted to create a quick Hyper-V Windows Development VM. Worked well enough. Thanks for the suggestion though :)

[Splaktar]

LGTM

[oliversalzburg]

Should I rewrite the commit message to use fix(menu-bar): as well?

[Splaktar]

That would be helpful, thank you.

[jelbourn]

LGTM