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

miscellaneous a11y improvements #137

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

miscellaneous a11y improvements #137

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6d411f1
miscellaneous a11y improvements
abaevbog Sep 5, 2024
8ae1105
move virtual cursor when page is set in toolbar
abaevbog Sep 6, 2024
e8f1a15
move virtualCursor handling into views
abaevbog Sep 6, 2024
577a916
record top page to focus in _updateViewStats
abaevbog Sep 6, 2024
a4af150
don't alter aria-label of the page
abaevbog Sep 10, 2024
9ad399f
move placeA11yVirtualCursor into utilities.js
abaevbog Sep 11, 2024
cadc0ee
move a11y search message handling to reader.js
abaevbog Sep 12, 2024
f28c24c
remove setA11yNavContent
abaevbog Sep 16, 2024
2bd6260
expose getLocalizeString to views
abaevbog Sep 16, 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
28 changes: 15 additions & 13 deletions src/common/components/sidebar/outline-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function Item({ item, id, children, onOpenLink, onUpdate, onSelect }) {
let { expanded, active } = item;

return (
<li>
<li id={`outline-${id}`} aria-label={item.title}>
<div
className={cx('item', { expandable: !!item.items?.length, expanded, active })}
data-id={id}
Expand Down Expand Up @@ -76,20 +76,20 @@ function OutlineView({ outline, onNavigate, onOpenLink, onUpdate}) {
}
}

function handleKeyDown(event) {
let { key } = event;

let list = [];
function flatten(items) {
for (let item of items) {
list.push(item);
if (item.items && item.expanded) {
flatten(item.items);
}
function flatten(items, list = []) {
for (let item of items) {
list.push(item);
if (item.items && item.expanded) {
flatten(item.items, list);
}
}
return list;
}

function handleKeyDown(event) {
let { key } = event;

flatten(outline);
let list = flatten(outline);

let currentIndex = list.findIndex(x => x.active);
let currentItem = list[currentIndex];
Expand Down Expand Up @@ -166,16 +166,18 @@ function OutlineView({ outline, onNavigate, onOpenLink, onUpdate}) {
);
}

let active = flatten(outline || []).findIndex(item => item.active);
return (
<div
ref={containerRef}
className={cx('outline-view', { loading: outline === null })}
data-tabstop="1"
tabIndex={-1}
id="outlineView"
role="tabpanel"
role="listbox"
aria-labelledby="viewOutline"
onKeyDown={handleKeyDown}
aria-activedescendant={active !== -1 ? `outline-${active}` : null}
>
{outline === null ? <div className="spinner"/> : renderItems(outline)}
</div>
Expand Down
4 changes: 2 additions & 2 deletions src/common/components/sidebar/sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function Sidebar(props) {
}

return (
<div id="sidebarContainer" className="sidebarOpen">
<div id="sidebarContainer" className="sidebarOpen" role="application">
<div className="sidebar-toolbar">
<div className="start" data-tabstop={1} role="tablist">
{props.type === 'pdf' &&
Expand Down Expand Up @@ -72,7 +72,7 @@ function Sidebar(props) {
<div id="annotationsView" role="tabpanel" aria-labelledby="viewAnnotations" className={cx("viewWrapper", { hidden: props.view !== 'annotations'})}>
{props.annotationsView}
</div>
<div className={cx("viewWrapper", { hidden: props.view !== 'outline'})}>
<div className={cx("viewWrapper", { hidden: props.view !== 'outline' })} role="tabpanel">
{props.outlineView}
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/common/components/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function Toolbar(props) {
}

return (
<div className="toolbar" data-tabstop={1}>
<div className="toolbar" data-tabstop={1} role="application">
<div className="start">
<button
id="sidebarToggle"
Expand Down
94 changes: 78 additions & 16 deletions src/common/reader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ class Reader {
entireWord: false,
result: null
},
a11yVirtualCursorTarget: {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to expose nodes from within the view’s iframe? Shouldn’t the view itself track and manage its virtual cursor independently from the reader UI? It seems like all the a11yVirtualCursorTarget related code could be moved into the views.

What happens if the view is split and both views modify the a11yVirtualCursorTarget?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually is a fair point... I'll offload the rest of virtual cursor logic into the views.

I thought of the split views as a very unlikely edge case because this is done just to help screen readers figure out where to look, so I think a split view is somewhat unlikely in this context. In a scenario that it does happen, I thought the later update would get the priority (as it's the view currently worked on), and if through some obscure focus movements we enter a view that the cursor target does not belongs to, we do nothing. But, yeah, if each frame keeps track of the node itself, this is no longer relevant.

Copy link
Member

Choose a reason for hiding this comment

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

But after removing a11yVirtualCursorTarget it now should be possible to move placeA11yVirtualCursor into views as well? Because it simply retrieves a target element from the view and doesn’t appear to interact with anything outside the iframe. If it needs to be triggered on focus, note that the views have a focus() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It totally is. placeA11yVirtualCursor is the same for all views though (at least as of now) and has some amount of logic. So I thought it would be better to avoid duplicating code in case we want to change something later to avoid having to edit it in multiple places. There is some duplication now already for the getter and setter of the a11yVirtualCursorTarget but those don't have as much logic so I figured those are not a big issue.

Is duplication OK in this instance? Or is there a better place for views to share this function?

Copy link
Member

Choose a reason for hiding this comment

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

If you just want to reuse the same function across views, you can add it to src/common/lib/utilities.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, gotcha. I moved placeA11yVirtualCursor to utils, so now we don't have anything virtualCursorTarget-related in the Reader. With some additional refactoring to simplify things, all the logic is pushed into the views, and there is actually pretty much no duplication left. So that's good!

node: null,
ts: null
}
};

if (options.secondaryViewState) {
Expand Down Expand Up @@ -655,33 +659,18 @@ class Reader {
this._onTextSelectionAnnotationModeChange(mode);
}

// Announce the index of current search result to screen readers
setA11ySearchResultMessage(primaryView) {
let result = (primaryView ? this._state.primaryViewFindState : this._state.secondaryViewFindState).result;
if (!result) return;
let searchIndex = `${this._getString("pdfReader.searchResultIndex")}: ${result.index + 1}`;
let totalResults = `${this._getString("pdfReader.searchResultTotal")}: ${result.total}`;
this.setA11yMessage(`${searchIndex}. ${totalResults}`);
}

findNext(primary) {
if (primary === undefined) {
primary = this._lastViewPrimary;
}
(primary ? this._primaryView : this._secondaryView).findNext();
setTimeout(() => {
this.setA11ySearchResultMessage(primary);
});
}

findPrevious(primary) {
if (primary === undefined) {
primary = this._lastViewPrimary;
}
(primary ? this._primaryView : this._secondaryView).findPrevious();
setTimeout(() => {
this.setA11ySearchResultMessage(primary);
});
}

toggleEPUBAppearancePopup({ open }) {
Expand Down Expand Up @@ -797,6 +786,7 @@ class Reader {
this.focusView(primary);
// A workaround for Firefox/Zotero because iframe focusing doesn't trigger 'focusin' event
this._focusManager._closeFindPopupIfEmpty();
this.placeA11yVirtualCursor();
};

let onRequestPassword = () => {
Expand Down Expand Up @@ -862,6 +852,35 @@ class Reader {
this.setA11yMessage(annotationContent);
}

// Add page number as aria-label to provided node to improve screen reader navigation
let setA11yNavContent = (node, pageIndex) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this function inside reader.js and not in the views, because views don't have localized strings? If so, we could introduce a this._getString function to the views. For now, you can just do it without localization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this function inside reader.js and not in the views, because views don't have localized strings?

Yes, exactly.

If so, we could introduce a this._getString function to the views.

That would be quite useful! All a11y-related tweaks require some kind of additional strings for context, so not having to drag it all from the reader would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the setA11yNavContent from the reader and just set the page label directly from epub view.

I then did try to just expose the _getString to views as a getLocalizedString to fetch the localized "page" label and there were no issues there. So I pushed it too as a separate commit. Correct me if that's not the best way to do it - I can drop that last change

node.setAttribute('aria-label', `${this._getString("pdfReader.page")}: ${pageIndex}`);
};

// Set which node should receive focus when the focus enters the reader to
// help screen readers place virtual cursor at the right location
let setA11yVirtualCursorTarget = (node) => {
if (node && node !== this._state.a11yVirtualCursorTarget.node) {
this._updateState({ a11yVirtualCursorTarget: { node, ts: Date.now() } });
}
// Clear the cursor only half a second after it was set. It ensures the
// target is not cleared by scrolling of the document during outline navigation.
// Particularly important for snapshots where a random scroll event would fire after
// debounceUntilScrollFinishes is done. In all other instances of scrolling,
// the virtual cursor target is cleared
if (node === null && Date.now() - this._state.a11yVirtualCursorTarget.ts > 500) {
this._updateState({ a11yVirtualCursorTarget: { node: null, ts: null } });
}
};

// Announce the search index, page and snippet of the search result
let a11yAnnounceSearchMessage = (index, total, pageLabel, snippet) => {
let searchIndex = `${this._getString("pdfReader.searchResultIndex")}: ${index + 1}.`;
let totalResults = `${this._getString("pdfReader.searchResultTotal")}: ${total}.`;
let page = pageLabel !== null ? `${this._getString("pdfReader.page")}: ${pageLabel}.` : "";
this.setA11yMessage(`${searchIndex} ${totalResults} ${snippet || ""} ${page}`);
};

let data;
if (this._type === 'pdf') {
data = this._data;
Expand Down Expand Up @@ -908,7 +927,9 @@ class Reader {
onTabOut,
onKeyDown,
onKeyUp,
onFocusAnnotation
onFocusAnnotation,
setA11yVirtualCursorTarget,
a11yAnnounceSearchMessage
};

if (this._type === 'pdf') {
Expand Down Expand Up @@ -939,6 +960,7 @@ class Reader {
fontFamily: this._state.fontFamily,
hyphenate: this._state.hyphenate,
onEPUBEncrypted,
setA11yNavContent,
});
} else if (this._type === 'snapshot') {
view = new SnapshotView({
Expand All @@ -965,6 +987,46 @@ class Reader {
document.getElementById("a11yAnnouncement").innerText = a11yMessage;
}

// Make a11yVirtualCursorTarget node set previously focusable and
// focus it to help screen readers understand where the virtual cursor needs to
// be positioned. This is required because screen readers are not aware of
// scroll positioning, so without this, the virtual cursor will always land
// at the start of the document.
placeA11yVirtualCursor() {
let target = this._state.a11yVirtualCursorTarget.node;
let doc = this._lastView._iframe.contentDocument;
// If the target is a text node, use its parent (e.g. <p> or <h>)
if (target?.nodeType === Node.TEXT_NODE) {
target = target.parentNode;
}
if (!target || !doc.contains(target)) return;
Copy link
Member

Choose a reason for hiding this comment

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

If the iframe doesn't contain the a11yVirtualCursorTarget, something has gone very wrong, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's certainly not intended but I think there can be some odd instances of when it would happen without something completely breaking...

For example, if one uses the split horizontal or vertical views. One can navigate the outline, which will remember a11yVirtualCursorTarget for one view, then hit shift-tab a bunch of times to wrap focus around so it enters the other view. That other view would not have that node from the first view. Keeping in mind that this is to improve the experience of screen reader users, this example is likely not very appropriate but still something I thought of.

// Make it temporarily focusable
target.setAttribute("tabindex", "-1");
target.focus();

// On blur or keypress, blur it
if (doc.activeElement == target) {
target.addEventListener("blur", (_) => {
target.removeAttribute("tabindex");
});
target.addEventListener("keydown", (_) => {
target.blur();
});
// Keypress may not fire if screen reader is being used, in which
// case remove tabindex next time the page content is scrolled
let cleanUpOnScroll = (_) => {
target.blur();
doc.removeEventListener("scroll", cleanUpOnScroll);
};
doc.addEventListener("scroll", cleanUpOnScroll);
}
// If the focus didn't take, make sure temp tabindex is removed
else {
target.removeAttribute("tabindex");
}
this._updateState({ a11yVirtualCursorTarget: { node: null, ts: null } });
}

getUnsavedAnnotations() {

}
Expand Down
6 changes: 6 additions & 0 deletions src/dom/common/dom-view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,9 @@ abstract class DOMView<State extends DOMViewState, Data> {
this._renderAnnotations();
this._repositionPopups();
});
// Clear whatever node we may have planned to focus for screen readers
// to not interfere with mouse navigation
this._options.setA11yVirtualCursorTarget(null);
}

protected _handleScrollCapture(event: Event) {
Expand Down Expand Up @@ -1412,6 +1415,9 @@ export type DOMViewOptions<State extends DOMViewState, Data> = {
onKeyUp: (event: KeyboardEvent) => void;
onKeyDown: (event: KeyboardEvent) => void;
onEPUBEncrypted: () => void;
setA11yVirtualCursorTarget: (node: Node | null) => void;
setA11yNavContent: (node: Node, pageIndex: string) => void;
a11yAnnounceSearchMessage: (index: number, total: number, pageLabel: string | null, snippet: string) => void;
data: Data & {
buf?: Uint8Array,
url?: string
Expand Down
3 changes: 3 additions & 0 deletions src/dom/common/lib/range.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@ export class PersistentRange {

endOffset: number;

collapsed: boolean;

constructor(range: AbstractRange) {
this.startContainer = range.startContainer;
this.startOffset = range.startOffset;
this.endContainer = range.endContainer;
this.endOffset = range.endOffset;
this.collapsed = range.collapsed;
}

toRange(): Range {
Expand Down
Loading
Loading