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

confirm if leaving a modified form without saving #12241

Merged
merged 1 commit into from
Aug 21, 2020
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
29 changes: 29 additions & 0 deletions src/display/annotation_storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@
class AnnotationStorage {
constructor() {
this._storage = new Map();
this._modified = false;

// Callbacks to signal when the modification state is set or reset.
// This is used by the viewer to only bind on `beforeunload` if forms
// are actually edited to prevent doing so unconditionally since that
// can have undesirable efffects.
this.onSetModified = null;
this.onResetModified = null;
}

/**
Expand Down Expand Up @@ -49,6 +57,9 @@ class AnnotationStorage {
* @param {Object} value
*/
setValue(key, value) {
if (this._storage.get(key) !== value) {
Copy link
Contributor

@timvandermeij timvandermeij Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only situation where this won't work is

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.setModified();
escapewindow marked this conversation as resolved.
Show resolved Hide resolved
}
this._storage.set(key, value);
}

Expand All @@ -62,6 +73,24 @@ class AnnotationStorage {
get size() {
return this._storage.size;
}

setModified() {
if (!this._modified) {
this._modified = true;
if (typeof this.onSetModified === "function") {
this.onSetModified();
}
}
}

resetModified() {
if (this._modified) {
this._modified = false;
if (typeof this.onResetModified === "function") {
this.onResetModified();
}
}
}
}

export { AnnotationStorage };
16 changes: 10 additions & 6 deletions src/display/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2536,12 +2536,16 @@ class WorkerTransport {
}

saveDocument(annotationStorage) {
return this.messageHandler.sendWithPromise("SaveDocument", {
numPages: this._numPages,
annotationStorage:
(annotationStorage && annotationStorage.getAll()) || null,
filename: this._fullReader.filename,
});
return this.messageHandler
.sendWithPromise("SaveDocument", {
numPages: this._numPages,
annotationStorage:
(annotationStorage && annotationStorage.getAll()) || null,
filename: this._fullReader.filename,
})
.finally(() => {
annotationStorage.resetModified();
});
}

getDestinations() {
Expand Down
43 changes: 43 additions & 0 deletions test/unit/annotation_storage_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,48 @@ describe("AnnotationStorage", function () {
expect(value).toEqual("an other string");
done();
});

it("should call onSetModified() if value is changed", function (done) {
const annotationStorage = new AnnotationStorage();
let called = false;
const callback = function () {
called = true;
};
annotationStorage.onSetModified = callback;
annotationStorage.getOrCreateValue("asdf", "original");
expect(called).toBe(false);

// not changing value
annotationStorage.setValue("asdf", "original");
expect(called).toBe(false);

// changing value
annotationStorage.setValue("asdf", "modified");
expect(called).toBe(true);
done();
});
});

describe("ResetModified", function () {
it("should call onResetModified() if set", function (done) {
const annotationStorage = new AnnotationStorage();
let called = false;
const callback = function () {
called = true;
};
annotationStorage.onResetModified = callback;
annotationStorage.getOrCreateValue("asdf", "original");

// not changing value
annotationStorage.setValue("asdf", "original");
annotationStorage.resetModified();
expect(called).toBe(false);

// changing value
annotationStorage.setValue("asdf", "modified");
annotationStorage.resetModified();
expect(called).toBe(true);
done();
});
});
});
12 changes: 12 additions & 0 deletions web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@ function isSameScale(oldScale, newScale) {
return false;
}

function beforeUnload(evt) {
evt.preventDefault();
evt.returnValue = "";
return false;
}

/**
* Simple viewer control to display PDF content/pages.
* @implements {IRenderableView}
Expand Down Expand Up @@ -436,6 +442,12 @@ class BaseViewer {
const firstPagePromise = pdfDocument.getPage(1);

const annotationStorage = pdfDocument.annotationStorage;
annotationStorage.onSetModified = function () {
window.addEventListener("beforeunload", beforeUnload);
};
annotationStorage.onResetModified = function () {
window.removeEventListener("beforeunload", beforeUnload);
};

this._pagesCapability.promise.then(() => {
this.eventBus.dispatch("pagesloaded", {
Expand Down