[api-minor] Add support for basic structure tree for accessibility. #13171

brendandahl · 2021-04-01T18:55:04Z

When a PDF is "marked" we now generate a separate DOM that represents
the structure tree from the PDF. This DOM is inserted into the
element and allows screen readers to walk the tree and have more
information about headings, images, links, etc. To link the structure
tree DOM (which is empty) to the text layer aria-owns is used. This
required modifying the text layer creation so that marked items are
now tracked.

Fixes #6269

This will need more work to wire up the annotation layer. We'll also need to come up with a way to better handle alt text for images. Currently, the browser/screen reader sees an empty figure so it doesn't announce the alt text.

brendandahl · 2021-04-01T18:56:16Z

Example of how this works:

<canvas>
  <span role="heading" aria_owns="heading_id"></span>
  <span aria_owns="some_paragraph"></span>
</canvas>

In the text layer:
<span id="heading_id">Some Heading</span>
<span id="some_paragaph">Hello world!</span>

For testing, I was hoping to use something that would provide us a dump of the accessibility tree, but nothing quit matched what we want. Chrome's puppeteer has a way to get the tree, but it doesn't handle aria-owns. Firefox's devtools also has a way to dump the tree to JSON, but there's no easy way to expose this to content. Alternatively, I started making a "FauxScreenReader", but it was starting to get rather complicated, so I'll move that to a patch on its own.

Snuffleupagus

I've added a few comments, based on a very quick look at the code; I can try to review this in more detail later on in case that's helpful.

Snuffleupagus · 2021-04-01T20:45:39Z

src/core/evaluator.js

-            args = [args[0].name];
+            args = [
+              args[0].name,
+              args[1] && args[1].get ? args[1].get("MCID") : null,


Did you actually mean to do a isDict(args[1])-check here, since this currently looks a bit weird?

src/core/document.js

Snuffleupagus · 2021-04-01T20:52:12Z

src/core/struct_tree.js

+  }
+
+  get role() {
+    const name = this.dict.has("S") ? this.dict.get("S").name : "";


Don't you need to also validate that the /S-entry actually is a Name?

Snuffleupagus · 2021-04-01T21:01:34Z

src/core/struct_tree.js

+    const obj = this.dict.get("Pg");
+    if (obj) {
+      pageObjId = obj.objId;
+    }


A general question, here and elsewhere in the patch: Don't you perhaps want to access a /Ref using getRaw instead, with a following isRef-check and toString-call, rather than trusting that a dictionary always have the objId-property set?

One advantage of that approach, I think, would be fewer data lookups overall.

Much better will do.

Snuffleupagus · 2021-04-01T21:06:36Z

src/core/struct_tree.js

+    // Find the dictionary for the kid.
+    let kidDict = null;
+    if (isRef(kid)) {
+      kidDict = this.dict.xref.fetchIfRef(kid);


This is a bit redundant, since you've already checked that it's a /Ref above; hence please change fetchIfRef -> fetch.

Furthermore, are we absolutely sure that all Dictionaries have a xref-property, or would it be generally safer to pass in the XRef-instance when initializing these classes?

test/unit/struct_tree_spec.js

web/struct_tree_layer_builder.js

Snuffleupagus · 2021-04-01T21:41:06Z

web/pdf_page_view.js

+      // The structure tree must be generated after the text layer for the
+      // aria-owns to work.
+      this._handleTextLayerRendered = event => {
+        if (event.pageNumber !== this.pdfPage.pageNumber) {


this.pdfPage.pageNumber -> this.id

Snuffleupagus · 2021-04-01T21:45:31Z

web/pdf_page_view.js

+    if (this.structTreeLayerFactory && this.textLayer && this.canvas) {
+      // The structure tree must be generated after the text layer for the
+      // aria-owns to work.
+      this._handleTextLayerRendered = event => {


Name-nit: Perhaps this._onTextLayerRendered instead?

Snuffleupagus · 2021-04-01T21:47:33Z

web/pdf_page_view.js

+          this.canvas.appendChild(treeDom);
+        });
+      };
+      this.eventBus._on("textlayerrendered", this._handleTextLayerRendered);


There's no guarantee that this event listener is ever removed, if the page is e.g. destroyed, hence please add something like the following to the cancelRendering-method:

if (this._onTextLayerRendered) { this.eventBus._off("textlayerrendered", this._onTextLayerRendered); this._onTextLayerRendered = null; }

timvandermeij · 2021-04-02T18:48:16Z

web/text_layer_builder.css

@@ -24,7 +24,7 @@
  line-height: 1;
 }

-.textLayer > span {
+.textLayer span {


If we change this, it should probably also be changed here since that file is based on this one: https://github.com/mozilla/pdf.js/blob/master/test/text_layer_test.css#L26

Additionally, there's a handful of .textLayer > span-related rules in https://github.com/mozilla/pdf.js/blob/master/web/viewer.css so please check if those also need to be updated.

Not sure how I missed those. Technically one or two can be left since the property is inherited, but for consistency it's probably better to just change them all.

src/core/document.js

Snuffleupagus · 2021-04-05T11:01:13Z

src/core/evaluator.js

-            args = [args[0].name];
+            args = [
+              args[0].name,
+              args[1] && isDict(args[1]) ? args[1].get("MCID") : null,


Nit: The following would probably suffice here

Suggested change

args[1] && isDict(args[1]) ? args[1].get("MCID") : null,

args[1] instanceof Dict ? args[1].get("MCID") : null,

Snuffleupagus · 2021-04-05T11:03:47Z

src/core/evaluator.js

+                type: "beginMarkedContentProps",
+                id:
+                  isDict(args[1]) && args[1].has("MCID")
+                    ? `page${pageObjId}_mcid${args[1].get("MCID")}`


With the changes suggested above, this would become

Suggested change

? `page${pageObjId}_mcid${args[1].get("MCID")}`

? `${this.idFactory.getPageObjId()}_mcid${args[1].get("MCID")}`

Snuffleupagus · 2021-04-05T11:05:14Z

src/core/evaluator.js

+              textContent.items.push({
+                type: "beginMarkedContentProps",
+                id:
+                  isDict(args[1]) && args[1].has("MCID")


Don't you need to validate that the MCID-data is of the expected type here?

Snuffleupagus · 2021-04-05T11:08:04Z

src/core/obj.js

+      if (ex instanceof MissingDataException) {
+        throw ex;
+      }
+      warn("Unable to structTreeRoot info.");


Nit: Missing word here?

Suggested change

warn("Unable to structTreeRoot info.");

warn("Unable to read structTreeRoot info.");

Snuffleupagus · 2021-04-05T11:23:23Z

src/core/struct_tree.js

+      obj.children = [];
+      parent.children.push(obj);
+      if (node.dict.has("Alt")) {
+        obj.alt = node.dict.get("Alt");


Shouldn't the Alt value be validated, before being used here, to ensure that it's of the expected type?

Also, since I guess that this's a string you probably need wrap to it in stringToPDFString as well.

Snuffleupagus · 2021-04-05T11:23:53Z

src/core/struct_tree.js

+        obj.alt = node.dict.get("Alt");
+      }
+
+      for (let i = 0; i < node.kids.length; ++i) {


Nit: Here, and below, for...of instead?

web/pdf_page_view.js

Snuffleupagus · 2021-04-05T11:30:28Z

web/struct_tree_layer_builder.js

+  }
+
+  _setAttributes(structElement, htmlElement) {
+    if ("alt" in structElement) {


Here, and below, can't we compare with ... !== undefined rather than using in?

web/struct_tree_layer_builder.js

Snuffleupagus · 2021-04-05T12:16:59Z

src/core/struct_tree.js

+    }
+  }
+
+  addNode(dict, map, level) {


Suggested change

addNode(dict, map, level) {

addNode(dict, map, level = 0) {

Snuffleupagus · 2021-04-05T12:17:14Z

src/core/struct_tree.js

+   */
+  get serializable() {
+    const MAX_DEPTH = 40;
+    function nodeToSerializable(node, parent, level) {


Suggested change

function nodeToSerializable(node, parent, level) {

function nodeToSerializable(node, parent, level = 0) {

test/unit/jasmine-boot.js

Snuffleupagus · 2021-04-09T07:51:55Z

/botio test

pdfjsbot · 2021-04-09T07:51:56Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/8b88b70d7e3875e/output.txt

pdfjsbot · 2021-04-09T07:51:56Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/0caba9cd0fe84fa/output.txt

pdfjsbot · 2021-04-09T08:16:51Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/8b88b70d7e3875e/output.txt

Total script time: 24.90 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/8b88b70d7e3875e/reftest-analyzer.html#web=eq.log

pdfjsbot · 2021-04-09T08:20:34Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/0caba9cd0fe84fa/output.txt

Total script time: 28.61 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/0caba9cd0fe84fa/reftest-analyzer.html#web=eq.log

Snuffleupagus

Looking at the reference-tests, a number of the text ones now seem to have something rendered in the top-left corner of the pages. Edit: Oh, it seems that those are actually the "container" <span>-elements that's added by this patch, for pages with struct-trees; can we please use e.g. a suitable CSS class to hide those special <span>-elements?

With the text-tests, and the last nits, fixed this looks good to me; thank you!

Snuffleupagus · 2021-04-09T08:19:31Z

src/core/document.js

        combineTextItems,
        sink,
+        pageObjId: this.ref.toString(),


Please remove this line, since it should be unused now.

Snuffleupagus · 2021-04-09T08:19:43Z

src/core/evaluator.js

    sink,
    seenStyles = new Set(),
+    pageObjId,


Please remove this line, since it should be unused now.

Snuffleupagus · 2021-04-09T08:20:41Z

src/core/evaluator.js

                    sink: sinkWrapper,
                    seenStyles,
+                    pageObjId,


Please remove this line, since it should be unused now.

Snuffleupagus · 2021-04-09T15:29:59Z

web/pdf_page_view.js

+        this._onTextLayerRendered = null;
+        this.pdfPage.getStructTree().then(tree => {
+          const treeDom = this.structTreeLayer.render(tree);
+          this.structTree = treeDom;


Nit: You're never accessing and/or resetting this property anywhere, is it actually necessary?

We don't really need to rebuild the struct tree on re-renders, but I'll fix that in a follow up.

When a PDF is "marked" we now generate a separate DOM that represents the structure tree from the PDF. This DOM is inserted into the <canvas> element and allows screen readers to walk the tree and have more information about headings, images, links, etc. To link the structure tree DOM (which is empty) to the text layer aria-owns is used. This required modifying the text layer creation so that marked items are now tracked.

brendandahl · 2021-04-09T16:57:07Z

/botio test

pdfjsbot · 2021-04-09T16:57:09Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/6f9a1b79b206976/output.txt

pdfjsbot · 2021-04-09T16:57:09Z

From: Bot.io (Windows)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://3.101.106.178:8877/f9c71badc41b1c0/output.txt

pdfjsbot · 2021-04-09T17:22:02Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/6f9a1b79b206976/output.txt

Total script time: 24.87 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/6f9a1b79b206976/reftest-analyzer.html#web=eq.log

pdfjsbot · 2021-04-09T17:26:10Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/f9c71badc41b1c0/output.txt

Total script time: 29.00 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/f9c71badc41b1c0/reftest-analyzer.html#web=eq.log

timvandermeij · 2021-04-09T19:33:04Z

Really nice work! I also did a pass over this, but it looks really good to me.

/botio makeref

pdfjsbot · 2021-04-09T19:33:05Z

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/b5777ba4db7ebc1/output.txt

pdfjsbot · 2021-04-09T19:33:05Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 1

Live output at: http://3.101.106.178:8877/2cf94295d99eef4/output.txt

pdfjsbot · 2021-04-09T19:56:01Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/b5777ba4db7ebc1/output.txt

Total script time: 22.91 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2021-04-09T20:04:00Z

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/2cf94295d99eef4/output.txt

Total script time: 26.51 mins

Lint: Passed
Make references: Passed
Check references: Passed

Snuffleupagus requested changes Apr 1, 2021

View reviewed changes

brendandahl force-pushed the struct-tree branch from ecdca17 to 13022c0 Compare April 2, 2021 16:31

timvandermeij added the core label Apr 2, 2021

timvandermeij reviewed Apr 2, 2021

View reviewed changes

Snuffleupagus requested changes Apr 5, 2021

View reviewed changes

Snuffleupagus reviewed Apr 5, 2021

View reviewed changes

Snuffleupagus reviewed Apr 6, 2021

View reviewed changes

test/unit/jasmine-boot.js Show resolved Hide resolved

brendandahl force-pushed the struct-tree branch 3 times, most recently from fccd302 to 2ce79b5 Compare April 8, 2021 22:45

Snuffleupagus requested changes Apr 9, 2021

View reviewed changes

Snuffleupagus reviewed Apr 9, 2021

View reviewed changes

brendandahl force-pushed the struct-tree branch from 2ce79b5 to fc9501a Compare April 9, 2021 16:56

Snuffleupagus approved these changes Apr 9, 2021

View reviewed changes

timvandermeij approved these changes Apr 9, 2021

View reviewed changes

timvandermeij merged commit 03c8c89 into mozilla:master Apr 9, 2021

Snuffleupagus mentioned this pull request Apr 24, 2021

Wrong highlighted text on search #13292

Closed

calixteman mentioned this pull request May 2, 2021

Highlight when searching can be misplaced #13323

Closed

timvandermeij mentioned this pull request May 22, 2021

getStructTree not available on es5 build? #13423

Closed

Snuffleupagus mentioned this pull request Jun 3, 2021

Fix scrolling of search results in documents with marked content (bug 1714183) #13482

Merged

mattdricker mentioned this pull request Jun 30, 2021

PDF.js does not recognize semantic markup in PDFs hypothesis/support-legacy#214

Closed

Snuffleupagus mentioned this pull request Aug 3, 2021

(Feature request) Denoting PDF pagination artifacts (i.e. page headers and footers) in HTML output #13857

Closed

rajagemini mentioned this pull request Oct 20, 2021

Latest version not reading entire pdf in Android Talkback #14168

Closed

Snuffleupagus mentioned this pull request Oct 28, 2021

Textlayer misplaced on Google Chrome #14205

Closed

MattL75 mentioned this pull request Nov 20, 2021

Improve accessibility for screen reader users. wojtekmaj/react-pdf#831

Closed

danielweck mentioned this pull request Sep 13, 2022

pdf: utilize bookmarks and heading for navigation edrlab/thorium-reader#1820

Open

	args[1] && isDict(args[1]) ? args[1].get("MCID") : null,
	args[1] instanceof Dict ? args[1].get("MCID") : null,

	? `page${pageObjId}_mcid${args[1].get("MCID")}`
	? `${this.idFactory.getPageObjId()}_mcid${args[1].get("MCID")}`

	warn("Unable to structTreeRoot info.");
	warn("Unable to read structTreeRoot info.");

	function nodeToSerializable(node, parent, level) {
	function nodeToSerializable(node, parent, level = 0) {

[api-minor] Add support for basic structure tree for accessibility. #13171

[api-minor] Add support for basic structure tree for accessibility. #13171

Conversation

brendandahl commented Apr 1, 2021 • edited

brendandahl commented Apr 1, 2021

Snuffleupagus 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

Snuffleupagus Apr 5, 2021 • edited

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

Snuffleupagus commented Apr 9, 2021

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment • edited

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

Snuffleupagus Apr 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandahl commented Apr 9, 2021

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Failed

timvandermeij commented Apr 9, 2021

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 9, 2021

From: Bot.io (Linux m4)

Success

pdfjsbot commented Apr 9, 2021

From: Bot.io (Windows)

Success

brendandahl commented Apr 1, 2021 •

edited

Snuffleupagus Apr 5, 2021 •

edited

Snuffleupagus left a comment •

edited

Snuffleupagus Apr 9, 2021 •

edited