Skip to content

Commit 3b4e128

Browse files
lutzhelmlutzhelm
authored
Merge pull request #3914 from fstoe/3903
fix keyboard navigation issues with windowTopMenu
2 parents 310d54d + fb1daa1 commit 3b4e128

7 files changed

Lines changed: 135 additions & 96 deletions

File tree

‎__tests__/src/components/WindowThumbnailSettings.test.js‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,28 +20,28 @@ describe('WindowThumbnailSettings', () => {
2020
it('renders all elements correctly', () => {
2121
createWrapper();
2222
expect(screen.getByRole('presentation', { selector: 'li' })).toBeInTheDocument();
23-
expect(screen.getByRole('menuitem', { name: /Off/ })).toBeInTheDocument();
24-
expect(screen.getByRole('menuitem', { name: /Bottom/ })).toBeInTheDocument();
25-
expect(screen.getByRole('menuitem', { name: /Right/ })).toBeInTheDocument();
23+
expect(screen.getByRole('menuitemradio', { name: /Off/ })).toBeInTheDocument();
24+
expect(screen.getByRole('menuitemradio', { name: /Bottom/ })).toBeInTheDocument();
25+
expect(screen.getByRole('menuitemradio', { name: /Right/ })).toBeInTheDocument();
2626
});
2727
it('for far-bottom it should set the correct label active (by setting the secondary color)', () => {
2828
createWrapper({ thumbnailNavigationPosition: 'far-bottom' });
29-
expect(screen.getByRole('menuitem', { name: /Bottom/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
30-
expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
31-
expect(screen.getByRole('menuitem', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
29+
expect(screen.getByRole('menuitemradio', { name: /Bottom/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
30+
expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
31+
expect(screen.getByRole('menuitemradio', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
3232
});
3333
it('for far-right it should set the correct label active (by setting the secondary color)', () => {
3434
createWrapper({ thumbnailNavigationPosition: 'far-right' });
35-
expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
36-
expect(screen.getByRole('menuitem', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
37-
expect(screen.getByRole('menuitem', { name: /Bottom/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
35+
expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
36+
expect(screen.getByRole('menuitemradio', { name: /Off/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
37+
expect(screen.getByRole('menuitemradio', { name: /Bottom/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
3838
});
3939

4040
it('updates state when the thumbnail config selection changes', async () => {
4141
const setWindowThumbnailPosition = vi.fn();
4242
const user = userEvent.setup();
4343
createWrapper({ setWindowThumbnailPosition });
44-
const menuItems = screen.queryAllByRole('menuitem');
44+
const menuItems = screen.queryAllByRole('menuitemradio');
4545
expect(menuItems.length).toBe(3);
4646
expect(menuItems[0]).toBeInTheDocument();
4747
expect(menuItems[1]).toBeInTheDocument();
@@ -57,6 +57,6 @@ describe('WindowThumbnailSettings', () => {
5757

5858
it('when rtl flips an icon', () => {
5959
createWrapper({ direction: 'rtl' });
60-
expect(screen.getByRole('menuitem', { name: /Right/ }).querySelector('svg')).toHaveStyle('transform: rotate(180deg);'); // eslint-disable-line testing-library/no-node-access
60+
expect(screen.getByRole('menuitemradio', { name: /Right/ }).querySelector('svg')).toHaveStyle('transform: rotate(180deg);'); // eslint-disable-line testing-library/no-node-access
6161
});
6262
});

‎__tests__/src/components/WindowTopMenu.test.js‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,16 +33,21 @@ describe('WindowTopMenu', () => {
3333

3434
const menuSections = within(screen.getByRole('menu')).getAllByRole('presentation');
3535
expect(menuSections).toHaveLength(2);
36+
3637
expect(menuSections[0]).toHaveTextContent('View');
37-
expect(menuSections[1]).toHaveTextContent('Thumbnails');
38+
const menus = within(screen.getByRole('menu')).getAllByRole('menubar');
3839

39-
const menuItems = screen.getAllByRole('menuitem');
40-
expect(menuItems).toHaveLength(5);
41-
expect(menuItems[0]).toHaveTextContent('Single');
42-
expect(menuItems[1]).toHaveTextContent('Gallery');
43-
expect(menuItems[2]).toHaveTextContent('Off');
44-
expect(menuItems[3]).toHaveTextContent('Bottom');
45-
expect(menuItems[4]).toHaveTextContent('Right');
40+
const viewItems = within(menus[0]).getAllByRole('menuitemradio');
41+
expect(viewItems).toHaveLength(2);
42+
expect(viewItems[0]).toHaveTextContent('Single');
43+
expect(viewItems[1]).toHaveTextContent('Gallery');
44+
45+
expect(menuSections[1]).toHaveTextContent('Thumbnails');
46+
const thumbnailItems = within(menus[1]).getAllByRole('menuitemradio');
47+
expect(thumbnailItems).toHaveLength(3);
48+
expect(thumbnailItems[0]).toHaveTextContent('Off');
49+
expect(thumbnailItems[1]).toHaveTextContent('Bottom');
50+
expect(thumbnailItems[2]).toHaveTextContent('Right');
4651
});
4752

4853
it('does not display unless open', () => {
@@ -66,7 +71,7 @@ describe('WindowTopMenu', () => {
6671
/>);
6772

6873
// click a menu item should close the menu
69-
const menuItems = screen.getAllByRole('menuitem');
74+
const menuItems = screen.getAllByRole('menuitemradio');
7075
await user.click(menuItems[0]);
7176
expect(handleClose).toHaveBeenCalledTimes(1);
7277
expect(toggleDraggingEnabled).toHaveBeenCalledTimes(1);

‎__tests__/src/components/WindowTopMenuButton.test.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('WindowTopMenuButton', () => {
3333
expect(screen.getByRole('menu')).toBeInTheDocument();
3434

3535
// click something else to close the menu (the windowMenu button is hidden at this point)
36-
await user.click(screen.getAllByRole('menuitem')[0]);
36+
await user.click(screen.getAllByRole('menuitemradio')[0]);
3737
expect(screen.queryByRole('menu')).not.toBeInTheDocument();
3838
});
3939

‎__tests__/src/components/WindowViewSettings.test.js‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ describe('WindowViewSettings', () => {
2020
it('renders all elements correctly', () => {
2121
createWrapper();
2222
expect(screen.getByRole('presentation', { selector: 'li' })).toBeInTheDocument();
23-
const menuItems = screen.queryAllByRole('menuitem');
23+
const menuItems = screen.queryAllByRole('menuitemradio');
2424
expect(menuItems.length).toBe(4);
2525
expect(menuItems[0]).toHaveTextContent(/Single/i);
2626
expect(menuItems[1]).toHaveTextContent(/Book/i);
@@ -29,29 +29,29 @@ describe('WindowViewSettings', () => {
2929
});
3030
it('single should set the correct label active (by setting the secondary color)', () => {
3131
createWrapper({ windowViewType: 'single' });
32-
expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
33-
expect(screen.getByRole('menuitem', { name: /Book/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
32+
expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
33+
expect(screen.getByRole('menuitemradio', { name: /Book/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
3434
});
3535
it('book should set the correct label active (by setting the secondary color)', () => {
3636
createWrapper({ windowViewType: 'book' });
37-
expect(screen.getByRole('menuitem', { name: /Book/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
38-
expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
37+
expect(screen.getByRole('menuitemradio', { name: /Book/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
38+
expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
3939
});
4040
it('scroll should set the correct label active (by setting the secondary color)', () => {
4141
createWrapper({ windowViewType: 'scroll' });
42-
expect(screen.getByRole('menuitem', { name: /Scroll/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
43-
expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
42+
expect(screen.getByRole('menuitemradio', { name: /Scroll/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
43+
expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
4444
});
4545
it('gallery should set the correct label active (by setting the secondary color)', () => {
4646
createWrapper({ windowViewType: 'gallery' });
47-
expect(screen.getByRole('menuitem', { name: /Gallery/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
48-
expect(screen.getByRole('menuitem', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
47+
expect(screen.getByRole('menuitemradio', { name: /Gallery/ }).querySelector('svg')).toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
48+
expect(screen.getByRole('menuitemradio', { name: /Single/ }).querySelector('svg')).not.toHaveClass('MuiSvgIcon-colorSecondary'); // eslint-disable-line testing-library/no-node-access
4949
});
5050
it('updates state when the view config selection changes', async () => {
5151
const setWindowViewType = vi.fn();
5252
createWrapper({ setWindowViewType });
5353
const user = userEvent.setup();
54-
const menuItems = screen.queryAllByRole('menuitem');
54+
const menuItems = screen.queryAllByRole('menuitemradio');
5555
expect(menuItems.length).toBe(4);
5656
await user.click(menuItems[0]);
5757
expect(setWindowViewType).toHaveBeenCalledWith('xyz', 'single');

‎src/components/WindowThumbnailSettings.js‎

Lines changed: 71 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { styled } from '@mui/material/styles';
22
import FormControlLabel from '@mui/material/FormControlLabel';
33
import ListSubheader from '@mui/material/ListSubheader';
44
import MenuItem from '@mui/material/MenuItem';
5+
import MenuList from '@mui/material/MenuList';
56
import ThumbnailsOffIcon from '@mui/icons-material/CropDinSharp';
67
import PropTypes from 'prop-types';
78
import { useTranslation } from 'react-i18next';
@@ -14,20 +15,23 @@ const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot
1415
...(selected && {
1516
borderBottomColor: theme.palette.secondary.main,
1617
}),
18+
'&.Mui-selected': {
19+
backgroundColor: 'transparent !important',
20+
},
21+
'&.Mui-selected.Mui-focusVisible': {
22+
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
23+
},
24+
'&:focused': {
25+
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
26+
},
27+
color: selected ? theme.palette.secondary.main : undefined,
28+
display: 'inline-flex',
1729
},
18-
'&.Mui-selected': {
19-
backgroundColor: 'transparent !important',
20-
},
21-
'&.Mui-selected.Mui-focusVisible': {
22-
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
23-
},
24-
'&:focused': {
25-
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
26-
},
27-
color: selected ? theme.palette.secondary.main : undefined,
28-
display: 'inline-block',
2930
}));
3031

32+
const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
33+
display: 'inline-flex',
34+
}));
3135
/**
3236
*
3337
*/
@@ -40,43 +44,63 @@ export function WindowThumbnailSettings({
4044

4145
return (
4246
<>
43-
<ListSubheader role="presentation" disableSticky tabIndex={-1}>{t('thumbnails')}</ListSubheader>
44-
45-
<ThumbnailOption selected={thumbnailNavigationPosition === 'off'} onClick={() => { handleChange('off'); }}>
46-
<FormControlLabel
47-
value="off"
48-
control={
49-
<ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
50-
}
51-
label={t('off')}
52-
labelPlacement="bottom"
53-
/>
54-
</ThumbnailOption>
55-
<ThumbnailOption selected={thumbnailNavigationPosition === 'far-bottom'} onClick={() => { handleChange('far-bottom'); }}>
56-
<FormControlLabel
57-
value="far-bottom"
58-
control={
59-
<ThumbnailNavigationBottomIcon color={thumbnailNavigationPosition === 'far-bottom' ? 'secondary' : undefined} fill="currentcolor" />
60-
}
61-
label={t('bottom')}
62-
labelPlacement="bottom"
63-
/>
64-
</ThumbnailOption>
65-
<ThumbnailOption selected={thumbnailNavigationPosition === 'far-right'} onClick={() => { handleChange('far-right'); }}>
66-
<FormControlLabel
67-
value="far-right"
68-
control={(
69-
<ThumbnailNavigationRightIcon
70-
color={thumbnailNavigationPosition === 'far-right' ? 'secondary' : undefined}
71-
fill="currentcolor"
72-
style={direction === 'rtl' ? { transform: 'rotate(180deg)' } : {}}
73-
/>
74-
)}
75-
label={t('right')}
76-
labelPlacement="bottom"
77-
/>
78-
</ThumbnailOption>
47+
<ListSubheader role="presentation" disableSticky>{t('thumbnails')}</ListSubheader>
48+
<StyledMenuList role="menubar">
49+
<ThumbnailOption
50+
aria-checked={thumbnailNavigationPosition === 'off'}
51+
key="off"
52+
onClick={() => { handleChange('off'); }}
53+
role="menuitemradio"
54+
selected={thumbnailNavigationPosition === 'off'}
55+
>
56+
<FormControlLabel
57+
control={
58+
<ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
59+
}
60+
label={t('off')}
61+
labelPlacement="bottom"
62+
value="off"
63+
/>
64+
</ThumbnailOption>
65+
<ThumbnailOption
66+
aria-checked={thumbnailNavigationPosition === 'far-bottom'}
67+
key="far-bottom"
68+
onClick={() => { handleChange('far-bottom'); }}
69+
role="menuitemradio"
70+
selected={thumbnailNavigationPosition === 'far-bottom'}
71+
>
72+
<FormControlLabel
73+
control={
74+
<ThumbnailNavigationBottomIcon color={thumbnailNavigationPosition === 'far-bottom' ? 'secondary' : undefined} fill="currentcolor" />
75+
}
76+
label={t('bottom')}
77+
labelPlacement="bottom"
78+
value="far-bottom"
79+
/>
80+
</ThumbnailOption>
81+
<ThumbnailOption
82+
aria-checked={thumbnailNavigationPosition === 'far-right'}
83+
key="far-right"
84+
onClick={() => { handleChange('far-right'); }}
85+
role="menuitemradio"
86+
selected={thumbnailNavigationPosition === 'far-right'}
87+
>
88+
<FormControlLabel
89+
control={(
90+
<ThumbnailNavigationRightIcon
91+
color={thumbnailNavigationPosition === 'far-right' ? 'secondary' : undefined}
92+
fill="currentcolor"
93+
style={direction === 'rtl' ? { transform: 'rotate(180deg)' } : {}}
94+
/>
95+
)}
96+
label={t('right')}
97+
labelPlacement="bottom"
98+
value="far-right"
99+
/>
100+
</ThumbnailOption>
101+
</StyledMenuList>
79102
</>
103+
80104
);
81105
}
82106

‎src/components/WindowTopMenu.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { useContext } from 'react';
2-
import Menu from '@mui/material/Menu';
32
import ListSubheader from '@mui/material/ListSubheader';
3+
import Popover from '@mui/material/Popover';
44
import PropTypes from 'prop-types';
55
import WindowThumbnailSettings from '../containers/WindowThumbnailSettings';
66
import WindowViewSettings from '../containers/WindowViewSettings';
@@ -28,7 +28,7 @@ export function WindowTopMenu({
2828
const pluginProps = arguments[0]; // eslint-disable-line prefer-rest-params
2929

3030
return (
31-
<Menu
31+
<Popover
3232
container={container?.current}
3333
anchorOrigin={{
3434
horizontal: 'right',
@@ -46,12 +46,13 @@ export function WindowTopMenu({
4646
orientation="horizontal"
4747
anchorEl={anchorEl}
4848
open={open}
49+
role="menu"
4950
>
5051
<WindowViewSettings windowId={windowId} handleClose={handleClose} />
5152
{showThumbnailNavigationSettings
5253
&& <WindowThumbnailSettings windowId={windowId} handleClose={handleClose} />}
5354
<PluginHookWithHeader {...pluginProps} />
54-
</Menu>
55+
</Popover>
5556
);
5657
}
5758

‎src/components/WindowViewSettings.js‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { styled } from '@mui/material/styles';
22
import FormControlLabel from '@mui/material/FormControlLabel';
33
import MenuItem from '@mui/material/MenuItem';
4+
import MenuList from '@mui/material/MenuList';
45
import ListSubheader from '@mui/material/ListSubheader';
56
import SingleIcon from '@mui/icons-material/CropOriginalSharp';
67
import ScrollViewIcon from '@mui/icons-material/ViewColumn';
@@ -15,18 +16,22 @@ const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option'
1516
...(selected && {
1617
borderBottomColor: theme.palette.secondary.main,
1718
}),
19+
'&.Mui-selected': {
20+
backgroundColor: 'transparent !important',
21+
},
22+
'&.Mui-selected.Mui-focusVisible': {
23+
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
24+
},
25+
'&:focused': {
26+
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
27+
},
28+
color: selected ? theme.palette.secondary.main : undefined,
29+
display: 'inline-block',
1830
},
19-
'&.Mui-selected': {
20-
backgroundColor: 'transparent !important',
21-
},
22-
'&.Mui-selected.Mui-focusVisible': {
23-
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
24-
},
25-
'&:focused': {
26-
backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`,
27-
},
28-
color: selected ? theme.palette.secondary.main : undefined,
29-
display: 'inline-block',
31+
}));
32+
33+
const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
34+
display: 'inline-flex',
3035
}));
3136

3237
/**
@@ -52,10 +57,12 @@ export function WindowViewSettings({
5257
none of the click handlers work? */
5358
const menuItem = ({ value, Icon }) => (
5459
<ViewOption
55-
selected={windowViewType === value}
56-
key={value}
60+
aria-checked={windowViewType === value}
5761
autoFocus={windowViewType === value}
62+
key={value}
5863
onClick={() => { handleChange(value); handleClose(); }}
64+
role="menuitemradio"
65+
selected={windowViewType === value}
5966
>
6067
<FormControlLabel
6168
value={value}
@@ -69,8 +76,10 @@ export function WindowViewSettings({
6976
if (viewTypes.length === 0) return null;
7077
return (
7178
<>
72-
<ListSubheader role="presentation" disableSticky tabIndex={-1}>{t('view')}</ListSubheader>
73-
{ viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) }
79+
<ListSubheader role="presentation" disableSticky>{t('view')}</ListSubheader>
80+
<StyledMenuList role="menubar">
81+
{ viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) }
82+
</StyledMenuList>
7483
</>
7584
);
7685
}

0 commit comments

Comments
 (0)