From 5bbe0d078288565fc2e5dbd7794eae43db08323e Mon Sep 17 00:00:00 2001 From: Calixte Denizet Date: Fri, 22 Jul 2022 12:13:47 +0200 Subject: [PATCH] [Editor] Unselect correctly removed editors - After undoing a deletion of several editors, they appeared to be selected (they had a red border) when in fact they were not, consequently, this patch aims to remove the selectedEditor class when an editor is removed; - Add a test with some ink editors. --- src/display/editor/ink.js | 3 + src/display/editor/tools.js | 13 +++- test/integration-boot.js | 1 + test/integration/freetext_editor_spec.js | 47 ++++++-------- test/integration/ink_editor_spec.js | 78 ++++++++++++++++++++++++ test/integration/test_utils.js | 16 +++++ 6 files changed, 127 insertions(+), 31 deletions(-) create mode 100644 test/integration/ink_editor_spec.js diff --git a/src/display/editor/ink.js b/src/display/editor/ink.js index 5f3d72cbebd0b..c906b81d2da3e 100644 --- a/src/display/editor/ink.js +++ b/src/display/editor/ink.js @@ -416,6 +416,9 @@ class InkEditor extends AnnotationEditor { // When commiting, the position of this editor is changed, hence we must // move it to the right position in the DOM. this.parent.moveDivInDOM(this); + // After the div has been moved in the DOM, the focus may have been stolen + // by document.body, hence we just keep it here. + this.div.focus(); } /** @inheritdoc */ diff --git a/src/display/editor/tools.js b/src/display/editor/tools.js index 32478508df6c2..8d67cc0b1cc73 100644 --- a/src/display/editor/tools.js +++ b/src/display/editor/tools.js @@ -709,9 +709,7 @@ class AnnotationEditorUIManager { */ removeEditor(editor) { this.#allEditors.delete(editor.id); - if (this.hasSelection) { - this.#selectedEditors.delete(editor); - } + this.unselect(editor); } /** @@ -962,6 +960,15 @@ class AnnotationEditorUIManager { * Unselect all the selected editors. */ unselectAll() { + if (this.#activeEditor) { + // An editor is being edited so just commit it. + this.#activeEditor.commitOrRemove(); + return; + } + + if (this.#selectEditors.size === 0) { + return; + } for (const editor of this.#selectedEditors) { editor.unselect(); } diff --git a/test/integration-boot.js b/test/integration-boot.js index 695576a6e1836..921511d9885c8 100644 --- a/test/integration-boot.js +++ b/test/integration-boot.js @@ -31,6 +31,7 @@ async function runTests(results) { "accessibility_spec.js", "find_spec.js", "freetext_editor_spec.js", + "ink_editor_spec.js", ], }); diff --git a/test/integration/freetext_editor_spec.js b/test/integration/freetext_editor_spec.js index 722ba2cad7792..b5461e0c8f0d5 100644 --- a/test/integration/freetext_editor_spec.js +++ b/test/integration/freetext_editor_spec.js @@ -13,9 +13,12 @@ * limitations under the License. */ -const { closePages, loadAndWait } = require("./test_utils.js"); - -const editorPrefix = "#pdfjs_internal_editor_"; +const { + closePages, + editorPrefix, + getSelectedEditors, + loadAndWait, +} = require("./test_utils.js"); describe("Editor", () => { describe("FreeText", () => { @@ -264,18 +267,6 @@ describe("Editor", () => { await closePages(pages); }); - function getSelected(page) { - return page.evaluate(prefix => { - const elements = document.querySelectorAll(".selectedEditor"); - const results = []; - for (const element of elements) { - results.push(parseInt(element.id.slice(prefix.length))); - } - results.sort(); - return results; - }, editorPrefix.slice(1)); - } - it("must select/unselect several editors and check copy, paste and delete operations", async () => { await Promise.all( pages.map(async ([browserName, page]) => { @@ -322,27 +313,27 @@ describe("Editor", () => { await page.keyboard.press("a"); await page.keyboard.up("Control"); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 1, 2, 3]); await page.keyboard.down("Control"); await page.mouse.click(editorCenters[1].x, editorCenters[1].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 2, 3]); await page.mouse.click(editorCenters[2].x, editorCenters[2].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 3]); await page.mouse.click(editorCenters[1].x, editorCenters[1].y); await page.keyboard.up("Control"); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 1, 3]); @@ -355,25 +346,25 @@ describe("Editor", () => { await page.keyboard.up("Control"); // 0,1,3 are unselected and new pasted editors are selected. - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([4, 5, 6]); // No ctrl here, hence all are unselected and 2 is selected. await page.mouse.click(editorCenters[2].x, editorCenters[2].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([2]); await page.mouse.click(editorCenters[1].x, editorCenters[1].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([1]); await page.keyboard.down("Control"); await page.mouse.click(editorCenters[3].x, editorCenters[3].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([1, 3]); @@ -386,25 +377,25 @@ describe("Editor", () => { await page.keyboard.press("a"); await page.keyboard.up("Control"); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 2, 4, 5, 6]); // Create an empty editor. await page.mouse.click(rect.x + 700, rect.y + 100); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([7]); // Set the focus to 2 and check that only 2 is selected. await page.mouse.click(editorCenters[2].x, editorCenters[2].y); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([2]); // Create an empty editor. await page.mouse.click(rect.x + 700, rect.y + 100); - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([8]); // Dismiss it. @@ -417,7 +408,7 @@ describe("Editor", () => { // Check that all the editors are correctly selected (and the focus // didn't move to the body when the empty editor was removed). - expect(await getSelected(page)) + expect(await getSelectedEditors(page)) .withContext(`In ${browserName}`) .toEqual([0, 2, 4, 5, 6]); }) diff --git a/test/integration/ink_editor_spec.js b/test/integration/ink_editor_spec.js new file mode 100644 index 0000000000000..fe18341996ab8 --- /dev/null +++ b/test/integration/ink_editor_spec.js @@ -0,0 +1,78 @@ +/* Copyright 2022 Mozilla Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const { + closePages, + getSelectedEditors, + loadAndWait, +} = require("./test_utils.js"); + +describe("Editor", () => { + describe("Ink", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer"); + }); + + afterAll(async () => { + await closePages(pages); + }); + + it("must draw, undo a deletion and check that the editors are not selected", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + await page.click("#editorInk"); + + const rect = await page.$eval(".annotationEditorLayer", el => { + // With Chrome something is wrong when serializing a DomRect, + // hence we extract the values and just return them. + const { x, y } = el.getBoundingClientRect(); + return { x, y }; + }); + + for (let i = 0; i < 3; i++) { + const x = rect.x + 100 + i * 100; + const y = rect.y + 100 + i * 100; + await page.mouse.move(x, y); + await page.mouse.down(); + await page.mouse.move(x + 50, y + 50); + await page.mouse.up(); + + await page.keyboard.press("Escape"); + } + + await page.keyboard.down("Control"); + await page.keyboard.press("a"); + await page.keyboard.up("Control"); + + expect(await getSelectedEditors(page)) + .withContext(`In ${browserName}`) + .toEqual([0, 2, 3]); + + await page.keyboard.press("Backspace"); + + await page.keyboard.down("Control"); + await page.keyboard.press("z"); + await page.keyboard.up("Control"); + + expect(await getSelectedEditors(page)) + .withContext(`In ${browserName}`) + .toEqual([]); + }) + ); + }); + }); +}); diff --git a/test/integration/test_utils.js b/test/integration/test_utils.js index 1a5d529e1392b..cba64a815783c 100644 --- a/test/integration/test_utils.js +++ b/test/integration/test_utils.js @@ -73,3 +73,19 @@ function getComputedStyleSelector(id) { return `getComputedStyle(${getQuerySelector(id)})`; } exports.getComputedStyleSelector = getComputedStyleSelector; + +const editorPrefix = "#pdfjs_internal_editor_"; +exports.editorPrefix = editorPrefix; + +function getSelectedEditors(page) { + return page.evaluate(prefix => { + const elements = document.querySelectorAll(".selectedEditor"); + const results = []; + for (const { id } of elements) { + results.push(parseInt(id.slice(prefix.length))); + } + results.sort(); + return results; + }, editorPrefix.slice(1)); +} +exports.getSelectedEditors = getSelectedEditors;