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.

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

Jump to bottom

Ink-editor being commited to the AnnotationStorage too soon #15241

Closed
Snuffleupagus opened this issue Jul 29, 2022 · 2 comments · Fixed by #15244
Closed

Ink-editor being commited to the AnnotationStorage too soon #15241

Snuffleupagus opened this issue Jul 29, 2022 · 2 comments · Fixed by #15244
Assignees
@calixteman
Labels
editor regression

Comments

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jul 29, 2022 •
edited

Attach (recommended) or Link to PDF file here: N/A

Configuration:

  • Web browser and its version: N/A
  • Operating system and its version: N/A
  • PDF.js version: master
  • Is a browser extension: N/A

Steps to reproduce the problem:

  1. Load http://localhost:8888/web/viewer.html
  2. Enable Ink-editing.
  3. Reload the viewer, e.g. with F5.

What is the expected behavior? (add screenshot)
That the viewer immediately reloads.

What went wrong? (add screenshot)
The "Do you want to leave this page"-dialog comes up, since the AnnotationStorage isn't empty.

This is probably fallout from PR #15163, since that one meant that we always initialize the Ink-editor immediately. However I don't believe that it's correct to insert a new Ink-Annotation into the AnnotationStorage unless the user has actually drawn something.
@calixteman calixteman self-assigned this Jul 29, 2022
@Snuffleupagus Snuffleupagus changed the title Ink-editor being commit to the AnnotationStorage too soon Ink-editor being commited to the AnnotationStorage too soon Jul 29, 2022
@Snuffleupagus
Copy link
Collaborator Author

Thinking about this a little bit, we should probably not commit any isEmpty === true editor in the AnnotationStorage regardless of its type.

@calixteman
Copy link
Contributor

Yes, agreed it's exactly what I'm doing.

calixteman added a commit to calixteman/pdf.js that referenced this issue Jul 29, 2022
@calixteman
[Editor] Add an editor in the annotation storage only when it's non-e…
e819834 
…mpty (mozilla#15241)
@calixteman calixteman linked a pull request Jul 29, 2022 that will close this issue
[Editor] Add an editor in the annotation storage only when it's non-empty (#15241) #15244
Merged
Snuffleupagus added a commit that referenced this issue Jul 29, 2022
@Snuffleupagus
Merge pull request #15244 from calixteman/15241
ee8fab9 
[Editor] Add an editor in the annotation storage only when it's non-empty (#15241)
rousek pushed a commit to signosoft/pdf.js that referenced this issue Aug 10, 2022
@calixteman @rousek
[Editor] Add an editor in the annotation storage only when it's non-e…
4f9dfbf 
…mpty (mozilla#15241)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@calixteman calixteman

Labels
editor regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Editor] Add an editor in the annotation storage only when it's non-empty (#15241) calixteman/pdf.js
2 participants
@Snuffleupagus @calixteman