fix keyboard navigation issues with windowTopMenu

__tests__/src/components/WindowThumbnailSettings.test.js

@@ -20,28 +20,28 @@ describe('WindowThumbnailSettings', () => {
   it('renders all elements correctly', () => {
     createWrapper();
     expect(screen.getByRole('presentation', { selector: 'li' })).toBeInTheDocument();
-    expect(screen.getByRole('menuitem', { name: /Off/ })).toBeInTheDocument();
-    expect(screen.getByRole('menuitem', { name: /Bottom/ })).toBeInTheDocument();
-    expect(screen.getByRole('menuitem', { name: /Right/ })).toBeInTheDocument();
+    expect(screen.getByRole('menuitemradio', { name: /Off/ })).toBeInTheDocument();
+    expect(screen.getByRole('menuitemradio', { name: /Bottom/ })).toBeInTheDocument();
+    expect(screen.getByRole('menuitemradio', { name: /Right/ })).toBeInTheDocument();
   });
   it('for far-bottom it should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ thumbnailNavigationPosition: 'far-bottom' });
-    expect(screen.getByRole('menuitem', { name: /Bottom/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Bottom/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
   it('for far-right it should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ thumbnailNavigationPosition: 'far-right' });
-    expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Bottom/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Bottom/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
 
   it('updates state when the thumbnail config selection changes', async () => {
     const setWindowThumbnailPosition = vi.fn();
     const user = userEvent.setup();
     createWrapper({ setWindowThumbnailPosition });
-    const menuItems = screen.queryAllByRole('menuitem');
+    const menuItems = screen.queryAllByRole('menuitemradio');
     expect(menuItems.length).toBe(3);
     expect(menuItems[0]).toBeInTheDocument();
     expect(menuItems[1]).toBeInTheDocument();
@@ -57,6 +57,6 @@ describe('WindowThumbnailSettings', () => {
 
   it('when rtl flips an icon', () => {
     createWrapper({ direction: 'rtl' });
-    expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).toHaveStyle('transform: rotate(180deg);'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).toHaveStyle('transform: rotate(180deg);'); // eslint-disable-line testing-library/no-node-access
   });
 });

__tests__/src/components/WindowTopMenu.test.js

@@ -33,16 +33,21 @@ describe('WindowTopMenu', () => {
 
     const menuSections = within(screen.getByRole('menu')).getAllByRole('presentation');
     expect(menuSections).toHaveLength(2);
+
     expect(menuSections[0]).toHaveTextContent('View');
-    expect(menuSections[1]).toHaveTextContent('Thumbnails');
+    const menus = within(screen.getByRole('menu')).getAllByRole('menubar');
 
-    const menuItems = screen.getAllByRole('menuitem');
-    expect(menuItems).toHaveLength(5);
-    expect(menuItems[0]).toHaveTextContent('Single');
-    expect(menuItems[1]).toHaveTextContent('Gallery');
-    expect(menuItems[2]).toHaveTextContent('Off');
-    expect(menuItems[3]).toHaveTextContent('Bottom');
-    expect(menuItems[4]).toHaveTextContent('Right');
+    const viewItems = within(menus[0]).getAllByRole('menuitemradio');
+    expect(viewItems).toHaveLength(2);
+    expect(viewItems[0]).toHaveTextContent('Single');
+    expect(viewItems[1]).toHaveTextContent('Gallery');
+
+    expect(menuSections[1]).toHaveTextContent('Thumbnails');
+    const thumbnailItems = within(menus[1]).getAllByRole('menuitemradio');
+    expect(thumbnailItems).toHaveLength(3);
+    expect(thumbnailItems[0]).toHaveTextContent('Off');
+    expect(thumbnailItems[1]).toHaveTextContent('Bottom');
+    expect(thumbnailItems[2]).toHaveTextContent('Right');
   });
 
   it('does not display unless open', () => {
@@ -66,7 +71,7 @@ describe('WindowTopMenu', () => {
     />);
 
     // click a menu item should close the menu
-    const menuItems = screen.getAllByRole('menuitem');
+    const menuItems = screen.getAllByRole('menuitemradio');
     await user.click(menuItems[0]);
     expect(handleClose).toHaveBeenCalledTimes(1);
     expect(toggleDraggingEnabled).toHaveBeenCalledTimes(1);

__tests__/src/components/WindowTopMenuButton.test.js

@@ -33,7 +33,7 @@ describe('WindowTopMenuButton', () => {
     expect(screen.getByRole('menu')).toBeInTheDocument();
 
     // click something else to close the menu (the windowMenu button is hidden at this point)
-    await user.click(screen.getAllByRole('menuitem')[0]);
+    await user.click(screen.getAllByRole('menuitemradio')[0]);
     expect(screen.queryByRole('menu')).not.toBeInTheDocument();
   });
 

__tests__/src/components/WindowViewSettings.test.js

@@ -20,7 +20,7 @@ describe('WindowViewSettings', () => {
   it('renders all elements correctly', () => {
     createWrapper();
     expect(screen.getByRole('presentation', { selector: 'li' })).toBeInTheDocument();
-    const menuItems = screen.queryAllByRole('menuitem');
+    const menuItems = screen.queryAllByRole('menuitemradio');
     expect(menuItems.length).toBe(4);
     expect(menuItems[0]).toHaveTextContent(/Single/i);
     expect(menuItems[1]).toHaveTextContent(/Book/i);
@@ -29,29 +29,29 @@ describe('WindowViewSettings', () => {
   });
   it('single should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ windowViewType: 'single' });
-    expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Book/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Book/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
   it('book should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ windowViewType: 'book' });
-    expect(screen.getByRole('menuitem', { name: /Book/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Book/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
   it('scroll should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ windowViewType: 'scroll' });
-    expect(screen.getByRole('menuitem', { name: /Scroll/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Scroll/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
   it('gallery should set the correct label active (by setting the secondary color)', () => {
     createWrapper({ windowViewType: 'gallery' });
-    expect(screen.getByRole('menuitem', { name: /Gallery/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
-    expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Gallery/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
+    expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
   });
   it('updates state when the view config selection changes', async () => {
     const setWindowViewType = vi.fn();
     createWrapper({ setWindowViewType });
     const user = userEvent.setup();
-    const menuItems = screen.queryAllByRole('menuitem');
+    const menuItems = screen.queryAllByRole('menuitemradio');
     expect(menuItems.length).toBe(4);
     await user.click(menuItems[0]);
     expect(setWindowViewType).toHaveBeenCalledWith('xyz', 'single');

src/components/WindowThumbnailSettings.js

@@ -2,6 +2,7 @@ import { styled } from '@mui/material/styles';
 import FormControlLabel from '@mui/material/FormControlLabel';
 import ListSubheader from '@mui/material/ListSubheader';
 import MenuItem from '@mui/material/MenuItem';
+import MenuList from '@mui/material/MenuList';
 import ThumbnailsOffIcon from '@mui/icons-material/CropDinSharp';
 import PropTypes from 'prop-types';
 import { useTranslation } from 'react-i18next';
@@ -14,20 +15,23 @@ const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot
     ...(selected && {
       borderBottomColor: theme.palette.secondary.main,
     }),
+    '&.Mui-selected': {
+      backgroundColor: 'transparent !important',
+    },
+    '&.Mui-selected.Mui-focusVisible': {
+      backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
+    },
+    '&:focused': {
+      backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
+    },
+    color: selected ? theme.palette.secondary.main : undefined,
+    display: 'inline-flex',
   },
-  '&.Mui-selected': {
-    backgroundColor: 'transparent !important',
-  },
-  '&.Mui-selected.Mui-focusVisible': {
-    backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
-  },
-  '&:focused': {
-    backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
-  },
-  color: selected ? theme.palette.secondary.main : undefined,
-  display: 'inline-block',
 }));
 
+const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
+  display: 'inline-flex',
+}));
 /**
  *
  */
@@ -40,43 +44,66 @@ export function WindowThumbnailSettings({
 
   return (
     <>
-      <ListSubheader role="presentation" disableSticky tabIndex={-1}>{t('thumbnails')}</ListSubheader>
-
-      <ThumbnailOption selected={thumbnailNavigationPosition === 'off'} onClick={() => { handleChange('off'); }}>
-        <FormControlLabel
-          value="off"
-          control={
-            <ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
-          }
-          label={t('off')}
-          labelPlacement="bottom"
-        />
-      </ThumbnailOption>
-      <ThumbnailOption selected={thumbnailNavigationPosition === 'far-bottom'} onClick={() => { handleChange('far-bottom'); }}>
-        <FormControlLabel
-          value="far-bottom"
-          control={
-            <ThumbnailNavigationBottomIcon color={thumbnailNavigationPosition === 'far-bottom' ? 'secondary' : undefined} fill="currentcolor" />
-          }
-          label={t('bottom')}
-          labelPlacement="bottom"
-        />
-      </ThumbnailOption>
-      <ThumbnailOption selected={thumbnailNavigationPosition === 'far-right'} onClick={() => { handleChange('far-right'); }}>
-        <FormControlLabel
-          value="far-right"
-          control={(
-            <ThumbnailNavigationRightIcon
-              color={thumbnailNavigationPosition === 'far-right' ? 'secondary' : undefined}
-              fill="currentcolor"
-              style={direction === 'rtl' ? { transform: 'rotate(180deg)' } : {}}
-            />
-          )}
-          label={t('right')}
-          labelPlacement="bottom"
-        />
-      </ThumbnailOption>
+      <ListSubheader role="presentation" disableSticky>{t('thumbnails')}</ListSubheader>
+      <StyledMenuList role="menubar">
+        <ThumbnailOption
+          aria-checked={thumbnailNavigationPosition === 'off'}
+          autoFocus={thumbnailNavigationPosition === 'off'}
+          key="off"
+          onClick={() => { handleChange('off'); }}
+          role="menuitemradio"
+          selected={thumbnailNavigationPosition === 'off'}
+        >
+          <FormControlLabel
+            control={
+              <ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
+            }
+            label={t('off')}
+            labelPlacement="bottom"
+            value="off"
+          />
+        </ThumbnailOption>
+        <ThumbnailOption
+          aria-checked={thumbnailNavigationPosition === 'far-bottom'}
+          autoFocus={thumbnailNavigationPosition === 'far-bottom'}
+          key="far-bottom"
+          onClick={() => { handleChange('far-bottom'); }}
+          role="menuitemradio"
+          selected={thumbnailNavigationPosition === 'far-bottom'}
+        >
+          <FormControlLabel
+            control={
+              <ThumbnailNavigationBottomIcon color={thumbnailNavigationPosition === 'far-bottom' ? 'secondary' : undefined} fill="currentcolor" />
+            }
+            label={t('bottom')}
+            labelPlacement="bottom"
+            value="far-bottom"
+          />
+        </ThumbnailOption>
+        <ThumbnailOption
+          aria-checked={thumbnailNavigationPosition === 'far-right'}
+          autoFocus={thumbnailNavigationPosition === 'far-right'}
+          key="far-right"
+          onClick={() => { handleChange('far-right'); }}
+          role="menuitemradio"
+          selected={thumbnailNavigationPosition === 'far-right'}
+        >
+          <FormControlLabel
+            control={(
+              <ThumbnailNavigationRightIcon
+                color={thumbnailNavigationPosition === 'far-right' ? 'secondary' : undefined}
+                fill="currentcolor"
+                style={direction === 'rtl' ? { transform: 'rotate(180deg)' } : {}}
+              />
+            )}
+            label={t('right')}
+            labelPlacement="bottom"
+            value="far-right"
+          />
+        </ThumbnailOption>
+      </StyledMenuList>
     </>
+
   );
 }
 

src/components/WindowTopMenu.js

@@ -1,6 +1,6 @@
 import { useContext } from 'react';
-import Menu from '@mui/material/Menu';
 import ListSubheader from '@mui/material/ListSubheader';
+import Popover from '@mui/material/Popover';
 import PropTypes from 'prop-types';
 import WindowThumbnailSettings from '../containers/WindowThumbnailSettings';
 import WindowViewSettings from '../containers/WindowViewSettings';
@@ -28,7 +28,7 @@ export function WindowTopMenu({
   const pluginProps = arguments[0]; // eslint-disable-line prefer-rest-params
 
   return (
-    <Menu
+    <Popover
       container={container?.current}
       anchorOrigin={{
         horizontal: 'right',
@@ -46,12 +46,13 @@ export function WindowTopMenu({
       orientation="horizontal"
       anchorEl={anchorEl}
       open={open}
+      role="menu"
     >
       <WindowViewSettings windowId={windowId} handleClose={handleClose} />
       {showThumbnailNavigationSettings
         && <WindowThumbnailSettings windowId={windowId} handleClose={handleClose} />}
       <PluginHookWithHeader {...pluginProps} />
-    </Menu>
+    </Popover>
   );
 }
 

src/components/WindowViewSettings.js

@@ -1,6 +1,7 @@
 import { styled } from '@mui/material/styles';
 import FormControlLabel from '@mui/material/FormControlLabel';
 import MenuItem from '@mui/material/MenuItem';
+import MenuList from '@mui/material/MenuList';
 import ListSubheader from '@mui/material/ListSubheader';
 import SingleIcon from '@mui/icons-material/CropOriginalSharp';
 import ScrollViewIcon from '@mui/icons-material/ViewColumn';
@@ -15,18 +16,22 @@ const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option'
     ...(selected && {
       borderBottomColor: theme.palette.secondary.main,
     }),
+    '&.Mui-selected': {
+      backgroundColor: 'transparent !important',
+    },
+    '&.Mui-selected.Mui-focusVisible': {
+      backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
+    },
+    '&:focused': {
+      backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
+    },
+    color: selected ? theme.palette.secondary.main : undefined,
+    display: 'inline-block',
   },
-  '&.Mui-selected': {
-    backgroundColor: 'transparent !important',
-  },
-  '&.Mui-selected.Mui-focusVisible': {
-    backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
-  },
-  '&:focused': {
-    backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
-  },
-  color: selected ? theme.palette.secondary.main : undefined,
-  display: 'inline-block',
+}));
+
+const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
+  display: 'inline-flex',
 }));
 
 /**
@@ -52,10 +57,12 @@ export function WindowViewSettings({
       none of the click handlers work? */
   const menuItem = ({ value, Icon }) => (
     <ViewOption
-      selected={windowViewType === value}
-      key={value}
+      aria-checked={windowViewType === value}
       autoFocus={windowViewType === value}
+      key={value}
       onClick={() => { handleChange(value); handleClose(); }}
+      role="menuitemradio"
+      selected={windowViewType === value}
     >
       <FormControlLabel
         value={value}
@@ -69,8 +76,10 @@ export function WindowViewSettings({
   if (viewTypes.length === 0) return null;
   return (
     <>
-      <ListSubheader role="presentation" disableSticky tabIndex={-1}>{t('view')}</ListSubheader>
-      { viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) }
+      <ListSubheader role="presentation" disableSticky>{t('view')}</ListSubheader>
+      <StyledMenuList role="menubar">
+        { viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) }
+      </StyledMenuList>
     </>
   );
 }