Skip to content

Move editToolbar and fix overlapping annotations with it #21723

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.

Sign up for GitHub

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

Open
LoukasPap wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move editToolbar and fix overlapping annotations with it #21723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion src/display/editor/tools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1479,8 +1479,12 @@ class AnnotationEditorUIManager {
if (!boxes) {
return;
}
const parent = textLayer.parentElement;
if (!parent) {
return;
}
this.#floatingToolbar ||= new FloatingToolbar(this);
this.#floatingToolbar.show(textLayer, boxes, this.direction === "ltr");
this.#floatingToolbar.show(parent, boxes, this.direction === "ltr");
}

/**
Expand Down
76 changes: 14 additions & 62 deletions test/integration/freetext_editor_spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ import {
awaitPromise,
clearEditors,
closePages,
commitFreeTextEditor,
copy,
copyToClipboard,
countSerialized,
countStorageEntries,
createFreeTextEditor,
createPromise,
decodePNG,
dragAndDrop,
Expand All @@ -29,7 +29,6 @@ import {
getEditors,
getEditorSelector,
getFirstSerialized,
getNextEditorId,
getRect,
getSerialized,
isCanvasMonochrome,
Expand Down Expand Up @@ -68,55 +67,8 @@ const selectAll = selectEditors.bind(null, "freeText");

const clearAll = clearEditors.bind(null, "freeText");

const commit = async page => {
await page.keyboard.press("Escape");
await page.waitForSelector(".freeTextEditor.selectedEditor .overlay.enabled");
};

const switchToFreeText = switchToEditor.bind(null, "FreeText");

const cancelFocusIn = async (page, selector) => {
page.evaluate(sel => {
const el = document.querySelector(sel);
el.addEventListener(
"focusin",
evt => {
evt.preventDefault();
evt.stopPropagation();
},
{ capture: true, once: true }
);
}, selector);
};

const createFreeTextEditor = async ({
page,
x,
y,
data = null,
noFocusIn = false,
}) => {
const editorSelector = getEditorSelector(await getNextEditorId(page));
const serializedCount = await countSerialized(page);
const storageEntriesCount = await countStorageEntries(page);

await page.mouse.click(x, y);
await page.waitForSelector(editorSelector, { visible: true });
if (data) {
await page.type(`${editorSelector} .internal`, data);
}
if (noFocusIn) {
await cancelFocusIn(page, editorSelector);
}
await commit(page);

await waitForSelectedEditor(page, editorSelector);
await waitForStorageEntries(page, storageEntriesCount + 1);
await waitForSerialized(page, serializedCount + 1);

return editorSelector;
};

describe("FreeText Editor", () => {
describe("FreeText", () => {
let pages;
Expand Down Expand Up @@ -157,7 +109,7 @@ describe("FreeText Editor", () => {
await page.waitForSelector(
`${editorSelector} .overlay:not(.enabled)`
);
await commit(page);
await commitFreeTextEditor(page);

content = await page.$eval(editorSelector, el =>
el.innerText.trimEnd()
Expand Down Expand Up @@ -395,7 +347,7 @@ describe("FreeText Editor", () => {

for (let i = 0; i < 5; i++) {
await page.type(`${editorSelector} .internal`, "A");
await commit(page);
await commitFreeTextEditor(page);

if (i < 4) {
await selectEditor(page, editorSelector, /* count = */ 2);
Expand Down Expand Up @@ -459,7 +411,7 @@ describe("FreeText Editor", () => {
// Add a new A.
await selectEditor(page, editorSelector, /* count = */ 2);
await page.type(`${editorSelector} .internal`, "A");
await commit(page);
await commitFreeTextEditor(page);

text = await getText();
expect(text).withContext(`In ${browserName}`).toEqual("AA");
Expand Down Expand Up @@ -968,7 +920,7 @@ describe("FreeText Editor", () => {
`${editorSelector} .internal`,
" and edited in Firefox"
);
await commit(page);
await commitFreeTextEditor(page);

const serialized = await getSerialized(page);
expect(serialized.length).withContext(`In ${browserName}`).toEqual(1);
Expand Down Expand Up @@ -1063,7 +1015,7 @@ describe("FreeText Editor", () => {
`${editorSelector} .internal`,
" and edited in Firefox"
);
await commit(page);
await commitFreeTextEditor(page);

// Disable editing mode.
await switchToFreeText(page, /* disable = */ true);
Expand Down Expand Up @@ -2113,7 +2065,7 @@ describe("FreeText Editor", () => {
await moveEditor(page, editorSelector, 2, () => kbBigMoveLeft(page));

await page.type(`${editorSelector} .internal`, data);
await commit(page);
await commitFreeTextEditor(page);

const [newX, newY] = await getFirstSerialized(page, x => x.rect);
expect(Math.round(newX))
Expand Down Expand Up @@ -2616,7 +2568,7 @@ describe("FreeText Editor", () => {

const data = "Hello PDF.js World !!";
await page.type(`${editorSelector} .internal`, data);
await commit(page);
await commitFreeTextEditor(page);

let content = await page.$eval(editorSelector, el =>
el.innerText.trimEnd()
Expand All @@ -2635,7 +2587,7 @@ describe("FreeText Editor", () => {
await moveEditor(page, editorSelector, 5, () => kbBigMoveDown(page));

await page.type(`${editorSelector} .internal`, data);
await commit(page);
await commitFreeTextEditor(page);

// Unselect.
await unselectEditor(page, editorSelector);
Expand Down Expand Up @@ -2663,7 +2615,7 @@ describe("FreeText Editor", () => {

const data = "Hello PDF.js World !!";
await page.type(`${editorSelector} .internal`, data);
await commit(page);
await commitFreeTextEditor(page);

// Unselect.
await unselectEditor(page, editorSelector);
Expand All @@ -2683,7 +2635,7 @@ describe("FreeText Editor", () => {
);

await page.type(`${editorSelector} .internal`, data);
await commit(page);
await commitFreeTextEditor(page);

// Unselect.
await unselectEditor(page, editorSelector);
Expand Down Expand Up @@ -3244,7 +3196,7 @@ describe("FreeText Editor", () => {
const html = await getHTML();
expect(html).withContext(`In ${browserName}`).toEqual(prevHTML);

await commit(page);
await commitFreeTextEditor(page);

editorSelector = getEditorSelector(1);
await page.mouse.click(rect.x + 200, rect.y + 200);
Expand Down Expand Up @@ -3406,7 +3358,7 @@ describe("FreeText Editor", () => {
await page.keyboard.press("Enter");
await page.keyboard.up("Shift");
}
await commit(page);
await commitFreeTextEditor(page);
await waitForSerialized(page, 1);

const serialized = await getSerialized(page, x => x.value);
Expand Down
140 changes: 135 additions & 5 deletions test/integration/highlight_editor_spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import {
awaitPromise,
closePages,
createFreeTextEditor,
FSI,
getAnnotationSelector,
getEditorSelector,
Expand Down Expand Up @@ -56,6 +57,8 @@ const selectAll = selectEditors.bind(null, "highlight");

const switchToHighlight = switchToEditor.bind(null, "Highlight");

const switchToFreeText = switchToEditor.bind(null, "FreeText");

describe("Highlight Editor", () => {
describe("Editor must be removed without exception", () => {
let pages;
Expand Down Expand Up @@ -1414,8 +1417,8 @@ describe("Highlight Editor", () => {
const y = rect.y + rect.height / 2;
await page.mouse.click(x, y, { count: 2, delay: 100 });

await page.waitForSelector(".textLayer .highlightButton");
await page.click(".textLayer .highlightButton");
await page.waitForSelector(".highlightButton");
await page.click(".highlightButton");

await page.waitForSelector(getEditorSelector(editorId));
const usedColor = await page.evaluate(() => {
Expand Down Expand Up @@ -1470,7 +1473,7 @@ describe("Highlight Editor", () => {
delay: 100,
});

const toolbarRect = await getRect(page, ".textLayer .editToolbar");
const toolbarRect = await getRect(page, ".editToolbar");

// In RTL, the left edge of the toolbar is aligned on the left edge
// of the selection (bug 2060032).
Expand Down Expand Up @@ -1891,8 +1894,8 @@ describe("Highlight Editor", () => {
const y = rect.y + rect.height / 2;
await page.mouse.click(x, y, { count: 3, delay: 100 });

await page.waitForSelector(".textLayer .highlightButton");
await page.click(".textLayer .highlightButton");
await page.waitForSelector(".highlightButton");
await page.click(".highlightButton");

await page.waitForSelector(getEditorSelector(0));
const usedColor = await page.evaluate(() => {
Expand Down Expand Up @@ -2909,4 +2912,131 @@ describe("Highlight Editor", () => {
});
});
});

describe("editToolbar is rendering over annotations", () => {

@timvandermeij timvandermeij Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for this PR, but in a follow-up we might want to consider moving the floating toolbar tests into their own spec. It feels a bit odd conceptually to have it here because we don't create actual highlight annotations in these tests, nor is the floating toolbar only for highlight annotations as comments can also be created with it now. (It originally used to only be for highlighting, and the preference name is also called that, but I think that predates the commenting addition.)

@calixteman What do you think?

@LoukasPap LoukasPap Sep 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

let pages;

beforeEach(async () => {
pages = await loadAndWait(
"toolbar-overlap-with-annotations.pdf",
".annotationEditorLayer",
120
);
});

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

it("must check that the edit toolbar is rendered above link annotations", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
await page.waitForSelector(
`.page[data-page-number = "1"] .textLayer .endOfContent`
);

const linkSelector = `a[href="https://github.com/mozilla/pdf.js"]`;
const linkRect = await getRect(page, linkSelector);

const topElementId = await page.evaluate(
(px, py) => document.elementFromPoint(px, py)?.id || null,
linkRect.x,
linkRect.y
);
expect(topElementId)
.withContext(`In ${browserName}`)
.toEqual("pdfjs_internal_id_14R");

// Double click on "A:" to show the edit toolbar.
const firstRect = await getSpanRectFromText(page, 1, "A:");
await page.mouse.click(firstRect.x, firstRect.y, {
count: 2,
delay: 100,
});

const highlightButtonSelector = `.page[data-page-number = "1"] .editToolbar button.highlightButton`;
await page.waitForSelector(highlightButtonSelector, {
visible: true,
});

const topElementClass = await page.evaluate(
(px, py) => document.elementFromPoint(px, py)?.className || null,
linkRect.x,
linkRect.y
);
expect(topElementClass)
.withContext(`In ${browserName}`)
.toContain("highlightButton");
})
);
});

it("must check that the edit toolbar is rendered above text annotations", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
await switchToFreeText(page);

const myText = await getSpanRectFromText(page, 1, "My text");
// "CHECK" is roughly 50px wide at the default font size and
// is roughly aligned with the end of "My text".
const editorSelector = await createFreeTextEditor({
page,
x: myText.x + myText.width / 2,
y: myText.y + myText.height + 20,

@timvandermeij timvandermeij Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason to not just have myText.height / 2 here to center it, like in the test above?

@LoukasPap LoukasPap Sep 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because myText.height + 20 places the free-text below the selected text where the toolbar appears, in contrast to myText.height / 2 which renders it in the center of "My text".

About the test above ("must check that the edit toolbar is rendered above link annotations"), I corrected it to just check:

document.elementFromPoint(px, py)?.id || null,
            linkRect.x,
            linkRect.y
          );

because that's where I later check for the highlighButton.

data: "CHECK",
});

const freeTextRect = await getRect(page, editorSelector);
const freeTextTopElement = await page.evaluate(
(px, py) =>
document.elementFromPoint(px, py)?.getAttribute("data-l10n-id") ||
null,
freeTextRect.x + freeTextRect.width / 2,
freeTextRect.y + freeTextRect.height / 2
);
expect(freeTextTopElement)
.withContext(`In ${browserName}`)
.toEqual("pdfjs-free-text2");

// Close the text editor.
await switchToFreeText(page, /* disable */ true);

// Double click on "myText" to show the edit toolbar.
await page.mouse.click(
myText.x + myText.width / 2,
myText.y + myText.height / 2,
{ count: 2, delay: 100 }
);

const toolbarSelector = `.page[data-page-number = "1"] .editToolbar:has(button.highlightButton)`;
const highlightButtonSelector = `${toolbarSelector} button.highlightButton`;
await page.waitForSelector(highlightButtonSelector, {
visible: true,
});

let topElement = await page.evaluate(
(px, py) => document.elementFromPoint(px, py)?.className || null,
freeTextRect.x + freeTextRect.width / 2,
freeTextRect.y + freeTextRect.height / 2
);
expect(topElement)
.withContext(`In ${browserName}`)
.toContain("highlightButton");

// Re-open the text editor without dismissing the edit toolbar,
// so both are present on the page at the same time.
await switchToFreeText(page);

@timvandermeij timvandermeij Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried this in the browser and in the integration test itself, but only enabling free text mode doesn't cause a second toolbar to appear: the annotation actually has to be clicked. It therefore looks like the assertion below passes "by accident" because the highlight button state from the previous assertion didn't change?

@LoukasPap LoukasPap Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A second toolbar doesn't need to appear because the first one is already there.

When switching to free-text, the toolbar is already opened from double-clicking on "My Text". The test checks which element is rendered above with document.elementFromPoint, so having both the toolbar and the free-text in (almost) the same position, we assert that the highlight button (and thus the toolbar) is on top.


topElement = await page.evaluate(
(px, py) => document.elementFromPoint(px, py)?.className || null,
freeTextRect.x + freeTextRect.width / 2,
freeTextRect.y + freeTextRect.height / 2
);
expect(topElement)
.withContext(`In ${browserName}`)
.toContain("highlightButton");
})
);
});
});
});
Loading