Skip to content

Commit 225b07a

Browse files
authored
Merge pull request #20283 from calixteman/bug1989304
[Editor] Make sure the comment dialog is visible on the screen (bug 1989304)
2 parents 44affa7 + 623d422 commit 225b07a

8 files changed

Lines changed: 195 additions & 13 deletions

File tree

src/display/editor/annotation_editor_layer.js

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -193,11 +193,16 @@ class AnnotationEditorLayer {
193193

194194
this.toggleAnnotationLayerPointerEvents(false);
195195
const { classList } = this.div;
196-
for (const editorType of AnnotationEditorLayer.#editorTypes.values()) {
197-
classList.toggle(
198-
`${editorType._type}Editing`,
199-
mode === editorType._editorType
200-
);
196+
if (mode === AnnotationEditorType.POPUP) {
197+
classList.toggle("commentEditing", true);
198+
} else {
199+
classList.toggle("commentEditing", false);
200+
for (const editorType of AnnotationEditorLayer.#editorTypes.values()) {
201+
classList.toggle(
202+
`${editorType._type}Editing`,
203+
mode === editorType._editorType
204+
);
205+
}
201206
}
202207
this.div.hidden = false;
203208
}

test/integration/comment_spec.mjs

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
/* Copyright 2025 Mozilla Foundation
2+
*
3+
* Licensed under the Apache License, Version 2.0 (the "License");
4+
* you may not use this file except in compliance with the License.
5+
* You may obtain a copy of the License at
6+
*
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
*
9+
* Unless required by applicable law or agreed to in writing, software
10+
* distributed under the License is distributed on an "AS IS" BASIS,
11+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
* See the License for the specific language governing permissions and
13+
* limitations under the License.
14+
*/
15+
16+
import {
17+
closePages,
18+
getEditorSelector,
19+
getRect,
20+
getSpanRectFromText,
21+
loadAndWait,
22+
scrollIntoView,
23+
switchToEditor,
24+
waitAndClick,
25+
} from "./test_utils.mjs";
26+
27+
const switchToHighlight = switchToEditor.bind(null, "Highlight");
28+
29+
describe("Comment", () => {
30+
describe("Comment edit dialog must be visible in ltr", () => {
31+
let pages;
32+
33+
beforeEach(async () => {
34+
pages = await loadAndWait(
35+
"bug1989304.pdf",
36+
".annotationEditorLayer",
37+
"page-width",
38+
null,
39+
{ enableComment: true }
40+
);
41+
});
42+
43+
afterEach(async () => {
44+
await closePages(pages);
45+
});
46+
47+
it("must set the comment dialog in the viewport (LTR)", async () => {
48+
await Promise.all(
49+
pages.map(async ([browserName, page]) => {
50+
await switchToHighlight(page);
51+
52+
await scrollIntoView(page, ".textLayer span:last-of-type");
53+
const rect = await getSpanRectFromText(page, 1, "...");
54+
const x = rect.x + rect.width / 2;
55+
const y = rect.y + rect.height / 2;
56+
// Here and elsewhere, we add a small delay between press and release
57+
// to make sure that a pointerup event is triggered after
58+
// selectionchange.
59+
// It works with a value of 1ms, but we use 100ms to be sure.
60+
await page.mouse.click(x, y, { count: 2, delay: 100 });
61+
await page.waitForSelector(getEditorSelector(0));
62+
63+
const commentButtonSelector = `${getEditorSelector(0)} button.comment`;
64+
await waitAndClick(page, commentButtonSelector);
65+
66+
await page.waitForSelector("#commentManagerDialog", {
67+
visible: true,
68+
});
69+
const dialogRect = await getRect(page, "#commentManagerDialog");
70+
const viewport = await page.evaluate(() => ({
71+
width: document.documentElement.clientWidth,
72+
height: document.documentElement.clientHeight,
73+
}));
74+
expect(dialogRect.x + dialogRect.width)
75+
.withContext(`In ${browserName}`)
76+
.toBeLessThanOrEqual(viewport.width);
77+
expect(dialogRect.y + dialogRect.height)
78+
.withContext(`In ${browserName}`)
79+
.toBeLessThanOrEqual(viewport.height);
80+
})
81+
);
82+
});
83+
});
84+
85+
describe("Comment edit dialog must be visible in rtl", () => {
86+
let pages;
87+
88+
beforeEach(async () => {
89+
pages = await loadAndWait(
90+
"bug1989304.pdf",
91+
".annotationEditorLayer",
92+
"page-width",
93+
null,
94+
{ enableComment: true, localeProperties: "ar" }
95+
);
96+
});
97+
98+
afterEach(async () => {
99+
await closePages(pages);
100+
});
101+
102+
it("must set the comment dialog in the viewport (RTL)", async () => {
103+
await Promise.all(
104+
pages.map(async ([browserName, page]) => {
105+
await switchToHighlight(page);
106+
107+
await scrollIntoView(page, ".textLayer span:nth-of-type(4)");
108+
const rect = await getSpanRectFromText(page, 1, "World");
109+
const x = rect.x + rect.width / 2;
110+
const y = rect.y + rect.height / 2;
111+
await page.mouse.click(x, y, { count: 2, delay: 100 });
112+
await page.waitForSelector(getEditorSelector(0));
113+
114+
const commentButtonSelector = `${getEditorSelector(0)} button.comment`;
115+
await waitAndClick(page, commentButtonSelector);
116+
117+
await page.waitForSelector("#commentManagerDialog", {
118+
visible: true,
119+
});
120+
const dialogRect = await getRect(page, "#commentManagerDialog");
121+
const viewport = await page.evaluate(() => ({
122+
height: document.documentElement.clientHeight,
123+
}));
124+
expect(dialogRect.x + dialogRect.width)
125+
.withContext(`In ${browserName}`)
126+
.toBeGreaterThanOrEqual(0);
127+
expect(dialogRect.y + dialogRect.height)
128+
.withContext(`In ${browserName}`)
129+
.toBeLessThanOrEqual(viewport.height);
130+
})
131+
);
132+
});
133+
});
134+
});

test/integration/jasmine-boot.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ async function runTests(results) {
3030
"annotation_spec.mjs",
3131
"autolinker_spec.mjs",
3232
"caret_browsing_spec.mjs",
33+
"comment_spec.mjs",
3334
"copy_paste_spec.mjs",
3435
"document_properties_spec.mjs",
3536
"find_spec.mjs",

test/pdfs/.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -744,3 +744,4 @@
744744
!bug1980958.pdf
745745
!tracemonkey_annotation_on_page_8.pdf
746746
!issue20232.pdf
747+
!bug1989304.pdf

test/pdfs/bug1989304.pdf

10.6 KB
Binary file not shown.

web/app.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,7 @@ const PDFViewerApplication = {
368368
docBaseUrl: x => x,
369369
enableAltText: x => x === "true",
370370
enableAutoLinking: x => x === "true",
371+
enableComment: x => x === "true",
371372
enableFakeMLManager: x => x === "true",
372373
enableGuessAltText: x => x === "true",
373374
enablePermissions: x => x === "true",
@@ -380,6 +381,7 @@ const PDFViewerApplication = {
380381
forcePageColors: x => x === "true",
381382
pageColorsBackground: x => x,
382383
pageColorsForeground: x => x,
384+
localeProperties: x => ({ lang: x }),
383385
});
384386
}
385387

web/comment_manager.css

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@
2020
min-width: 200px;
2121
position: absolute;
2222
padding: 8px 16px 16px;
23-
margin: 0;
23+
margin-left: 0;
24+
margin-top: 0;
2425
box-sizing: border-box;
2526

2627
border-radius: 8px;

web/comment_manager.js

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class CommentManager {
4646
dateStyle: "long",
4747
});
4848
this.dialogElement = commentDialog.dialog;
49-
this.#dialog = new CommentDialog(commentDialog, overlayManager);
49+
this.#dialog = new CommentDialog(commentDialog, overlayManager, ltr);
5050
this.#popup = new CommentPopup(dateFormat, ltr, this.dialogElement);
5151
this.#sidebar = new CommentSidebar(
5252
sidebar,
@@ -572,15 +572,19 @@ class CommentDialog {
572572

573573
#dialogY = 0;
574574

575+
#isLTR;
576+
575577
constructor(
576578
{ dialog, toolbar, title, textInput, cancelButton, saveButton },
577-
overlayManager
579+
overlayManager,
580+
ltr
578581
) {
579582
this.#dialog = dialog;
580583
this.#textInput = textInput;
581584
this.#overlayManager = overlayManager;
582585
this.#saveButton = saveButton;
583586
this.#title = title;
587+
this.#isLTR = ltr;
584588

585589
const finishBound = this.#finish.bind(this);
586590
dialog.addEventListener("close", finishBound);
@@ -682,7 +686,38 @@ class CommentDialog {
682686
}
683687
this.#uiManager?.removeEditListeners();
684688
this.#saveButton.disabled = true;
685-
689+
const parentDimensions = options?.parentDimensions;
690+
if (editor.hasDefaultPopupPosition()) {
691+
const { dialogWidth, dialogHeight } = this._dialogDimensions;
692+
if (parentDimensions) {
693+
if (
694+
this.#isLTR &&
695+
posX + dialogWidth >
696+
Math.min(
697+
parentDimensions.x + parentDimensions.width,
698+
window.innerWidth
699+
)
700+
) {
701+
const buttonWidth = this.#editor.commentButtonWidth;
702+
posX -= dialogWidth - buttonWidth * parentDimensions.width;
703+
} else if (!this.#isLTR) {
704+
const buttonWidth =
705+
this.#editor.commentButtonWidth * parentDimensions.width;
706+
if (posX - dialogWidth < Math.max(0, parentDimensions.x)) {
707+
posX = Math.max(0, posX);
708+
} else {
709+
posX -= dialogWidth - buttonWidth;
710+
}
711+
}
712+
}
713+
const height = Math.max(dialogHeight, options?.height || 0);
714+
if (posY + height > window.innerHeight) {
715+
posY = window.innerHeight - height;
716+
}
717+
if (posY < 0) {
718+
posY = 0;
719+
}
720+
}
686721
this.#setPosition(posX, posY);
687722

688723
await this.#overlayManager.open(this.#dialog);
@@ -694,14 +729,17 @@ class CommentDialog {
694729
this.#finish();
695730
}
696731

697-
get _dialogWidth() {
732+
get _dialogDimensions() {
698733
const dialog = this.#dialog;
699734
const { style } = dialog;
700735
style.opacity = "0";
701736
style.display = "block";
702-
const width = dialog.getBoundingClientRect().width;
737+
const { width, height } = dialog.getBoundingClientRect();
703738
style.opacity = style.display = "";
704-
return shadow(this, "_dialogWidth", width);
739+
return shadow(this, "_dialogDimensions", {
740+
dialogWidth: width,
741+
dialogHeight: height,
742+
});
705743
}
706744

707745
#setPosition(x, y) {
@@ -1021,7 +1059,7 @@ class CommentPopup {
10211059
}
10221060
}
10231061

1024-
#setPosition(x, y, correctPosition = true) {
1062+
#setPosition(x, y, correctPosition) {
10251063
if (!correctPosition) {
10261064
this.#editor.commentPopupPosition = [x, y];
10271065
} else {

0 commit comments

Comments
 (0)