Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Editor] Make the text layer focusable before the editors (bug 1881746) #17790

Merged
merged 1 commit into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/display/editor/annotation_editor_layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ class AnnotationEditorLayer {
* editor creation.
*/
enable() {
this.div.tabIndex = 0;
this.togglePointerEvents(true);
const annotationElementIds = new Set();
for (const editor of this.#editors.values()) {
Expand Down Expand Up @@ -274,6 +275,7 @@ class AnnotationEditorLayer {
*/
disable() {
this.#isDisabling = true;
this.div.tabIndex = -1;
this.togglePointerEvents(false);
const hiddenAnnotationIds = new Set();
for (const editor of this.#editors.values()) {
Expand Down Expand Up @@ -333,6 +335,7 @@ class AnnotationEditorLayer {
}

enableTextSelection() {
this.div.tabIndex = -1;
if (this.#textLayer?.div && !this.#boundTextLayerPointerDown) {
this.#boundTextLayerPointerDown = this.#textLayerPointerDown.bind(this);
this.#textLayer.div.addEventListener(
Expand All @@ -344,6 +347,7 @@ class AnnotationEditorLayer {
}

disableTextSelection() {
this.div.tabIndex = 0;
if (this.#textLayer?.div && this.#boundTextLayerPointerDown) {
this.#textLayer.div.removeEventListener(
"pointerdown",
Expand Down
18 changes: 17 additions & 1 deletion src/display/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class AnnotationEditor {

#altText = null;

#disabled = false;

#keepAspectRatio = false;

#resizersDiv = null;
Expand Down Expand Up @@ -1002,7 +1004,7 @@ class AnnotationEditor {
this.div.setAttribute("data-editor-rotation", (360 - this.rotation) % 360);
this.div.className = this.name;
this.div.setAttribute("id", this.id);
this.div.setAttribute("tabIndex", 0);
this.div.tabIndex = this.#disabled ? -1 : 0;
if (!this._isVisible) {
this.div.classList.add("hidden");
}
Expand Down Expand Up @@ -1680,6 +1682,20 @@ class AnnotationEditor {
this.div.classList.toggle("hidden", !visible);
this._isVisible = visible;
}

enable() {
if (this.div) {
this.div.tabIndex = 0;
}
this.#disabled = false;
}

disable() {
if (this.div) {
this.div.tabIndex = -1;
}
this.#disabled = true;
}
}

// This class is used to fake an editor which has been deleted.
Expand Down
6 changes: 6 additions & 0 deletions src/display/editor/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -1596,6 +1596,9 @@ class AnnotationEditorUIManager {
for (const layer of this.#allLayers.values()) {
layer.enable();
}
for (const editor of this.#allEditors.values()) {
editor.enable();
}
}
}

Expand All @@ -1609,6 +1612,9 @@ class AnnotationEditorUIManager {
for (const layer of this.#allLayers.values()) {
layer.disable();
}
for (const editor of this.#allEditors.values()) {
editor.disable();
}
}
}

Expand Down
10 changes: 8 additions & 2 deletions test/integration/freetext_editor_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
closePages,
createPromise,
dragAndDropAnnotation,
firstPageOnTop,
getEditors,
getEditorSelector,
getFirstSerialized,
Expand All @@ -39,6 +40,7 @@ import {
kbUndo,
loadAndWait,
scrollIntoView,
waitForAnnotationEditorLayer,
waitForEvent,
waitForSelectedEditor,
waitForSerialized,
Expand Down Expand Up @@ -923,6 +925,7 @@ describe("FreeText Editor", () => {
let currentId = 0;

for (let step = 0; step < 3; step++) {
await firstPageOnTop(page);
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.
Expand All @@ -931,8 +934,8 @@ describe("FreeText Editor", () => {
});

const data = `Hello ${step}`;
const x = rect.x + 0.1 * rect.width;
const y = rect.y + 0.1 * rect.height;
const x = Math.max(rect.x + 0.1 * rect.width, 10);
const y = Math.max(rect.y + 0.1 * rect.height, 10);
await page.mouse.click(x, y);
await page.waitForSelector(getEditorSelector(currentId), {
visible: true,
Expand All @@ -945,9 +948,12 @@ describe("FreeText Editor", () => {
`${getEditorSelector(currentId)} .overlay.enabled`
);

const promise = await waitForAnnotationEditorLayer(page);
await page.evaluate(() => {
document.getElementById("pageRotateCw").click();
});
await awaitPromise(promise);

currentId += 1;
await page.waitForSelector(
".page[data-page-number='1'] .canvasWrapper",
Expand Down
50 changes: 50 additions & 0 deletions test/integration/highlight_editor_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
getSerialized,
kbBigMoveLeft,
kbBigMoveUp,
kbFocusNext,
kbFocusPrevious,
kbSelectAll,
loadAndWait,
scrollIntoView,
Expand Down Expand Up @@ -1556,4 +1558,52 @@ describe("Highlight Editor", () => {
);
});
});

describe("Text layer must have the focus before highlights", () => {
let pages;

beforeAll(async () => {
pages = await loadAndWait("tracemonkey.pdf", ".annotationEditorLayer");
});

afterAll(async () => {
await closePages(pages);
});

it("must check the focus order", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
await page.click("#editorHighlight");
await page.waitForSelector(".annotationEditorLayer.highlightEditing");

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, delay: 100 });
await page.waitForSelector(getEditorSelector(0));

rect = await getSpanRectFromText(page, 1, "Languages");
x = rect.x + rect.width / 2;
y = rect.y + rect.height / 2;
await page.mouse.click(x, y, { count: 2, delay: 100 });
await page.waitForSelector(getEditorSelector(1));
await page.focus(getEditorSelector(1));

await kbFocusPrevious(page);
await page.waitForFunction(
sel => document.querySelector(sel) === document.activeElement,
{},
`.page[data-page-number="1"] > .textLayer`
);

await kbFocusNext(page);
await page.waitForFunction(
sel => document.querySelector(sel) === document.activeElement,
{},
getEditorSelector(1)
);
})
);
});
});
});
45 changes: 42 additions & 3 deletions test/integration/test_utils.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,12 @@ async function scrollIntoView(page, selector) {
const handle = await page.evaluateHandle(
sel => [
new Promise(resolve => {
document
.getElementById("viewerContainer")
.addEventListener("scrollend", resolve, { once: true });
const container = document.getElementById("viewerContainer");
if (container.scrollHeight <= container.clientHeight) {
resolve();
return;
}
container.addEventListener("scrollend", resolve, { once: true });
const element = document.querySelector(sel);
element.scrollIntoView({ behavior: "instant", block: "start" });
}),
Expand All @@ -330,6 +333,21 @@ async function scrollIntoView(page, selector) {
return awaitPromise(handle);
}

async function firstPageOnTop(page) {
const handle = await page.evaluateHandle(() => [
new Promise(resolve => {
const container = document.getElementById("viewerContainer");
if (container.scrollTop === 0 && container.scrollLeft === 0) {
resolve();
return;
}
container.addEventListener("scrollend", resolve, { once: true });
container.scrollTo(0, 0);
}),
]);
return awaitPromise(handle);
}

async function hover(page, selector) {
const rect = await page.$eval(selector, el => {
const { x, y, width, height } = el.getBoundingClientRect();
Expand Down Expand Up @@ -461,12 +479,31 @@ async function kbDeleteLastWord(page) {
}
}

async function kbFocusNext(page) {
const handle = await createPromise(page, resolve => {
window.addEventListener("focusin", resolve, { once: true });
});
await page.keyboard.press("Tab");
await awaitPromise(handle);
}

async function kbFocusPrevious(page) {
const handle = await createPromise(page, resolve => {
window.addEventListener("focusin", resolve, { once: true });
});
await page.keyboard.down("Shift");
await page.keyboard.press("Tab");
await page.keyboard.up("Shift");
await awaitPromise(handle);
}

export {
awaitPromise,
clearInput,
closePages,
createPromise,
dragAndDropAnnotation,
firstPageOnTop,
getAnnotationStorage,
getComputedStyleSelector,
getEditorDimensions,
Expand All @@ -484,6 +521,8 @@ export {
kbBigMoveUp,
kbCopy,
kbDeleteLastWord,
kbFocusNext,
kbFocusPrevious,
kbGoToBegin,
kbGoToEnd,
kbModifierDown,
Expand Down
1 change: 0 additions & 1 deletion web/annotation_editor_layer_builder.css
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@
font-size: calc(100px * var(--scale-factor));
transform-origin: 0 0;
cursor: auto;
z-index: 4;
}

.annotationEditorLayer.waiting {
Expand Down
10 changes: 5 additions & 5 deletions web/annotation_editor_layer_builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,22 @@ import { GenericL10n } from "web-null_l10n";
/**
* @typedef {Object} AnnotationEditorLayerBuilderOptions
* @property {AnnotationEditorUIManager} [uiManager]
* @property {HTMLDivElement} pageDiv
* @property {PDFPageProxy} pdfPage
* @property {IL10n} [l10n]
* @property {TextAccessibilityManager} [accessibilityManager]
* @property {AnnotationLayer} [annotationLayer]
* @property {TextLayer} [textLayer]
* @property {DrawLayer} [drawLayer]
* @property {function} [onAppend]
*/

class AnnotationEditorLayerBuilder {
#annotationLayer = null;

#drawLayer = null;

#onAppend = null;

#textLayer = null;

#uiManager;
Expand All @@ -52,7 +54,6 @@ class AnnotationEditorLayerBuilder {
* @param {AnnotationEditorLayerBuilderOptions} options
*/
constructor(options) {
this.pageDiv = options.pageDiv;
calixteman marked this conversation as resolved.
Show resolved Hide resolved
calixteman marked this conversation as resolved.
Show resolved Hide resolved
this.pdfPage = options.pdfPage;
this.accessibilityManager = options.accessibilityManager;
this.l10n = options.l10n;
Expand All @@ -66,6 +67,7 @@ class AnnotationEditorLayerBuilder {
this.#annotationLayer = options.annotationLayer || null;
this.#textLayer = options.textLayer || null;
this.#drawLayer = options.drawLayer || null;
this.#onAppend = options.onAppend || null;
}

/**
Expand All @@ -91,10 +93,9 @@ class AnnotationEditorLayerBuilder {
// Create an AnnotationEditor layer div
const div = (this.div = document.createElement("div"));
div.className = "annotationEditorLayer";
div.tabIndex = 0;
div.hidden = true;
div.dir = this.#uiManager.direction;
this.pageDiv.append(div);
this.#onAppend?.(div);

this.annotationEditorLayer = new AnnotationEditorLayer({
uiManager: this.#uiManager,
Expand Down Expand Up @@ -125,7 +126,6 @@ class AnnotationEditorLayerBuilder {
if (!this.div) {
return;
}
this.pageDiv = null;
this.annotationEditorLayer.destroy();
this.div.remove();
}
Expand Down
1 change: 0 additions & 1 deletion web/annotation_layer_builder.css
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@
left: 0;
pointer-events: none;
transform-origin: 0 0;
z-index: 3;

&[data-main-rotation="90"] .norotate {
transform: rotate(270deg) translateX(-100%);
Expand Down
Loading