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.

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

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

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

calixteman
Copy link
Contributor

This patch doesn't structurally change the text layer: it just adds some aria-owns
attributes to some spans.
The aria-owns attribute expect to have an element id, hence it's why it adds back an
id on the element rendering an annotation, but this id is built in using crypto.randomUUID
to avoid any potential issues with the hash in the url.
The elements in the annotation layer are moved into the DOM in order to have them in the
same "order" as they visually are.
The overall goal is to help screen readers to present to the user the annotations as
they visually are and as they come in the text flow.
It is clearly not perfect, but it should improve readability for some people with visual
disabilities.

@calixteman
Copy link
Contributor Author

@jcsteh, @MReschenberg, your feedback will be appreciated.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jul 29, 2022

Please note: I'll hold off on reviewing this until feedback has been provided from the a11y-devs, as was requested above, on the off chance that the implementation changes significantly.

Furthermore, it'd also be a really good idea to create a new release first given the size/scope of these changes. (Which hopefully happens soon, since we agreed to try and have a new release at the end of each month.)

@timvandermeij
Copy link
Contributor

Indeed, I'll make a new release tomorrow since it's the end of the month.

@jcsteh
Copy link

jcsteh commented Aug 1, 2022

I took a look at this, but I wasn't able to see the annotations with a screen reader. Some questions:

  1. Perhaps I'm not understanding how to commit/save annotations to a document. I made a text annotation, dismissed the editor with control+enter, saved the PDF with control+s and then opened the saved PDF in the modified pdf.js. Is there anything else I need to do? I'm pretty sure I can see the annotation with OCR, but it's hard to be certain.
  2. Is there anything else I need to do when switching pdf.js branches apart from npm install and gulp server?
  3. Are you able to see the annotations in the Firefox Accessibility Inspector?

Also, how will this interact with tagged PDF? I don't fully understand how the text and structure layers fit together, but I know tagged PDF (structure) uses aria-owns as well.

@jcsteh
Copy link

jcsteh commented Aug 1, 2022

When the text annotations are added to the DOM, do they have C SS display: none and/or visibility: hidden? That will prevent them from being exposed to a11y.

One other thing I've just noticed is that when a text editor is dismissed, the editor still has role="textbox". That should be removed when contentEditable is disabled. I guess that needs to be handled elsewhere, though?

@@ -807,6 +812,7 @@ class PDFPageView {
}
this.eventBus._off("textlayerrendered", this._onTextLayerRendered);
this._onTextLayerRendered = null;
this._accessibilityManager.onTextLayerRendered();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most likely this line, or rather its placement, explains the problem mentioned in #15237 (comment).

By placing this line here you've limited this functionality to only documents with structureTree-information, which I'm guessing isn't actually intended?
If you actually want to call this method unconditionally, you'll need to pass in the AccessibilityManager-instance to the TextLayerBuilder-instance and invoke that method there instead (similar to how the TextHighlighter-instance is handled).

Basically, I'm guessing that you want to call it after the following line instead:

this.highlighter?.enable();

Copy link
Contributor Author

@calixteman calixteman Aug 2, 2022

Choose a reason for hiding this comment

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

I took a look at this, but I wasn't able to see the annotations with a screen reader. Some questions:

1. Perhaps I'm not understanding how to commit/save annotations to a document. I made a text annotation, dismissed the editor with control+enter, saved the PDF with control+s and then opened the saved PDF in the modified pdf.js. Is there anything else I need to do? I'm pretty sure I can see the annotation with OCR, but it's hard to be certain.

2. Is there anything else I need to do when switching pdf.js branches apart from npm install and gulp server?

It's exactly what you've to do.

3. Are you able to see the annotations in the Firefox Accessibility Inspector?

Also, how will this interact with tagged PDF? I don't fully understand how the text and structure layers fit together, but I know tagged PDF (structure) uses aria-owns as well.

I fixed the bug mentioned by Jonas and it should be fine now: I can see it correctly in the inspector.
Anyway, for now the content of the annotation is still not available: I just added an aria-owns between a span in the text layer and an annotation in the annotation layer. So it should be useful to see at least the annotations which don't have any content like a text field or a checkbox.
I've a wip to make the text content of a FreeText annotation in its HTML counterpart.

Copy link

Choose a reason for hiding this comment

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

Anyway, for now the content of the annotation is still not available: I just added an aria-owns between a span in the text layer and an annotation in the annotation layer. So it should be useful to see at least the annotations which don't have any content like a text field or a checkbox.

I'm not really sure how to test this/provide feedback if text annotations aren't fully exposed yet, as I can't create other kinds of annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once this PR is merged (it should be in 30 minutes):
#15267

it should be a way easier to test this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcsteh, I merged the other PR and I rebased this one, so we should be good now.
Sorry for the inconvenience: I should have written the second patch in first.

@calixteman calixteman force-pushed the annotation_a11y branch 3 times, most recently from a3b02f5 to 0e13a95 Compare August 2, 2022 18:59
calixteman added a commit to calixteman/pdf.js that referenced this pull request Aug 3, 2022
…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.
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Leaving a few basic comments about the overall shape/structure of the new AccessibilityManager-class, since I don't expect the a11y-review to really change that.

* annotations in the annotation layer. The goal is to help to know
* where the annotations are in the text flow.
*/
class AccessibilityManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this class perhaps be named TextAccessibilityManager instead, and the filename changed to text_accessibility.js, to more clearly indicate what it does?

In any case, given how this is now initialized and used, I think this file should be moved into the web/-folder instead.

Comment on lines 23 to 26
* - to reorder element in the DOM with respect to the visual order;
* - make a link in using aria-owns between spans in the text layer and
* annotations in the annotation layer. The goal is to help to know
* where the annotations are in the text flow.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nits:

Suggested change
* - to reorder element in the DOM with respect to the visual order;
* - make a link in using aria-owns between spans in the text layer and
* annotations in the annotation layer. The goal is to help to know
* where the annotations are in the text flow.
* - to reorder elements in the DOM with respect to the visual order;
* - to create a link, using aria-owns, between spans in the textLayer and
* annotations in the annotationLayer. The goal is to help to know
* where the annotations are in the text flow.

Comment on lines 165 to 175
if (
typeof PDFJSDev === "undefined" ||
PDFJSDev.test("!PRODUCTION || TESTING")
) {
if (DEBUG) {
node.style.backgroundColor = "red";
setTimeout(() => {
node.style.backgroundColor = "";
}, 2000);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having to manually edit the code, in order to enable debugging, seems less than ideal.

The elements in question are always placed in the textLayer, right?
Because if so, it'd be much better to piggyback on the existing textLayer=visible debugging hash-parameter with appropriate rules added in the web/debugger.css file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea !

Comment on lines 27 to 29
if (typeof globalThis.crypto !== "undefined") {
return `${globalThis.crypto.randomUUID()}-`;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the performance of this code like?


Given that we use a hard-coded prefix string with the Editing-functionality, I suppose that we could try a similar approach here as well if that'd make things simpler and more efficient. (As long as the resulting id is unlikely to clash with named destinations.)

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 ran this on my powerful computer:

s = performance.now(); for (let i = 0; i < 1e7; i++) { crypto.randomUUID(); }; d = performance.now() - s 

and I got 9475ms so a unique call should be around 1ms, so in considering that it's called one time I think it's acceptable.
That said, it's on my powerful computer...

I use the ids in some tests (here and in m-c (i.e. https://searchfox.org/mozilla-central/search?q=%23pdfjs_internal_editor_&path=&case=false&regexp=false)).
I think we could use isInAutomation and/or preprocessing stuff we have here to generate a specific id for test or add an attribute... I'm not sure that's a good idea...

Comment on lines 187 to 188
this._accessibilityManager = new AccessibilityManager(div);

Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably don't want to initialize this unconditionally, since the textLayer may not actually be used (depending on options). It ought to suffice, as far as I can tell, to initialize it a the top this block:

if (this.textLayerMode !== TextLayerMode.DISABLE && this.textLayerFactory) {

@@ -114,6 +119,7 @@ class TextLayerBuilder {
this.textLayerDiv.append(textLayerFrag);
this._finishRendering();
this.highlighter?.enable();
this.accessibilityManager?.onTextLayerRendered();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally speaking, let's make the AccessibilityManager a bit more similar to the TextHighlighter class.
Hence, let's call this method enable instead. (You might also want to introduce a disable method, to handle any clean-up when the textLayer goes away.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thing, that could perhaps simplify things and reduce the number of DOM lookups in the AccessibilityManager-code, would be to again follow the TextHighlighter-class and pass in the textLayer-divs similar to:

this.highlighter?.setTextMapping(this.textDivs, this.textContentItemsStr);

#pageDiv;

/**
*
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Aug 3, 2022

Choose a reason for hiding this comment

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

Nit: This line can be removed.

calixteman added a commit to calixteman/pdf.js that referenced this pull request Aug 3, 2022
…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 added a commit to calixteman/pdf.js that referenced this pull request Aug 3, 2022
…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 added a commit to calixteman/pdf.js that referenced this pull request Aug 3, 2022
…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 added a commit to calixteman/pdf.js that referenced this pull request Aug 3, 2022
…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 added a commit to calixteman/pdf.js that referenced this pull request Aug 4, 2022
…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.
@jcsteh
Copy link

jcsteh commented Aug 10, 2022

Thanks. I tested this with both a tagged and an untagged PDF and I can now see the text annotations. Nice!

Ideally, I think the annotations would have role="insertion" or similar so you can see they're annotations (as opposed to part of the normal text). It's tricky to know what role is best here; some might argue these are more like comments than suggested insertions, in which case role="comment" is probably better. Regardless, I don't think this should block this; it's a nice-to-have and could be done in a follow-up.

rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022
…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
Copy link
Contributor Author

@Snuffleupagus, could you have a final look on this PR ?

Comment on lines 26 to 27
const IdRandomPrefix = (() => {
if (typeof globalThis.crypto !== "undefined") {
return `${globalThis.crypto.randomUUID()}-`;
}

const digits = new Array(36)
.fill(null)
.map(() => Math.floor(Math.random() * 16).toString(16));
digits[8] = digits[13] = digits[18] = digits[23] = "-";

return digits.join("");
})();

Copy link
Collaborator

Choose a reason for hiding this comment

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

To simplify things, how about we just use a fixed prefix for now?
Similar to the one used with the editor, since that ought the limit risk of accidental matches with any named destinations:

AnnotationIdPrefix = "pdfjs_internal_id_";

If this turns out to be problematical, we can always change things later on.

@@ -53,7 +53,7 @@ class IdManager {
* @returns {string}
*/
getId() {
return `${AnnotationEditorPrefix}${this.#id++}`;
return `${AnnotationEditorPrefix}${IdRandomPrefix}${this.#id++}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not immediately obvious to me why do we need "double" prefixes here?

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 was to be sure to have an unique id and make it findable (with a query selector) in tests.

@@ -1650,6 +1653,7 @@ class BaseViewer {
enhanceTextSelection = false,
eventBus,
highlighter,
accessibilityManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency the method below:

Suggested change
accessibilityManager,
accessibilityManager = null,

@@ -1752,6 +1761,7 @@ class BaseViewer {
uiManager = this.#annotationEditorUIManager,
pageDiv,
pdfPage,
accessibilityManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency the method above:

Suggested change
accessibilityManager,
accessibilityManager = null,

@@ -118,13 +124,15 @@ class DefaultAnnotationEditorLayerFactory {
uiManager = null,
pageDiv,
pdfPage,
accessibilityManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, also given the JSDoc:

Suggested change
accessibilityManager,
accessibilityManager = null,

@@ -176,6 +185,7 @@ class DefaultTextLayerFactory {
enhanceTextSelection = false,
eventBus,
highlighter,
accessibilityManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, also given the JSDoc:

Suggested change
accessibilityManager,
accessibilityManager = null,

Comment on lines 43 to 45
get #hasTextLayer() {
return !!this.#pageDiv.querySelector(":scope > .textLayer .endOfContent");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we still need this method and its DOM access, since you already know that the textLayer is available once the enable-method has run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some elements could have been added before the text layer finished to render, and when enable is called those elements are really inserted.
So we need to know that the text layer is still rendering in order to push those elements in a queue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we need to know that the text layer is still rendering in order to push those elements in a queue.

But the enable-call occurs at the point when the textLayer has finished rendering, so you already know when that happens. Perhaps what you really want here is an #enabled-property, similar to the TextHighlighter-class, that you check instead?

web/debugger.css Outdated
@@ -101,3 +101,7 @@
background-color: rgba(255, 255, 255, 0.6);
color: rgba(0, 0, 0, 1);
}

#viewer.textLayer-visible .textLayer span[aria-owns] {
background-color: red;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you perhaps want set an opacity here as well, given the background-color definition for "normal" textLayer spans above?

pdf.js/web/debugger.css

Lines 88 to 89 in e1e97c6

#viewer.textLayer-visible .textLayer span {
background-color: rgba(255, 255, 0, 0.1);

Also, can we please move this block to just after that definition above since they're connected?

// When zooming the text layer is removed from the DOM and sometimes
// it's rebuilt hence the nodes are no longer valid.

const textLayer = this.#pageDiv.getElementsByClassName("textLayer").item(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the TextHighlighter-class, why can't you add a setTextMapping-method in this class instead and call that one using something like the following

this.accessibilityManager?.setTextMapping(this.textDivs);

placed just after this line?

That way, the DOM elements should be directly available without the need for any additional DOM lookups here.

@@ -697,6 +698,7 @@ class PDFPageView {

let textLayer = null;
if (this.textLayerMode !== TextLayerMode.DISABLE && this.textLayerFactory) {
this._accessibilityManager = new TextAccessibilityManager(div);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given how the textLayer is being re-created on zooming/rotation, but the annotationLayer and annotationEditorLayer are kept, will this work correctly as-is?
With this code the annotationLayer/annotationEditorLayer will potentially contain a reference to an "old" TextAccessibilityManager, which seems like it probably doesn't work.

Can we just re-use the same TextAccessibilityManager-instance, but still lazily initialized, with the following code?

Suggested change
this._accessibilityManager = new TextAccessibilityManager(div);
this._accessibilityManager ||= new TextAccessibilityManager(div);

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 found this bug.

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

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/8a13b720f1d5e46/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@@ -210,7 +210,7 @@

.annotationLayer .popup {
position: absolute;
z-index: 200;
z-index: 2000;
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Aug 12, 2022

Choose a reason for hiding this comment

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

Another thing that just occurred to me, although I've not tested it:
Do you actually need this property here, now that an explicit z-index is assigned in the JS code?

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 seems useless: I removed it.
I changed:
https://github.com/mozilla/pdf.js/pull/15237/files#diff-d7b040092cf6be09a276b785ba03c46e79bc8342cf4ae4fb1bc139dea8513425R1906

because the += was just concatenating "1000" to the zindex string when the -= below was subtracting 1000.

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 25.56 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 8

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/8a13b720f1d5e46/output.txt

Total script time: 26.28 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 18

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

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

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/1b42d8931691167/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/91374a9669fcaf8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 26.32 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 26
  different first/second rendering: 2

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/91374a9669fcaf8/output.txt

Total script time: 28.88 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 10

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

@calixteman
Copy link
Contributor Author

The "regressions" are a matter of z-index when several popups are displayed.
As said in an other PR, I think we should have a strategy to position a popup depending on its position on the page and maybe on the positions of other open popups.
Anyway, I think those "regressions" are ok, @Snuffleupagus is it ok for you either ?

@Snuffleupagus
Copy link
Collaborator

The "regressions" are a matter of z-index when several popups are displayed.

Unless my memory is completely off, isn't part of the issue (or possibly all of it) caused by the fact that serializing the HTML content isn't able to take the z-index into account?

Anyway, I think those "regressions" are ok,

Have you tested the affected documents in the viewer, to make sure that these "regressions" are indeed ref-test only and that nothing is actually functionally broken in the viewer?

@calixteman
Copy link
Contributor Author

Yep I tested the affected documents and it was ok.
I had an other look just to be sure that nothing was wrong and I found something in issue14438.pdf:
image
the section wrapping the popup has some "large" dimensions, hence the other popups cannot catch the mouse events:
image
And I noticed they're few empty other sections:
image
which should likely removed or at least made invisible because they contribute to increase the viewport width and consequently a vertical scrollbar can appear.

I think the change around z-index just shows a pre-existing issue: an other ordering in the pdf should have shown the same issue.

}

this.#enabled = true;
this.#textChildren.sort(TextAccessibilityManager.#compareElementPositions);
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Aug 12, 2022

Choose a reason for hiding this comment

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

Wait, this is in-place sort right?
Given how objects are passed by reference in JS, this object is the same as e.g. the textDivs property used in the TextHighlighter-class. Hence, can this re-ordering possibly affect highlighting of search-results?

In any case, to prevent future bugs, you might want to create a copy rather than modifying in place here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@calixteman
Copy link
Contributor Author

calixteman commented Aug 12, 2022

I think the change around z-index just shows a pre-existing issue: an other ordering in the pdf should have shown the same issue.

A way to fix those issues, would be to set the dimension to auto for those specific annotation (popupAnnotation). Wdyt ?

…ions (bug 1780375)

This patch doesn't structurally change the text layer: it just adds some aria-owns
attributes to some spans.
The aria-owns attribute expect to have an element id, hence it's why it adds back an
id on the element rendering an annotation, but this id is built in using crypto.randomUUID
to avoid any potential issues with the hash in the url.
The elements in the annotation layer are moved into the DOM in order to have them in the
same "order" as they visually are.
The overall goal is to help screen readers to present to the user the annotations as
they visually are and as they come in the text flow.
It is clearly not perfect, but it should improve readability for some people with visual
disabilities.
@Snuffleupagus
Copy link
Collaborator

A way to fix those issues, would be to set the dimension to auto for those specific annotation (popupAnnotation). Wdyt ?

I'd be a bit surprised if there's not other fallout from such a change, so probably not somewhat that we want to attempt here.


Let's land this as-is, since hopefully most issues should've been found now :-)

@calixteman calixteman merged commit 6b4c246 into mozilla:master Aug 12, 2022
@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

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/8d5ed16ff3494f6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8d5ed16ff3494f6/output.txt

Total script time: 22.85 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 22.60 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants