Merge pull request #14023 from Snuffleupagus/AnnotationLayer-getElementsByName

Re-factor `document.getElementsByName` lookups in the AnnotationLayer (issue 14003)

src/display/annotation_layer.js

@@ -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
@@ -50,6 +51,7 @@ const DEFAULT_TAB_INDEX = 1000;
  * @property {Object} svgFactory
  * @property {boolean} [enableScripting]
  * @property {boolean} [hasJSActions]
+ * @property {Object} [fieldObjects]
  * @property {Object} [mouseState]
  */
 
@@ -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) {
@@ -363,6 +366,51 @@ class AnnotationElement {
     unreachable("Abstract method `AnnotationElement.render` called");
   }
 
+  /**
+   * @private
+   * @returns {Array}
+   */
+  _getElementsByName(name, skipId = null) {
+    const fields = [];
+
+    if (this._fieldObjects) {
+      const fieldObj = this._fieldObjects[name];
+      if (fieldObj) {
+        for (const { page, id, exportValues } of fieldObj) {
+          if (page === -1) {
+            continue;
+          }
+          if (id === skipId) {
+            continue;
+          }
+          const exportValue =
+            typeof exportValues === "string" ? exportValues : null;
+
+          const domElement = document.getElementById(id);
+          if (domElement && !GetElementsByNameSet.has(domElement)) {
+            warn(`_getElementsByName - element not allowed: ${id}`);
+            continue;
+          }
+          fields.push({ id, exportValue, domElement });
+        }
+      }
+      return fields;
+    }
+    // Fallback to a regular DOM lookup, to ensure that the standalone
+    // viewer components won't break.
+    for (const domElement of document.getElementsByName(name)) {
+      const { id, exportValue } = domElement;
+      if (id === skipId) {
+        continue;
+      }
+      if (!GetElementsByNameSet.has(domElement)) {
+        continue;
+      }
+      fields.push({ id, exportValue, domElement });
+    }
+    return fields;
+  }
+
   static get platform() {
     const platform = typeof navigator !== "undefined" ? navigator.platform : "";
 
@@ -687,13 +735,14 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
 
   setPropertyOnSiblings(base, key, value, keyInStorage) {
     const storage = this.annotationStorage;
-    for (const element of document.getElementsByName(base.name)) {
+    for (const element of this._getElementsByName(
-      if (element !== base) {
+      base.name,
-        element[key] = value;
+      /* skipId = */ base.id
-        const data = Object.create(null);
+    )) {
-        data[keyInStorage] = value;
+      if (element.domElement) {
-        storage.setValue(element.getAttribute("id"), data);
+        element.domElement[key] = value;
       }
+      storage.setValue(element.id, { [keyInStorage]: value });
     }
   }
 
@@ -728,6 +777,9 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
         element.type = "text";
         element.setAttribute("value", textContent);
       }
+      GetElementsByNameSet.add(element);
+      element.disabled = this.data.readOnly;
+      element.name = this.data.fieldName;
       element.tabIndex = DEFAULT_TAB_INDEX;
 
       elementData.userValue = textContent;
@@ -900,9 +952,6 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement {
         element.addEventListener("blur", blurListener);
       }
 
-      element.disabled = this.data.readOnly;
-      element.name = this.data.fieldName;
-
       if (this.data.maxLen !== null) {
         element.maxLength = this.data.maxLen;
       }
@@ -978,6 +1027,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 = data.fieldName;
@@ -988,19 +1038,14 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
     element.setAttribute("exportValue", data.exportValue);
     element.tabIndex = DEFAULT_TAB_INDEX;
 
-    element.addEventListener("change", function (event) {
+    element.addEventListener("change", event => {
-      const name = event.target.name;
+      const { name, checked } = event.target;
-      const checked = event.target.checked;
+      for (const checkbox of this._getElementsByName(name, /* skipId = */ id)) {
-      for (const checkbox of document.getElementsByName(name)) {
+        const curChecked = checked && checkbox.exportValue === data.exportValue;
-        if (checkbox !== event.target) {
+        if (checkbox.domElement) {
-          checkbox.checked =
+          checkbox.domElement.checked = curChecked;
-            checked &&
-            checkbox.getAttribute("exportValue") === data.exportValue;
-          storage.setValue(
-            checkbox.parentNode.getAttribute("data-annotation-id"),
-            { value: false }
-          );
         }
+        storage.setValue(checkbox.id, { value: curChecked });
       }
       storage.setValue(id, { value: checked });
     });
@@ -1057,6 +1102,7 @@ class RadioButtonWidgetAnnotationElement extends WidgetAnnotationElement {
     }
 
     const element = document.createElement("input");
+    GetElementsByNameSet.add(element);
     element.disabled = data.readOnly;
     element.type = "radio";
     element.name = data.fieldName;
@@ -1066,26 +1112,26 @@ 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;
+      const { name, checked } = event.target;
-      for (const radio of document.getElementsByName(target.name)) {
+      for (const radio of this._getElementsByName(name, /* skipId = */ id)) {
-        if (radio !== target) {
+        storage.setValue(radio.id, { value: false });
-          storage.setValue(radio.getAttribute("id"), { value: false });
-        }
       }
-      storage.setValue(id, { value: target.checked });
+      storage.setValue(id, { value: checked });
     });
 
     if (this.enableScripting && this.hasJSActions) {
       const pdfButtonValue = data.buttonValue;
       element.addEventListener("updatefromsandbox", jsEvent => {
         const actions = {
-          value(event) {
+          value: event => {
             const checked = pdfButtonValue === event.detail.value;
-            for (const radio of document.getElementsByName(event.target.name)) {
+            for (const radio of this._getElementsByName(event.target.name)) {
-              const radioId = radio.getAttribute("id");
+              const curChecked = checked && radio.id === id;
-              radio.checked = radioId === id && checked;
+              if (radio.domElement) {
-              storage.setValue(radioId, { value: radio.checked });
+                radio.domElement.checked = curChecked;
+              }
+              storage.setValue(radio.id, { value: curChecked });
             }
           },
         };
@@ -1158,6 +1204,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);
@@ -2090,6 +2137,7 @@ class AnnotationLayer {
           parameters.annotationStorage || new AnnotationStorage(),
         enableScripting: parameters.enableScripting,
         hasJSActions: parameters.hasJSActions,
+        fieldObjects: parameters.fieldObjects,
         mouseState: parameters.mouseState || { isDown: false },
       });
       if (element.isRenderable) {

src/display/api.js

@@ -1017,9 +1017,9 @@ class PDFDocumentProxy {
   }
 
   /**
-   * @returns {Promise<Array<Object> | null>} A promise that is resolved with an
+   * @returns {Promise<Object<string, Array<Object>> | null>} A promise that is
-   *   {Array<Object>} containing /AcroForm field data for the JS sandbox,
+   *   resolved with an {Object} containing /AcroForm field data for the JS
-   *   or `null` when no field data is present in the PDF file.
+   *   sandbox, or `null` when no field data is present in the PDF file.
    */
   getFieldObjects() {
     return this._transport.getFieldObjects();
@@ -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) {
@@ -2921,7 +2922,8 @@ class WorkerTransport {
   }
 
   getFieldObjects() {
-    return this.messageHandler.sendWithPromise("GetFieldObjects", null);
+    return (this._getFieldObjectsPromise ||=
+      this.messageHandler.sendWithPromise("GetFieldObjects", null));
   }
 
   hasJSActions() {
@@ -3050,6 +3052,7 @@ class WorkerTransport {
     if (!keepLoadedFonts) {
       this.fontLoader.clear();
     }
+    this._getFieldObjectsPromise = null;
     this._hasJSActionsPromise = null;
   }
 

test/integration/annotation_spec.js

@@ -126,3 +126,98 @@ describe("Text widget", () => {
     });
   });
 });
+
+describe("Annotation and storage", () => {
+  describe("issue14023.pdf", () => {
+    let pages;
+
+    beforeAll(async () => {
+      pages = await loadAndWait("issue14023.pdf", "[data-annotation-id='64R']");
+    });
+
+    afterAll(async () => {
+      await closePages(pages);
+    });
+
+    it("must let checkboxes with the same name behave like radio buttons", async () => {
+      const text1 = "hello world!";
+      const text2 = "!dlrow olleh";
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          // Text field.
+          await page.type("#\\36 4R", text1);
+          // Checkbox.
+          await page.click("[data-annotation-id='65R']");
+          // Radio.
+          await page.click("[data-annotation-id='67R']");
+
+          for (const [pageNumber, textId, checkId, radio1Id, radio2Id] of [
+            [2, "#\\31 8R", "#\\31 9R", "#\\32 1R", "#\\32 0R"],
+            [5, "#\\32 3R", "#\\32 4R", "#\\32 2R", "#\\32 5R"],
+          ]) {
+            await page.evaluate(n => {
+              window.document
+                .querySelectorAll(`[data-page-number="${n}"][class="page"]`)[0]
+                .scrollIntoView();
+            }, pageNumber);
+
+            // Need to wait to have a displayed text input.
+            await page.waitForSelector(textId, {
+              timeout: 0,
+            });
+
+            const text = await page.$eval(textId, el => el.value);
+            expect(text).withContext(`In ${browserName}`).toEqual(text1);
+
+            let checked = await page.$eval(checkId, el => el.checked);
+            expect(checked).toEqual(true);
+
+            checked = await page.$eval(radio1Id, el => el.checked);
+            expect(checked).toEqual(false);
+
+            checked = await page.$eval(radio2Id, el => el.checked);
+            expect(checked).toEqual(false);
+          }
+
+          // Change data on page 5 and check that other pages changed.
+          // Text field.
+          await page.type("#\\32 3R", text2);
+          // Checkbox.
+          await page.click("[data-annotation-id='24R']");
+          // Radio.
+          await page.click("[data-annotation-id='25R']");
+
+          for (const [pageNumber, textId, checkId, radio1Id, radio2Id] of [
+            [1, "#\\36 4R", "#\\36 5R", "#\\36 7R", "#\\36 8R"],
+            [2, "#\\31 8R", "#\\31 9R", "#\\32 1R", "#\\32 0R"],
+          ]) {
+            await page.evaluate(n => {
+              window.document
+                .querySelectorAll(`[data-page-number="${n}"][class="page"]`)[0]
+                .scrollIntoView();
+            }, pageNumber);
+
+            // Need to wait to have a displayed text input.
+            await page.waitForSelector(textId, {
+              timeout: 0,
+            });
+
+            const text = await page.$eval(textId, el => el.value);
+            expect(text)
+              .withContext(`In ${browserName}`)
+              .toEqual(text2 + text1);
+
+            let checked = await page.$eval(checkId, el => el.checked);
+            expect(checked).toEqual(false);
+
+            checked = await page.$eval(radio1Id, el => el.checked);
+            expect(checked).toEqual(false);
+
+            checked = await page.$eval(radio2Id, el => el.checked);
+            expect(checked).toEqual(false);
+          }
+        })
+      );
+    });
+  });
+});

test/pdfs/.gitignore

@@ -112,6 +112,7 @@
 !issue11651.pdf
 !issue11878.pdf
 !issue13916.pdf
+!issue14023.pdf
 !bad-PageLabels.pdf
 !decodeACSuccessive.pdf
 !filled-background.pdf

web/annotation_layer_builder.js

@@ -33,6 +33,8 @@ import { SimpleLinkService } from "./pdf_link_service.js";
  * @property {IL10n} l10n - Localization service.
  * @property {boolean} [enableScripting]
  * @property {Promise<boolean>} [hasJSActionsPromise]
+ * @property {Promise<Object<string, Array<Object>> | null>}
+ *   [fieldObjectsPromise]
  * @property {Object} [mouseState]
  */
 
@@ -51,6 +53,7 @@ class AnnotationLayerBuilder {
     l10n = NullL10n,
     enableScripting = false,
     hasJSActionsPromise = null,
+    fieldObjectsPromise = null,
     mouseState = null,
   }) {
     this.pageDiv = pageDiv;
@@ -63,6 +66,7 @@ class AnnotationLayerBuilder {
     this.annotationStorage = annotationStorage;
     this.enableScripting = enableScripting;
     this._hasJSActionsPromise = hasJSActionsPromise;
+    this._fieldObjectsPromise = fieldObjectsPromise;
     this._mouseState = mouseState;
 
     this.div = null;
@@ -75,46 +79,49 @@ class AnnotationLayerBuilder {
    * @returns {Promise<void>} A promise that is resolved when rendering of the
    *   annotations is complete.
    */
-  render(viewport, intent = "display") {
+  async render(viewport, intent = "display") {
-    return Promise.all([
+    const [annotations, hasJSActions = false, fieldObjects = null] =
-      this.pdfPage.getAnnotations({ intent }),
+      await Promise.all([
-      this._hasJSActionsPromise,
+        this.pdfPage.getAnnotations({ intent }),
-    ]).then(([annotations, hasJSActions = false]) => {
+        this._hasJSActionsPromise,
-      if (this._cancelled || annotations.length === 0) {
+        this._fieldObjectsPromise,
-        return;
+      ]);
-      }
 
-      const parameters = {
+    if (this._cancelled || annotations.length === 0) {
-        viewport: viewport.clone({ dontFlip: true }),
+      return;
-        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.div) {
+    const parameters = {
-        // If an annotationLayer already exists, refresh its children's
+      viewport: viewport.clone({ dontFlip: true }),
-        // transformation matrices.
+      div: this.div,
-        AnnotationLayer.update(parameters);
+      annotations,
-      } else {
+      page: this.pdfPage,
-        // Create an annotation layer div and render the annotations
+      imageResourcesPath: this.imageResourcesPath,
-        // if there is at least one annotation.
+      renderForms: this.renderForms,
-        this.div = document.createElement("div");
+      linkService: this.linkService,
-        this.div.className = "annotationLayer";
+      downloadManager: this.downloadManager,
-        this.pageDiv.appendChild(this.div);
+      annotationStorage: this.annotationStorage,
-        parameters.div = this.div;
+      enableScripting: this.enableScripting,
+      hasJSActions,
+      fieldObjects,
+      mouseState: this._mouseState,
+    };
 
-        AnnotationLayer.render(parameters);
+    if (this.div) {
-        this.l10n.translate(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() {
@@ -144,6 +151,8 @@ class DefaultAnnotationLayerFactory {
    * @param {boolean} [enableScripting]
    * @param {Promise<boolean>} [hasJSActionsPromise]
    * @param {Object} [mouseState]
+   * @param {Promise<Object<string, Array<Object>> | null>}
+   *   [fieldObjectsPromise]
    * @returns {AnnotationLayerBuilder}
    */
   createAnnotationLayerBuilder(
@@ -155,7 +164,8 @@ class DefaultAnnotationLayerFactory {
     l10n = NullL10n,
     enableScripting = false,
     hasJSActionsPromise = null,
-    mouseState = null
+    mouseState = null,
+    fieldObjectsPromise = null
   ) {
     return new AnnotationLayerBuilder({
       pageDiv,
@@ -167,6 +177,7 @@ class DefaultAnnotationLayerFactory {
       annotationStorage,
       enableScripting,
       hasJSActionsPromise,
+      fieldObjectsPromise,
       mouseState,
     });
   }

web/base_viewer.js

@@ -1318,6 +1318,8 @@ class BaseViewer {
    * @param {boolean} [enableScripting]
    * @param {Promise<boolean>} [hasJSActionsPromise]
    * @param {Object} [mouseState]
+   * @param {Promise<Object<string, Array<Object>> | null>}
+   *   [fieldObjectsPromise]
    * @returns {AnnotationLayerBuilder}
    */
   createAnnotationLayerBuilder(
@@ -1329,7 +1331,8 @@ class BaseViewer {
     l10n = NullL10n,
     enableScripting = null,
     hasJSActionsPromise = null,
-    mouseState = null
+    mouseState = null,
+    fieldObjectsPromise = null
   ) {
     return new AnnotationLayerBuilder({
       pageDiv,
@@ -1344,6 +1347,8 @@ class BaseViewer {
       enableScripting: enableScripting ?? this.enableScripting,
       hasJSActionsPromise:
         hasJSActionsPromise || this.pdfDocument?.hasJSActions(),
+      fieldObjectsPromise:
+        fieldObjectsPromise || this.pdfDocument?.getFieldObjects(),
       mouseState: mouseState || this._scriptingManager?.mouseState,
     });
   }

web/interfaces.js

@@ -166,6 +166,8 @@ class IPDFAnnotationLayerFactory {
    * @param {boolean} [enableScripting]
    * @param {Promise<boolean>} [hasJSActionsPromise]
    * @param {Object} [mouseState]
+   * @param {Promise<Object<string, Array<Object>> | null>}
+   *   [fieldObjectsPromise]
    * @returns {AnnotationLayerBuilder}
    */
   createAnnotationLayerBuilder(
@@ -177,7 +179,8 @@ class IPDFAnnotationLayerFactory {
     l10n = undefined,
     enableScripting = false,
     hasJSActionsPromise = null,
-    mouseState = null
+    mouseState = null,
+    fieldObjectsPromise = null
   ) {}
 }
 

web/pdf_page_view.js

@@ -675,7 +675,8 @@ class PDFPageView {
             this.l10n,
             /* enableScripting = */ null,
             /* hasJSActionsPromise = */ null,
-            /* mouseState = */ null
+            /* mouseState = */ null,
+            /* fieldObjectsPromise = */ null
           );
       }
       this._renderAnnotationLayer();