From 64a0e59662148ff8bf95c79a22930745743341db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Mon, 22 Jul 2024 15:02:52 +0200 Subject: [PATCH] Disable link annotations during text selection When selecting text, hovering over an element causes all the text between (according the the dom order) the current selection and that element to be selected. This means that when, while selecting, the cursor moves over a link, all the text in the page gets selected. Setting `user-select: none` on the link annotations would improve the situation, but it still makes it impossible to extend the selection within a link without using Shift+arrows keys on the keyboard. This commit fixes the problem by setting `pointer-events: none` on the `
`s in the annotation layer while selecting some text. This way, they are ignored for hit-testing and do not affect selection. It is still impossible to _start_ a selection inside a link, as the link text is covered by the link annotation. Fixes #18266 --- test/integration/test_utils.mjs | 2 +- test/integration/text_layer_spec.mjs | 340 ++++++++++++++++++++------- web/annotation_layer_builder.css | 4 + web/text_layer_builder.css | 6 +- web/text_layer_builder.js | 34 ++- 5 files changed, 287 insertions(+), 99 deletions(-) diff --git a/test/integration/test_utils.mjs b/test/integration/test_utils.mjs index 207007fa41ef4..be2bbbcb54007 100644 --- a/test/integration/test_utils.mjs +++ b/test/integration/test_utils.mjs @@ -173,7 +173,7 @@ async function getSpanRectFromText(page, pageNumber, text) { return page.evaluate( (number, content) => { for (const el of document.querySelectorAll( - `.page[data-page-number="${number}"] > .textLayer > span` + `.page[data-page-number="${number}"] > .textLayer span:not(:has(> span))` )) { if (el.textContent === content) { const { x, y, width, height } = el.getBoundingClientRect(); diff --git a/test/integration/text_layer_spec.mjs b/test/integration/text_layer_spec.mjs index 83779da82f9ff..13b796efd5a14 100644 --- a/test/integration/text_layer_spec.mjs +++ b/test/integration/text_layer_spec.mjs @@ -14,6 +14,7 @@ */ import { + awaitPromise, closePages, closeSinglePage, getSpanRectFromText, @@ -98,111 +99,268 @@ describe("Text layer", () => { }); describe("using mouse", () => { - let pages; + describe("doesn't jump when hovering on an empty area", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait( + "tracemonkey.pdf", + `.page[data-page-number = "1"] .endOfContent` + ); + }); + afterAll(async () => { + await closePages(pages); + }); - beforeAll(async () => { - pages = await loadAndWait( - "tracemonkey.pdf", - `.page[data-page-number = "1"] .endOfContent` - ); - }); - afterAll(async () => { - await closePages(pages); - }); + it("in a single page", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const [positionStart, positionEnd] = await Promise.all([ + getSpanRectFromText( + page, + 1, + "(frequently executed) bytecode sequences, records" + ).then(middlePosition), + getSpanRectFromText( + page, + 1, + "them, and compiles them to fast native code. We call such a se-" + ).then(belowEndPosition), + ]); + + await page.mouse.move(positionStart.x, positionStart.y); + await page.mouse.down(); + await moveInSteps(page, positionStart, positionEnd, 20); + await page.mouse.up(); + + await expectAsync(page) + .withContext(`In ${browserName}`) + .toHaveRoughlySelected( + "code sequences, records\n" + + "them, and compiles them to fast native code. We call suc" + ); + }) + ); + }); - it("doesn't jump when hovering on an empty area", async () => { - await Promise.all( - pages.map(async ([browserName, page]) => { - const [positionStart, positionEnd] = await Promise.all([ - getSpanRectFromText( + it("across multiple pages", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const scrollTarget = await getSpanRectFromText( page, 1, - "(frequently executed) bytecode sequences, records" - ).then(middlePosition), - getSpanRectFromText( - page, - 1, - "them, and compiles them to fast native code. We call such a se-" - ).then(belowEndPosition), - ]); - - await page.mouse.move(positionStart.x, positionStart.y); - await page.mouse.down(); - await moveInSteps(page, positionStart, positionEnd, 20); - await page.mouse.up(); - - await expectAsync(page) - .withContext(`In ${browserName}`) - .toHaveRoughlySelected( - "code sequences, records\n" + - "them, and compiles them to fast native code. We call suc" + "Unlike method-based dynamic compilers, our dynamic com-" ); - }) - ); + await page.evaluate(top => { + document.getElementById("viewerContainer").scrollTop = top; + }, scrollTarget.y - 50); + + const [ + positionStartPage1, + positionEndPage1, + positionStartPage2, + positionEndPage2, + ] = await Promise.all([ + getSpanRectFromText( + page, + 1, + "Each compiled trace covers one path through the program with" + ).then(middlePosition), + getSpanRectFromText( + page, + 1, + "or that the same types will occur in subsequent loop iterations." + ).then(middlePosition), + getSpanRectFromText( + page, + 2, + "Hence, recording and compiling a trace" + ).then(middlePosition), + getSpanRectFromText( + page, + 2, + "cache. Alternatively, the VM could simply stop tracing, and give up" + ).then(belowEndPosition), + ]); + + await page.mouse.move(positionStartPage1.x, positionStartPage1.y); + await page.mouse.down(); + + await moveInSteps(page, positionStartPage1, positionEndPage1, 20); + await moveInSteps(page, positionEndPage1, positionStartPage2, 20); + + await expectAsync(page) + .withContext(`In ${browserName}, first selection`) + .toHaveRoughlySelected( + /path through the program .*Hence, recording a/s + ); + + await moveInSteps(page, positionStartPage2, positionEndPage2, 20); + await page.mouse.up(); + + await expectAsync(page) + .withContext(`In ${browserName}, second selection`) + .toHaveRoughlySelected( + /path through.*Hence, recording and .* tracing, and give/s + ); + }) + ); + }); }); - it("doesn't jump when hovering on an empty area (multi-page)", async () => { - await Promise.all( - pages.map(async ([browserName, page]) => { - const scrollTarget = await getSpanRectFromText( - page, - 1, - "Unlike method-based dynamic compilers, our dynamic com-" - ); - await page.evaluate(top => { - document.getElementById("viewerContainer").scrollTop = top; - }, scrollTarget.y - 50); - - const [ - positionStartPage1, - positionEndPage1, - positionStartPage2, - positionEndPage2, - ] = await Promise.all([ - getSpanRectFromText( - page, - 1, - "Each compiled trace covers one path through the program with" - ).then(middlePosition), - getSpanRectFromText( - page, - 1, - "or that the same types will occur in subsequent loop iterations." - ).then(middlePosition), - getSpanRectFromText( - page, - 2, - "Hence, recording and compiling a trace" - ).then(middlePosition), - getSpanRectFromText( - page, - 2, - "cache. Alternatively, the VM could simply stop tracing, and give up" - ).then(belowEndPosition), - ]); + describe("when selecting over a link", () => { + let pages; + + beforeAll(async () => { + pages = await loadAndWait( + "annotation-link-text-popup.pdf", + `.page[data-page-number = "1"] .endOfContent` + ); + }); + afterAll(async () => { + await closePages(pages); + }); + afterEach(() => + Promise.all( + pages.map(([_, page]) => + page.evaluate(() => window.getSelection().removeAllRanges()) + ) + ) + ); - await page.mouse.move(positionStartPage1.x, positionStartPage1.y); - await page.mouse.down(); + function waitForClick(page, selector, timeout) { + return page.evaluateHandle( + (sel, timeoutDelay) => { + const element = document.querySelector(sel); + const timeoutSignal = AbortSignal.timeout(timeoutDelay); + return [ + new Promise(resolve => { + timeoutSignal.addEventListener( + "abort", + () => resolve(false), + { once: true } + ); + element.addEventListener( + "click", + e => { + e.preventDefault(); + resolve(true); + }, + { once: true, signal: timeoutSignal } + ); + }), + ]; + }, + selector, + timeout + ); + } + + it("allows selecting within the link", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const [positionStart, positionEnd] = await Promise.all([ + getSpanRectFromText(page, 1, "Link").then(middleLeftPosition), + getSpanRectFromText(page, 1, "mozilla.org").then( + middlePosition + ), + ]); + + await page.mouse.move(positionStart.x, positionStart.y); + await page.mouse.down(); + await moveInSteps(page, positionStart, positionEnd, 20); + await page.mouse.up(); + + await expectAsync(page) + .withContext(`In ${browserName}`) + .toHaveRoughlySelected("Link\nmozil"); + }) + ); + }); - await moveInSteps(page, positionStartPage1, positionEndPage1, 20); - await moveInSteps(page, positionEndPage1, positionStartPage2, 20); + it("allows selecting within the link when going backwards", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const [positionStart, positionEnd] = await Promise.all([ + getSpanRectFromText(page, 1, "Text").then(middlePosition), + getSpanRectFromText(page, 1, "mozilla.org").then( + middlePosition + ), + ]); + + await page.mouse.move(positionStart.x, positionStart.y); + await page.mouse.down(); + await moveInSteps(page, positionStart, positionEnd, 20); + await page.mouse.up(); + + await expectAsync(page) + .withContext(`In ${browserName}`) + .toHaveRoughlySelected("a.org\nTe"); + }) + ); + }); - await expectAsync(page) - .withContext(`In ${browserName}, first selection`) - .toHaveRoughlySelected( - /path through the program .*Hence, recording a/s + it("allows clicking the link after selecting", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const [positionStart, positionEnd] = await Promise.all([ + getSpanRectFromText(page, 1, "Link").then(middleLeftPosition), + getSpanRectFromText(page, 1, "mozilla.org").then( + middlePosition + ), + ]); + + await page.mouse.move(positionStart.x, positionStart.y); + await page.mouse.down(); + await moveInSteps(page, positionStart, positionEnd, 20); + await page.mouse.up(); + + const clickPromiseHandle = await waitForClick( + page, + "#pdfjs_internal_id_8R", + 1000 ); - await moveInSteps(page, positionStartPage2, positionEndPage2, 20); - await page.mouse.up(); + await page.mouse.click(positionEnd.x, positionEnd.y); + + const clicked = await awaitPromise(clickPromiseHandle); + expect(clicked).toBeTrue(); + }) + ); + }); - await expectAsync(page) - .withContext(`In ${browserName}, second selection`) - .toHaveRoughlySelected( - /path through.*Hence, recording and .* tracing, and give/s + it("allows clicking the link after changing selection with the keyboard", async () => { + await Promise.all( + pages.map(async ([browserName, page]) => { + const [positionStart, positionEnd] = await Promise.all([ + getSpanRectFromText(page, 1, "Link").then(middleLeftPosition), + getSpanRectFromText(page, 1, "mozilla.org").then( + middlePosition + ), + ]); + + await page.mouse.move(positionStart.x, positionStart.y); + await page.mouse.down(); + await moveInSteps(page, positionStart, positionEnd, 20); + await page.mouse.up(); + + await page.keyboard.down("Shift"); + await page.keyboard.press("ArrowRight"); + await page.keyboard.up("Shift"); + + const clickPromiseHandle = await waitForClick( + page, + "#pdfjs_internal_id_8R", + 1000 ); - }) - ); + + await page.mouse.click(positionEnd.x, positionEnd.y); + + const clicked = await awaitPromise(clickPromiseHandle); + expect(clicked).toBeTrue(); + }) + ); + }); }); }); diff --git a/web/annotation_layer_builder.css b/web/annotation_layer_builder.css index 56c71887d8fb1..3047adbb2ecfd 100644 --- a/web/annotation_layer_builder.css +++ b/web/annotation_layer_builder.css @@ -126,6 +126,10 @@ } } + .textLayer.selecting ~ & section { + pointer-events: none; + } + :is(.linkAnnotation, .buttonWidgetAnnotation.pushButton) > a { position: absolute; font-size: 1em; diff --git a/web/text_layer_builder.css b/web/text_layer_builder.css index 0937130c9a974..841b47be7057a 100644 --- a/web/text_layer_builder.css +++ b/web/text_layer_builder.css @@ -118,9 +118,9 @@ z-index: 0; cursor: default; user-select: none; + } - &.active { - top: 0; - } + &.selecting .endOfContent { + top: 0; } } diff --git a/web/text_layer_builder.js b/web/text_layer_builder.js index 1ff52c2893b3a..62ce1f2f057f5 100644 --- a/web/text_layer_builder.js +++ b/web/text_layer_builder.js @@ -150,8 +150,8 @@ class TextLayerBuilder { #bindMouse(end) { const { div } = this; - div.addEventListener("mousedown", evt => { - end.classList.add("active"); + div.addEventListener("mousedown", () => { + div.classList.add("selecting"); }); div.addEventListener("copy", event => { @@ -193,16 +193,42 @@ class TextLayerBuilder { end.style.width = ""; end.style.height = ""; } - end.classList.remove("active"); + textLayer.classList.remove("selecting"); }; + let isPointerDown = false; + document.addEventListener( + "pointerdown", + () => { + isPointerDown = true; + }, + { signal } + ); document.addEventListener( "pointerup", () => { + isPointerDown = false; this.#textLayers.forEach(reset); }, { signal } ); + window.addEventListener( + "blur", + () => { + isPointerDown = false; + this.#textLayers.forEach(reset); + }, + { signal } + ); + document.addEventListener( + "keyup", + () => { + if (!isPointerDown) { + this.#textLayers.forEach(reset); + } + }, + { signal } + ); if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) { // eslint-disable-next-line no-var @@ -237,7 +263,7 @@ class TextLayerBuilder { for (const [textLayerDiv, endDiv] of this.#textLayers) { if (activeTextLayers.has(textLayerDiv)) { - endDiv.classList.add("active"); + textLayerDiv.classList.add("selecting"); } else { reset(endDiv, textLayerDiv); }