Skip to content

JS - Don't use document.getElementsByName when updating some annotations #14008

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

JS - Don't use document.getElementsByName when updating some annotations #14008

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 46 additions & 38 deletions src/display/annotation_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ class AnnotationElement {
this.enableScripting = parameters.enableScripting;
this.hasJSActions = parameters.hasJSActions;
this._mouseState = parameters.mouseState;
this._annotationsWithSameName = parameters.annotationsWithSameName;
this._htmlElement = null;

this._annotationsWithSameName.push(this);

if (isRenderable) {
this.container = this._createContainer(ignoreBorder);
Expand Down Expand Up @@ -681,18 +685,6 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
super(parameters, { isRenderable });
}

setPropertyOnSiblings(base, key, value, keyInStorage) {
const storage = this.annotationStorage;
for (const element of document.getElementsByName(base.name)) {
if (element !== base) {
element[key] = value;
const data = Object.create(null);
data[keyInStorage] = value;
storage.setValue(element.getAttribute("id"), data);
}
}
}

render() {
const storage = this.annotationStorage;
const id = this.data.id;
Expand Down Expand Up @@ -724,19 +716,23 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
element.type = "text";
element.setAttribute("value", textContent);
}
this._htmlElement = element;
element.tabIndex = DEFAULT_TAB_INDEX;

elementData.userValue = textContent;
element.setAttribute("id", id);

element.addEventListener("input", event => {
storage.setValue(id, { value: event.target.value });
this.setPropertyOnSiblings(
element,
"value",
event.target.value,
"value"
);
const value = event.target.value;
storage.setValue(id, { value });
for (const other of this._annotationsWithSameName) {
const htmlElement = other._htmlElement;
if (htmlElement && htmlElement !== element) {
const data = Object.create(null);
htmlElement.value = data.value = value;
storage.setValue(htmlElement.getAttribute("id"), data);
}
}
});

let blurListener = event => {
Expand Down Expand Up @@ -973,7 +969,7 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {

this.container.className = "buttonWidgetAnnotation checkBox";

const element = document.createElement("input");
const element = (this._htmlElement = document.createElement("input"));
element.disabled = data.readOnly;
element.type = "checkbox";
element.name = this.data.fieldName;
Expand All @@ -983,13 +979,13 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
element.setAttribute("id", id);
element.tabIndex = DEFAULT_TAB_INDEX;

element.addEventListener("change", function (event) {
const name = event.target.name;
for (const checkbox of document.getElementsByName(name)) {
if (checkbox !== event.target) {
checkbox.checked = false;
element.addEventListener("change", event => {
for (const other of this._annotationsWithSameName) {
const htmlElement = other._htmlElement;
if (htmlElement && htmlElement !== element) {
htmlElement.checked = false;
storage.setValue(
checkbox.parentNode.getAttribute("data-annotation-id"),
htmlElement.parentNode.getAttribute("data-annotation-id"),
{ value: false }
);
}
Expand Down Expand Up @@ -1048,7 +1044,7 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement {
storage.setValue(id, { value });
}

const element = document.createElement("input");
const element = (this._htmlElement = document.createElement("input"));
element.disabled = data.readOnly;
element.type = "radio";
element.name = data.fieldName;
Expand All @@ -1058,26 +1054,30 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement {
element.setAttribute("id", id);
element.tabIndex = DEFAULT_TAB_INDEX;

element.addEventListener("change", function (event) {
const { target } = event;
for (const radio of document.getElementsByName(target.name)) {
if (radio !== target) {
storage.setValue(radio.getAttribute("id"), { value: false });
element.addEventListener("change", event => {
for (const other of this._annotationsWithSameName) {
const htmlElement = other._htmlElement;
if (htmlElement && htmlElement !== element) {
storage.setValue(htmlElement.getAttribute("id"), { value: false });
}
}
storage.setValue(id, { value: target.checked });
storage.setValue(id, { value: element.checked });
});

if (this.enableScripting && this.hasJSActions) {
const pdfButtonValue = data.buttonValue;
element.addEventListener("updatefromsandbox", jsEvent => {
const annotationsWithSameName = this._annotationsWithSameName;
const actions = {
value(event) {
const checked = pdfButtonValue === event.detail.value;
for (const radio of document.getElementsByName(event.target.name)) {
const radioId = radio.getAttribute("id");
radio.checked = radioId === id && checked;
storage.setValue(radioId, { value: radio.checked });
for (const other of annotationsWithSameName) {
const htmlElement = other._htmlElement;
if (htmlElement) {
const radioId = htmlElement.getAttribute("id");
htmlElement.checked = radioId === id && checked;
storage.setValue(radioId, { value: htmlElement.checked });
}
}
},
};
Expand Down Expand Up @@ -2049,7 +2049,8 @@ class AnnotationLayer {
*/
static render(parameters) {
const sortedAnnotations = [],
popupAnnotations = [];
popupAnnotations = [],
sameName = new Map();
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could annotations with the same name ever be found on different pages, because if so I'm not sure that this will work (since it's limited to just the current page)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I'd say that it was already an issue (I mean before this patch).

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My (possibly very stupid) idea when I initially saw the issue was that you could maybe track the created input elements in e.g. a WeakSet defined "globally" in this file, and then just check the elements that we lookup with document.getElementsByName against that.

Based on the following quote from #14003 (comment), Rob probably has better ideas though:

I suggest to replace the document.getElementsByTagName calls with a function that checks that the elements are created by the annotation_layer.js code before returning them. There are several ways to do so, e.g. by adding a custom attribute to each input element.

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 won't avoid the problem with fields with same name across different pages: document.getElementsByName will get only the visible elements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I'd say that it was already an issue (I mean before this patch).

I see what you're saying, but it seems to me (I may be wrong of course) that it'd be slightly worse since before you got the Annotations on all active pages whereas with this patch it's only the current page.

Anyway, it seems that we might need to expand the AnnotationStorage to also have a name -> id mapping such that the initial value gets set correctly when rendering new pages? If so, that probably shouldn't be too hard to implement (famous last words).

This comment was marked as off-topic.

Sign in to view

Copy link
Member

Choose a reason for hiding this comment

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

When I filed the issue I was thinking of something simple like using a WeakSet, tagging the element with a custom attribute, or even just prepending a prefix to the name to have something unique that can't overlap with existing ones. This draft patch looks more complex than that - why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@calixteman Sorry if I'm massively overstepping here, but I've got a (possibly stupid) idea.

What if we utilized the data returned by the getFieldObjects-method in the API, since then we'd be able to both access the fields on all active pages and also update the AnnotationStorage for not-yet-rendered pages?

master...Snuffleupagus:AnnotationLayer-getElementsByName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Snuffleupagus, no, you aren't and I think that's a very good idea.
This way we don't have to change AnnotationStorage implementation because we'll have all the ids for fields with the same name. Would you mind to write the patch since you've almost thought about everything here ? If yes, please close this one.

@Rob--W, my idea was to remove the call to getElementsByName and just update the fields with the same name. But as pointed out by Jonas, it was a bad idea and in the meantime it was not so bad because it helped us to find a bug.

// Ensure that Popup annotations are handled last, since they're dependant
// upon the parent annotation having already been rendered (please refer to
// the `PopupAnnotationElement.render` method); fixes issue 11362.
Expand All @@ -2068,6 +2069,11 @@ class AnnotationLayer {
}

for (const data of sortedAnnotations) {
let annotationsWithSameName = sameName.get(data.fieldName);
if (!annotationsWithSameName) {
annotationsWithSameName = [];
sameName.set(data.fieldName, annotationsWithSameName);
}
const element = AnnotationElementFactory.create({
data,
layer: parameters.div,
Expand All @@ -2083,7 +2089,9 @@ class AnnotationLayer {
enableScripting: parameters.enableScripting,
hasJSActions: parameters.hasJSActions,
mouseState: parameters.mouseState || { isDown: false },
annotationsWithSameName,
});

if (element.isRenderable) {
const rendered = element.render();
if (data.hidden) {
Expand Down