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] When deleting an annotation with popup, then delete the popup too #18800

Merged
merged 1 commit into from
Sep 26, 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
6 changes: 6 additions & 0 deletions src/core/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,12 @@ class Page {
}
if (annotation.deleted) {
deletedAnnotations.put(ref, ref);
if (annotation.popupRef) {
const popupRef = Ref.fromString(annotation.popupRef);
if (popupRef) {
deletedAnnotations.put(popupRef, popupRef);
}
}
continue;
}
existingAnnotations?.put(ref);
Expand Down
21 changes: 16 additions & 5 deletions src/display/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ class AnnotationEditor {

_initialOptions = Object.create(null);

_initialData = null;

_isVisible = true;

_uiManager = null;
Expand Down Expand Up @@ -1335,6 +1337,19 @@ class AnnotationEditor {
*/
rotate(_angle) {}

/**
* Serialize the editor when it has been deleted.
* @returns {Object}
*/
serializeDeleted() {
return {
id: this.annotationElementId,
deleted: true,
pageIndex: this.pageIndex,
popupRef: this._initialData?.popupRef || "",
};
}

/**
* Serialize the editor.
* The result of the serialization will be used to construct a
Expand Down Expand Up @@ -1809,11 +1824,7 @@ class FakeEditor extends AnnotationEditor {
}

serialize() {
return {
id: this.annotationElementId,
deleted: true,
pageIndex: this.pageIndex,
};
return this.serializeDeleted();
}
}

Expand Down
16 changes: 6 additions & 10 deletions src/display/editor/freetext.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ class FreeTextEditor extends AnnotationEditor {

#fontSize;

#initialData = null;

static _freeTextDefaultContent = "";

static _internalPadding = 0;
Expand Down Expand Up @@ -598,7 +596,7 @@ class FreeTextEditor extends AnnotationEditor {

// position is the position of the first glyph in the annotation
// and it's relative to its container.
const { position } = this.#initialData;
const { position } = this._initialData;
let [tx, ty] = this.getInitialTranslation();
[tx, ty] = this.pageTranslationToScreen(tx, ty);
const [pageWidth, pageHeight] = this.pageDimensions;
Expand Down Expand Up @@ -781,6 +779,7 @@ class FreeTextEditor extends AnnotationEditor {
rect,
rotation,
id,
popupRef,
},
textContent,
textPosition,
Expand All @@ -805,14 +804,15 @@ class FreeTextEditor extends AnnotationEditor {
rotation,
id,
deleted: false,
popupRef,
};
}
const editor = super.deserialize(data, parent, uiManager);
editor.#fontSize = data.fontSize;
editor.#color = Util.makeHexColor(...data.color);
editor.#content = FreeTextEditor.#deserializeContent(data.value);
editor.annotationElementId = data.id || null;
editor.#initialData = initialData;
editor._initialData = initialData;

return editor;
}
Expand All @@ -824,11 +824,7 @@ class FreeTextEditor extends AnnotationEditor {
}

if (this.deleted) {
return {
pageIndex: this.pageIndex,
id: this.annotationElementId,
deleted: true,
};
return this.serializeDeleted();
}

const padding = FreeTextEditor._internalPadding * this.parentScale;
Expand Down Expand Up @@ -866,7 +862,7 @@ class FreeTextEditor extends AnnotationEditor {
}

#hasElementChanged(serialized) {
const { value, fontSize, color, pageIndex } = this.#initialData;
const { value, fontSize, color, pageIndex } = this._initialData;

return (
this._hasBeenMoved ||
Expand Down
17 changes: 7 additions & 10 deletions src/display/editor/highlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ class HighlightEditor extends AnnotationEditor {

#id = null;

#initialData = null;

#isFreeHighlight = false;

#lastPoint = null;
Expand Down Expand Up @@ -785,7 +783,7 @@ class HighlightEditor extends AnnotationEditor {
let initialData = null;
if (data instanceof HighlightAnnotationElement) {
const {
data: { quadPoints, rect, rotation, id, color, opacity },
data: { quadPoints, rect, rotation, id, color, opacity, popupRef },
parent: {
page: { pageNumber },
},
Expand All @@ -801,6 +799,7 @@ class HighlightEditor extends AnnotationEditor {
rotation,
id,
deleted: false,
popupRef,
};
} else if (data instanceof InkAnnotationElement) {
const {
Expand All @@ -811,6 +810,7 @@ class HighlightEditor extends AnnotationEditor {
id,
color,
borderStyle: { rawWidth: thickness },
popupRef,
},
parent: {
page: { pageNumber },
Expand All @@ -827,6 +827,7 @@ class HighlightEditor extends AnnotationEditor {
rotation,
id,
deleted: false,
popupRef,
};
}

Expand All @@ -839,7 +840,7 @@ class HighlightEditor extends AnnotationEditor {
editor.#thickness = data.thickness;
}
editor.annotationElementId = data.id || null;
editor.#initialData = initialData;
editor._initialData = initialData;

const [pageWidth, pageHeight] = editor.pageDimensions;
const [pageX, pageY] = editor.pageTranslation;
Expand Down Expand Up @@ -902,11 +903,7 @@ class HighlightEditor extends AnnotationEditor {
}

if (this.deleted) {
return {
pageIndex: this.pageIndex,
id: this.annotationElementId,
deleted: true,
};
return this.serializeDeleted();
}

const rect = this.getRect(0, 0);
Expand Down Expand Up @@ -934,7 +931,7 @@ class HighlightEditor extends AnnotationEditor {
}

#hasElementChanged(serialized) {
const { color } = this.#initialData;
const { color } = this._initialData;
return serialized.color.some((c, i) => c !== color[i]);
}

Expand Down
1 change: 1 addition & 0 deletions test/integration/freetext_editor_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,7 @@ describe("FreeText Editor", () => {
pageIndex: 0,
id: "51R",
deleted: true,
popupRef: "",
},
]);

Expand Down
44 changes: 44 additions & 0 deletions test/integration/highlight_editor_spec.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1972,6 +1972,50 @@ describe("Highlight Editor", () => {
});
});

describe("Highlight (delete an existing annotation)", () => {
let pages;

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

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

it("must delete an existing annotation and its popup", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
const modeChangedHandle = await waitForAnnotationModeChanged(page);
await waitAndClick(page, "[data-annotation-id='24R']", { count: 2 });
await awaitPromise(modeChangedHandle);
await page.waitForSelector("#highlightParamsToolbarContainer");

const editorSelector = getEditorSelector(0);
await page.waitForSelector(editorSelector);
await page.waitForSelector(`${editorSelector} button.delete`);
await page.click(`${editorSelector} button.delete`);
await waitForSerialized(page, 1);

const serialized = await getSerialized(page);
expect(serialized)
.withContext(`In ${browserName}`)
.toEqual([
{
pageIndex: 0,
id: "24R",
deleted: true,
popupRef: "25R",
},
]);
})
);
});
});

describe("Free Highlight (edit existing in double clicking on it)", () => {
let pages;

Expand Down
1 change: 1 addition & 0 deletions test/pdfs/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -670,3 +670,4 @@
!bug1918115.pdf
!bug1919513.pdf
!issue16038.pdf
!highlight_popup.pdf
Binary file added test/pdfs/highlight_popup.pdf
Binary file not shown.
23 changes: 23 additions & 0 deletions test/unit/api_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2926,6 +2926,29 @@ describe("api", function () {
await loadingTask.destroy();
});

it("write an highlight annotation and delete its popup", async function () {
let loadingTask = getDocument(
buildGetDocumentParams("highlight_popup.pdf")
);
let pdfDoc = await loadingTask.promise;
pdfDoc.annotationStorage.setValue("pdfjs_internal_editor_0", {
deleted: true,
id: "24R",
pageIndex: 0,
popupRef: "25R",
});
const data = await pdfDoc.saveDocument();
await loadingTask.destroy();

loadingTask = getDocument(data);
pdfDoc = await loadingTask.promise;
const page = await pdfDoc.getPage(1);
const annotations = await page.getAnnotations();

expect(annotations).toEqual([]);
await loadingTask.destroy();
});

it("read content from multiline textfield containing an empty line", async function () {
const loadingTask = getDocument(buildGetDocumentParams("issue17492.pdf"));
const pdfDoc = await loadingTask.promise;
Expand Down