Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r?mak
MozReview-Commit-ID: 7supBHcrpdu
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -377,37 +377,41 @@ var BookmarkPropertiesPanel = {
// Hack for implementing batched-Undo around the editBookmarkOverlay
// instant-apply code. For all the details see the comment above beginBatch
// in browser-places.js
_batchBlockingDeferred: null,
_beginBatch() {
if (this._batching)
return;
if (PlacesUIUtils.useAsyncTransactions) {
+ this._topUndoEntry = PlacesTransactions.topUndoEntry;
this._batchBlockingDeferred = PromiseUtils.defer();
PlacesTransactions.batch(async () => {
await this._batchBlockingDeferred.promise;
});
} else {
PlacesUtils.transactionManager.beginBatch(null);
}
this._batching = true;
},
_endBatch() {
if (!this._batching)
- return;
+ return false;
if (PlacesUIUtils.useAsyncTransactions) {
this._batchBlockingDeferred.resolve();
this._batchBlockingDeferred = null;
} else {
PlacesUtils.transactionManager.endBatch(false);
}
this._batching = false;
+ let changed = this._topUndoEntry != PlacesTransactions.topUndoEntry;
+ delete this._topUndoEntry;
+ return changed;
},
// nsISupports
QueryInterface: function BPP_QueryInterface(aIID) {
if (aIID.equals(Ci.nsIDOMEventListener) ||
aIID.equals(Ci.nsISupports))
return this;
@@ -441,21 +445,24 @@ var BookmarkPropertiesPanel = {
window.arguments[0].performed = true;
},
onDialogCancel() {
// The order here is important! We have to uninit the panel first, otherwise
// changes done as part of Undo may change the panel contents and by
// that force it to commit more transactions.
gEditItemOverlay.uninitPanel(true);
- this._endBatch();
- if (PlacesUIUtils.useAsyncTransactions)
- PlacesTransactions.undo().catch(Components.utils.reportError);
- else
+ let changed = this._endBatch();
+ if (PlacesUIUtils.useAsyncTransactions) {
+ if (changed) {
+ PlacesTransactions.undo().catch(Components.utils.reportError);
+ }
+ } else {
PlacesUtils.transactionManager.undoTransaction();
+ }
window.arguments[0].performed = false;
},
/**
* This method checks to see if the input fields are in a valid state.
*
* @returns true if the input is valid, false otherwise
*/
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -16,16 +16,17 @@ support-files =
[browser_bookmark_folder_moveability.js]
[browser_bookmarklet_windowOpen.js]
support-files =
pageopeningwindow.html
[browser_bookmarkProperties_addFolderDefaultButton.js]
[browser_bookmarkProperties_addKeywordForThisSearch.js]
[browser_bookmarkProperties_addLivemark.js]
[browser_bookmarkProperties_bookmarkAllTabs.js]
+[browser_bookmarkProperties_cancel.js]
[browser_bookmarkProperties_editTagContainer.js]
[browser_bookmarkProperties_readOnlyRoot.js]
[browser_bookmarksProperties.js]
[browser_check_correct_controllers.js]
[browser_click_bookmarks_on_toolbar.js]
[browser_cutting_bookmarks.js]
subsuite = clipboard
[browser_controller_onDrop.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
@@ -0,0 +1,118 @@
+"use strict";
+
+/* global sinon */
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
+
+const sandbox = sinon.sandbox.create();
+
+registerCleanupFunction(async function() {
+ sandbox.restore();
+ delete window.sinon;
+ await PlacesUtils.bookmarks.eraseEverything();
+ await PlacesTestUtils.clearHistory();
+});
+
+let bookmarks; // Bookmarks added via insertTree.
+let bookmarkIds; // Map of Guids to Ids.
+
+add_task(async function setup() {
+ bookmarks = await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children: [{
+ title: "bm1",
+ url: "http://example.com",
+ }, {
+ title: "bm2",
+ url: "http://example.com/2",
+ }]
+ });
+
+ bookmarkIds = await PlacesUtils.promiseManyItemIds([
+ bookmarks[0].guid, bookmarks[1].guid
+ ]);
+
+ // Undo is called asynchronously - and not waited for. Since we're not
+ // expecting undo to be called, we can only tell this by stubbing it.
+ sandbox.stub(PlacesTransactions, "undo").returns(Promise.resolve());
+});
+
+// Tests for bug 1391393 - Ensures that if the user cancels the bookmark properties
+// dialog without having done any changes, then no undo is called.
+add_task(async function test_cancel_with_no_changes() {
+ if (!PlacesUIUtils.useAsyncTransactions) {
+ Assert.ok(true, "Skipping test as async transactions are turned off");
+ return;
+ }
+
+ await withSidebarTree("bookmarks", async (tree) => {
+ tree.selectItems([bookmarkIds.get(bookmarks[0].guid)]);
+
+ // Delete the bookmark to put something in the undo history.
+ // Rather than calling cmd_delete, we call the remove directly, so that we
+ // can await on it finishing, and be guaranteed that there's something
+ // in the history.
+ await tree.controller.remove("Remove Selection");
+
+ tree.selectItems([bookmarkIds.get(bookmarks[1].guid)]);
+
+ // Now open the bookmarks dialog and cancel it.
+ await withBookmarksDialog(
+ true,
+ function openDialog() {
+ tree.controller.doCommand("placesCmd_show:info");
+ },
+ async function test(dialogWin) {
+ let acceptButton = dialogWin.document.documentElement.getButton("accept");
+ await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled,
+ "The accept button should be enabled");
+ }
+ );
+
+ Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");
+
+ // Check the bookmark is still removed.
+ Assert.ok(!(await PlacesUtils.bookmarks.fetch(bookmarks[0].guid)),
+ "The originally removed bookmark should not exist.");
+
+ Assert.ok(await PlacesUtils.bookmarks.fetch(bookmarks[1].guid),
+ "The second bookmark should still exist");
+ });
+});
+
+add_task(async function test_cancel_with_changes() {
+ if (!PlacesUIUtils.useAsyncTransactions) {
+ Assert.ok(true, "Skipping test as async transactions are turned off");
+ return;
+ }
+
+ await withSidebarTree("bookmarks", async (tree) => {
+ tree.selectItems([bookmarkIds.get(bookmarks[1].guid)]);
+
+ // Now open the bookmarks dialog and cancel it.
+ await withBookmarksDialog(
+ true,
+ function openDialog() {
+ tree.controller.doCommand("placesCmd_show:info");
+ },
+ async function test(dialogWin) {
+ let acceptButton = dialogWin.document.documentElement.getButton("accept");
+ await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled,
+ "The accept button should be enabled");
+
+ let promiseTitleChangeNotification = promiseBookmarksNotification(
+ "onItemChanged", (itemId, prop, isAnno, val) => prop == "title" && val == "n");
+
+ fillBookmarkTextField("editBMPanel_namePicker", "n", dialogWin);
+
+ // The dialog is instant apply.
+ await promiseTitleChangeNotification;
+
+ // Ensure that the addition really is finished before we hit cancel.
+ await PlacesTestUtils.promiseAsyncUpdates();
+ }
+ );
+
+ Assert.ok(PlacesTransactions.undo.calledOnce,
+ "undo should have been called once.");
+ });
+});
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
@@ -59,16 +59,19 @@ add_task(async function() {
// Check the shortcut's title.
Assert.equal(tree.selectedNode.title, "tag2", "The node has the correct title");
// Check the tags have been edited.
let tags = PlacesUtils.tagging.getTagsForURI(uri);
Assert.equal(tags.length, 1, "Found the right number of tags");
Assert.ok(tags.includes("tag2"), "Found the expected tag");
+
+ // Ensure that the addition really is finished before we hit cancel.
+ await PlacesTestUtils.promiseAsyncUpdates();
}
);
await promiseTitleResetNotification;
// Check the tag change has been reverted.
let tags = PlacesUtils.tagging.getTagsForURI(uri);
Assert.equal(tags.length, 1, "Found the right number of tags");
--- a/browser/components/places/tests/browser/browser_check_correct_controllers.js
+++ b/browser/components/places/tests/browser/browser_check_correct_controllers.js
@@ -1,15 +1,25 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/
*/
"use strict";
add_task(async function test() {
+ let bookmark = await PlacesUtils.bookmarks.insert({
+ parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+ title: "Plain Bob",
+ url: "http://example.com"
+ });
+
+ registerCleanupFunction(async () => {
+ await PlacesUtils.bookmarks.remove(bookmark);
+ });
+
let sidebarBox = document.getElementById("sidebar-box");
is(sidebarBox.hidden, true, "The sidebar should be hidden");
// Uncollapse the personal toolbar if needed.
let toolbar = document.getElementById("PersonalToolbar");
let wasCollapsed = toolbar.collapsed;
if (wasCollapsed) {
await promiseSetToolbarVisibility(toolbar, true);