-
Notifications
You must be signed in to change notification settings - Fork 10k
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
[Editor] Improve a11y for newly added element (#15109) #15110
Conversation
@jcsteh, @MReschenberg I'd like to have your feedback. |
src/display/editor/ink.js
Outdated
@@ -479,6 +479,7 @@ class InkEditor extends AnnotationEditor { | |||
#createCanvas() { | |||
this.canvas = document.createElement("canvas"); | |||
this.canvas.className = "inkEditorCanvas"; | |||
this.canvas.setAttribute("aria-label", "User created image"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all aria-label
s in this patch fetch a localized string from the translation files?
@calixteman Sorry about the delay in getting to this! |
l10n/en-US/viewer.properties
Outdated
editor_ink_canvas_aria_label=User created image | ||
editor_ink_aria_label=Ink Editor | ||
editor_freetext_aria_label=FreeText Editor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, both in naming and ordering, should this be formatted as follows instead?
editor_ink_canvas_aria_label=User created image | |
editor_ink_aria_label=Ink Editor | |
editor_freetext_aria_label=FreeText Editor | |
editor_free_text_aria_label=FreeText Editor | |
editor_ink_aria_label=Ink Editor | |
editor_ink_canvas_aria_label=User created image |
Please also add the strings in https://github.com/mozilla/pdf.js/blob/master/web/l10n_utils.js as well, otherwise fetching of l10n-strings won't work correctly in general.
const textLayer = this.div.parentNode | ||
.getElementsByClassName("textLayer") | ||
.item(0); | ||
if (!textLayer) { | ||
return shadow(this, "textLayerElements", null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how most things in the viewer are asynchronous, the textLayer could potentially not yet exist when this code runs or at least not have been completely rendered yet; hence this could lead to intermittent behaviour/failures.
If the textLayer has finished rendering, its last element will be a div
with a endOfContent
-className. Otherwise you'd have probably have to, somehow, wait for the relevant "textlayerrendered"-event before running this code.
In any case, it seems to me that this code (and probably the rest of this patch) needs to be able to properly deal with the asynchronicity of the viewer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some (possible) inspiration, please note how the structTree
-functionality is handled in the viewer to account for the various kinds of asynchronicity involved:
Lines 785 to 815 in a1ac1a6
// The structure tree is currently only supported when the text layer is | |
// enabled and a canvas is used for rendering. | |
if (this.structTreeLayerFactory && this.textLayer && this.canvas) { | |
// The structure tree must be generated after the text layer for the | |
// aria-owns to work. | |
this._onTextLayerRendered = event => { | |
if (event.pageNumber !== this.id) { | |
return; | |
} | |
this.eventBus._off("textlayerrendered", this._onTextLayerRendered); | |
this._onTextLayerRendered = null; | |
if (!this.canvas) { | |
return; // The canvas was removed, prevent errors below. | |
} | |
this.pdfPage.getStructTree().then(tree => { | |
if (!tree) { | |
return; | |
} | |
if (!this.canvas) { | |
return; // The canvas was removed, prevent errors below. | |
} | |
const treeDom = this.structTreeLayer.render(tree); | |
treeDom.classList.add("structTree"); | |
this.canvas.append(treeDom); | |
}); | |
}; | |
this.eventBus._on("textlayerrendered", this._onTextLayerRendered); | |
this.structTreeLayer = | |
this.structTreeLayerFactory.createStructTreeLayerBuilder(pdfPage); | |
} |
removePointerInTextLayer(editor) { | ||
const { id } = editor; | ||
const node = this.#textNodes.get(id); | ||
if (!node) { | ||
return; | ||
} | ||
|
||
this.#textNodes.delete(id); | ||
let owns = node.getAttribute("aria-owns"); | ||
if (owns?.includes(id)) { | ||
owns = owns | ||
.split(" ") | ||
.filter(x => x !== id) | ||
.join(" "); | ||
if (owns) { | ||
node.setAttribute("aria-owns", owns); | ||
} else { | ||
node.removeAttribute("aria-owns"); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the textLayer has already been removed, as part of viewer clean-up (or just zooming), when this code runs; are we attempting to access dead DOM-elements in that case?
Could we perhaps avoid that sort of problem by re-factoring the #textNodes
into a WeakMap
instead?
src/display/editor/freetext.js
Outdated
|
||
FreeTextEditor._l10nPromise | ||
.get("editor_freetext_aria_label") | ||
.then(msg => this.div.setAttribute("aria-label", msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the asynchronicity of the code, would it be safer to do the following?
.then(msg => this.div.setAttribute("aria-label", msg)); | |
.then(msg => this.div?.setAttribute("aria-label", msg)); |
src/display/editor/freetext.js
Outdated
|
||
FreeTextEditor._l10nPromise | ||
.get("freetext_default_content") | ||
.then(msg => this.editorDiv.setAttribute("default-content", msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the asynchronicity of the code, would it be safer to do the following?
.then(msg => this.editorDiv.setAttribute("default-content", msg)); | |
.then(msg => this.editorDiv?.setAttribute("default-content", msg)); |
src/display/editor/ink.js
Outdated
|
||
InkEditor._l10nPromise | ||
.get("editor_ink_canvas_aria_label") | ||
.then(msg => this.canvas.setAttribute("aria-label", msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the asynchronicity of the code, would it be safer to do the following?
.then(msg => this.canvas.setAttribute("aria-label", msg)); | |
.then(msg => this.canvas?.setAttribute("aria-label", msg)); |
src/display/editor/ink.js
Outdated
|
||
InkEditor._l10nPromise | ||
.get("editor_ink_aria_label") | ||
.then(msg => this.div.setAttribute("aria-label", msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the asynchronicity of the code, would it be safer to do the following?
.then(msg => this.div.setAttribute("aria-label", msg)); | |
.then(msg => this.div?.setAttribute("aria-label", msg)); |
src/shared/util.js
Outdated
* @returns {number} Index of the first array element to pass the test, | ||
* or |items.length| if no such element exists. | ||
*/ | ||
function binarySearchFirstItem(items, condition, start = 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you expecting to use this code anywhere in the src/core/
-folder?
If not, you probably want to place this in https://github.com/mozilla/pdf.js/blob/master/src/display/display_utils.js instead to avoid unnecessarily duplicating this code in both of the built pdf.js
and pdf.worker.js
files.
I pinged Jamie and Morgan on slack, but today is the 4th of July, so I hope we'll have some feedback this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. Sorry for the delay in taking a look.
Is this expected to work on both tagged and untagged PDFs? It's hard for me to verify at present due to the aria-owns bug; see below.
const node = children[Math.max(0, index - 1)]; | ||
const owns = node.getAttribute("aria-owns"); | ||
if (!owns?.includes(id)) { | ||
node.setAttribute("aria-owns", owns ? `${owns} ${id}` : id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't working as expected. role="presentation" is incompatible with aria-owns because aria-owns requires that the element is in the a11y tree, but role="presentation" removes the element from the a11y tree. Firefox probably should ignore role="presentation" in this case (bug 1762116), but it currently doesn't. We can fix that in Firefox, but regardless, these attributes really shouldn't be set simultaneously. That is, role="presentation" should be removed if aria-owns is set and replaced when aria-owns is removed. Unfortunately, this is going to require changes to a bunch of other code here. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll remove the presentation role and add it back.
@@ -213,7 +213,7 @@ class AnnotationEditor { | |||
this.div.setAttribute("data-editor-rotation", (360 - this.rotation) % 360); | |||
this.div.className = this.name; | |||
this.div.setAttribute("id", this.id); | |||
this.div.tabIndex = 100; | |||
this.div.tabIndex = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wrapping div shouldn't have a tabindex. The contenteditable is already focusable and we should rely on that for focusability. Otherwise, we end up with a pointless (arguably broken) tab stop here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contenteditable
attribute is removed when the editor isn't in an editable state: for example when the user drags it to move it somewhere else on the page.
Anyway I'll remove the tab-index when the editor is in editing mode.
And there is too an "ink" editor in order to draw something on a pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contenteditable attribute is removed when the editor isn't in an editable state: for example when the user drags it to move it somewhere else on the page.
Ah. It'll be hard for screen reader users to drag anyway, but regardless, I'm curious: what happens when a user drags the editor? Does it become non-editable altogether after that point or just while the user is holding down the mouse button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A text editor can changed in double-clicking on it (simple click makes it the current one).
The idea behind being able to move is, is to help to adjust the position when you want to fill a form.
src/display/editor/freetext.js
Outdated
|
||
FreeTextEditor._l10nPromise | ||
.get("editor_free_text_aria_label") | ||
.then(msg => this.div?.setAttribute("aria-label", msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to focus, the role, aria-multiline and aria-label attributes must be set on the contentEditable itself (this.editorDiv), not the wrapping div.
I had a look on a tagged pdf: and I noticed the spans containing the text are inside some span with the class so I'm adding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better; thank you.
Is there supposed to be some way to dismiss the editor once you're done or does it just stay editable?
Also, how are text annotations rendered into the document after you save it? Any fixes there would probably be a separate piece of work, but I thought I'd ask here anyway.
@@ -213,7 +213,7 @@ class AnnotationEditor { | |||
this.div.setAttribute("data-editor-rotation", (360 - this.rotation) % 360); | |||
this.div.className = this.name; | |||
this.div.setAttribute("id", this.id); | |||
this.div.tabIndex = 100; | |||
this.div.tabIndex = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contenteditable attribute is removed when the editor isn't in an editable state: for example when the user drags it to move it somewhere else on the page.
Ah. It'll be hard for screen reader users to drag anyway, but regardless, I'm curious: what happens when a user drags the editor? Does it become non-editable altogether after that point or just while the user is holding down the mouse button?
I don't think it matters. It seems to work as expected as is. |
9ce3678
to
4c0f442
Compare
@Snuffleupagus , anything to add ? |
Sorry; I've not (yet) had time to look at this since the last round of changes, but will hopefully have time for that during the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this modifies the DOM of the textLayer
, have you tested that highlighting of search results still work correctly with this patch (once new annotations are added)?
Naively, without having tested, it seems to me that this could potentially cause issues. E.g. consider the case where you first search and then add a new annotation, what happens to the existing search-highlight in that case?
Assuming that this does indeed work, would it be feasible to add an integration-test for searching in "edited" documents?
} | ||
|
||
/** | ||
* Update the toolbar if it's required to reflect the tool currently used. | ||
* @param {number} mode | ||
* @returns {undefined} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and in number of places below, there's empty lines inside of JSDoc-comment blocks.
This is likely caused by search-and-replace, but please go through the patch and remove these unneeded new-lines.
return textChildren; | ||
} | ||
|
||
#hasTextLayer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps make this a getter instead?
|
||
/** | ||
* Find the text node which is the nearest and add an aria-owns attribute | ||
* in order to position correctly this editor in the text flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar nit:
* in order to position correctly this editor in the text flow. | |
* in order to correctly position this editor in the text flow. |
src/display/editor/freetext.js
Outdated
FreeTextEditor._l10nPromise.then(msg => | ||
this.editorDiv.setAttribute("default-content", msg) | ||
); | ||
this.editorDiv.id = `${this.id}-editor`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another case where I'm not sure that it's generally a good idea to add element id
s to the DOM, since that makes them linkable.
Similar to other recent patches, can we use a custom data-element-id
attribute instead?
(Also, if there's other pre-existing cases of id
s being used in the Editor they probably need to be updated as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately aria-owns
value must be an id
or a list of ids:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-owns#values
All the ideas are prefixed by:
Line 55 in e08b079
const AnnotationEditorPrefix = "pdfjs_internal_editor_"; |
We can make it a bit more complex and dynamic to avoid any conflicts.
src/display/editor/ink.js
Outdated
@@ -418,6 +429,10 @@ class InkEditor extends AnnotationEditor { | |||
this.#fitToContent(); | |||
|
|||
this.parent.addInkEditorIfNeeded(/* isCommitting = */ true); | |||
|
|||
// When commiting, the position of this editor is changed, hence we must | |||
// move it at the right position in the DOM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar nit:
// move it at the right position in the DOM. | |
// move it to the right position in the DOM. |
src/display/editor/tools.js
Outdated
this.#eventBus._on( | ||
"textlayerrendered", | ||
this.#onTextLayerRendered.bind(this) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like the event above, this one also needs to be removed on destruction.
Once the user finished to add some text, they can click outside the area of the editor to commit it.
Text annotation are rendered as basic FreeText annotations it appears that I didn't think about a11y when saving: I need to check how it works in pdf format. |
The
Yep I can but I'm not sure to see how it could be useful. |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/467f21b0eddbec9/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/467f21b0eddbec9/output.txt Total script time: 5.09 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/57a129dacc30229/output.txt Total script time: 12.04 mins
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/75fab4705366967/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/6a4f65955766292/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/75fab4705366967/output.txt Total script time: 4.51 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/6a4f65955766292/output.txt Total script time: 10.03 mins
|
- In the annotationEditorLayer, reorder the editors in the DOM according the position of the elements on the screen; - add an aria-owns attribute on the "nearest" element in the text layer which points to the added editor.
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/18e5845e66ce512/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/11ba4890119d0d6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/18e5845e66ce512/output.txt Total script time: 4.54 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/11ba4890119d0d6/output.txt Total script time: 7.16 mins
|
/botio-windows integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8f5289254ea60f0/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/8f5289254ea60f0/output.txt Total script time: 9.66 mins
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/ad8ad86eccb330f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/d0929c543c7c034/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/ad8ad86eccb330f/output.txt Total script time: 4.55 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/d0929c543c7c034/output.txt Total script time: 9.44 mins
|
@Snuffleupagus, once I've some time I'll try to fix the PageOpen issue on windows (I can't reproduce it locally). |
It's the same for me as well, it works just fine locally on Windows. |
the position of the elements on the screen;
which points to the added editor.