Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix keyboard navigation issues with windowTopMenu #3914

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

fix keyboard navigation issues with windowTopMenu #3914

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7d46a3f
fix keyboard navigation issues with windowTopMenu
Jun 11, 2024
39611b2
fix typo
Jun 11, 2024
13ca194
fix: styling issues, refactored props alphabetically
Jun 11, 2024
2fb038f
fix: focus reset to anchorEl
Jun 12, 2024
076311b
fix: use aria-checked and role menuitemradio
Nov 25, 2024
9603e9b
refactor: use designers styling preference
Nov 25, 2024
bcfbef1
chore: resolve mr conflicts
Dec 19, 2024
a3e62ce
chore: resolve mr conflicts
Dec 19, 2024
07fb693
tests: fix tests
Dec 19, 2024
29de071
chore: fix tests
Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 62 additions & 40 deletions src/components/WindowThumbnailSettings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,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 ThumbnailNavigationBottomIcon from './icons/ThumbnailNavigationBottomIcon';
Expand All @@ -14,12 +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,
display: 'inline-block',
}));

const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
display: 'inline-flex',
}));
/**
*
*/
Expand Down Expand Up @@ -53,43 +55,63 @@ export class WindowThumbnailSettings extends Component {

return (
<>
<ListSubheader role="presentation" disableSticky tabIndex={-1}>{t('thumbnails')}</ListSubheader>

<ThumbnailOption selected={thumbnailNavigationPosition === 'off'} onClick={() => { this.handleChange('off'); handleClose(); }}>
<FormControlLabel
value="off"
control={
<ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
}
label={t('off')}
labelPlacement="bottom"
/>
</ThumbnailOption>
<ThumbnailOption selected={thumbnailNavigationPosition === 'far-bottom'} onClick={() => { this.handleChange('far-bottom'); handleClose(); }}>
<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={() => { this.handleChange('far-right'); handleClose(); }}>
<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-selected={thumbnailNavigationPosition === 'off'}
autoFocus={thumbnailNavigationPosition === 'off'}
key="off"
onClick={() => { this.handleChange('off'); handleClose(); }}
selected={thumbnailNavigationPosition === 'off'}
>
<FormControlLabel
value="off"
control={
<ThumbnailsOffIcon color={thumbnailNavigationPosition === 'off' ? 'secondary' : undefined} fill="currentcolor" />
}
label={t('off')}
labelPlacement="bottom"
/>
</ThumbnailOption>
<ThumbnailOption
aria-selected={thumbnailNavigationPosition === 'far-bottom'}
autoFocus={thumbnailNavigationPosition === 'far-bottom'}
key="far-bottom"
onClick={() => { this.handleChange('far-bottom'); handleClose(); }}
selected={thumbnailNavigationPosition === 'far-bottom'}
>
<FormControlLabel
value="far-bottom"
control={
<ThumbnailNavigationBottomIcon color={thumbnailNavigationPosition === 'far-bottom' ? 'secondary' : undefined} fill="currentcolor" />
}
label={t('bottom')}
labelPlacement="bottom"
/>
</ThumbnailOption>
<ThumbnailOption
aria-selected={thumbnailNavigationPosition === 'far-right'}
autoFocus={thumbnailNavigationPosition === 'far-right'}
key="far-right"
onClick={() => { this.handleChange('far-right'); handleClose(); }}
selected={thumbnailNavigationPosition === '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>
</StyledMenuList>
</>

);
}
}
Expand Down
13 changes: 7 additions & 6 deletions src/components/WindowTopMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -31,7 +31,7 @@ export class WindowTopMenu extends Component {
} = this.props;

return (
<Menu
<Popover
container={container?.current}
anchorOrigin={{
horizontal: 'right',
Expand All @@ -41,20 +41,21 @@ export class WindowTopMenu extends Component {
horizontal: 'right',
vertical: 'top',
}}
onClose={handleClose}
onClose={() => handleClose(anchorEl)}
TransitionProps={{
onEntering: toggleDraggingEnabled,
onExit: toggleDraggingEnabled,
}}
orientation="horizontal"
anchorEl={anchorEl}
open={open}
role="menu"
>
<WindowViewSettings windowId={windowId} handleClose={handleClose} />
<WindowViewSettings windowId={windowId} handleClose={() => handleClose(anchorEl)} />
{showThumbnailNavigationSettings
&& <WindowThumbnailSettings windowId={windowId} handleClose={handleClose} />}
&& <WindowThumbnailSettings windowId={windowId} handleClose={() => handleClose(anchorEl)} />}
<PluginHookWithHeader {...this.props} />
</Menu>
</Popover>
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/components/WindowTopMenuButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ export class WindowTopMenuButton extends Component {
/**
* @private
*/
handleMenuClose() {
handleMenuClose(anchorEl) {
this.setState({
anchorEl: null,
anchorEl,
open: false,
});
}
Expand Down
21 changes: 14 additions & 7 deletions src/components/WindowViewSettings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -15,10 +16,13 @@ const ViewOption = styled(MenuItem, { name: 'WindowViewSettings', slot: 'option'
...(selected && {
borderBottomColor: theme.palette.secondary.main,
}),
backgroundColor: 'transparent !important',
fstoe marked this conversation as resolved.
Show resolved Hide resolved
color: selected ? theme.palette.secondary.main : undefined,
},
backgroundColor: 'transparent !important',
color: selected ? theme.palette.secondary.main : undefined,
display: 'inline-block',
}));

const StyledMenuList = styled(MenuList, { name: 'WindowViewSettings', slot: 'option' })(() => ({
display: 'inline-flex',
}));

/**
Expand Down Expand Up @@ -63,10 +67,11 @@ export class WindowViewSettings extends Component {
none of the click handlers work? */
const menuItem = ({ value, Icon }) => (
<ViewOption
selected={windowViewType === value}
key={value}
aria-selected={windowViewType === value}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is aria-selected a valid/useful attribute for menu items? I'm not seeing it on https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/menuitem_role. Maybe this should be a menuitemradio + aria-checked?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for spotting :)
i added aria-checked and menuitemrole

autoFocus={windowViewType === value}
key={value}
onClick={() => { this.handleChange(value); handleClose(); }}
selected={windowViewType === value}
>
<FormControlLabel
value={value}
Expand All @@ -80,8 +85,10 @@ export class WindowViewSettings extends Component {
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>
</>
);
}
Expand Down
Loading