Merge pull request #17720 from calixteman/editor_simplify_highlighting

[Editor] In caret browsing mode, allow to select in pressing shift and arrow down (bug 1881802)

src/display/editor/annotation_editor_layer.js

@@ -63,16 +63,12 @@ class AnnotationEditorLayer {
 
   #boundPointerup = this.pointerup.bind(this);
 
-  #boundPointerUpAfterSelection = this.pointerUpAfterSelection.bind(this);
-
   #boundPointerdown = this.pointerdown.bind(this);
 
   #boundTextLayerPointerDown = this.#textLayerPointerDown.bind(this);
 
   #editorFocusTimeoutId = null;
 
-  #boundSelectionStart = this.selectionStart.bind(this);
-
   #editors = new Map();
 
   #hadPointerDown = false;
@@ -338,7 +334,6 @@ class AnnotationEditorLayer {
 
   enableTextSelection() {
     if (this.#textLayer?.div) {
-      document.addEventListener("selectstart", this.#boundSelectionStart);
       this.#textLayer.div.addEventListener(
         "pointerdown",
         this.#boundTextLayerPointerDown
@@ -349,7 +344,6 @@ class AnnotationEditorLayer {
 
   disableTextSelection() {
     if (this.#textLayer?.div) {
-      document.removeEventListener("selectstart", this.#boundSelectionStart);
       this.#textLayer.div.removeEventListener(
         "pointerdown",
         this.#boundTextLayerPointerDown
@@ -686,54 +680,6 @@ class AnnotationEditorLayer {
     this.#uiManager.unselect(editor);
   }
 
-  /**
-   * SelectionChange callback.
-   * @param {Event} event
-   */
-  selectionStart(event) {
-    if (
-      !this.#textLayer ||
-      event.target.parentElement.closest(".textLayer") !== this.#textLayer.div
-    ) {
-      return;
-    }
-
-    if (this.#uiManager.isShiftKeyDown) {
-      const keyup = () => {
-        if (this.#uiManager.isShiftKeyDown) {
-          return;
-        }
-
-        window.removeEventListener("keyup", keyup);
-        window.removeEventListener("blur", keyup);
-        this.pointerUpAfterSelection({});
-      };
-      window.addEventListener("keyup", keyup);
-      window.addEventListener("blur", keyup);
-    } else {
-      this.#textLayer.div.addEventListener(
-        "pointerup",
-        this.#boundPointerUpAfterSelection,
-        { once: true }
-      );
-    }
-  }
-
-  /**
-   * Called when the user releases the mouse button after having selected
-   * some text.
-   * @param {PointerEvent} event
-   */
-  pointerUpAfterSelection(event) {
-    const boxes = this.#uiManager.getSelectionBoxes(this.#textLayer?.div);
-    if (boxes) {
-      this.createAndAddNewEditor(event, false, {
-        boxes,
-      });
-      document.getSelection().empty();
-    }
-  }
-
   /**
    * Pointerup callback.
    * @param {PointerEvent} event

src/display/editor/tools.js

@@ -57,11 +57,19 @@ function opacityToHex(opacity) {
 class IdManager {
   #id = 0;
 
+  constructor() {
+    if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) {
+      Object.defineProperty(this, "reset", {
+        value: () => (this.#id = 0),
+      });
+    }
+  }
+
   /**
    * Get a unique id.
    * @returns {string}
    */
-  getId() {
+  get id() {
     return `${AnnotationEditorPrefix}${this.#id++}`;
   }
 }
@@ -551,10 +559,10 @@ class AnnotationEditorUIManager {
 
   #focusMainContainerTimeoutId = null;
 
-  #hasSelection = false;
-
   #highlightColors = null;
 
+  #highlightWhenShiftUp = false;
+
   #idManager = new IdManager();
 
   #isEnabled = false;
@@ -780,6 +788,16 @@ class AnnotationEditorUIManager {
       rotation: 0,
     };
     this.isShiftKeyDown = false;
+
+    if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("TESTING")) {
+      Object.defineProperty(this, "reset", {
+        value: () => {
+          this.selectAll();
+          this.delete();
+          this.#idManager.reset();
+        },
+      });
+    }
   }
 
   destroy() {
@@ -958,8 +976,7 @@ class AnnotationEditorUIManager {
   #selectionChange() {
     const selection = document.getSelection();
     if (!selection || selection.isCollapsed) {
-      if (this.#hasSelection) {
+      if (this.#selectedTextNode) {
-        this.#hasSelection = false;
         this.#selectedTextNode = null;
         this.#dispatchUpdateStates({
           hasSelectedText: false,
@@ -977,8 +994,7 @@ class AnnotationEditorUIManager {
         ? anchorNode.parentElement
         : anchorNode;
     if (!anchorElement.closest(".textLayer")) {
-      if (this.#hasSelection) {
+      if (this.#selectedTextNode) {
-        this.#hasSelection = false;
         this.#selectedTextNode = null;
         this.#dispatchUpdateStates({
           hasSelectedText: false,
@@ -986,11 +1002,30 @@ class AnnotationEditorUIManager {
       }
       return;
     }
-    this.#hasSelection = true;
     this.#selectedTextNode = anchorNode;
     this.#dispatchUpdateStates({
       hasSelectedText: true,
     });
+
+    if (this.#mode !== AnnotationEditorType.HIGHLIGHT) {
+      return;
+    }
+    this.#highlightWhenShiftUp = this.isShiftKeyDown;
+    if (!this.isShiftKeyDown) {
+      const pointerup = e => {
+        if (e.type === "pointerup" && e.button !== 0) {
+          // Do nothing on right click.
+          return;
+        }
+        window.removeEventListener("pointerup", pointerup);
+        window.removeEventListener("blur", pointerup);
+        if (e.type === "pointerup") {
+          this.highlightSelection();
+        }
+      };
+      window.addEventListener("pointerup", pointerup);
+      window.addEventListener("blur", pointerup);
+    }
   }
 
   #addSelectionListener() {
@@ -1013,6 +1048,10 @@ class AnnotationEditorUIManager {
 
   blur() {
     this.isShiftKeyDown = false;
+    if (this.#highlightWhenShiftUp) {
+      this.#highlightWhenShiftUp = false;
+      this.highlightSelection();
+    }
     if (!this.hasSelection) {
       return;
     }
@@ -1199,6 +1238,10 @@ class AnnotationEditorUIManager {
   keyup(event) {
     if (this.isShiftKeyDown && event.key === "Shift") {
       this.isShiftKeyDown = false;
+      if (this.#highlightWhenShiftUp) {
+        this.#highlightWhenShiftUp = false;
+        this.highlightSelection();
+      }
     }
   }
 
@@ -1287,7 +1330,7 @@ class AnnotationEditorUIManager {
    * @returns {string}
    */
   getId() {
-    return this.#idManager.getId();
+    return this.#idManager.id;
   }
 
   get currentLayer() {

test/integration/highlight_editor_spec.mjs

@@ -83,7 +83,11 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          // Here and elsewhere, we add a small delay between press and release
+          // to make sure that a pointerup event is triggered after
+          // selectionchange.
+          // It works with a value of 1ms, but we use 100ms to be sure.
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
 
@@ -128,7 +132,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.waitForSelector(
@@ -179,7 +183,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.waitForSelector(
@@ -225,7 +229,7 @@ describe("Highlight Editor", () => {
           let rect = await getSpanRectFromText(page, 1, "Abstract");
           let x = rect.x + rect.width / 2;
           let y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.waitForSelector(
@@ -248,7 +252,7 @@ describe("Highlight Editor", () => {
           rect = await getSpanRectFromText(page, 14, "References");
           x = rect.x + rect.width / 2;
           y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
           await page.waitForSelector(`${getEditorSelector(1)}`);
           await page.waitForSelector(
             `.page[data-page-number = "14"] svg.highlightOutline.selected`
@@ -314,7 +318,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.waitForSelector(
@@ -376,7 +380,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(sel);
           await page.waitForSelector(
@@ -487,7 +491,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.waitForSelector(
@@ -535,7 +539,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(sel);
           await page.waitForSelector(
@@ -577,7 +581,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(sel);
           await page.waitForSelector(
@@ -871,7 +875,7 @@ describe("Highlight Editor", () => {
           );
           const x = rect.x + 0.75 * rect.width;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
 
           await page.waitForSelector(`${getEditorSelector(0)}`);
           const usedColor = await page.evaluate(() => {
@@ -981,7 +985,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
           await page.waitForFunction(() => window.editingEvents.length > 0);
 
           let editingEvent = await page.evaluate(() => {
@@ -1008,7 +1012,7 @@ describe("Highlight Editor", () => {
             .withContext(`In ${browserName}`)
             .toBe(false);
 
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
           await page.waitForFunction(() => window.editingEvents.length > 0);
 
           await page.evaluate(() => {
@@ -1039,7 +1043,17 @@ describe("Highlight Editor", () => {
         "tracemonkey.pdf",
         ".annotationEditorLayer",
         null,
-        null,
+        async page => {
+          await page.evaluate(async () => {
+            await window.PDFViewerApplication.initializedPromise;
+            window.PDFViewerApplication.eventBus.on(
+              "annotationeditoruimanager",
+              ({ uiManager }) => {
+                window.uiManager = uiManager;
+              }
+            );
+          });
+        },
         {
           highlightEditorColors: "red=#AB0000",
           supportsCaretBrowsingMode: true,
@@ -1047,6 +1061,19 @@ describe("Highlight Editor", () => {
       );
     });
 
+    afterEach(async () => {
+      for (const [, page] of pages) {
+        await page.evaluate(() => {
+          window.uiManager.reset();
+        });
+        // Disable editing mode.
+        await page.click("#editorHighlight");
+        await page.waitForSelector(
+          `.annotationEditorLayer:not(.highlightEditing)`
+        );
+      }
+    });
+
     afterAll(async () => {
       await closePages(pages);
     });
@@ -1060,8 +1087,7 @@ describe("Highlight Editor", () => {
           const rect = await getSpanRectFromText(page, 1, "Abstract");
           const x = rect.x + rect.width / 2;
           const y = rect.y + rect.height / 2;
-          await page.mouse.click(x, y, { count: 2 });
+          await page.mouse.click(x, y, { count: 2, delay: 100 });
-
           await page.waitForSelector(`${getEditorSelector(0)}`);
           await page.keyboard.press("Escape");
           await page.waitForSelector(
@@ -1092,5 +1118,41 @@ describe("Highlight Editor", () => {
         })
       );
     });
+
+    it("must check that selection is correctly highlighted on arrow down key pressed", async () => {
+      await Promise.all(
+        pages.map(async ([browserName, page]) => {
+          await page.click("#editorHighlight");
+          await page.waitForSelector(".annotationEditorLayer.highlightEditing");
+
+          await page.evaluate(() => {
+            const text =
+              "Dynamic languages such as JavaScript are more difficult to com-";
+            for (const el of document.querySelectorAll(
+              `.page[data-page-number="${1}"] > .textLayer > span`
+            )) {
+              if (el.textContent === text) {
+                window.getSelection().setPosition(el.firstChild, 15);
+                break;
+              }
+            }
+          });
+
+          await page.keyboard.down("Shift");
+          await page.keyboard.press("ArrowDown");
+          await page.keyboard.up("Shift");
+
+          await page.waitForSelector(getEditorSelector(0));
+          const usedColor = await page.evaluate(() => {
+            const highlight = document.querySelector(
+              `.page[data-page-number = "1"] .canvasWrapper > svg.highlight`
+            );
+            return highlight.getAttribute("fill");
+          });
+
+          expect(usedColor).withContext(`In ${browserName}`).toEqual("#AB0000");
+        })
+      );
+    });
   });
 });

test/test.mjs

@@ -949,6 +949,8 @@ async function startBrowser({ browserName, headless, startUrl }) {
       "layout.css.round.enabled": true,
       // This allow to copy some data in the clipboard.
       "dom.events.asyncClipboard.clipboardItem": true,
+      // It's helpful to see where the caret is.
+      "accessibility.browsewithcaret": true,
     };
   }