Skip to content

Commit

Permalink
[Editor] Avoid to focus an existing editor when enabling the layer
Browse files Browse the repository at this point in the history
It fixes #18911.
  • Loading branch information
calixteman committed Dec 11, 2024
1 parent fbac4ab commit d61b882
Show file tree
Hide file tree
Showing 9 changed files with 79 additions and 10 deletions.
7 changes: 6 additions & 1 deletion src/display/editor/annotation_editor_layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ class AnnotationEditorLayer {

#isDisabling = false;

#isEnabling = false;

#drawingAC = null;

#focusedElement = null;
Expand Down Expand Up @@ -229,6 +231,7 @@ class AnnotationEditorLayer {
* editor creation.
*/
async enable() {
this.#isEnabling = true;
this.div.tabIndex = 0;
this.togglePointerEvents(true);
const annotationElementIds = new Set();
Expand All @@ -242,6 +245,7 @@ class AnnotationEditorLayer {
}

if (!this.#annotationLayer) {
this.#isEnabling = false;
return;
}

Expand All @@ -262,6 +266,7 @@ class AnnotationEditorLayer {
this.addOrRebuild(editor);
editor.enableEditing();
}
this.#isEnabling = false;
}

/**
Expand Down Expand Up @@ -508,7 +513,7 @@ class AnnotationEditorLayer {

// The editor will be correctly moved into the DOM (see fixAndSetPosition).
editor.fixAndSetPosition();
editor.onceAdded();
editor.onceAdded(/* focus = */ !this.#isEnabling);
this.#uiManager.addToAnnotationStorage(editor);
editor._reportTelemetry(editor.telemetryInitialData);
}
Expand Down
4 changes: 2 additions & 2 deletions src/display/editor/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ class DrawingEditor extends AnnotationEditor {
}

/** @inheritdoc */
onceAdded() {
onceAdded(focus) {
if (!this.annotationElementId) {
this.parent.addUndoableEditor(this);
}
Expand All @@ -354,7 +354,7 @@ class DrawingEditor extends AnnotationEditor {
this.#mustBeCommitted = false;
this.commit();
this.parent.setSelected(this);
if (this.isOnScreen) {
if (focus && this.isOnScreen) {
this.div.focus();
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/display/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -1367,8 +1367,9 @@ class AnnotationEditor {

/**
* Executed once this editor has been rendered.
* @param {boolean} focus - true if the editor should be focused.
*/
onceAdded() {}
onceAdded(focus) {}

/**
* Check if the editor contains something.
Expand Down
6 changes: 4 additions & 2 deletions src/display/editor/freetext.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,13 +363,15 @@ class FreeTextEditor extends AnnotationEditor {
}

/** @inheritdoc */
onceAdded() {
onceAdded(focus) {
if (this.width) {
// The editor was created in using ctrl+c.
return;
}
this.enableEditMode();
this.editorDiv.focus();
if (focus) {
this.editorDiv.focus();
}
if (this._initialOptions?.isCentered) {
this.center();
}
Expand Down
6 changes: 4 additions & 2 deletions src/display/editor/highlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -439,11 +439,13 @@ class HighlightEditor extends AnnotationEditor {
}

/** @inheritdoc */
onceAdded() {
onceAdded(focus) {
if (!this.annotationElementId) {
this.parent.addUndoableEditor(this);
}
this.div.focus();
if (focus) {
this.div.focus();
}
}

/** @inheritdoc */
Expand Down
6 changes: 4 additions & 2 deletions src/display/editor/stamp.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,11 @@ class StampEditor extends AnnotationEditor {
}

/** @inheritdoc */
onceAdded() {
onceAdded(focus) {
this._isDraggable = true;
this.div.focus();
if (focus) {
this.div.focus();
}
}

/** @inheritdoc */
Expand Down
56 changes: 56 additions & 0 deletions test/integration/highlight_editor_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
waitForAnnotationModeChanged,
waitForSelectedEditor,
waitForSerialized,
waitForTimeout,
} from "./test_utils.mjs";
import { fileURLToPath } from "url";
import fs from "fs";
Expand Down Expand Up @@ -2613,4 +2614,59 @@ describe("Highlight Editor", () => {
);
});
});

describe("Highlight mustn't trigger a scroll when edited", () => {
let pages;

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

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

it("must check that there is no scroll because of focus", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
await switchToHighlight(page);

const page4Selector = ".page[data-page-number = '4']";
const page5Selector = ".page[data-page-number = '5']";
await scrollIntoView(page, page4Selector);
await page.waitForSelector(`${page5Selector} .annotationEditorLayer`);

// When moving to page 4, the highlight editor mustn't be focused (it
// was causing a scroll to page 5).
// So here we're waiting a bit and checking that the page is still 4.
// eslint-disable-next-line no-restricted-syntax
await waitForTimeout(100);

// Get the length of the intersection between two ranges.
const inter = ([a, b], [c, d]) =>
d < a || b < c ? 0 : Math.min(b, d) - Math.max(a, c);

const page4Rect = await getRect(page, page4Selector);
const page5Rect = await getRect(page, page5Selector);
const viewportRect = await getRect(page, "#viewerContainer");
const viewportRange = [
viewportRect.y,
viewportRect.y + viewportRect.height,
];

const interPage4 = inter(
[page4Rect.y, page4Rect.y + page4Rect.height],
viewportRange
);
const interPage5 = inter(
[page5Rect.y, page5Rect.y + page5Rect.height],
viewportRange
);
expect(interPage4)
.withContext(`In ${browserName}`)
.toBeGreaterThan(0.5 * interPage5);
})
);
});
});
});
1 change: 1 addition & 0 deletions test/pdfs/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -689,3 +689,4 @@
!inks.pdf
!inks_basic.pdf
!issue19182.pdf
!issue18911.pdf
Binary file added test/pdfs/issue18911.pdf
Binary file not shown.

0 comments on commit d61b882

Please sign in to comment.