[Annotation] Add a div containing the text of a FreeText annotation (bug 1780375) #15267

calixteman · 2022-08-03T11:57:37Z

An annotation doesn't have to be in the text flow, hence it's likely a bad idea
to insert its text in the text layer. But the text must be visible from a screen
reader point of view so it must somewhere in the DOM.
So with this patch, the text from a FreeText annotation is extracted and added in
a div in its HTML counterpart, and with the patch #15237 the text should be visible
and positioned relatively to the text flow.

Snuffleupagus · 2022-08-03T12:12:22Z

src/core/annotation.js

+      includeMarkedContent: true,
+      combineTextItems: true,
+      sink,
+      viewBox: [-Infinity, -Infinity, Infinity, Infinity],


Does it simply not matter what these values are, or do you perhaps want to pass in the view-property (on the Page-instance) similar to how the "normal" text-extraction does it?

Snuffleupagus · 2022-08-03T12:14:20Z

src/core/document.js

+      const partialEvaluator = new PartialEvaluator({
+        xref: this.xref,
+        handler,
+        pageIndex: this.pageIndex,
+        idFactory: this._localIdFactory,
+        fontCache: this.fontCache,
+        builtInCMapCache: this.builtInCMapCache,
+        standardFontDataCache: this.standardFontDataCache,
+        globalImageCache: this.globalImageCache,
+        options: this.evaluatorOptions,
+      });
+      const textContentPromises = [];


Please move this down to after the length === 0 early return, to avoid unconditionally initializing this data.

Snuffleupagus · 2022-08-03T12:17:38Z

src/core/document.js

@@ -578,8 +578,20 @@ class Page {
    return tree;
  }

-  getAnnotationsData(intent) {
-    return this._parsedAnnotations.then(function (annotations) {
+  getAnnotationsData(handler, task, intent) {


I know that this is pre-existing code, but would things become simpler if you change this method to be async instead?

Snuffleupagus · 2022-08-03T12:17:54Z

src/core/document.js

+          textContentPromises.push(
+            annotation.extractTextContent(partialEvaluator, task).then(text => {
+              if (text) {
+                annotation.data.text = text;


Would it work, and if so make sense, to instead have the extractTextContent-method itself assign the data.text-property when a non-empty string exists?

Snuffleupagus · 2022-08-03T14:48:39Z

/botio test

pdfjsbot · 2022-08-03T14:48:41Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/428131fe9a90327/output.txt

pdfjsbot · 2022-08-03T14:48:41Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/fa9ce29dd7eb747/output.txt

pdfjsbot · 2022-08-03T15:14:31Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/fa9ce29dd7eb747/output.txt

Total script time: 25.83 mins

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

  different ref/snapshot: 14

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

pdfjsbot · 2022-08-03T15:17:55Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/428131fe9a90327/output.txt

Total script time: 29.22 mins

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

  different ref/snapshot: 6

Image differences available at: http://54.193.163.58:8877/428131fe9a90327/reftest-analyzer.html#web=eq.log

calixteman · 2022-08-03T15:28:39Z

My test is not ideal... There isn't enough transparency, I'll fix.

Snuffleupagus · 2022-08-03T16:02:22Z

src/core/annotation.js

+    }
+
+    if (text.length > 0) {
+      this.data.text = text;


It occurred to me that this property name is very generic, so how about the following instead?

Suggested change

this.data.text = text;

this.data.textContent = text;

Snuffleupagus · 2022-08-03T16:03:20Z

src/core/annotation.js

@@ -941,6 +941,56 @@ class Annotation {
    return null;
  }

+  get hasSomeText() {


For consistency, with the new property name:

Suggested change

get hasSomeText() {

get hasTextContent() {

Snuffleupagus · 2022-08-03T16:06:18Z

src/core/document.js

-        ) {
-          annotationsData.push(annotation.data);
-        }
+      if (intentAny || (intentDisplay && annotation.viewable)) {


To reduce the amount of parsing and asynchronicity in this method, since most Annotations don't have textContent, how about we limit this to only the affected Annotations instead?

Suggested change

if (intentAny || (intentDisplay && annotation.viewable)) {

if (

annotation.hasTextContent &&

(intentAny || (intentDisplay && annotation.viewable))

) {

Snuffleupagus · 2022-08-03T16:08:37Z

src/core/document.js

-        }
+      if (intentAny || (intentDisplay && annotation.viewable)) {
+        textContentPromises.push(
+          annotation.extractTextContent(partialEvaluator, task, this.view)


We need to ensure that a possible Error in the new method won't break all Annotation-parsing here, so something like the following is probably necessary.

Suggested change

annotation.extractTextContent(partialEvaluator, task, this.view)

annotation

.extractTextContent(partialEvaluator, task, this.view)

.catch(function (reason) {

warn(

`getAnnotationsData - ignoring textContent during "${task.name}" task: "${reason}".`

);

})

Snuffleupagus · 2022-08-03T16:17:58Z

web/annotation_layer_builder.css

+  opacity: 0;
+  color: transparent;


I'm assuming that this is necessary for the a11y-engine to actually "find" the elements, and that e.g. display: none; doesn't work; is that correct?

Yep, this is my understanding of:
#15237 (comment)

In the future, we could create a specific text layer for the annotation and then make the text selectable, highlightable and then searchable.

calixteman · 2022-08-03T16:39:30Z

My test is not ideal... There isn't enough transparency, I'll fix.

In fact it isn't just a matter of taste but then rendering on the canvas was broken.
I had to add:
https://github.com/mozilla/pdf.js/pull/15267/files#diff-a7e409abd3d3209d32d3adc8457c5e23feb4c06113a2e065b0c6a9322a3a3369R984

in order to have a correct rendering.

Snuffleupagus · 2022-08-03T16:58:18Z

In fact it isn't just a matter of taste but then rendering on the canvas was broken. I had to add: https://github.com/mozilla/pdf.js/pull/15267/files#diff-a7e409abd3d3209d32d3adc8457c5e23feb4c06113a2e065b0c6a9322a3a3369R984

in order to have a correct rendering.

Oh, that makes a lot of sense!

What happened to the test/test_manifest.json file?

Snuffleupagus · 2022-08-03T17:58:55Z

src/core/annotation.js

+      sink,
+      viewBox,
+    });
+    this.appearance.reset();


Looking at the rest of this file, don't you actually want this.reset() here?

calixteman · 2022-08-03T19:30:16Z

/botio test

pdfjsbot · 2022-08-03T19:30:17Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/d6a1b1ce9b0f69c/output.txt

pdfjsbot · 2022-08-03T19:30:17Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/905f60bf2a3adf6/output.txt

pdfjsbot · 2022-08-03T19:56:18Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/905f60bf2a3adf6/output.txt

Total script time: 26.00 mins

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

  different ref/snapshot: 11

Image differences available at: http://54.241.84.105:8877/905f60bf2a3adf6/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-08-03T19:59:17Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/d6a1b1ce9b0f69c/output.txt

Total script time: 28.98 mins

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

  different ref/snapshot: 3

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

Snuffleupagus · 2022-08-04T08:30:40Z

src/core/document.js

+    const partialEvaluator = new PartialEvaluator({
+      xref: this.xref,
+      handler,
+      pageIndex: this.pageIndex,
+      idFactory: this._localIdFactory,
+      fontCache: this.fontCache,
+      builtInCMapCache: this.builtInCMapCache,
+      standardFontDataCache: this.standardFontDataCache,
+      globalImageCache: this.globalImageCache,
+      options: this.evaluatorOptions,
+    });
+    const textContentPromises = [];
+    const annotationsData = [];


My final suggestion would be that we don't create a PartialEvaluator-instance unconditionally, since most pages won't actually need that:

Suggested change

const partialEvaluator = new PartialEvaluator({

xref: this.xref,

handler,

pageIndex: this.pageIndex,

idFactory: this._localIdFactory,

fontCache: this.fontCache,

builtInCMapCache: this.builtInCMapCache,

standardFontDataCache: this.standardFontDataCache,

globalImageCache: this.globalImageCache,

options: this.evaluatorOptions,

});

const textContentPromises = [];

const annotationsData = [];

const annotationsData = [],

textContentPromises = [];

let partialEvaluator;

Snuffleupagus · 2022-08-04T08:31:03Z

src/core/document.js

-          annotationsData.push(annotation.data);
-        }
+      if (annotation.hasTextContent && isVisible) {
+        textContentPromises.push(


With the comment above, this would become:

Suggested change

textContentPromises.push(

if (!partialEvaluator) {

partialEvaluator = new PartialEvaluator({

xref: this.xref,

handler,

pageIndex: this.pageIndex,

idFactory: this._localIdFactory,

fontCache: this.fontCache,

builtInCMapCache: this.builtInCMapCache,

standardFontDataCache: this.standardFontDataCache,

globalImageCache: this.globalImageCache,

options: this.evaluatorOptions,

});

}

textContentPromises.push(

…bug 1780375) An annotation doesn't have to be in the text flow, hence it's likely a bad idea to insert its text in the text layer. But the text must be visible from a screen reader point of view so it must somewhere in the DOM. So with this patch, the text from a FreeText annotation is extracted and added in a div in its HTML counterpart, and with the patch mozilla#15237 the text should be visible and positioned relatively to the text flow.

calixteman · 2022-08-04T09:15:04Z

/botio test

pdfjsbot · 2022-08-04T09:15:05Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/1f2e41b6c917329/output.txt

pdfjsbot · 2022-08-04T09:15:05Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/adc609ea372b366/output.txt

pdfjsbot · 2022-08-04T09:41:04Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/1f2e41b6c917329/output.txt

Total script time: 25.96 mins

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

  different ref/snapshot: 13
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/1f2e41b6c917329/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-08-04T09:44:03Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/adc609ea372b366/output.txt

Total script time: 28.95 mins

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

  different ref/snapshot: 4

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

calixteman · 2022-08-04T09:49:39Z

/botio makeref

pdfjsbot · 2022-08-04T09:49:41Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/f37ab75057429d0/output.txt

pdfjsbot · 2022-08-04T09:49:41Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/03aadcccfd048a9/output.txt

pdfjsbot · 2022-08-04T10:11:47Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/f37ab75057429d0/output.txt

Total script time: 22.09 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-08-04T10:12:29Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/03aadcccfd048a9/output.txt

Total script time: 22.80 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman added annotations accessibility editor labels Aug 3, 2022

calixteman requested a review from Snuffleupagus August 3, 2022 11:57

Snuffleupagus reviewed Aug 3, 2022

View reviewed changes

calixteman force-pushed the freetext_a11y branch from 1a48107 to b3047d1 Compare August 3, 2022 14:33

calixteman requested a review from Snuffleupagus August 3, 2022 14:34

Snuffleupagus reviewed Aug 3, 2022

View reviewed changes

calixteman force-pushed the freetext_a11y branch from b3047d1 to d5b1d83 Compare August 3, 2022 16:37

calixteman force-pushed the freetext_a11y branch from d5b1d83 to 89a51c8 Compare August 3, 2022 17:01

Snuffleupagus reviewed Aug 3, 2022

View reviewed changes

calixteman force-pushed the freetext_a11y branch from 89a51c8 to bfd21f9 Compare August 3, 2022 18:05

calixteman requested a review from Snuffleupagus August 4, 2022 07:53

Snuffleupagus approved these changes Aug 4, 2022

View reviewed changes

calixteman force-pushed the freetext_a11y branch from bfd21f9 to 3115574 Compare August 4, 2022 09:14

calixteman mentioned this pull request Aug 4, 2022

[Annotations] Add some aria-owns in the text layer to link to annotations (bug 1780375) #15237

Merged

calixteman merged commit b985eaa into mozilla:master Aug 4, 2022

Snuffleupagus mentioned this pull request Feb 10, 2023

Ensure that "GetAnnotations" errors are propagated to the main-thread (PR 15267 follow-up) #16042

Merged

ZeroXClem mentioned this pull request Aug 12, 2024

[Snyk] Upgrade pdfjs-dist from 2.9.359 to 2.16.105 ZeroXClem/metamesa#3

Closed

earthywh mentioned this pull request Sep 24, 2024

[Snyk] Upgrade pdfjs-dist from 2.6.347 to 2.16.105 earthywh/filestash#1

Open

[Annotation] Add a div containing the text of a FreeText annotation (bug 1780375) #15267

[Annotation] Add a div containing the text of a FreeText annotation (bug 1780375) #15267

Conversation

calixteman commented Aug 3, 2022

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 Aug 3, 2022

pdfjsbot commented Aug 3, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 3, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 3, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Aug 3, 2022

From: Bot.io (Windows)

Failed

calixteman commented Aug 3, 2022

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

calixteman commented Aug 3, 2022

Snuffleupagus commented Aug 3, 2022

Choose a reason for hiding this comment

calixteman commented Aug 3, 2022

pdfjsbot commented Aug 3, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 3, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 3, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Aug 3, 2022

From: Bot.io (Windows)

Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Aug 4, 2022

pdfjsbot commented Aug 4, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 4, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 4, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Aug 4, 2022

From: Bot.io (Windows)

Failed

calixteman commented Aug 4, 2022

pdfjsbot commented Aug 4, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 4, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 4, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Aug 4, 2022

From: Bot.io (Windows)

Success