confirm if leaving a modified form without saving #12241

escapewindow · 2020-08-18T21:21:33Z

There may be a nicer way to track form modification; not sure.

timvandermeij · 2020-08-18T21:36:35Z

Relates to #12202.

src/display/annotation_layer.js

src/display/annotation_storage.js

web/app.js

brendandahl · 2020-08-18T23:34:23Z

/botio-linux preview

pdfjsbot · 2020-08-18T23:34:24Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/a56a6e04f9e6b15/output.txt

pdfjsbot · 2020-08-18T23:37:45Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/a56a6e04f9e6b15/output.txt

Total script time: 3.34 mins

Published

Viewer: http://54.67.70.0:8877/a56a6e04f9e6b15/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/a56a6e04f9e6b15/es5/web/viewer.html

Snuffleupagus · 2020-08-19T07:50:30Z

According to https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event, beforeunload has really undesirable effects in general:

[...] Attaching an event handler/listener to window or document's beforeunload event prevents browsers from using in-memory page navigation caches, like Firefox's Back-Forward cache or WebKit's Page Cache.

Furthermore, years ago, we used to have a beforeunload event listener in another part of the code-base, but removed it because it caused problems. (Unfortunately I don't remember all the details right now, since it was well over five years ago now.)

All-in-all, it seems really unfortunate as far as I'm concerned to use a beforeunload event unconditionally like this. Can't we at least limit this to documents with forms, to lessen the impact of these changes (unless a solution without beforeunload can be had)?

Snuffleupagus

In addition to #12241 (comment), I've also got a couple of questions/suggestions based on a cursory look at the code.

src/display/annotation_storage.js

src/display/annotation_layer.js

src/display/api.js

escapewindow · 2020-08-19T20:19:20Z

According to https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event, beforeunload has really undesirable effects in general:

[...] Attaching an event handler/listener to window or document's beforeunload event prevents browsers from using in-memory page navigation caches, like Firefox's Back-Forward cache or WebKit's Page Cache.

Furthermore, years ago, we used to have a beforeunload event listener in another part of the code-base, but removed it because it caused problems. (Unfortunately I don't remember all the details right now, since it was well over five years ago now.)

All-in-all, it seems really unfortunate as far as I'm concerned to use a beforeunload event unconditionally like this. Can't we at least limit this to documents with forms, to lessen the impact of these changes (unless a solution without beforeunload can be had)?

I took a look at this. It looks like this.bindWindowEvents() happens before _initializeMetadata, and possibly even load, so I don't have a this.documentInfo.IsAcroFormPresent or a this.pdfDocument to refer to. Another solution could be to bind beforeunload elsewhere, outside of bindWindowEvents. Do you have any recommendations here? (Or am I mistaken about any of my assumptions?)

escapewindow · 2020-08-19T20:31:40Z

bdahl
aki: what about binding/unbinding on modified?
aki
that makes sense. is there a destroy() or similar?
i'll try that and test
bdahl
may need to add an event that is dispatched from annotation storage on modification and reset?

escapewindow · 2020-08-19T22:35:20Z

bdahl
aki: what about binding/unbinding on modified?
aki
that makes sense. is there a destroy() or similar?
i'll try that and test
bdahl
may need to add an event that is dispatched from annotation storage on modification and reset?

This patch doesn't appear to work. I git stashed that, then tried looking for how to send an event from annotation storage. dispatchEvent seems to only be in web/, and message handlers appear to only go between core and display?

escapewindow · 2020-08-20T00:12:02Z

bdahl
aki: could add two properties to class AnnotationStorage e.g. onReset and onModified that the viewer could set as callbacks. then when those are called in the viewer you'd add/remove the unload listener

This worked! 488f674

brendandahl · 2020-08-20T18:45:09Z

/botio-linux preview

pdfjsbot · 2020-08-20T18:45:10Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/a63abdef2cb89a4/output.txt

pdfjsbot · 2020-08-20T18:48:33Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/a63abdef2cb89a4/output.txt

Total script time: 3.37 mins

Published

Viewer: http://54.67.70.0:8877/a63abdef2cb89a4/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/a63abdef2cb89a4/es5/web/viewer.html

timvandermeij

Looks good to me with these comments addressed.

src/display/annotation_storage.js

timvandermeij · 2020-08-20T21:21:25Z

src/display/annotation_storage.js

@@ -49,6 +53,9 @@ class AnnotationStorage {
   * @param {Object} value
   */
  setValue(key, value) {
+    if (this._storage.get(key) !== value) {


The only situation where this won't work is

pdf.js/src/display/annotation_layer.js

Line 687 in b061c30

storage.setValue(id, option.displayValue);

because that class is the only widget that doesn't use getOrCreateValue first. You can verify this by opening https://github.com/mozilla/pdf.js/blob/master/test/pdfs/annotation-choice-widget.pdf and without changing anything navigating away from the page, and you'll get the warning.

However, I'll most likely need to change that anyway for #12233, so this should be fine, but I'd like to fix that one first before landing this one just to make sure it's all fine.

We're getting down to the wire on Firefox's soft freeze coming up. Since the failing behavior isn't too bad, I'm going to merge this in so we have as small as possible updates in mozilla central.

src/display/annotation_storage.js

web/base_viewer.js

src/display/annotation_storage.js

brendandahl · 2020-08-21T00:34:19Z

/botio-linux unittest

pdfjsbot · 2020-08-21T00:34:20Z

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7d9eb4ffbc0de69/output.txt

pdfjsbot · 2020-08-21T00:38:07Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/7d9eb4ffbc0de69/output.txt

Total script time: 3.77 mins

Unit Tests: Passed

Snuffleupagus · 2020-08-21T07:57:51Z

Sorry, but I'm finding the decision to add a global event listener in web/base_viewer.js slightly strange and inconsistent (since there's no others anywhere in the file). Note that when the viewer was originally split into components, we purposely tried to move e.g. any direct DOM accesses out of them.

Hence it does seem, at least to me, that placing that code in PDFViewerApplication.load (in web/app.js) instead would have been slightly more expected overall.

Also, I understand that the following is more difficult to implement, but nonetheless: Note that this will not warn when switching documents, which means that if you call e.g. PDFViewerApplication.open with a new document (or directly call BaseViewer.setDocument when using the viewer components standalone) there won't be any warning about losing form data.

escapewindow · 2020-08-21T16:55:41Z

Sorry, but I'm finding the decision to add a global event listener in web/base_viewer.js slightly strange and inconsistent (since there's no others anywhere in the file). Note that when the viewer was originally split into components, we purposely tried to move e.g. any direct DOM accesses out of them.

Hence it does seem, at least to me, that placing that code in PDFViewerApplication.load (in web/app.js) instead would have been slightly more expected overall.

I thought we might not have an annotationStorage yet, but it looks like that works! #12256

Also, I understand that the following is more difficult to implement, but nonetheless: Note that this will not warn when switching documents, which means that if you call e.g. PDFViewerApplication.open with a new document (or directly call BaseViewer.setDocument when using the viewer components standalone) there won't be any warning about losing form data.

We may want to file this as an issue to track and prioritize.

timvandermeij · 2020-08-21T20:18:45Z

Thank you for your work! The first point is addressed in a follow-up PR and the second point is filed in issue #12257.

#12241 followup - move event listener to PDFViewerApplication.load

…ion storage This commit makes the following improvements: - The code is similar to the other interactive form widgets now, with a clear note for the only difference. - Calling `getOrCreateValue` unconditionally ensures that choice widgets always have a value in the annotation storage. Previously we only inserted a value in the annotation storage when an option matched or when a selection was changed. However, this causes breakage when saving/printing because comboboxes, which we don't fully support yet but are rendered, might not have a value in storage at all. Their field value might not match any option since it allows the user to enter a custom value. - Calling `getOrCreateValue` unconditionally ensures that forms with choice widgets no longer always trigger a warning when the user navigates away from the page. This fixes mozilla#12241 (comment)

timvandermeij added viewer form-acroform labels Aug 18, 2020

timvandermeij reviewed Aug 18, 2020

View reviewed changes

src/display/annotation_layer.js Outdated Show resolved Hide resolved

src/display/annotation_storage.js Outdated Show resolved Hide resolved

web/app.js Outdated Show resolved Hide resolved

Snuffleupagus reviewed Aug 19, 2020

View reviewed changes

src/display/annotation_storage.js Outdated Show resolved Hide resolved

src/display/annotation_layer.js Outdated Show resolved Hide resolved

src/display/api.js Outdated Show resolved Hide resolved

timvandermeij reviewed Aug 20, 2020

View reviewed changes

src/display/annotation_storage.js Show resolved Hide resolved

confirm if leaving a modified form without saving

83365a3

brendandahl merged commit 10f61b8 into mozilla:master Aug 21, 2020

escapewindow mentioned this pull request Aug 21, 2020

#12241 followup - move event listener to PDFViewerApplication.load #12256

Merged

timvandermeij mentioned this pull request Aug 21, 2020

Warn the user if navigating away from form with un-saved data #12202

Closed

timvandermeij mentioned this pull request Aug 21, 2020

Notify the user about form data loss when another document is opened #12257

Closed

timvandermeij added a commit that referenced this pull request Aug 21, 2020

Merge pull request #12256 from escapewindow/12241-followup

909c5af

#12241 followup - move event listener to PDFViewerApplication.load

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

confirm if leaving a modified form without saving #12241

confirm if leaving a modified form without saving #12241

escapewindow commented Aug 18, 2020

timvandermeij commented Aug 18, 2020

brendandahl commented Aug 18, 2020

pdfjsbot commented Aug 18, 2020

pdfjsbot commented Aug 18, 2020

Snuffleupagus commented Aug 19, 2020 •

edited

Loading

Snuffleupagus left a comment

escapewindow commented Aug 19, 2020

escapewindow commented Aug 19, 2020

escapewindow commented Aug 19, 2020

escapewindow commented Aug 20, 2020

brendandahl commented Aug 20, 2020

pdfjsbot commented Aug 20, 2020

pdfjsbot commented Aug 20, 2020

timvandermeij left a comment

timvandermeij Aug 20, 2020 •

edited

Loading

brendandahl Aug 21, 2020

brendandahl commented Aug 21, 2020

pdfjsbot commented Aug 21, 2020

pdfjsbot commented Aug 21, 2020

Snuffleupagus commented Aug 21, 2020 •

edited

Loading

escapewindow commented Aug 21, 2020

timvandermeij commented Aug 21, 2020

confirm if leaving a modified form without saving #12241

confirm if leaving a modified form without saving #12241

Conversation

escapewindow commented Aug 18, 2020

timvandermeij commented Aug 18, 2020

brendandahl commented Aug 18, 2020

pdfjsbot commented Aug 18, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 18, 2020

From: Bot.io (Linux m4)

Success

Published

Snuffleupagus commented Aug 19, 2020 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

escapewindow commented Aug 19, 2020

escapewindow commented Aug 19, 2020

escapewindow commented Aug 19, 2020

escapewindow commented Aug 20, 2020

brendandahl commented Aug 20, 2020

pdfjsbot commented Aug 20, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 20, 2020

From: Bot.io (Linux m4)

Success

Published

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

brendandahl Aug 21, 2020

Choose a reason for hiding this comment

brendandahl commented Aug 21, 2020

pdfjsbot commented Aug 21, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Aug 21, 2020

From: Bot.io (Linux m4)

Success

Snuffleupagus commented Aug 21, 2020 • edited Loading

escapewindow commented Aug 21, 2020

timvandermeij commented Aug 21, 2020

Snuffleupagus commented Aug 19, 2020 •

edited

Loading

timvandermeij Aug 20, 2020 •

edited

Loading

Snuffleupagus commented Aug 21, 2020 •

edited

Loading