Repository: ProjectMirador/mirador
Pull request #3914 (2024-06-11 09:29:21)

fix keyboard navigation issues with windowTopMenu
Hello, 
this PR tries to fix the issue described here: https://github.com/ProjectMirador/mirador/issues/3903

The reason why view and thumbnail options could not be controlled correctly was due to the use of the MUI Menu component and not the wrong TabIndex as described in the issue. The component expects MenuItems as direct children. This was fixed by using a popover instead of the menu, (on which menu is based upon) and setting the MenuLists in WindowViewSettings and WindowThumbnailSettings.




Patch:
diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js
index a5b94b20..281eda58 100644
--- a/src/components/WindowThumbnailSettings.js
+++ b/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,63 @@ 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'}
+          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'}
+          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'}
+          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>
     </>
+
   );
 }
 
diff --git a/src/components/WindowTopMenu.js b/src/components/WindowTopMenu.js
index a39e940e..542ee66f 100644
--- a/src/components/WindowTopMenu.js
+++ b/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>
   );
 }
 
diff --git a/src/components/WindowViewSettings.js b/src/components/WindowViewSettings.js
index 76a0f480..e9cebbaf 100644
--- a/src/components/WindowViewSettings.js
+++ b/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>
     </>
   );
 }


Test patch:
diff --git a/__tests__/src/components/WindowThumbnailSettings.test.js b/__tests__/src/components/WindowThumbnailSettings.test.js
index a91b88e0..9112981a 100644
--- a/__tests__/src/components/WindowThumbnailSettings.test.js
+++ b/__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
   });
 });
diff --git a/__tests__/src/components/WindowTopMenu.test.js b/__tests__/src/components/WindowTopMenu.test.js
index 6fd518c9..f023e06c 100644
--- a/__tests__/src/components/WindowTopMenu.test.js
+++ b/__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);
diff --git a/__tests__/src/components/WindowTopMenuButton.test.js b/__tests__/src/components/WindowTopMenuButton.test.js
index baf91aef..a1c0ce9b 100644
--- a/__tests__/src/components/WindowTopMenuButton.test.js
+++ b/__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();
   });
 
diff --git a/__tests__/src/components/WindowViewSettings.test.js b/__tests__/src/components/WindowViewSettings.test.js
index ad8efe97..80e0cc62 100644
--- a/__tests__/src/components/WindowViewSettings.test.js
+++ b/__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');