[menu-bar] Add possibility to disable roving tabindex

[jcgueriaud1]

Description

MenuBar is sometimes used with styling that makes MenuItems appear as separate Buttons. Especially in this case the expected focus behavior would be to move focus between MenuItems with TAB key instead of arrow keys.

Fixes #6718

Acceptance Criteria: vaadin/platform#6552

Few things that are not in the AC.
When the tabNavigation is true, the navigation with Arrow keys is not removed. You can still navigate with TAB and arrow keys. The behavior is similar to a MacOs menu (for example).
You can still create submenu and they will work the same way (same navigation).
The overflow menu will work as before (like a submenu).
There is no specific tests when it's working the same way.

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.
• I have not completed some of the steps above and my pull request can be closed immediately.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[web-padawan]

Found one issue so far: when pressing Arrow Left / Arrow Right on the sub-menu item, we move the focus to the other button while also opening corresponding sub-menu.

This doesn't currently work with Tab, see the following example:

menu-tab.mp4

<vaadin-menu-bar tab-navigation></vaadin-menu-bar>

<script type="module">
  import '@vaadin/menu-bar';

  const menuBar = document.querySelector('vaadin-menu-bar');
  menuBar.items = [
    { text: 'View' },
    { text: 'Edit' },
    {
      text: 'Share',
      children: [{ text: 'By email' }, { text: 'Get link' }],
    },
    {
      text: 'Move',
      tooltip: 'Move to a different folder or trash.',
      children: [{ text: 'To folder' }, { text: 'To trash' }],
    },
  ];
</script>

1. Focus the last button with Tab or arrow key
2. Press Enter to open the sub-menu
3. Press Arrow Left - the sub-menu will move to previous button
4. Press Tab - focus moves but the sub-menu does not

[jcgueriaud1]

Thanks for the fast review, I'll do the changes asap.

I noticed that the snapshot was failing but I'm not sure if it's 'bad' or not.
Previously the vaadin-menu-bar was creating and all the vaadin-menu-bar-button have tabindex=0 by default.
On focus, the tab index is updated and the first item has tabindex=0 the other tabindex=-1.

Screen.Recording.2024-07-10.at.14.10.11.mov

Now with the changes, the observer _tabNavigationChanged is called then (without tabNavigation) the first item is tabindex=0 and the other items have tabindex=-1.

The changed behavior doesn't look wrong.

Hopefully the video and screenshot will help to understand the change.

[web-padawan]

I noticed that the snapshot was failing but I'm not sure if it's 'bad' or not.

Good point. In fact it only affects the case of using Shift Tab to get to the MenuBar from another focused element placed after it in the DOM: previously, the last button would gain focus, and now it's the first one.

Personally I think it's a minor change, and the old behavior could be considered confusing. So IMO we can keep it and just update the snapshot to pass with the change using yarn update:snapshots --group menu-bar.

[jcgueriaud1]

I've just added some tests for:

• Navigation with Tab and shiftTab (I'm using import { shiftTab, tab } from '@vaadin/app-layout/test/helpers.js'; ), maybe that should be changed
• Test the submenu on Tab navigation (That should still be opened)

[web-padawan]

I'm using import { shiftTab, tab } from '@vaadin/app-layout/test/helpers.js'; ), maybe that should be changed

Yes, it's better to copy these helpers and place them in a separate file packages/menu-bar/test/helpers.js so that we don't have an unnecessary implicit dependency on app-layout package.

[jcgueriaud1]

Done

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[web-padawan]

SauceLabs tests failed due to the fact that it's a PR from fork. This is fine in this case as theme isn't changed.

The code LGTM, made some small tweaks (see latest commits). I'll go ahead and merge the PR.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.5.0.alpha10 and is also targeting the upcoming stable 24.5.0 version.