Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r?mak draft
authorMark Banner <standard8@mozilla.com>
Thu, 24 Aug 2017 12:51:38 +0100
changeset 658532 260603f0d3a4f8645740faf401197b1bb9d4cced
parent 658358 8e05298328da75f3056a9f1f9609938870d756a0
child 729695 70c1817ca377b50d1ebbacfa20d8fd1b1ab61be6
push id77812
push userbmo:standard8@mozilla.com
push dateMon, 04 Sep 2017 10:38:34 +0000
reviewersmak
bugs1391393
milestone57.0a1
Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r?mak MozReview-Commit-ID: 7supBHcrpdu
browser/components/places/content/bookmarkProperties.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
browser/components/places/tests/browser/browser_check_correct_controllers.js
--- 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);