miscellaneous a11y improvements #137

abaevbog · 2024-09-04T04:20:40Z

set role="application" on toolbar and sidebar to force screen readers to use focus mode in those areas since reading mode is not applicable here and one should not have to manually switch to focus mode
made outline values visible to screen readers
improved aria-live message announced during search navigation to include the page number as well as the snippet of the result
added role="navigation" to start containers of epub ranges so that screen readers indicate when one moves to a new page. It also enabled navigation via d/shift-d for NVDA and r/shift-r for JAWS to go to next/previous page as with PDFs.
added a state variable a11yVirtualCursorTarget to record which node the screen readers should place its virtual cursor on next time the focus enters the reader. It forces virtual cursor to be moved onto that node, as opposed to landing in the beginning of the document. It is currently used to make sure screen readers begin reading the chapter/section selected in the outline, as well as to place virtual cursor on the last search result. On scroll, a11yVirtualCursorTarget is cleared to not interfere with mouse navigation.

Example of the virtual cursor jumping to the top (still this PR but without virtual cursor handling): recording

Current behavior: recording. Notice how the screen reader does read roughly where we left off in the search and where the chapter begins after outline navigation.

I think this already would be a bit improvement for the usability issues described in https://forums.zotero.org/discussion/comment/469784/#Comment_469784. There are a few other instances when it might be good to explicitly tell screen readers where to place the virtual cursor, such as when the page input is changes and when we move to the beginning/end of the document with Home/End shortcuts. But I wanted to check in to make sure this is on the right track.

@AbeJellinek, @mrtcode let me know what you think!

Fixes: zotero/zotero#4515

AbeJellinek · 2024-09-04T12:50:43Z

src/common/components/sidebar/outline-view.js

-				if (item.items && item.expanded) {
-					flatten(item.items);
-				}
+	function flatten (items, list = []) {


Remove extra space!

(Is ESLint highlighting enabled in your editor?)

Hm, thank you, that's a good point. It definitely is enabled, as I can see eslint issues in ZoteroPane but none in the reader though... So that's something to figure out! For this and other eslint issues below.

The reader has its own ESLint config (with "root": true, so it doesn't inherit from zotero-client), and that really confuses some editors. I've had the same issue with IntelliJ. Try opening reader/ as its own project in your editor, instead of editing its files within a project whose root is zotero-client/.

Yes, that was exactly it! While trying to figure it out, I temporarily changed the path to babel.config.js from relative to absolute (just to see) and everything worked. So the small syntax errors should now be taken care of. I rebased and did a new commit to make it easier to fix my edits.

AbeJellinek · 2024-09-04T12:51:35Z

src/common/components/sidebar/outline-view.js

@@ -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}>


Generally we use hyphens (-) in IDs, but it's not a big deal either way

For consistency I changed it for outline-${id}

AbeJellinek · 2024-09-04T12:53:36Z

src/common/reader.js

@@ -862,6 +849,27 @@ class Reader {
 			this.setA11yMessage(annotationContent);
 		}

+		// Add page number as aria-label to provided node to improve screen reader navigation
+		let setA11yNavContent = (node, pageIndex) => {
+			node.setAttribute("aria-label", `${this._getString("pdfReader.page")}: ${pageIndex}`);


Strings, besides React string attributes, are single-quoted in reader code ('aria-label', not "aria-label")

AbeJellinek · 2024-09-04T12:54:27Z

src/common/reader.js

+			let totalResults = `${this._getString("pdfReader.searchResultTotal")}: ${total}.`;
+			let page = pageLabel !== null ? `${this._getString("pdfReader.page")}: ${pageLabel}.` : "";
+			this.setA11yMessage(`${searchIndex} ${totalResults} ${snippet || ""} ${page}`);
+		}


Missing semicolon ESLint errors here and above

AbeJellinek · 2024-09-04T12:55:10Z

src/common/reader.js

+	// 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 () {


Remove extra space

AbeJellinek · 2024-09-04T13:13:59Z

src/dom/snapshot/snapshot-view.ts

+			elem.scrollIntoView(options);
+			// Remember which node was navigated to for screen readers to place
+			// virtual cursor on it later. Used for navigating between sections in the outline.
+			debounceUntilScrollFinishes(this._iframeDocument, 200).then(() => {


Why the varying timeout? 100 on other calls (and by default), 200 here.

On a general scroll event, we want to clear all virtual cursor target variables, otherwise, one would navigate the outline, scroll, click on the document with a mouse, and the focus would jump to our focused element. For this reason, before almost every setA11yVirtualCursorTarget, we wait for debounceUntilScrollFinishes - to make sure that the node is set once the scrolling is done. In case of snapshot, you would navigate the outline, the document would scroll, then await debounceUntilScrollFinishes(this._iframeDocument, 100) would return, we'd set the target, and sometimes a random scroll event would fire after that, which would clear the node (as if the user scrolled the document themselves).

I agree that different timeouts are not very obvious so instead, I went back to using a default timeout everywhere but added a tweak to setA11yVirtualCursorTarget where if the target was set within the last 0.5 second, it cannot be cleared. It takes care of that edge case as well, just maybe in a bit more constructive manner.

But correct me if I overthought this and there is some better way to undo all this virtual cursor target focusing on scroll of a mouse (or click or anything like that) without such overhead.

AbeJellinek · 2024-09-04T13:19:59Z

src/dom/epub/epub-view.ts

+		await debounceUntilScrollFinishes(this._iframeDocument, 100);
+
+		let searchResult = getStartElement(range);
+		// @ts-ignore


Don't @ts-ignore legitimate errors - we're passing an instance of the wrong class here. But rebase on master and this will work.

Good point. I rebased but was still getting an error due to PersistentRange not having collapsed property, so I just added it into the constructor and it seems to be good now.

AbeJellinek · 2024-09-04T13:22:58Z

src/dom/snapshot/snapshot-view.ts

+	// search result is in to place screen readers' virtual cursor on it
+	// + announce the result.
+	async a11yHandleSearchResultUpdate(range: PersistentRange) {
+		await debounceUntilScrollFinishes(this._iframeDocument, 100);


Do we need to debounce here? None of the code below seems to depend on the scroll position.

It's the same cause as in #137 (comment). A scroll should clear whatever node we planned to focus to not interfere with mouse or touchpad interactions, and navigating the search results does cause some scrolling. So this sets the current virtual cursor target AFTER scrolling that would have cleared it is done. The scroll clearing of the current state happens here

AbeJellinek · 2024-09-04T13:23:49Z

src/common/reader.js

+		}
+
+		// Announce the search index, page and snippet of the search result
+		let a11yAnnounceSearchMsg = (index, total, pageLabel, snippet) => {


Write out Msg -> Message

AbeJellinek · 2024-09-04T13:24:48Z

src/common/reader.js

+		if (target?.nodeName == "#text") {
+			target = target.parentNode;
+		}
+		if (!target || !doc.contains(target)) return;


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

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.

- set role="application" on toolbar and sidebar to force screen readers to use focus mode in those areas since reading mode is not applicable here and one should not have to manually switch to focus mode - made outline values visible to screen readers - improved aria-live message announced during search navigation to include the page number as well as the snippet of the result - added role="navigation" to start containers of epub ranges so that screen readers indicate when one moves to a new page. It also enabled navigation via d/shift-d for NVDA and r/shift-r for JAWS to go to next/previous page as with PDFs. - added a state variable a11yVirtualCursorTarget to record which node the screen readers should place its virtual cursor on next time the focus enters the reader. It forces virtual cursor to be moved onto that node, as opposed to landing in the beginning of the document. It is currently used to make sure screen readers begin reading the chapter/section selected in the outline, as well as to place virtual cursor on the last search result. On scroll, a11yVirtualCursorTarget is cleared to not interfere with mouse navigation. To make sure that scroll events of document that fire when outline is navigated don't clear the a11yVirtualCursorTarget that was just set, we wait for scrolling to finish and do not allow a11yVirtualCursorTarget to be cleared if it was added within the last 0.5 second.

So that screen reader users land on the right page after changing the page number in the input or via buttons next to it. Once the input is changed, Escape keypress re-focuses the reader and it will focus an element at the top of the page for virtual cursor to move.

mrtcode

Additionally, at least for the PDF view, we might be interested not only in the page number but also in the page label visible on specific pages.

mrtcode · 2024-09-06T12:54:37Z

src/pdf/pdf-view.js

@@ -519,6 +522,8 @@ class PDFView {
 		for (let page of this._pages) {
 			if (!pageIndexes || pageIndexes.includes(page.pageIndex)) {
 				page.render();
+				// Aria label with page index set by pdf.js is off by one, so fix it here
+				page.originalPage.div.dataset.l10nArgs = JSON.stringify({ page: this._getPageLabel(page.pageIndex) });


Can you explain more what is needed here? I can change existing or add extra attributes directly in PDF.js.

I noticed that each page has the aria-label of essentially the next page. So each page label needs to be decremented to be correct. The PDF reader accessibility setup is actually pretty good, as each page is a "region" and you can move virtual cursor from one page to another with standard screen reader shortcuts. When it happens, the aria-label of the page gets announced, and it's an issue because in the example below, you'll hear "Page 42", press arrowDown and it'll begin reading what's actually page 41, and so on.

Is it better to handle this elsewhere? Should I revert this?

What exactly do you want to include in the ARIA message—the page number (page index + 1), the page label (the actual label displayed on the page), or both?

We definitely want the page label because it is what you hear when the screen reader reads the page. So I think page label provides the most context and is a better way to identify the page.

I am not sure about page index... On one hand, it might add some additional info as to where in the document you are, so why not include it as well? On the other hand, maybe having two different identifications of a page will be distracting and a bit confusing?

Why don't we include just the page label for now to keep it simple, and I will make sure to add aria properties to page edit input in the toolbar to have the page index and total page count announced there. For casual navigation within the document, page label should be good but if you are wondering about the page index (which is something you need less often), you will check on the input in the toolbar. How does that sound?

mrtcode · 2024-09-06T13:09:25Z

src/common/reader.js

@@ -186,6 +186,10 @@ class Reader {
 				entireWord: false,
 				result: null
 			},
+			a11yVirtualCursorTarget: {


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?

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.

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.

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?

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

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!

mrtcode · 2024-09-06T13:19:41Z

src/pdf/pdf-view.js

+	async a11yHandleSearchResultUpdate() {
+		await debounceUntilScrollFinishes(this._iframeWindow.document);
+		let searchResult = this._iframeWindow.document.querySelector(".highlight.selected.appended");
+		if (!searchResult || !this._findState.result) return;
+
+		this._options.setA11yVirtualCursorTarget(searchResult.parentNode);
+
+		let { index, total } = this._findState.result;
+		let { currentPageNumber } = this._iframeWindow.PDFViewerApplication.pdfViewer;
+		let pageLabel = this._getPageLabel(currentPageNumber - 1);
+
+		let span = searchResult.parentNode;
+		let previousSpanText = span.previousSibling?.textContent || "";
+		let nextSpanText = span.nextSibling?.textContent || "";
+		let snippet = previousSpanText + span.textContent + nextSpanText;
+
+		this._options.a11yAnnounceSearchMessage(index, total, pageLabel, snippet);
+	}


I think a11yAnnounceSearchMessage might not be necessary at all, because whenever the current find result changes here, it can generate the message from it. Currently there is no snippet, but in future versions we can add it to find result.

Additionally, we had thoughts about adding a search result sidebar, similar to the one in Preview.app.

I moved the search message announcement logic into the views because of the page label. While trying to actually use the search with a screen reader, I realized that the search result index announced now is not useful but what one needs is the page label and the snippet around the search result. Right now (as indicated by that feedback from the forums), it is still preferable for people to use screen reader's search functionality, which is not great since it wouldn't find content from pages that are not currently rendered.

So thinking that we need to at least include the page label, my initial attempt was to just modify the existing setA11ySearchMessage by including the page label via this._state.primaryViewStats.pageLabel into the message and moving that function call into onSetFindState that you pointed out. So it looked like this:

let onSetFindState = (params) => { this._updateState({ [primary ? 'primaryViewFindState' : 'secondaryViewFindState']: params }); this.setA11ySearchResultMessage(primary); };

The issue was that this._state.primaryViewStats.pageLabel I was using in this.setA11ySearchResultMessage would return the label from the page that the last result was on, as opposed to the page of the new search result. Likely just a race condition when I was trying to fetch it before the state updates are done. A workaround would be some kind of timeout but that felt not reliable. So I thought we can just offload that onto the view where we can wait for the scroll to be done and grab this._iframeWindow.PDFViewerApplication.pdfViewer.currentPageNumber to tell a11yAnnounceSearchMessage which page needs to be announced.

Another benefit was that from the view, I could fetch the neighboring spans to make a snippet ourselves for the pdf pages.

So I suppose the main question is: is there a better way that I missed to get the page number of the new search result from reader.js to not have the views handle this? Or to wait for the next state update, maybe, before getting this._state.primaryViewStats.pageLabel? If the search result sidebar is added, I think this might indeed become somewhat redundant and we'd just announce that to navigate search results, check the sidebar.

We can make primaryViewFindState.result to include all the necessary current result data like snippet or page label. For now you can check if primaryViewFindState.result.currentPageLabel or primaryViewFindState.result.currentSnippet exist and use them, and otherwise don't include them. After merging this I can make PDF view to actually include those properties into the result. Additionally, the current code that retrieves snippets will break, as I've completely rewritten how PDF search works in that branch to enable highlighting/underlining search result.

Right, so I moved the a11yAnnounceSearchMessage back to Reader and I am calling it with a debounce in onSetFindState. It will add currentPageLabel and currentSnippet from the updated findState if present. It used to be called from findNext/findPrev after delay, which I don't think was quite right at the very least because it doesn't fire on the very first search result.

We can wait on for PDF view to return those properties. I assumed we'd also want epub and snapshot to include those properties in their search results as well so that everything is consistent. I added them for epub and snapshot, so those are already announced now. This part is obviously subject to review by @AbeJellinek.

This way, we could actually remove the entire a11yHandleSearchResultUpdate for snapshot and epub, since all we did there was set virtualCursorTarget and that can just be set in findNext/findPrev because it is known from the search result. For pdf view, I renamed it as a11yWillPlaceVirtCursorOnSearchResult to be more descriptive, and calling it from setFindState again just so it fires on the very first search result.

Quite a few pieces refactored here but I think it's way cleaner now than before!

mrtcode · 2024-09-06T13:25:19Z

src/pdf/pdf-view.js

@@ -827,10 +871,12 @@ class PDFView {

 	navigateToNextPage() {
 		this._iframeWindow.PDFViewerApplication.pdfViewer.nextPage();
+		this.a11yWillPlaceVirtCursorOnTop();


_updateViewStats would be a better place to track whenever page changes, because there are many ways how page number can be changed.

I suppose the question is if we always want to focus a node on the when the page changes or only in some cases. For instance, if one just scrolls the page normally with a mouse, we don't want to do that. In fact, the virtual cursor target is cleared on scroll to avoid jumping to the focused element on mouse click into the document. I'll try out what will happen if we move it to updateViewStats!

Yeah, it was a great call! Doing it through _updateViewStats allows us to have pretty just one place where the target is updated after any kind of scrolling, instead of having it scattered across multiple methods. I refactored it a bit to run through a debounce to only perform the update once when all the scrolling is finished.

Except for placeA11yVirtualCursor which has the logic shared by all view types but it now fetches the node to focus from the view.

- Update the target node to focus for in _updateViewStats which triggers on every view scroll update. It allows us to have one place where we record which page to refocus, as opposed to doing it for every navigation method - To make sure our updates don't run too often and are not cleared by the scroll listener right after they are set, a11yWillPlaceVirtCursorOnTop runs through a debounce which will set the node when view updates are all finished. - Removed timestamp of what the target was recorded since it's irrelevant now - Instead, added allowUpdates variable which will not allow the node of _a11yVirtualCursorTarget to be updated. It is needed for search results to not get overriden by the a11yWillPlaceVirtCursorOnTop when scrolling is done

Have pdfjs handle it

So that views can share it without keeping it in the reader.js. Also, clear the virtual cursor target on mousedown instead of during scroll - it also fixes the issue of focus jumping to focused element on click after scroll after an outline entry is selected. But it also allows us to avoid multiple processes fighting to set or clear virtual cursor and reduce debounce timeout as a result.

General refactoring to handle search result announcements from top-level Reader. a11yAnnounceSearchMessage adds readers currentSnippet and currentPageLabel from findState result if provided. Those don't exist for pdfs yet. Added currentSnippet and currentPageLabel to be included in epub and snapshot search results, so those are announced. More refactoring to avoid different event handlers fighting over who gets to set _a11yVirtualCursorTarget. It will just not be set by the navigation handler during search mode, which allows us to remove extraneous .allowUpdates field. Removed a11yHandleSearchResultUpdate from all views since messages are handler in Reader and curtorTargets can now be just set directly in findNext/findPrevious. Added a11yWillPlaceVirtCursorOnSearchResult specifically for pdf-view to update virtual cursor target, since findNext does not fire on first result, so we call a11yWillPlaceVirtCursorOnSearchResult when findState is updated.

mrtcode · 2024-09-16T16:35:05Z

src/common/reader.js

@@ -862,6 +860,11 @@ class Reader {
 			this.setA11yMessage(annotationContent);
 		}

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


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.

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

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

Set aria-label on epub paragraphs directly

And use it to get localized page label for epub

AbeJellinek requested changes Sep 4, 2024

View reviewed changes

abaevbog force-pushed the virtual_cursor_tweaks_2 branch from fe84482 to a80e9e9 Compare September 5, 2024 07:11

abaevbog force-pushed the virtual_cursor_tweaks_2 branch from a80e9e9 to 6d411f1 Compare September 5, 2024 07:38

mrtcode reviewed Sep 6, 2024

View reviewed changes

abaevbog added 5 commits September 6, 2024 12:27

move virtualCursor handling into views

e8f1a15

Except for placeA11yVirtualCursor which has the logic shared by all view types but it now fetches the node to focus from the view.

don't alter aria-label of the page

a4af150

Have pdfjs handle it

mrtcode reviewed Sep 16, 2024

View reviewed changes

abaevbog added 2 commits September 16, 2024 15:36

remove setA11yNavContent

f28c24c

Set aria-label on epub paragraphs directly

expose getLocalizeString to views

2bd6260

And use it to get localized page label for epub

abaevbog mentioned this pull request Sep 30, 2024

Keyboard accessibility improvements #138

Merged

miscellaneous a11y improvements #137

Are you sure you want to change the base?

miscellaneous a11y improvements #137

Conversation

abaevbog commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrtcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment