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.

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

Desktop: Accessibility: Improve "change application layout" screen keyboard accessibility #11718

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fa99c6d
Desktop: Accessibility: Fix missing button labels in "change app layout"
personalizedrefrigerator Jan 23, 2025
b9d69a5
Types
personalizedrefrigerator Jan 23, 2025
00a19de
Show move controls in a dialog
personalizedrefrigerator Jan 23, 2025
15c51f4
Fix move mode header styles
personalizedrefrigerator Jan 23, 2025
54b7ce6
Add accessible labels to the move buttons (describe what they move)
personalizedrefrigerator Jan 23, 2025
44306d0
Improve move button label styling
personalizedrefrigerator Jan 23, 2025
92c4127
Fix button loses focus after clicking
personalizedrefrigerator Jan 23, 2025
12d8430
Fix refocus when a button becomes disabled after clicking
personalizedrefrigerator Jan 24, 2025
1c2c758
Make styles closer to the originals
personalizedrefrigerator Jan 24, 2025
b2c553c
Attempting to announce the position of the moved item
personalizedrefrigerator Jan 24, 2025
550fa20
Revert attempt to announce position of the just-moved menu
personalizedrefrigerator Jan 24, 2025
c215796
Refactoring: Stronger types
personalizedrefrigerator Jan 24, 2025
a5e64a4
Refactoring: Convert function -> component
personalizedrefrigerator Jan 24, 2025
4180890
Refactoring, scan resize screen in integration tests
personalizedrefrigerator Jan 24, 2025
677f4c3
Fix unique key error
personalizedrefrigerator Jan 24, 2025
2f0bc12
Use group role rather than manually adding aria-describedby, add test
personalizedrefrigerator Jan 24, 2025
72e054c
Fix reset layout dialog is shown behind the app layout change screen
personalizedrefrigerator Jan 24, 2025
87dd767
Code formatting
personalizedrefrigerator Jan 24, 2025
6003790
Improve comment
personalizedrefrigerator Jan 24, 2025
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
3 changes: 3 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ packages/app-desktop/gui/PasswordInput/types.js
packages/app-desktop/gui/PdfViewer.js
packages/app-desktop/gui/PluginNotification/PluginNotification.js
packages/app-desktop/gui/PromptDialog.js
packages/app-desktop/gui/ResizableLayout/LayoutItemContainer.js
packages/app-desktop/gui/ResizableLayout/MoveButtons.js
packages/app-desktop/gui/ResizableLayout/ResizableLayout.js
packages/app-desktop/gui/ResizableLayout/utils/findItemByKey.js
Expand Down Expand Up @@ -500,6 +501,7 @@ packages/app-desktop/gulpfile.js
packages/app-desktop/integration-tests/goToAnything.spec.js
packages/app-desktop/integration-tests/main.spec.js
packages/app-desktop/integration-tests/markdownEditor.spec.js
packages/app-desktop/integration-tests/models/ChangeAppLayoutScreen.js
packages/app-desktop/integration-tests/models/GoToAnything.js
packages/app-desktop/integration-tests/models/MainScreen.js
packages/app-desktop/integration-tests/models/NoteEditorScreen.js
Expand All @@ -508,6 +510,7 @@ packages/app-desktop/integration-tests/models/SettingsScreen.js
packages/app-desktop/integration-tests/models/Sidebar.js
packages/app-desktop/integration-tests/noteList.spec.js
packages/app-desktop/integration-tests/pluginApi.spec.js
packages/app-desktop/integration-tests/resizableLayout.spec.js
packages/app-desktop/integration-tests/richTextEditor.spec.js
packages/app-desktop/integration-tests/settings.spec.js
packages/app-desktop/integration-tests/sidebar.spec.js
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ packages/app-desktop/gui/PasswordInput/types.js
packages/app-desktop/gui/PdfViewer.js
packages/app-desktop/gui/PluginNotification/PluginNotification.js
packages/app-desktop/gui/PromptDialog.js
packages/app-desktop/gui/ResizableLayout/LayoutItemContainer.js
packages/app-desktop/gui/ResizableLayout/MoveButtons.js
packages/app-desktop/gui/ResizableLayout/ResizableLayout.js
packages/app-desktop/gui/ResizableLayout/utils/findItemByKey.js
Expand Down Expand Up @@ -475,6 +476,7 @@ packages/app-desktop/gulpfile.js
packages/app-desktop/integration-tests/goToAnything.spec.js
packages/app-desktop/integration-tests/main.spec.js
packages/app-desktop/integration-tests/markdownEditor.spec.js
packages/app-desktop/integration-tests/models/ChangeAppLayoutScreen.js
packages/app-desktop/integration-tests/models/GoToAnything.js
packages/app-desktop/integration-tests/models/MainScreen.js
packages/app-desktop/integration-tests/models/NoteEditorScreen.js
Expand All @@ -483,6 +485,7 @@ packages/app-desktop/integration-tests/models/SettingsScreen.js
packages/app-desktop/integration-tests/models/Sidebar.js
packages/app-desktop/integration-tests/noteList.spec.js
packages/app-desktop/integration-tests/pluginApi.spec.js
packages/app-desktop/integration-tests/resizableLayout.spec.js
packages/app-desktop/integration-tests/richTextEditor.spec.js
packages/app-desktop/integration-tests/settings.spec.js
packages/app-desktop/integration-tests/sidebar.spec.js
Expand Down
60 changes: 25 additions & 35 deletions packages/app-desktop/gui/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,21 @@ export enum ButtonSize {
Normal = 2,
}

interface Props {
type ReactButtonProps = React.DetailedHTMLProps<React.HTMLAttributes<HTMLButtonElement>, HTMLButtonElement>;
interface Props extends Omit<ReactButtonProps, 'onClick'> {
Comment on lines +21 to +22
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the changes to the Button component are to pass unused props through to the <button> component it wraps. This removes the need to enumerate all aria-* props that can be used with custom <Button>s.

title?: string;
iconName?: string;
level?: ButtonLevel;
iconLabel?: string;
className?: string;
// eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied
onClick?: Function;
onClick?: ()=> void;
color?: string;
iconAnimation?: string;
tooltip?: string;
disabled?: boolean;
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied;
style?: any;
size?: ButtonSize;
isSquare?: boolean;
iconOnly?: boolean;
fontSize?: number;

'aria-controls'?: string;
'aria-describedby'?: string;
'aria-expanded'?: string;
}

const StyledTitle = styled.span`
Expand Down Expand Up @@ -216,55 +209,52 @@ function buttonClass(level: ButtonLevel) {
return StyledButtonSecondary;
}

const Button = React.forwardRef(({
iconName, iconLabel, iconAnimation, color, title, level, fontSize, isSquare, tooltip, disabled, onClick: propsOnClick, ...unusedProps
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied;
const Button = React.forwardRef((props: Props, ref: any) => {
const iconOnly = props.iconName && !props.title;
}: Props, ref: any) => {
const iconOnly = iconName && !title;

const StyledButton = buttonClass(props.level);
const StyledButton = buttonClass(level);

function renderIcon() {
if (!props.iconName) return null;
if (!iconName) return null;
return <StyledIcon
aria-label={props.iconLabel ?? undefined}
aria-hidden={!props.iconLabel}
animation={props.iconAnimation}
aria-label={iconLabel ?? undefined}
aria-hidden={!iconLabel}
animation={iconAnimation}
mr={iconOnly ? '0' : '6px'}
color={props.color}
className={props.iconName}
color={color}
className={iconName}
role='img'
/>;
}

function renderTitle() {
if (!props.title) return null;
return <StyledTitle color={props.color}>{props.title}</StyledTitle>;
if (!title) return null;
return <StyledTitle color={color}>{title}</StyledTitle>;
}

function onClick() {
if (props.disabled) return;
props.onClick();
if (disabled) return;
propsOnClick();
}

return (
<StyledButton
ref={ref}
fontSize={props.fontSize}
isSquare={props.isSquare}
size={props.size}
style={props.style}
disabled={props.disabled}
title={props.tooltip}
className={props.className}
fontSize={fontSize}
isSquare={isSquare}
disabled={disabled}
title={tooltip}
iconOnly={iconOnly}
onClick={onClick}

// When there's no title, the button needs a label. In this case, fall back
// to the tooltip.
aria-label={props.title ? undefined : props.tooltip}
aria-disabled={props.disabled}
aria-expanded={props['aria-expanded']}
aria-controls={props['aria-controls']}
aria-describedby={props['aria-describedby']}
aria-label={title ? undefined : tooltip}
aria-disabled={disabled}
{...unusedProps}
>
{renderIcon()}
{renderTitle()}
Expand Down
18 changes: 18 additions & 0 deletions packages/app-desktop/gui/MainScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import UpdateNotification from './UpdateNotification/UpdateNotification';
import NoteEditor from './NoteEditor/NoteEditor';
import PluginNotification from './PluginNotification/PluginNotification';
import { Toast } from '@joplin/lib/services/plugins/api/types';
import PluginService from '@joplin/lib/services/plugins/PluginService';

const ipcRenderer = require('electron').ipcRenderer;

Expand Down Expand Up @@ -121,6 +122,18 @@ const defaultLayout: LayoutItem = {
],
};

const layoutKeyToLabel = (key: string, plugins: PluginStates) => {
if (key === 'sideBar') return _('Sidebar');
if (key === 'noteList') return _('Note list');
if (key === 'editor') return _('Editor');

const viewInfo = pluginUtils.viewInfoByViewId(plugins, key);
if (viewInfo) {
return PluginService.instance().safePluginNameById(viewInfo.plugin.id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
return PluginService.instance().safePluginNameById(viewInfo.plugin.id);
return PluginService.instance().pluginNameById(viewInfo.plugin.id);

The above is currently used to label panels from plugins. The "safe" in "safePluginNameById" was intended to mean that the method doesn't throw if the plugin isn't installed/loaded. However, just pluginNameById may make more sense.

}
return key;
};

class MainScreenComponent extends React.Component<Props, State> {

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
Expand Down Expand Up @@ -728,6 +741,10 @@ class MainScreenComponent extends React.Component<Props, State> {
);
}

private layoutKeyToLabel = (key: string) => {
return layoutKeyToLabel(key, this.props.plugins);
};

public render() {
const theme = themeStyle(this.props.themeId);
const style = {
Expand All @@ -746,6 +763,7 @@ class MainScreenComponent extends React.Component<Props, State> {
onResize={this.resizableLayout_resize}
onMoveButtonClick={this.resizableLayout_moveButtonClick}
renderItem={this.resizableLayout_renderItem}
layoutKeyToLabel={this.layoutKeyToLabel}
moveMode={this.props.layoutMoveMode}
moveModeMessage={_('Use the arrows to move the layout items. Press "Escape" to exit.')}
/>
Expand Down
69 changes: 69 additions & 0 deletions packages/app-desktop/gui/ResizableLayout/LayoutItemContainer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import * as React from 'react';
import { Resizable, ResizeCallback, ResizeStartCallback, Size } from 're-resizable';
import { LayoutItem } from './utils/types';
import { itemMinHeight, itemMinWidth, itemSize, LayoutItemSizes } from './utils/useLayoutItemSizes';

interface Props {
item: LayoutItem;
parent: LayoutItem|null;
sizes: LayoutItemSizes;
resizedItemMaxSize: Size|null;
onResizeStart: ResizeStartCallback;
onResize: ResizeCallback;
onResizeStop: ResizeCallback;
children: React.ReactNode;
isLastChild: boolean;
visible: boolean;
}

const LayoutItemContainer: React.FC<Props> = ({
item, visible, parent, sizes, resizedItemMaxSize, onResize, onResizeStart, onResizeStop, children, isLastChild,
}) => {
const style: React.CSSProperties = {
display: visible ? 'flex' : 'none',
flexDirection: item.direction,
};

const size: Size = itemSize(item, parent, sizes, true);

const className = `resizableLayoutItem rli-${item.key}`;
if (item.resizableRight || item.resizableBottom) {
const enable = {
top: false,
right: !!item.resizableRight && !isLastChild,
bottom: !!item.resizableBottom && !isLastChild,
left: false,
topRight: false,
bottomRight: false,
bottomLeft: false,
topLeft: false,
};

return (
<Resizable
key={item.key}
className={className}
style={style}
size={size}
onResizeStart={onResizeStart}
onResize={onResize}
onResizeStop={onResizeStop}
enable={enable}
minWidth={'minWidth' in item ? item.minWidth : itemMinWidth}
minHeight={'minHeight' in item ? item.minHeight : itemMinHeight}
maxWidth={resizedItemMaxSize?.width}
maxHeight={resizedItemMaxSize?.height}
>
{children}
</Resizable>
);
} else {
return (
<div key={item.key} className={className} style={{ ...style, ...size }}>
{children}
</div>
);
}
};

export default LayoutItemContainer;
59 changes: 55 additions & 4 deletions packages/app-desktop/gui/ResizableLayout/MoveButtons.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
import * as React from 'react';
import { useCallback } from 'react';
import { useCallback, useId } from 'react';
import Button, { ButtonLevel } from '../Button/Button';
import { MoveDirection } from './utils/movements';
import styled from 'styled-components';
import { _ } from '@joplin/lib/locale';

const StyledRoot = styled.div`
display: flex;
flex-direction: column;
padding: 5px;
background-color: ${props => props.theme.backgroundColor};
border-radius: 5px;

> .label {
// Used only for accessibility tools
display: none;
}
`;

const ButtonRow = styled.div`
Expand All @@ -26,23 +32,32 @@ const ArrowButton = styled(Button)`
opacity: ${props => props.disabled ? 0.2 : 1};
`;

type ButtonKey = string;

export interface MoveButtonClickEvent {
direction: MoveDirection;
itemKey: string;
buttonKey: ButtonKey;
}

interface Props {
onClick(event: MoveButtonClickEvent): void;
itemKey: string;
itemLabel: string;
canMoveLeft: boolean;
canMoveRight: boolean;
canMoveUp: boolean;
canMoveDown: boolean;

// Specifies which button to auto-focus (if any). Clicking a "Move ..." button changes the app's layout. By default, this
// causes focus to jump to the start of the move dialog. Providing the key of the last-clicked button allows focus
// to be restored after changing the app layout:
autoFocusKey: ButtonKey|null;
}

export default function MoveButtons(props: Props) {
const onButtonClick = useCallback((direction: MoveDirection) => {
props.onClick({ direction, itemKey: props.itemKey });
props.onClick({ direction, itemKey: props.itemKey, buttonKey: `${props.itemKey}-${direction}` });
}, [props.onClick, props.itemKey]);

function canMove(dir: MoveDirection) {
Expand All @@ -53,28 +68,64 @@ export default function MoveButtons(props: Props) {
throw new Error('Unreachable');
}

const iconLabel = (dir: MoveDirection) => {
if (dir === MoveDirection.Up) return _('Move up');
if (dir === MoveDirection.Down) return _('Move down');
if (dir === MoveDirection.Left) return _('Move left');
if (dir === MoveDirection.Right) return _('Move right');
const unreachable: never = dir;
throw new Error(`Invalid direction: ${unreachable}`);
};

const descriptionId = useId();

const buttonKey = (dir: MoveDirection) => `${props.itemKey}-${dir}`;
const autoFocusDirection = (() => {
if (!props.autoFocusKey) return undefined;

const buttonDirections = [MoveDirection.Up, MoveDirection.Down, MoveDirection.Left, MoveDirection.Right];
const autoFocusDirection = buttonDirections.find(
direction => buttonKey(direction) === props.autoFocusKey,
);

if (!autoFocusDirection) {
return null;
}

const autoFocusDirectionEnabled = autoFocusDirection && canMove(autoFocusDirection);
if (autoFocusDirectionEnabled) {
return autoFocusDirection;
} else {
// Select an enabled direction instead
return buttonDirections.find(dir => canMove(dir));
}
})();

function renderButton(dir: MoveDirection) {
return <ArrowButton
disabled={!canMove(dir)}
level={ButtonLevel.Primary}
iconName={`fas fa-arrow-${dir}`}
iconLabel={iconLabel(dir)}
autoFocus={autoFocusDirection === dir}
onClick={() => onButtonClick(dir)}
/>;
}

return (
<StyledRoot>
<StyledRoot role='group' aria-labelledby={descriptionId}>
<ButtonRow>
{renderButton(MoveDirection.Up)}
</ButtonRow>
<ButtonRow>
{renderButton(MoveDirection.Left)}
<EmptyButton iconName="fas fa-arrow-down" disabled={true}/>
<EmptyButton iconName="fas fa-arrow-down" aria-hidden={true} disabled={true}/>
{renderButton(MoveDirection.Right)}
</ButtonRow>
<ButtonRow>
{renderButton(MoveDirection.Down)}
</ButtonRow>
<div className='label' id={descriptionId}>{props.itemLabel}</div>
</StyledRoot>
);
}
Loading
Loading