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

When annotaion exceeds 10 pages, print and download are abnormal #15780

Closed
OnlY0226 opened this issue Dec 5, 2022 · 2 comments · Fixed by #15782
Closed

When annotaion exceeds 10 pages, print and download are abnormal #15780

OnlY0226 opened this issue Dec 5, 2022 · 2 comments · Fixed by #15782
Labels
editor release-blocker Blocker for the upcoming release

Comments

@OnlY0226
Copy link

OnlY0226 commented Dec 5, 2022

Attach (recommended) or Link to PDF file here: https://mozilla.github.io/pdf.js/legacy/web/viewer.html

Configuration:

  • Web browser and its version: Google 版本 104.0.5112.79(正式版本) (64 位)
  • Operating system and its version: linux
  • PDF.js version: v3.0.279
  • Is a browser extension:

Steps to reproduce the problem:

  1. add annotation on page 1
  2. add annotation on page 11
  3. print

What is the expected behavior? (add screenshot)
print with annotation
What went wrong? (add screenshot)
report error:
editor.js:298 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'viewportBaseDimensions')
at InkEditor.getRect (editor.js:298:53)
at InkEditor.serialize (ink.js:1065:23)
at get serializable [as serializable] (annotation_storage.js:168:47)
at new PrintAnnotationStorage (annotation_storage.js:204:49)
at get print [as print] (annotation_storage.js:153:12)
at app.js:1683:52
at async Promise.all (index 1)
image

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension):
https://mozilla.github.io/pdf.js/legacy/web/viewer.html

@Snuffleupagus
Copy link
Collaborator

Naively, it seems to me that we should probably serialize the applicable editors when destroying the layer in

/**
* Destroy the main editor.
*/
destroy() {
if (this.#uiManager.getActive()?.parent === this) {
this.#uiManager.setActiveEditor(null);
}
for (const editor of this.#editors.values()) {
this.#accessibilityManager?.removePointerInTextLayer(editor.contentDiv);
editor.isAttachedToDOM = false;
editor.div.remove();
editor.parent = null;
}
this.div = null;
this.#editors.clear();
this.#uiManager.removeLayer(this);
}

Similarly, we should probably deserialize any applicable editors when re-constructing the layer in
/**
* @param {AnnotationEditorLayerOptions} options
*/
constructor(options) {
if (!AnnotationEditorLayer._initialized) {
AnnotationEditorLayer._initialized = true;
FreeTextEditor.initialize(options.l10n);
InkEditor.initialize(options.l10n);
}
options.uiManager.registerEditorTypes([FreeTextEditor, InkEditor]);
this.#uiManager = options.uiManager;
this.annotationStorage = options.annotationStorage;
this.pageIndex = options.pageIndex;
this.div = options.div;
this.#accessibilityManager = options.accessibilityManager;
this.#uiManager.addLayer(this);
}

/cc @calixteman, @marco-c This seems like a pretty serious usability issue, and it's possible that this is also the same bug as outlined in https://www.reddit.com/r/firefox/comments/yxlgx3/how_to_actually_save_an_edited_pdf/

@Snuffleupagus Snuffleupagus added the release-blocker Blocker for the upcoming release label Dec 5, 2022
@calixteman
Copy link
Contributor

Naively, it seems to me that we should probably serialize the applicable editors when destroying the layer in

Good idea.

/cc @calixteman, @marco-c This seems like a pretty serious usability issue, and it's possible that this is also the same bug as outlined in https://www.reddit.com/r/firefox/comments/yxlgx3/how_to_actually_save_an_edited_pdf/

It's very likely the same issue.

calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 5, 2022
When printing/saving the created editors are serialized and they're using some
properties (e.g. parent viewport, color, ...) which are unavailable because
the parent layer has been removed.
Consequently, when the parent layer is destroyed we save the serialized version of
the editors.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
…#15780

The main issue is due to the fact that an editor's parent can be null when
we want to serialize it and that lead to an exception which break all the
saving/printing process.
So this incomplete patch fixes only the saving/printing issue but not the
underlying problem (i.e. having a null parent) and doesn't bring that much
complexity, so it should help to uplift it the next Firefox release.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 6, 2022
…#15780

The main issue is due to the fact that an editor's parent can be null when
we want to serialize it and that lead to an exception which break all the
saving/printing process.
So this incomplete patch fixes only the saving/printing issue but not the
underlying problem (i.e. having a null parent) and doesn't bring that much
complexity, so it should help to uplift it the next Firefox release.
calixteman added a commit that referenced this issue Dec 6, 2022
[Editor] Add a very basic and incomplete workaround for issue #15780
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 7, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 7, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 7, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
calixteman added a commit to calixteman/pdf.js that referenced this issue Dec 8, 2022
An annotation editor layer can be destroyed when it's invisible, hence some
annotations can have a null parent but when printing/saving or when changing
font size, color, ... of all added annotations (when selected with ctrl+a) we
still need to have some parent properties especially the page dimensions, global
scale factor and global rotation angle.
This patch aims to remove all the references to the parent in the editor instances
except in some cases where an editor should obviously have one.
It fixes mozilla#15780.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor release-blocker Blocker for the upcoming release
Projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

3 participants