Skip to content

Commit

Permalink
Re-factor document.getElementsByName lookups in the AnnotationLayer…
Browse files Browse the repository at this point in the history
… (issue 14003)

This replaces direct `document.getElementsByName` lookups with a helper method which:
 - Lets the AnnotationLayer use the data returned by the `PDFDocumentProxy.getFieldObjects` API-method, such that we can directly lookup only the necessary DOM elements.
 - Fallback to using `document.getElementsByName` as before, such that e.g. the standalone viewer components still work.

Finally, to fix the problems reported in issue 14003, regardless of the code-path we now also enforce that the DOM elements found were actually created by the AnnotationLayer code.
With these changes we'll thus be able to update form elements on all visible pages just as before, but we'll additionally update the AnnotationStorage for not-yet-rendered elements thus fixing a pre-existing bug.
  • Loading branch information
Snuffleupagus committed Sep 14, 2021
1 parent 064c21d commit f54431a
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 65 deletions.
94 changes: 71 additions & 23 deletions src/display/annotation_layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { AnnotationStorage } from "./annotation_storage.js";
import { ColorConverters } from "../shared/scripting_utils.js";

const DEFAULT_TAB_INDEX = 1000;
const GetElementsByNameSet = new WeakSet();

/**
* @typedef {Object} AnnotationElementParameters
Expand All @@ -50,6 +51,7 @@ const DEFAULT_TAB_INDEX = 1000;
* @property {Object} svgFactory
* @property {boolean} [enableScripting]
* @property {boolean} [hasJSActions]
* @property {Object} [fieldObjects]
* @property {Object} [mouseState]
*/

Expand Down Expand Up @@ -159,6 +161,7 @@ class AnnotationElement {
this.annotationStorage = parameters.annotationStorage;
this.enableScripting = parameters.enableScripting;
this.hasJSActions = parameters.hasJSActions;
this._fieldObjects = parameters.fieldObjects;
this._mouseState = parameters.mouseState;

if (isRenderable) {
Expand Down Expand Up @@ -363,6 +366,43 @@ class AnnotationElement {
unreachable("Abstract method `AnnotationElement.render` called");
}

/**
* @private
* @returns {Array}
*/
_getElementsByName(name) {
const fields = [];

if (this._fieldObjects) {
const fieldObj = this._fieldObjects[name];
if (fieldObj) {
for (const { id, page } of fieldObj) {
if (page === -1) {
continue;
}
const domElement = document.getElementById(id) || null;
if (domElement && !GetElementsByNameSet.has(domElement)) {
warn(`_getElementsByName - element not allowed: ${id}`);
continue;
}
fields.push({ id, domElement });
}
}
return fields;
}
// Fallback to a regular DOM lookup.
for (const domElement of document.getElementsByName(name)) {
if (!GetElementsByNameSet.has(domElement)) {
continue;
}
fields.push({
id: domElement.getAttribute("id"),
domElement,
});
}
return fields;
}

static get platform() {
const platform = typeof navigator !== "undefined" ? navigator.platform : "";

Expand Down Expand Up @@ -683,13 +723,14 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {

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);
for (const element of this._getElementsByName(base.name)) {
if (element.domElement === base) {
continue;
}
if (element.domElement) {
element.domElement[key] = value;
}
storage.setValue(element.id, { [keyInStorage]: value });
}
}

Expand Down Expand Up @@ -724,6 +765,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
element.type = "text";
element.setAttribute("value", textContent);
}
GetElementsByNameSet.add(element);
element.tabIndex = DEFAULT_TAB_INDEX;

elementData.userValue = textContent;
Expand Down Expand Up @@ -974,6 +1016,7 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
this.container.className = "buttonWidgetAnnotation checkBox";

const element = document.createElement("input");
GetElementsByNameSet.add(element);
element.disabled = data.readOnly;
element.type = "checkbox";
element.name = this.data.fieldName;
Expand All @@ -983,16 +1026,15 @@ 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;
storage.setValue(
checkbox.parentNode.getAttribute("data-annotation-id"),
{ value: false }
);
element.addEventListener("change", event => {
for (const checkbox of this._getElementsByName(event.target.name)) {
if (checkbox.domElement === event.target) {
continue;
}
if (checkbox.domElement) {
checkbox.domElement.checked = false;
}
storage.setValue(checkbox.id, { value: false });
}
storage.setValue(id, { value: event.target.checked });
});
Expand Down Expand Up @@ -1049,6 +1091,7 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement {
}

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

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

if (this.enableScripting && this.hasJSActions) {
const pdfButtonValue = data.buttonValue;
const self = this;
element.addEventListener("updatefromsandbox", jsEvent => {
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 radio of self._getElementsByName(event.target.name)) {
const curChecked = radio.id === id && checked;
if (radio.domElement) {
radio.domElement.checked = curChecked;
}
storage.setValue(radio.id, { value: curChecked });
}
},
};
Expand Down Expand Up @@ -1150,6 +1196,7 @@ class ChoiceWidgetAnnotationElement extends WidgetAnnotationElement {
const fontSizeStyle = `calc(${fontSize}px * var(--zoom-factor))`;

const selectElement = document.createElement("select");
GetElementsByNameSet.add(selectElement);
selectElement.disabled = this.data.readOnly;
selectElement.name = this.data.fieldName;
selectElement.setAttribute("id", id);
Expand Down Expand Up @@ -2082,6 +2129,7 @@ class AnnotationLayer {
parameters.annotationStorage || new AnnotationStorage(),
enableScripting: parameters.enableScripting,
hasJSActions: parameters.hasJSActions,
fieldObjects: parameters.fieldObjects,
mouseState: parameters.mouseState || { isDown: false },
});
if (element.isRenderable) {
Expand Down
5 changes: 4 additions & 1 deletion src/display/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2480,6 +2480,7 @@ class WorkerTransport {
Promise.all(waitOn).then(() => {
this.commonObjs.clear();
this.fontLoader.clear();
this._getFieldObjectsPromise = null;
this._hasJSActionsPromise = null;

if (this._networkStream) {
Expand Down Expand Up @@ -2921,7 +2922,8 @@ class WorkerTransport {
}

getFieldObjects() {
return this.messageHandler.sendWithPromise("GetFieldObjects", null);
return (this._getFieldObjectsPromise ||=
this.messageHandler.sendWithPromise("GetFieldObjects", null));
}

hasJSActions() {
Expand Down Expand Up @@ -3050,6 +3052,7 @@ class WorkerTransport {
if (!keepLoadedFonts) {
this.fontLoader.clear();
}
this._getFieldObjectsPromise = null;
this._hasJSActionsPromise = null;
}

Expand Down
85 changes: 47 additions & 38 deletions web/annotation_layer_builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { SimpleLinkService } from "./pdf_link_service.js";
* @property {IL10n} l10n - Localization service.
* @property {boolean} [enableScripting]
* @property {Promise<boolean>} [hasJSActionsPromise]
* @property {Promise<Array<Object> | null} [fieldObjectsPromise]
* @property {Object} [mouseState]
*/

Expand All @@ -51,6 +52,7 @@ class AnnotationLayerBuilder {
l10n = NullL10n,
enableScripting = false,
hasJSActionsPromise = null,
fieldObjectsPromise = null,
mouseState = null,
}) {
this.pageDiv = pageDiv;
Expand All @@ -63,6 +65,7 @@ class AnnotationLayerBuilder {
this.annotationStorage = annotationStorage;
this.enableScripting = enableScripting;
this._hasJSActionsPromise = hasJSActionsPromise;
this._fieldObjectsPromise = fieldObjectsPromise;
this._mouseState = mouseState;

this.div = null;
Expand All @@ -75,46 +78,49 @@ class AnnotationLayerBuilder {
* @returns {Promise<void>} A promise that is resolved when rendering of the
* annotations is complete.
*/
render(viewport, intent = "display") {
return Promise.all([
this.pdfPage.getAnnotations({ intent }),
this._hasJSActionsPromise,
]).then(([annotations, hasJSActions = false]) => {
if (this._cancelled || annotations.length === 0) {
return;
}
async render(viewport, intent = "display") {
const [annotations, hasJSActions = false, fieldObjects = null] =
await Promise.all([
this.pdfPage.getAnnotations({ intent }),
this._hasJSActionsPromise,
this._fieldObjectsPromise,
]);

const parameters = {
viewport: viewport.clone({ dontFlip: true }),
div: this.div,
annotations,
page: this.pdfPage,
imageResourcesPath: this.imageResourcesPath,
renderForms: this.renderForms,
linkService: this.linkService,
downloadManager: this.downloadManager,
annotationStorage: this.annotationStorage,
enableScripting: this.enableScripting,
hasJSActions,
mouseState: this._mouseState,
};
if (this._cancelled || annotations.length === 0) {
return;
}

if (this.div) {
// If an annotationLayer already exists, refresh its children's
// transformation matrices.
AnnotationLayer.update(parameters);
} else {
// Create an annotation layer div and render the annotations
// if there is at least one annotation.
this.div = document.createElement("div");
this.div.className = "annotationLayer";
this.pageDiv.appendChild(this.div);
parameters.div = this.div;
const parameters = {
viewport: viewport.clone({ dontFlip: true }),
div: this.div,
annotations,
page: this.pdfPage,
imageResourcesPath: this.imageResourcesPath,
renderForms: this.renderForms,
linkService: this.linkService,
downloadManager: this.downloadManager,
annotationStorage: this.annotationStorage,
enableScripting: this.enableScripting,
hasJSActions,
fieldObjects,
mouseState: this._mouseState,
};

AnnotationLayer.render(parameters);
this.l10n.translate(this.div);
}
});
if (this.div) {
// If an annotationLayer already exists, refresh its children's
// transformation matrices.
AnnotationLayer.update(parameters);
} else {
// Create an annotation layer div and render the annotations
// if there is at least one annotation.
this.div = document.createElement("div");
this.div.className = "annotationLayer";
this.pageDiv.appendChild(this.div);
parameters.div = this.div;

AnnotationLayer.render(parameters);
this.l10n.translate(this.div);
}
}

cancel() {
Expand Down Expand Up @@ -144,6 +150,7 @@ class DefaultAnnotationLayerFactory {
* @param {boolean} [enableScripting]
* @param {Promise<boolean>} [hasJSActionsPromise]
* @param {Object} [mouseState]
* @param {Promise<Array<Object> | null} [fieldObjectsPromise]
* @returns {AnnotationLayerBuilder}
*/
createAnnotationLayerBuilder(
Expand All @@ -155,7 +162,8 @@ class DefaultAnnotationLayerFactory {
l10n = NullL10n,
enableScripting = false,
hasJSActionsPromise = null,
mouseState = null
mouseState = null,
fieldObjectsPromise = null
) {
return new AnnotationLayerBuilder({
pageDiv,
Expand All @@ -167,6 +175,7 @@ class DefaultAnnotationLayerFactory {
annotationStorage,
enableScripting,
hasJSActionsPromise,
fieldObjectsPromise,
mouseState,
});
}
Expand Down
6 changes: 5 additions & 1 deletion web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,7 @@ class BaseViewer {
* @param {boolean} [enableScripting]
* @param {Promise<boolean>} [hasJSActionsPromise]
* @param {Object} [mouseState]
* @param {Promise<Array<Object> | null} [fieldObjectsPromise]
* @returns {AnnotationLayerBuilder}
*/
createAnnotationLayerBuilder(
Expand All @@ -1316,7 +1317,8 @@ class BaseViewer {
l10n = NullL10n,
enableScripting = null,
hasJSActionsPromise = null,
mouseState = null
mouseState = null,
fieldObjectsPromise = null
) {
return new AnnotationLayerBuilder({
pageDiv,
Expand All @@ -1331,6 +1333,8 @@ class BaseViewer {
enableScripting: enableScripting ?? this.enableScripting,
hasJSActionsPromise:
hasJSActionsPromise || this.pdfDocument?.hasJSActions(),
fieldObjectsPromise:
fieldObjectsPromise || this.pdfDocument?.getFieldObjects(),
mouseState: mouseState || this._scriptingManager?.mouseState,
});
}
Expand Down
4 changes: 3 additions & 1 deletion web/interfaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ class IPDFAnnotationLayerFactory {
* @param {boolean} [enableScripting]
* @param {Promise<boolean>} [hasJSActionsPromise]
* @param {Object} [mouseState]
* @param {Promise<Array<Object> | null} [fieldObjectsPromise]
* @returns {AnnotationLayerBuilder}
*/
createAnnotationLayerBuilder(
Expand All @@ -177,7 +178,8 @@ class IPDFAnnotationLayerFactory {
l10n = undefined,
enableScripting = false,
hasJSActionsPromise = null,
mouseState = null
mouseState = null,
fieldObjectsPromise = null
) {}
}

Expand Down
Loading

0 comments on commit f54431a

Please sign in to comment.