[Menu] Use strict mode compatible testing API

[eps1lon]

@testing-library/react makes these test so much more readable. Got rid of the focus trackers. IMO this is more of a bug that should be fixed but it's probably best the wait for Flare instead of implementing this ourselves.

Moved the fixture into the test. This was likely a remnant of the "one component per file" paradigm. But we want to see what we're testing so let's move them into the test.

Reduces as much indirection in tests as possible (isolate each case, prefer wet tests).

Reordered tests so that tests that only differ in variant are next to each other.
/cc @ryancogswell

[mui-pr-bot]

No bundle size changes comparing b3218e7...aa2ac87

Generated by 🚫 dangerJS against aa2ac87

[eps1lon]

This test has the same outcome as [variant=selectedMenu] nothing selected. Should [variant=selectedMenu] nothing selected focus the first item and this test shouldn't?

[ryancogswell]

This test has the same outcome as [variant=selectedMenu] nothing selected

The content anchor is different for this test.

[eps1lon]

Could you explain what you mean by different? I can't spot a difference visually between menu and selectedMenu for pre-selected items, no selected items or null-items:

https://github.com/mui-org/material-ui/blob/275a966d6ed7a479904e5c544978220e3e8d9711/test/regressions/tests/Menu/MenuContentAnchors.js

[ryancogswell]

selectedMenu is the default, so to see the variant-specific differences, you need to specify variant="menu"

[eps1lon]

That makes a lot more sense:

[eps1lon]

Updated descriptions to describe the test case not repeat what is tested.

If you write [variant=menu] nothing selected and render(<OpenMenu variant="menu"><MenuItem /></Menu>) then both of these lines contain the same information i.e. one contains only noise. Those description lines are used to describe the full test case which means what did we do and what is the expected behavior. They're useful to get a quick overview how a component behaves and check to we don't test too much in a single case.

Applied the following diff:

- it [variant=menu] nothing selected
+ specify [variant=menu] will focus the menu if nothing is selected

- it [variant=selectedMenu] nothing selected
+ specify [variant=selectedMenu] will focus the menu if nothing is selected

- it  [variant=menu] nothing selected, autoFocus on third
+ specify [variant=menu] ignores `autoFocus` on `MenuItem`

- it [variant=menu] second item selected
+ specify [variant=menu] ignores `selected` on `MenuItem`

- it [variant=selectedMenu] second item selected
+ specify [variant=selectedMenu] focuses the `selected` `MenuItem`

- it [variant=selectedMenu] second item selected, explicit tabIndex
+ specify [variant=selectedMenu] allows overriding `tabIndex` on `MenuItem`

- it [variant=selectedMenu] second item selected and disabled
+ specify [variant=selectedMenu] focuses the menu if the selected menuitem is disabled

- it [variant=selectedMenu] second item selected, no autoFocus
+ specify [variant=selectedMenu] focuses no part of the menu when `autoFocus={false}`

@ryancogswell I'm still missing some context about menu vs selectedMenu. The're grouped inside "Menu variant differences" but most of the test cases don't test how the other variant behaves. I thought initially this means they behave the same but this doesn't hold true for * will focus the menu if nothing is selected where both variants behave the same.

Do we even need this variant switch? Currently we allow <Menu variant="menu"><MenuItem selected /></Menu> where selected is a no-op for MenuItem. It seems simpler to just focus the item if the selected prop is found regardless of the variant.

The vertical alignment should probably be a single prop? I'd even consider removing that behavior completely in v5.

[ryancogswell]

The're grouped inside "Menu variant differences"

Yes, sorry about this bad description. Initially I had three values for variant, because I thought I needed slightly different behavior for Select. This group of tests was methodically verifying the focus, tabindex, and content anchor for all three variants for a number of different scenarios. As I iterated on it, I removed the select variant. As I made further changes, I found myself maintaining a lot of tests that weren't adding any value, so I stripped out the ones that weren't adding value. As I made further changes, I added more tests in here that weren't about the variant differences, but were just tests where I wanted to leverage the focus/tabindex/content anchor verifications. I should have updated the description at that point, but didn't.

Do we even need this variant switch? Currently we allow <Menu variant="menu"><MenuItem selected /></Menu> where selected is a no-op for MenuItem.

Its value is pretty minimal and based on use cases invented in my head rather than asked for by users. I was also planning to eventually add more values for the variant property to support dropdowns and possibly for cascading menus. So, all in all, not sufficient justification for broadening the API surface area, since even if it is needed for dropdowns it could be added at that point.

My intent with the menu variant, was to allow someone to leverage the selected property for styling purposes (make the MenuItem look selected) without getting the associated focus and content anchor behaviors. In particular, as I was working on supporting disabled items in the focus navigation, I was thinking about someone having a "selected" and "disabled" menu item (i.e. not letting the user interact with the current selection). If someone wanted this behavior, using variant="menu" would avoid the library trying to put focus on the disabled item. But again, no one was asking for this.