From 7d46a3f8ec002785d0b5be75ae457beea47cad29 Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Tue, 11 Jun 2024 11:19:39 +0200 Subject: [PATCH 1/9] fix keyboard navigation issues with windowTopMenu --- src/components/WindowThumbnailSettings.js | 100 ++++++++++++++-------- src/components/WindowTopMenu.js | 7 +- src/components/WindowViewSettings.js | 20 +++-- 3 files changed, 80 insertions(+), 47 deletions(-) diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js index 042355e4d0..faa0fdef8c 100644 --- a/src/components/WindowThumbnailSettings.js +++ b/src/components/WindowThumbnailSettings.js @@ -3,12 +3,13 @@ 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 ThumbnailNavigationBottomIcon from './icons/ThumbnailNavigationBottomIcon'; import ThumbnailNavigationRightIcon from './icons/ThumbnailNavigationRightIcon'; -const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot: 'option' })(({ selected, theme }) => ({ +const ThumbnailOptions = styled(MenuItem, { name: 'WindowThumbnailSettings', slot: 'option' })(({ selected, theme }) => ({ '& .MuiFormControlLabel-label': { borderBottom: '2px solid transparent', ...(selected && { @@ -17,7 +18,10 @@ const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot }, backgroundColor: 'transparent !important', color: selected ? theme.palette.secondary.main : undefined, - display: 'inline-block', +})); + +const StyledMenuList = styled(MenuList, { name: 'WindowThumbnailSettings', slot: 'option' })(() => ({ + display: 'inline-flex', })); /** @@ -53,43 +57,63 @@ export class WindowThumbnailSettings extends Component { return ( <> - {t('thumbnails')} - - { this.handleChange('off'); handleClose(); }}> - - } - label={t('off')} - labelPlacement="bottom" - /> - - { this.handleChange('far-bottom'); handleClose(); }}> - - } - label={t('bottom')} - labelPlacement="bottom" - /> - - { this.handleChange('far-right'); handleClose(); }}> - - )} - label={t('right')} - labelPlacement="bottom" - /> - + {t('thumbnails')} + + { this.handleChange('off'); handleClose(); }} + autoFocus={thumbnailNavigationPosition === 'off'} + key="off" + > + + } + label={t('off')} + labelPlacement="bottom" + /> + + { this.handleChange('far-bottom'); handleClose(); }} + autoFocus={thumbnailNavigationPosition === 'far-bottom'} + key="far-bottom" + > + + } + label={t('bottom')} + labelPlacement="bottom" + /> + + { this.handleChange('far-right'); handleClose(); }} + > + + )} + label={t('right')} + labelPlacement="bottom" + /> + + + ); } } diff --git a/src/components/WindowTopMenu.js b/src/components/WindowTopMenu.js index 2170c58335..2cb5d3c19b 100644 --- a/src/components/WindowTopMenu.js +++ b/src/components/WindowTopMenu.js @@ -1,6 +1,6 @@ import { Component } 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'; @@ -31,7 +31,7 @@ export class WindowTopMenu extends Component { } = this.props; return ( - {showThumbnailNavigationSettings && } - + ); } } diff --git a/src/components/WindowViewSettings.js b/src/components/WindowViewSettings.js index e693b2ffcd..488ff5bdef 100644 --- a/src/components/WindowViewSettings.js +++ b/src/components/WindowViewSettings.js @@ -2,6 +2,7 @@ import { Component } from 'react'; 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'; @@ -9,7 +10,7 @@ import PropTypes from 'prop-types'; import BookViewIcon from './icons/BookViewIcon'; import GalleryViewIcon from './icons/GalleryViewIcon'; -const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' })(({ selected, theme }) => ({ +const ViewOptions = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' })(({ selected, theme }) => ({ '& .MuiFormControlLabel-label': { borderBottom: '2px solid transparent', ...(selected && { @@ -18,7 +19,11 @@ const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' }, backgroundColor: 'transparent !important', color: selected ? theme.palette.secondary.main : undefined, - display: 'inline-block', + display: 'inline-flex', +})); + +const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'menuList' })(({ selected, theme }) => ({ + display: 'inline-flex', })); /** @@ -62,7 +67,8 @@ export class WindowViewSettings extends Component { /** Suspiciously similar to a component, yet if it is invoked through JSX none of the click handlers work? */ const menuItem = ({ value, Icon }) => ( - - + ); if (viewTypes.length === 0) return null; return ( <> - {t('view')} - { viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) } + {t('view')} + + { viewTypes.map(value => menuItem({ Icon: iconMap[value], value })) } + ); } From 39611b2ec647e2af588893cc9df96415a0ceb6bd Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Tue, 11 Jun 2024 11:30:17 +0200 Subject: [PATCH 2/9] fix typo --- src/components/WindowThumbnailSettings.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js index faa0fdef8c..350c4fbffd 100644 --- a/src/components/WindowThumbnailSettings.js +++ b/src/components/WindowThumbnailSettings.js @@ -9,7 +9,7 @@ import PropTypes from 'prop-types'; import ThumbnailNavigationBottomIcon from './icons/ThumbnailNavigationBottomIcon'; import ThumbnailNavigationRightIcon from './icons/ThumbnailNavigationRightIcon'; -const ThumbnailOptions = styled(MenuItem, { name: 'WindowThumbnailSettings', slot: 'option' })(({ selected, theme }) => ({ +const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot: 'option' })(({ selected, theme }) => ({ '& .MuiFormControlLabel-label': { borderBottom: '2px solid transparent', ...(selected && { @@ -59,7 +59,7 @@ export class WindowThumbnailSettings extends Component { <> {t('thumbnails')} - { this.handleChange('off'); handleClose(); }} From 13ca194b93349892e6312cd2afeb6369eb0c3dfe Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Tue, 11 Jun 2024 11:50:48 +0200 Subject: [PATCH 3/9] fix: styling issues, refactored props alphabetically --- src/components/WindowThumbnailSettings.js | 30 +++++++++++------------ src/components/WindowViewSettings.js | 17 ++++++------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js index 350c4fbffd..9d7eef75f9 100644 --- a/src/components/WindowThumbnailSettings.js +++ b/src/components/WindowThumbnailSettings.js @@ -15,15 +15,13 @@ const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot ...(selected && { borderBottomColor: theme.palette.secondary.main, }), + backgroundColor: 'transparent !important', + color: selected ? theme.palette.secondary.main : undefined, }, - backgroundColor: 'transparent !important', - color: selected ? theme.palette.secondary.main : undefined, })); - -const StyledMenuList = styled(MenuList, { name: 'WindowThumbnailSettings', slot: 'option' })(() => ({ +const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({ display: 'inline-flex', })); - /** * */ @@ -61,10 +59,10 @@ export class WindowThumbnailSettings extends Component { { this.handleChange('off'); handleClose(); }} autoFocus={thumbnailNavigationPosition === 'off'} key="off" + onClick={() => { this.handleChange('off'); handleClose(); }} + selected={thumbnailNavigationPosition === 'off'} > - - + { this.handleChange('far-bottom'); handleClose(); }} autoFocus={thumbnailNavigationPosition === 'far-bottom'} key="far-bottom" + onClick={() => { this.handleChange('far-bottom'); handleClose(); }} + selected={thumbnailNavigationPosition === 'far-bottom'} > - - + { this.handleChange('far-right'); handleClose(); }} + selected={thumbnailNavigationPosition === 'far-right'} > - + diff --git a/src/components/WindowViewSettings.js b/src/components/WindowViewSettings.js index 488ff5bdef..0c24279f3d 100644 --- a/src/components/WindowViewSettings.js +++ b/src/components/WindowViewSettings.js @@ -10,19 +10,18 @@ import PropTypes from 'prop-types'; import BookViewIcon from './icons/BookViewIcon'; import GalleryViewIcon from './icons/GalleryViewIcon'; -const ViewOptions = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' })(({ selected, theme }) => ({ +const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' })(({ selected, theme }) => ({ '& .MuiFormControlLabel-label': { borderBottom: '2px solid transparent', ...(selected && { borderBottomColor: theme.palette.secondary.main, }), + backgroundColor: 'transparent !important', + color: selected ? theme.palette.secondary.main : undefined, }, - backgroundColor: 'transparent !important', - color: selected ? theme.palette.secondary.main : undefined, - display: 'inline-flex', })); -const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'menuList' })(({ selected, theme }) => ({ +const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({ display: 'inline-flex', })); @@ -67,12 +66,12 @@ export class WindowViewSettings extends Component { /** Suspiciously similar to a component, yet if it is invoked through JSX none of the click handlers work? */ const menuItem = ({ value, Icon }) => ( - { this.handleChange(value); handleClose(); }} + selected={windowViewType === value} > - + ); if (viewTypes.length === 0) return null; From 2fb038fca49870b51d43d480f31da56cdf6dfe30 Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Wed, 12 Jun 2024 11:28:24 +0200 Subject: [PATCH 4/9] fix: focus reset to anchorEl --- src/components/WindowTopMenu.js | 6 +++--- src/components/WindowTopMenuButton.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/WindowTopMenu.js b/src/components/WindowTopMenu.js index 2cb5d3c19b..3e10732ed2 100644 --- a/src/components/WindowTopMenu.js +++ b/src/components/WindowTopMenu.js @@ -41,7 +41,7 @@ export class WindowTopMenu extends Component { horizontal: 'right', vertical: 'top', }} - onClose={handleClose} + onClose={() => handleClose(anchorEl)} TransitionProps={{ onEntering: toggleDraggingEnabled, onExit: toggleDraggingEnabled, @@ -51,9 +51,9 @@ export class WindowTopMenu extends Component { open={open} role="menu" > - + handleClose(anchorEl)} /> {showThumbnailNavigationSettings - && } + && handleClose(anchorEl)} />} ); diff --git a/src/components/WindowTopMenuButton.js b/src/components/WindowTopMenuButton.js index e48bd84d2c..c7d7da606e 100644 --- a/src/components/WindowTopMenuButton.js +++ b/src/components/WindowTopMenuButton.js @@ -33,9 +33,9 @@ export class WindowTopMenuButton extends Component { /** * @private */ - handleMenuClose() { + handleMenuClose(anchorEl) { this.setState({ - anchorEl: null, + anchorEl, open: false, }); } From 076311b7cec9a4e62b464f10bbf3fe259ed6f402 Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Mon, 25 Nov 2024 14:27:02 +0100 Subject: [PATCH 5/9] fix: use aria-checked and role menuitemradio --- src/components/WindowViewSettings.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/WindowViewSettings.js b/src/components/WindowViewSettings.js index 0c24279f3d..9ff11498c8 100644 --- a/src/components/WindowViewSettings.js +++ b/src/components/WindowViewSettings.js @@ -67,11 +67,12 @@ export class WindowViewSettings extends Component { none of the click handlers work? */ const menuItem = ({ value, Icon }) => ( { this.handleChange(value); handleClose(); }} selected={windowViewType === value} + role="menuitemradio" > Date: Mon, 25 Nov 2024 14:29:19 +0100 Subject: [PATCH 6/9] refactor: use designers styling preference --- src/components/WindowViewSettings.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/components/WindowViewSettings.js b/src/components/WindowViewSettings.js index 9ff11498c8..2f627dd91b 100644 --- a/src/components/WindowViewSettings.js +++ b/src/components/WindowViewSettings.js @@ -16,8 +16,17 @@ const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option' ...(selected && { borderBottomColor: theme.palette.secondary.main, }), - backgroundColor: 'transparent !important', + '&.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', }, })); From a3e62cec5847068f6acd19d6b7db8f9cab8c9631 Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Thu, 19 Dec 2024 12:54:46 +0100 Subject: [PATCH 7/9] chore: resolve mr conflicts --- src/components/WindowThumbnailSettings.js | 1 + src/components/WindowTopMenu.js | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js index a85dafeaa8..7dd9df26f4 100644 --- a/src/components/WindowThumbnailSettings.js +++ b/src/components/WindowThumbnailSettings.js @@ -25,6 +25,7 @@ const ThumbnailOption = styled(MenuItem, { name: 'WindowThumbnailSettings', slot backgroundColor: `${(theme.vars || theme).palette.action.focus} !important`, }, color: selected ? theme.palette.secondary.main : undefined, + display: 'inline-flex', }, })); diff --git a/src/components/WindowTopMenu.js b/src/components/WindowTopMenu.js index 5df3ef4097..542ee66fa9 100644 --- a/src/components/WindowTopMenu.js +++ b/src/components/WindowTopMenu.js @@ -48,9 +48,9 @@ export function WindowTopMenu({ open={open} role="menu" > - handleClose(anchorEl)} /> + {showThumbnailNavigationSettings - && handleClose(anchorEl)} />} + && } ); From 07fb693ac1088db2f24c273535e7b7646e674581 Mon Sep 17 00:00:00 2001 From: Fabian Stoehr Date: Thu, 19 Dec 2024 13:49:48 +0100 Subject: [PATCH 8/9] tests: fix tests --- .../src/components/WindowTopMenu.test.js | 23 +++++++++++-------- .../src/components/WindowViewSettings.test.js | 20 ++++++++-------- src/components/WindowThumbnailSettings.js | 9 +++++--- src/components/WindowViewSettings.js | 2 +- 4 files changed, 31 insertions(+), 23 deletions(-) diff --git a/__tests__/src/components/WindowTopMenu.test.js b/__tests__/src/components/WindowTopMenu.test.js index 6fd518c953..f023e06cf9 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/WindowViewSettings.test.js b/__tests__/src/components/WindowViewSettings.test.js index ad8efe97e3..80e0cc627a 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'); diff --git a/src/components/WindowThumbnailSettings.js b/src/components/WindowThumbnailSettings.js index 7dd9df26f4..7fb3b8a3b3 100644 --- a/src/components/WindowThumbnailSettings.js +++ b/src/components/WindowThumbnailSettings.js @@ -47,10 +47,11 @@ export function WindowThumbnailSettings({ {t('thumbnails')} { handleChange('off'); }} + role="menuitemradio" selected={thumbnailNavigationPosition === 'off'} > { handleChange('far-bottom'); }} + role="menuitemradio" selected={thumbnailNavigationPosition === 'far-bottom'} > { handleChange('far-right'); }} + role="menuitemradio" selected={thumbnailNavigationPosition === 'far-right'} > { handleChange(value); handleClose(); }} - selected={windowViewType === value} role="menuitemradio" + selected={windowViewType === value} > Date: Thu, 19 Dec 2024 14:18:39 +0100 Subject: [PATCH 9/9] chore: fix tests --- .../WindowThumbnailSettings.test.js | 22 +++++++++---------- .../components/WindowTopMenuButton.test.js | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/__tests__/src/components/WindowThumbnailSettings.test.js b/__tests__/src/components/WindowThumbnailSettings.test.js index a91b88e0a6..9112981a15 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/WindowTopMenuButton.test.js b/__tests__/src/components/WindowTopMenuButton.test.js index baf91aef59..a1c0ce9bb1 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(); });