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

Improve keyboard and screen reader accessibility #2726

Merged
merged 17 commits into from
Mar 17, 2021
Merged

Improve keyboard and screen reader accessibility #2726

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c219a08
Add ARIA roles for virtualized list
dcalhoun Mar 8, 2021
06e8ed6
Improve accessibility of notes list
dcalhoun Mar 8, 2021
bb67c3e
Reduce noise in screen reader audio
dcalhoun Mar 8, 2021
4c06696
Increase accuracy of search field descriptions
dcalhoun Mar 8, 2021
565ac58
Adjust search button styles to improve tap area
dcalhoun Mar 8, 2021
a75033e
Increase accuracy of tag list accessibility labels
dcalhoun Mar 8, 2021
51894d5
Allow scaling to improve accessibility
dcalhoun Mar 8, 2021
18c6c3e
Improve ARIA descriptions for application layout
dcalhoun Mar 8, 2021
6cddfb7
Add ARIA label for all icon buttons
dcalhoun Mar 8, 2021
bd5d02b
Set language for HTML document
dcalhoun Mar 8, 2021
2032f63
Fix cut off icon in macOS Safari
dcalhoun Mar 8, 2021
390f1d3
Add additional clarity to dev note
dcalhoun Mar 8, 2021
c324e9f
Refactor for code style preference
dcalhoun Mar 9, 2021
0797ce0
Repair note list styles
dcalhoun Mar 9, 2021
42d4ac5
[wip] Refactor sidebar title computation
dcalhoun Mar 9, 2021
2a00644
Adjust search placeholder
dcalhoun Mar 11, 2021
c8b9f2f
Display selected tag in menu bar
dcalhoun Mar 11, 2021
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
14 changes: 10 additions & 4 deletions lib/app-layout/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,22 +118,28 @@ export class AppLayout extends Component<Props> {
return (
<div className={mainClasses}>
<Suspense fallback={placeholder}>
<div className="app-layout__source-column theme-color-bg theme-color-fg">
<aside
aria-label="Notes list"
className="app-layout__source-column theme-color-bg theme-color-fg"
>
<MenuBar />
<SearchField />
<NoteList />
{showSortBar && <SortOrderSelector />}
</div>
</aside>
{editorVisible && (
<div className="app-layout__note-column theme-color-bg theme-color-fg theme-color-border">
<main
aria-label="Note editor"
className="app-layout__note-column theme-color-bg theme-color-fg theme-color-border"
>
<NoteToolbar />
{showRevisions ? (
<NotePreview noteId={openedNote} note={openedRevision} />
) : (
<NoteEditor />
)}
{hasRevisions && <RevisionSelector />}
</div>
</main>
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
)}
</Suspense>
</div>
Expand Down
6 changes: 5 additions & 1 deletion lib/components/dev-badge/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import React from 'react';

const DevBadge = () => {
return <div className="dev-badge">DEV</div>;
return (
<div aria-hidden="true" className="dev-badge">
DEV
</div>
);
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
};

export default DevBadge;
19 changes: 9 additions & 10 deletions lib/icon-button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,15 @@ export const IconButton = ({ icon, title, ...props }: Props) => (
enterDelay={200}
title={title}
>
<span>
<button
className="icon-button"
type="button"
data-title={title}
{...props}
>
{icon}
</button>
</span>
<button
aria-label={title}
className="icon-button"
type="button"
data-title={title}
{...props}
>
{icon}
</button>
</Tooltip>
);

Expand Down
4 changes: 2 additions & 2 deletions lib/index.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<!DOCTYPE html>
<html>
<html lang="en">
<head>
<meta http-equiv="Content-type" content="text/html; charset=utf-8"/>
<title><%= htmlWebpackPlugin.options.title %></title>
<meta name="app-build-reference" content="<%= htmlWebpackPlugin.options[ 'build-reference' ] %>">
<meta name="node-version" content="<%= htmlWebpackPlugin.options[ 'node-version' ] %>">
<meta name="build-platform" content="<%= htmlWebpackPlugin.options[ 'build-platform' ] %>">
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no">
<meta name="viewport" content="width=device-width, initial-scale=1">
</head>
<body>
<div id="root"></div>
Expand Down
4 changes: 3 additions & 1 deletion lib/menu-bar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export const MenuBar: FunctionComponent<Props> = ({
onClick={toggleNavigation}
title={`Menu • ${CmdOrCtrl}+Shift+U`}
/>
<div className="notes-title">{placeholder}</div>
<div className="notes-title" aria-hidden="true">
{placeholder}
</div>
<IconButton
disabled={collection.type === 'trash'}
icon={<NewNoteIcon />}
Expand Down
20 changes: 20 additions & 0 deletions lib/note-list/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import * as S from '../state';
import * as T from '../types';

type StateProps = {
collection: T.Collection;
filteredNotes: T.EntityId[];
isSmallScreen: boolean;
keyboardShortcuts: boolean;
Expand Down Expand Up @@ -212,6 +213,19 @@ export class NoteList extends Component<Props> {
this.toggleShortcuts(false);
}

computedLabel() {
switch (this.props.collection.type) {
case 'tag':
return 'Notes With Selected Tag';
case 'trash':
return 'Trash';
case 'untagged':
return 'Untagged Notes';
default:
return 'All Notes';
}
}
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved

handleShortcut = (event: KeyboardEvent) => {
if (!this.props.keyboardShortcuts) {
return;
Expand Down Expand Up @@ -311,6 +325,10 @@ export class NoteList extends Component<Props> {
<AutoSizer>
{({ height, width }) => (
<List
// Ideally aria-label is changed to aria-labelledby to
// reference the existing notes title element instead of
// computing the label https://git.io/JqLvR
aria-label={this.computedLabel()}
ref={this.list}
estimatedRowSize={24 + 18 + 21 * 4}
height={height}
Expand All @@ -320,6 +338,7 @@ export class NoteList extends Component<Props> {
rowHeight={heightCache.rowHeight}
rowRenderer={renderNoteRow}
scrollToIndex={selectedIndex}
tabIndex={null}
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference between tabIndex={-1} and tabIndex={null}?

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference being that tabIndex={null} unsets a tabIndex="0" set by default from react-virtualized and tabIndex={-1} disables the element from being focused by keyboard tabbing while still allowing programmatic focus.

Given that the contents of our virtualized list are focusable (buttons to pin or edit a note), IMO it doesn't make sense for the entire list to be a focusable element. We also, to my knowledge, have no need to programmatically focus the list, so null makes more sense than -1.

Copy link
Member

Choose a reason for hiding this comment

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

interesting. thanks! so the list is no longer focusable: does that have any implications for scrolling the list with the keyboard?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I had not explicitly tested scrolling the list with the keyboard until just now. It would appear the scrolling the list is still possible and matches the experience as when tabindex="0" is set. 👍🏻

width={width}
/>
)}
Expand All @@ -335,6 +354,7 @@ export class NoteList extends Component<Props> {

const mapStateToProps: S.MapState<StateProps> = (state) => {
return {
collection: state.ui.collection,
isSmallScreen: selectors.isSmallScreen(state),
keyboardShortcuts: state.settings.keyboardShortcuts,
noteDisplay: state.settings.noteDisplay,
Expand Down
111 changes: 57 additions & 54 deletions lib/note-list/note-cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,67 +100,70 @@ export class NoteCell extends Component<Props> {
const pinnerClasses = classNames('note-list-item-pinner', {
'note-list-item-pinned': isPinned,
});
const pinnerLabel = isPinned ? `Unpin note ${title}` : `Pin note ${title}`;

const decorators = getTerms(searchQuery).map(makeFilterDecorator);

return (
<div style={style} className={classes}>
<div className="note-list-item-status">
<div
className={pinnerClasses}
tabIndex={0}
onClick={() => pinNote(noteId, !isPinned)}
>
<SmallPinnedIcon />
<div style={style} className={classes} role="row">
<div className="note-list-item-content" role="cell">
<div className="note-list-item-status">
<button
aria-label={pinnerLabel}
className={pinnerClasses}
onClick={() => pinNote(noteId, !isPinned)}
>
<SmallPinnedIcon />
</button>
</div>
</div>

<div
className="note-list-item-text theme-color-border"
tabIndex={0}
onClick={() => openNote(noteId)}
>
<div className="note-list-item-title">
<span>
{decorateWith(decorators, withCheckboxCharacters(title))}
</span>
</div>
{'expanded' === displayMode && preview.length > 0 && (
<div className="note-list-item-excerpt theme-color-fg-dim">
{withCheckboxCharacters(preview)
.split('\n')
.map((line, index) => (
<React.Fragment key={index}>
{index > 0 && <br />}
{decorateWith(decorators, line.slice(0, 200))}
</React.Fragment>
))}
</div>
)}
{'comfy' === displayMode && preview.length > 0 && (
<div className="note-list-item-excerpt theme-color-fg-dim">
{decorateWith(
decorators,
withCheckboxCharacters(preview).slice(0, 200)
)}
<button
aria-label={`Edit note ${title}`}
className="note-list-item-text theme-color-border"
onClick={() => openNote(noteId)}
>
<div className="note-list-item-title">
<span>
{decorateWith(decorators, withCheckboxCharacters(title))}
</span>
</div>
)}
</div>
<div className="note-list-item-status-right theme-color-border">
{hasPendingChanges && (
<span
className={classNames('note-list-item-pending-changes', {
'is-offline': isOffline,
})}
>
<SmallSyncIcon />
</span>
)}
{isPublished && (
<span className="note-list-item-published-icon">
<PublishIcon />
</span>
)}
{'expanded' === displayMode && preview.length > 0 && (
<div className="note-list-item-excerpt theme-color-fg-dim">
{withCheckboxCharacters(preview)
.split('\n')
.map((line, index) => (
<React.Fragment key={index}>
{index > 0 && <br />}
{decorateWith(decorators, line.slice(0, 200))}
</React.Fragment>
))}
</div>
)}
{'comfy' === displayMode && preview.length > 0 && (
<div className="note-list-item-excerpt theme-color-fg-dim">
{decorateWith(
decorators,
withCheckboxCharacters(preview).slice(0, 200)
)}
</div>
)}
</button>
<div className="note-list-item-status-right theme-color-border">
{hasPendingChanges && (
<span
className={classNames('note-list-item-pending-changes', {
'is-offline': isOffline,
})}
>
<SmallSyncIcon />
</span>
)}
{isPublished && (
<span className="note-list-item-published-icon">
<PublishIcon />
</span>
)}
</div>
</div>
</div>
);
Expand Down
6 changes: 6 additions & 0 deletions lib/note-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@
font-family: 'Simplenote Tasks', $sans, sans-serif;
min-height: 64px;

.note-list-item-content {
display: flex;
width: 100%;
}

.note-list-item-status {
padding: 9px 8px 0 0;
}
Expand All @@ -151,6 +156,7 @@
.note-list-item-title,
.note-list-item-excerpt {
white-space: nowrap;
text-align: left;
text-overflow: ellipsis;
overflow: hidden;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/note-toolbar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export class NoteToolbar extends Component<Props> {
return !note ? (
<div className="note-toolbar-placeholder theme-color-border" />
) : (
<div className="note-toolbar">
<div aria-label="note actions" role="toolbar" className="note-toolbar">
<div className="note-toolbar__column-left">
<div className="note-toolbar__button new-note-toolbar__button-sidebar theme-color-border">
<IconButton
Expand Down
15 changes: 9 additions & 6 deletions lib/search-field/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,24 @@ export class SearchField extends Component<Props> {
render() {
const { openedTag, searchQuery } = this.props;
const hasQuery = searchQuery.length > 0;
const placeholder = openedTag ?? 'Search notes and tags';

const screenReaderLabel =
'Search ' + (openedTag ? 'notes with tag ' : '') + placeholder;
const description =
'Search ' +
(openedTag ? 'notes with tag ' + openedTag : 'notes and tags');
Copy link
Member Author

@dcalhoun dcalhoun Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to draw attention to this change. Previously the placeholder value and the ARIA label were different strings. From reviewing the history of the UI, this at one point made sense because it appears the search field was utilized as the "title" of the list of notes, but also served a search input.

However, several changes were made to this portion of the app — the UI was moved around, the structure of the state, etc. These various changes led to a screen reader description that read "Search Search notes and tags Search notes and tags, search text field." 😂 Needless to say, this description is a bit...confusing.

So, these changes are an attempt to fix this label, but it also involves changing the visual placeholder for the search input. I would appreciate if others would please look carefully at this piece to ensure the current state matches the desired experience.


return (
<div className="search-field theme-color-fg theme-color-border">
<button onClick={this.props.focusSearchField} className="icon-button">
<button
aria-label="Focus search field"
onClick={this.props.focusSearchField}
className="icon-button"
>
<SmallSearchIcon />
</button>
<input
aria-label={screenReaderLabel}
ref={this.inputField}
type="search"
placeholder={placeholder}
placeholder={description}
onChange={this.update}
onKeyUp={this.interceptEsc}
value={searchQuery}
Expand Down
6 changes: 1 addition & 5 deletions lib/search-field/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
flex-direction: row;
align-items: center;
width: 100%;
padding: 6px 5px 5px 20px;
padding: 6px 5px 5px 14px;
border-bottom: 1px solid;
height: $toolbar-height;

Expand All @@ -23,10 +23,6 @@
flex: 0 0 auto;
}

.icon-button:first-child() {
text-align: left;
}

.icon-cross-small {
transition: $anim-transition;
}
Expand Down
2 changes: 2 additions & 0 deletions lib/tag-field/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,15 @@ export class TagField extends Component<Props, OwnState> {
return (
<div ref={this.container} className="tag-field">
<div
aria-label="List of tags for the current note, click a tag to remove it"
className={classNames('tag-editor', {
'has-selection': this.hasSelection(),
})}
tabIndex={-1}
onKeyDown={this.onKeyDown}
>
<input
aria-hidden="true"
className="hidden-tag"
tabIndex={-1}
ref={this.storeHiddenTag}
Expand Down