Merge pull request #16746 from calixteman/editor_keyboard_movable

[Editor] Make editors movable in using the keyboard (bug 1845088)

src/display/editor/editor.js

@@ -18,8 +18,13 @@
 // eslint-disable-next-line max-len
 /** @typedef {import("./tools.js").AnnotationEditorUIManager} AnnotationEditorUIManager */
 
+import {
+  AnnotationEditorParamsType,
+  FeatureTest,
+  shadow,
+  unreachable,
+} from "../../shared/util.js";
 import { bindEvents, ColorManager } from "./tools.js";
-import { FeatureTest, shadow, unreachable } from "../../shared/util.js";
 
 /**
  * @typedef {Object} AnnotationEditorParameters
@@ -261,13 +266,7 @@ class AnnotationEditor {
     this.fixAndSetPosition();
   }
 
-  /**
+  #translate([width, height], x, y) {
-   * Translate the editor position within its parent.
-   * @param {number} x - x-translation in screen coordinates.
-   * @param {number} y - y-translation in screen coordinates.
-   */
-  translate(x, y) {
-    const [width, height] = this.parentDimensions;
     [x, y] = this.screenToPageTranslation(x, y);
 
     this.x += x / width;
@@ -276,6 +275,26 @@ class AnnotationEditor {
     this.fixAndSetPosition();
   }
 
+  /**
+   * Translate the editor position within its parent.
+   * @param {number} x - x-translation in screen coordinates.
+   * @param {number} y - y-translation in screen coordinates.
+   */
+  translate(x, y) {
+    this.#translate(this.parentDimensions, x, y);
+  }
+
+  /**
+   * Translate the editor position within its page and adjust the scroll
+   * in order to have the editor in the view.
+   * @param {number} x - x-translation in page coordinates.
+   * @param {number} y - y-translation in page coordinates.
+   */
+  translateInPage(x, y) {
+    this.#translate(this.pageDimensions, x, y);
+    this.div.scrollIntoView({ block: "nearest" });
+  }
+
   fixAndSetPosition() {
     const [pageWidth, pageHeight] = this.pageDimensions;
     let { x, y, width, height } = this;
@@ -663,7 +682,7 @@ class AnnotationEditor {
       cmd,
       undo,
       mustExec: true,
-      type: this.resizeType,
+      type: AnnotationEditorParamsType.RESIZE,
       overwriteIfSameType: true,
       keepUndo: true,
     });
@@ -922,13 +941,6 @@ class AnnotationEditor {
     }
   }
 
-  /**
-   * @returns {number} the type to use in the undo/redo stack when resizing.
-   */
-  get resizeType() {
-    return -1;
-  }
-
   /**
    * @returns {boolean} true if this editor can be resized.
    */

src/display/editor/freetext.js

@@ -71,7 +71,7 @@ class FreeTextEditor extends AnnotationEditor {
           // See bug 1831574.
           ["ctrl+s", "mac+meta+s", "ctrl+p", "mac+meta+p"],
           FreeTextEditor.prototype.commitOrRemove,
-          /* bubbles = */ true,
+          { bubbles: true },
         ],
         [
           ["ctrl+Enter", "mac+meta+Enter", "Escape", "mac+Escape"],

src/display/editor/ink.js

@@ -157,11 +157,6 @@ class InkEditor extends AnnotationEditor {
     ];
   }
 
-  /** @inheritdoc */
-  get resizeType() {
-    return AnnotationEditorParamsType.INK_DIMS;
-  }
-
   /**
    * Update the thickness and make this action undoable.
    * @param {number} thickness

src/display/editor/stamp.js

@@ -13,11 +13,8 @@
  * limitations under the License.
  */
 
-import {
-  AnnotationEditorParamsType,
-  AnnotationEditorType,
-} from "../../shared/util.js";
 import { AnnotationEditor } from "./editor.js";
+import { AnnotationEditorType } from "../../shared/util.js";
 import { PixelsPerInch } from "../display_utils.js";
 import { StampAnnotationElement } from "../annotation_layer.js";
 
@@ -126,11 +123,6 @@ class StampEditor extends AnnotationEditor {
     }
   }
 
-  /** @inheritdoc */
-  get resizeType() {
-    return AnnotationEditorParamsType.STAMP_DIMS;
-  }
-
   /** @inheritdoc */
   remove() {
     if (this.#bitmapId) {

src/display/editor/tools.js

@@ -355,14 +355,14 @@ class KeyboardManager {
     this.allKeys = new Set();
 
     const { isMac } = FeatureTest.platform;
-    for (const [keys, callback, bubbles = false] of callbacks) {
+    for (const [keys, callback, options = {}] of callbacks) {
       for (const key of keys) {
         const isMacKey = key.startsWith("mac+");
         if (isMac && isMacKey) {
-          this.callbacks.set(key.slice(4), { callback, bubbles });
+          this.callbacks.set(key.slice(4), { callback, options });
           this.allKeys.add(key.split("+").at(-1));
         } else if (!isMac && !isMacKey) {
-          this.callbacks.set(key, { callback, bubbles });
+          this.callbacks.set(key, { callback, options });
           this.allKeys.add(key.split("+").at(-1));
         }
       }
@@ -410,8 +410,15 @@ class KeyboardManager {
     if (!info) {
       return;
     }
-    const { callback, bubbles } = info;
+    const {
-    callback.bind(self)();
+      callback,
+      options: { bubbles = false, args = [], checker = null },
+    } = info;
+
+    if (checker && !checker(self, event)) {
+      return;
+    }
+    callback.bind(self, ...args)();
 
     // For example, ctrl+s in a FreeText must be handled by the viewer, hence
     // the event must bubble.
@@ -548,9 +555,29 @@ class AnnotationEditorUIManager {
     hasSelectedEditor: false,
   };
 
+  #translation = [0, 0];
+
+  #translationTimeoutId = null;
+
   #container = null;
 
+  static #TRANSLATE_SMALL = 1; // page units.
+
+  static #TRANSLATE_BIG = 10; // page units.
+
   static get _keyboardManager() {
+    const arrowChecker = self => {
+      // If the focused element is an input, we don't want to handle the arrow.
+      // For example, sliders can be controlled with the arrow keys.
+      const { activeElement } = document;
+      return (
+        activeElement &&
+        self.#container.contains(activeElement) &&
+        self.hasSomethingToControl()
+      );
+    };
+    const small = this.#TRANSLATE_SMALL;
+    const big = this.#TRANSLATE_BIG;
     return shadow(
       this,
       "_keyboardManager",
@@ -592,6 +619,46 @@ class AnnotationEditorUIManager {
           ["Escape", "mac+Escape"],
           AnnotationEditorUIManager.prototype.unselectAll,
         ],
+        [
+          ["ArrowLeft", "mac+ArrowLeft"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [-small, 0], checker: arrowChecker },
+        ],
+        [
+          ["ctrl+ArrowLeft", "mac+meta+ArrowLeft"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [-big, 0], checker: arrowChecker },
+        ],
+        [
+          ["ArrowRight", "mac+ArrowRight"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [small, 0], checker: arrowChecker },
+        ],
+        [
+          ["ctrl+ArrowRight", "mac+meta+ArrowRight"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [big, 0], checker: arrowChecker },
+        ],
+        [
+          ["ArrowUp", "mac+ArrowUp"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [0, -small], checker: arrowChecker },
+        ],
+        [
+          ["ctrl+ArrowUp", "mac+meta+ArrowUp"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [0, -big], checker: arrowChecker },
+        ],
+        [
+          ["ArrowDown", "mac+ArrowDown"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [0, small], checker: arrowChecker },
+        ],
+        [
+          ["ctrl+ArrowDown", "mac+meta+ArrowDown"],
+          AnnotationEditorUIManager.prototype.translateSelectedEditors,
+          { args: [0, big], checker: arrowChecker },
+        ],
       ])
     );
   }
@@ -1260,6 +1327,10 @@ class AnnotationEditorUIManager {
     this.#activeEditor?.commitOrRemove();
   }
 
+  hasSomethingToControl() {
+    return this.#activeEditor || this.hasSelection;
+  }
+
   /**
    * Select the editors.
    * @param {Array<AnnotationEditor>} editors
@@ -1296,7 +1367,7 @@ class AnnotationEditorUIManager {
       return;
     }
 
-    if (this.#selectedEditors.size === 0) {
+    if (!this.hasSelection) {
       return;
     }
     for (const editor of this.#selectedEditors) {
@@ -1308,6 +1379,53 @@ class AnnotationEditorUIManager {
     });
   }
 
+  translateSelectedEditors(x, y) {
+    this.commitOrRemove();
+    if (!this.hasSelection) {
+      return;
+    }
+
+    this.#translation[0] += x;
+    this.#translation[1] += y;
+    const [totalX, totalY] = this.#translation;
+    const editors = [...this.#selectedEditors];
+
+    // We don't want to have an undo/redo for each translation so we wait a bit
+    // before adding the command to the command manager.
+    const TIME_TO_WAIT = 1000;
+
+    if (this.#translationTimeoutId) {
+      clearTimeout(this.#translationTimeoutId);
+    }
+
+    this.#translationTimeoutId = setTimeout(() => {
+      this.#translationTimeoutId = null;
+      this.#translation[0] = this.#translation[1] = 0;
+
+      this.addCommands({
+        cmd: () => {
+          for (const editor of editors) {
+            if (this.#allEditors.has(editor.id)) {
+              editor.translateInPage(totalX, totalY);
+            }
+          }
+        },
+        undo: () => {
+          for (const editor of editors) {
+            if (this.#allEditors.has(editor.id)) {
+              editor.translateInPage(-totalX, -totalY);
+            }
+          }
+        },
+        mustExec: false,
+      });
+    }, TIME_TO_WAIT);
+
+    for (const editor of editors) {
+      editor.translateInPage(x, y);
+    }
+  }
+
   /**
    * Is the current editor the one passed as argument?
    * @param {AnnotationEditor} editor

src/shared/util.js

@@ -77,14 +77,13 @@ const AnnotationEditorType = {
 };
 
 const AnnotationEditorParamsType = {
-  FREETEXT_SIZE: 1,
+  RESIZE: 1,
-  FREETEXT_COLOR: 2,
+  FREETEXT_SIZE: 11,
-  FREETEXT_OPACITY: 3,
+  FREETEXT_COLOR: 12,
-  INK_COLOR: 11,
+  FREETEXT_OPACITY: 13,
-  INK_THICKNESS: 12,
+  INK_COLOR: 21,
-  INK_OPACITY: 13,
+  INK_THICKNESS: 22,
-  INK_DIMS: 14,
+  INK_OPACITY: 23,
-  STAMP_DIMS: 21,
 };
 
 // Permission flags from Table 22, Section 7.6.3.2 of the PDF specification.

test/integration/freetext_editor_spec.js

@@ -18,6 +18,7 @@ const {
   getEditors,
   getEditorSelector,
   getSelectedEditors,
+  getFirstSerialized,
   getSerialized,
   loadAndWait,
   waitForEvent,
@@ -752,7 +753,7 @@ describe("FreeText Editor", () => {
               "switchannotationeditorparams",
               {
                 source: null,
-                type: /* AnnotationEditorParamsType.FREETEXT_SIZE */ 1,
+                type: window.pdfjsLib.AnnotationEditorParamsType.FREETEXT_SIZE,
                 value: 13,
               }
             );
@@ -769,7 +770,7 @@ describe("FreeText Editor", () => {
               "switchannotationeditorparams",
               {
                 source: null,
-                type: /* AnnotationEditorParamsType.FREETEXT_COLOR */ 2,
+                type: window.pdfjsLib.AnnotationEditorParamsType.FREETEXT_COLOR,
                 value: "#FF0000",
               }
             );
@@ -1310,7 +1311,7 @@ describe("FreeText Editor", () => {
               "switchannotationeditorparams",
               {
                 source: null,
-                type: /* AnnotationEditorParamsType.FREETEXT_SIZE */ 1,
+                type: window.pdfjsLib.AnnotationEditorParamsType.FREETEXT_SIZE,
                 value: 50,
               }
             );
@@ -1780,4 +1781,142 @@ describe("FreeText Editor", () => {
       );
     });
   });
+
+  describe("Move editor with arrows", () => {
+    let pages;
+
+    beforeAll(async () => {
+      pages = await loadAndWait("empty.pdf", ".annotationEditorLayer");
+    });
+
+    afterAll(async () => {
+      await closePages(pages);
+    });
+
+    it("must check the position of moved editor", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await page.click("#editorFreeText");
+
+          const rect = await page.$eval(".annotationEditorLayer", el => {
+            const { x, y } = el.getBoundingClientRect();
+            return { x, y };
+          });
+
+          const data = "Hello PDF.js World !!";
+          await page.mouse.click(rect.x + 200, rect.y + 200);
+          await page.type(`${getEditorSelector(0)} .internal`, data);
+
+          const editorRect = await page.$eval(getEditorSelector(0), el => {
+            const { x, y, width, height } = el.getBoundingClientRect();
+            return {
+              x,
+              y,
+              width,
+              height,
+            };
+          });
+
+          // Commit.
+          await page.mouse.click(
+            editorRect.x,
+            editorRect.y + 2 * editorRect.height
+          );
+          await page.waitForTimeout(10);
+
+          const [pageX, pageY] = await getFirstSerialized(page, x => x.rect);
+
+          for (let i = 0; i < 20; i++) {
+            await page.keyboard.press("ArrowRight");
+            await page.waitForTimeout(1);
+          }
+
+          let [newX, newY] = await getFirstSerialized(page, x => x.rect);
+          expect(Math.round(newX))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageX + 20));
+          expect(Math.round(newY))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageY));
+
+          for (let i = 0; i < 20; i++) {
+            await page.keyboard.press("ArrowDown");
+            await page.waitForTimeout(1);
+          }
+
+          [newX, newY] = await getFirstSerialized(page, x => x.rect);
+          expect(Math.round(newX))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageX + 20));
+          expect(Math.round(newY))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageY - 20));
+
+          for (let i = 0; i < 2; i++) {
+            await page.keyboard.down("Control");
+            await page.keyboard.press("ArrowLeft");
+            await page.keyboard.up("Control");
+            await page.waitForTimeout(1);
+          }
+
+          [newX, newY] = await getFirstSerialized(page, x => x.rect);
+          expect(Math.round(newX))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageX));
+          expect(Math.round(newY))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageY - 20));
+
+          for (let i = 0; i < 2; i++) {
+            await page.keyboard.down("Control");
+            await page.keyboard.press("ArrowUp");
+            await page.keyboard.up("Control");
+            await page.waitForTimeout(1);
+          }
+
+          [newX, newY] = await getFirstSerialized(page, x => x.rect);
+          expect(Math.round(newX))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageX));
+          expect(Math.round(newY))
+            .withContext(`In ${browserName}`)
+            .toEqual(Math.round(pageY));
+        })
+      );
+    });
+
+    it("must check arrow doesn't move an editor when a slider is focused", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await page.keyboard.down("Control");
+          await page.keyboard.press("a");
+          await page.keyboard.up("Control");
+          await page.waitForTimeout(10);
+
+          await page.focus("#editorFreeTextFontSize");
+
+          const [page1X, , page2X] = await getFirstSerialized(
+            page,
+            x => x.rect
+          );
+          const pageWidth = page2X - page1X;
+
+          for (let i = 0; i < 5; i++) {
+            await page.keyboard.press("ArrowRight");
+            await page.waitForTimeout(1);
+          }
+          await page.waitForTimeout(10);
+
+          const [new1X, , new2X] = await getFirstSerialized(page, x => x.rect);
+          const newWidth = new2X - new1X;
+          expect(Math.round(new1X))
+            .withContext(`In ${browserName}`)
+            .not.toEqual(Math.round(page1X + 5));
+          expect(newWidth)
+            .withContext(`In ${browserName}`)
+            .not.toEqual(pageWidth);
+        })
+      );
+    });
+  });
 });

test/integration/test_utils.js

@@ -137,14 +137,20 @@ const mockClipboard = async pages => {
 };
 exports.mockClipboard = mockClipboard;
 
-const getSerialized = page =>
+async function getSerialized(page, filter = undefined) {
-  page.evaluate(() => {
+  const values = await page.evaluate(() => {
     const { map } =
       window.PDFViewerApplication.pdfDocument.annotationStorage.serializable;
     return map ? [...map.values()] : [];
   });
+  return filter ? values.map(filter) : values;
+}
 exports.getSerialized = getSerialized;
 
+const getFirstSerialized = async (page, filter = undefined) =>
+  (await getSerialized(page, filter))[0];
+exports.getFirstSerialized = getFirstSerialized;
+
 function getEditors(page, kind) {
   return page.evaluate(aKind => {
     const elements = document.querySelectorAll(`.${aKind}Editor`);