Bug 1506461 - Enable inline suggestions for bookmark tags autocomplete. r=Standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 21 Nov 2018 15:10:53 +0000
changeset 503916 9db1fb9190c7c5b9ba75b3ca83099b2a329c931f
parent 503915 a38af3c7c0e4743645888fdaf63c50101d813297
child 503917 1c8f22180210bf31334c61b0c884ab8611ba7c15
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8
bugs1506461
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1506461 - Enable inline suggestions for bookmark tags autocomplete. r=Standard8 Differential Revision: https://phabricator.services.mozilla.com/D11773
browser/base/content/browser-places.js
browser/base/content/test/general/browser_bookmark_popup.js
browser/components/places/content/editBookmark.js
browser/components/places/content/editBookmarkPanel.inc.xul
toolkit/components/places/nsTaggingService.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -281,17 +281,20 @@ var StarUI = {
   // we start a PlacesTransactions batch when the star UI panel is shown, and
   // we keep the batch ongoing until the panel is hidden.
   _batchBlockingDeferred: null,
   beginBatch() {
     if (this._batching)
       return;
     this._batchBlockingDeferred = PromiseUtils.defer();
     PlacesTransactions.batch(async () => {
+      // First await for the batch to be concluded.
       await this._batchBlockingDeferred.promise;
+      // And then for any pending promises added in the meanwhile.
+      await Promise.all(gEditItemOverlay.transactionPromises);
     });
     this._batching = true;
   },
 
   endBatch() {
     if (!this._batching)
       return;
 
--- a/browser/base/content/test/general/browser_bookmark_popup.js
+++ b/browser/base/content/test/general/browser_bookmark_popup.js
@@ -82,17 +82,18 @@ async function test_bookmarks_popup({isN
       if (!shouldAutoClose) {
         await new Promise(resolve => setTimeout(resolve, 400));
         Assert.equal(bookmarkPanel.state, "open", "Panel should still be 'open' for non-autoclose");
       }
 
       let onItemRemovedPromise = Promise.resolve();
       if (isBookmarkRemoved) {
         onItemRemovedPromise = PlacesTestUtils.waitForNotification("onItemRemoved",
-          (id, parentId, index, type, itemUrl) => TEST_URL == itemUrl.spec);
+          (id, parentId, index, type, uri, guid, parentGuid) =>
+            parentGuid == PlacesUtils.bookmarks.unfiledGuid && TEST_URL == uri.spec);
       }
 
       let hiddenPromise = promisePopupHidden(bookmarkPanel);
       if (popupHideFn) {
         await popupHideFn();
       }
       await Promise.all([hiddenPromise, onItemRemovedPromise]);
 
@@ -132,21 +133,21 @@ add_task(async function panel_shown_once
   });
 });
 
 add_task(async function panel_shown_once_for_slow_doubleclick_on_new_bookmark_star_and_autocloses() {
   todo(false, "bug 1250267, may need to add some tracking state to " +
               "browser-places.js for this.");
 
   /*
-  yield test_bookmarks_popup({
+  await test_bookmarks_popup({
     isNewBookmark: true,
     *popupShowFn() {
       EventUtils.synthesizeMouse(bookmarkStar, 10, 10, window);
-      yield new Promise(resolve => setTimeout(resolve, 300));
+      await new Promise(resolve => setTimeout(resolve, 300));
       EventUtils.synthesizeMouse(bookmarkStar, 10, 10, window);
     },
     shouldAutoClose: true,
     isBookmarkRemoved: false,
   });
   */
 });
 
@@ -475,17 +476,17 @@ add_task(async function enter_during_aut
 
       Assert.equal(tagsField.value, "Abc",
         "Autocomplete should've inserted the selected item");
     },
     shouldAutoClose: false,
     popupHideFn() {
       EventUtils.synthesizeKey("KEY_Escape", {}, window);
     },
-    isBookmarkRemoved: false,
+    isBookmarkRemoved: true,
   });
 });
 
 add_task(async function escape_during_autocomplete_should_prevent_autoclose() {
   await test_bookmarks_popup({
     isNewBookmark: true,
     async popupShowFn(browser) {
       EventUtils.synthesizeKey("d", {accelKey: true}, window);
@@ -506,25 +507,28 @@ add_task(async function escape_during_au
       tagsField.focus();
       EventUtils.sendString("a", window);
       await promiseShown;
       ok(promiseShown, "autocomplete shown");
 
       // Select first candidate.
       EventUtils.synthesizeKey("KEY_ArrowDown", {}, window);
 
-      // Type Enter key to choose the item.
+      // Type Escape key to close autocomplete.
       EventUtils.synthesizeKey("KEY_Escape", {}, window);
 
-      Assert.equal(tagsField.value, "Abc",
-        "Autocomplete should've inserted the selected item and shouldn't clear it");
+      // The text reverts to what was typed.
+      // Note, it's important that this is different from the previously
+      // inserted tag, since it will test an untag/tag undo condition.
+      Assert.equal(tagsField.value, "a",
+        "Autocomplete should revert to what was typed");
     },
     shouldAutoClose: false,
     popupHideFn() {
       EventUtils.synthesizeKey("KEY_Escape", {}, window);
     },
-    isBookmarkRemoved: false,
+    isBookmarkRemoved: true,
   });
 });
 
 registerCleanupFunction(function() {
   delete StarUI._closePanelQuickForTesting;
 });
--- a/browser/components/places/content/editBookmark.js
+++ b/browser/components/places/content/editBookmark.js
@@ -2,16 +2,19 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 const MAX_FOLDER_ITEM_IN_MENU_LIST = 5;
 
 var gEditItemOverlay = {
+  // Array of PlacesTransactions accumulated by internal changes. It can be used
+  // to wait for completion.
+  transactionPromises: null,
   _observersAdded: false,
   _staticFoldersListBuilt: false,
 
   _paneInfo: null,
   _setPaneInfo(aInitInfo) {
     if (!aInitInfo)
       return this._paneInfo = null;
 
@@ -191,16 +194,18 @@ var gEditItemOverlay = {
       }
     }
 
     // For sanity ensure that the implementer has uninited the panel before
     // trying to init it again, or we could end up leaking due to observers.
     if (this.initialized)
       this.uninitPanel(false);
 
+    this.transactionPromises = [];
+
     let { parentGuid, isItem, isURI,
           isBookmark, bulkTagging, uris,
           visibleRows, focusedElement,
           onPanelReady } = this._setPaneInfo(aInfo);
 
     let showOrCollapse =
       (rowId, isAppropriateForInput, nameInHiddenRows = null) => {
         let visible = isAppropriateForInput;
@@ -459,16 +464,17 @@ var gEditItemOverlay = {
 
     if (this._folderMenuListListenerAdded) {
       this._folderMenuList.removeEventListener("select", this);
       this._folderMenuListListenerAdded = false;
     }
 
     this._setPaneInfo(null);
     this._firstEditedField = "";
+    this.transactionPromises = [];
   },
 
   get selectedFolderGuid() {
     return this._folderMenuList.selectedItem && this._folderMenuList.selectedItem.folderGuid;
   },
 
   onTagsFieldChange() {
     // Check for _paneInfo existing as the dialog may be closing but receiving
@@ -506,37 +512,48 @@ var gEditItemOverlay = {
     let removedTags = aCurrentTags.filter(t => !lcInputTags.includes(t.toLowerCase()));
     let newTags = inputTags.filter(t => !aCurrentTags.includes(t));
     return { removedTags, newTags };
   },
 
   // Adds and removes tags for one or more uris.
   _setTagsFromTagsInputField(aCurrentTags, aURIs) {
     let { removedTags, newTags } = this._getTagsChanges(aCurrentTags);
-    if (removedTags.length + newTags.length == 0)
+    if (removedTags.length + newTags.length == 0) {
       return false;
+    }
 
-    let setTags = async function() {
+    let setTags = async () => {
+      let promises = [];
       if (removedTags.length > 0) {
-        await PlacesTransactions.Untag({ urls: aURIs, tags: removedTags })
-                                .transact();
+        let promise = PlacesTransactions.Untag({ urls: aURIs, tags: removedTags })
+                                        .transact().catch(Cu.reportError);
+        this.transactionPromises.push(promise);
+        promises.push(promise);
       }
       if (newTags.length > 0) {
-        await PlacesTransactions.Tag({ urls: aURIs, tags: newTags })
-                                .transact();
+        let promise = PlacesTransactions.Tag({ urls: aURIs, tags: newTags })
+                                        .transact().catch(Cu.reportError);
+        this.transactionPromises.push(promise);
+        promises.push(promise);
+      }
+      // Don't use Promise.all because we want these to be executed in order.
+      for (let promise of promises) {
+        await promise;
       }
     };
 
     // Only in the library info-pane it's safe (and necessary) to batch these.
     // TODO bug 1093030: cleanup this mess when the bookmarksProperties dialog
     // and star UI code don't "run a batch in the background".
-    if (window.document.documentElement.id == "places")
-      PlacesTransactions.batch(setTags).catch(Cu.reportError);
-    else
-      setTags().catch(Cu.reportError);
+    if (window.document.documentElement.id == "places") {
+      PlacesTransactions.batch(setTags);
+    } else {
+      setTags();
+    }
     return true;
   },
 
   async _updateTags() {
     let uris = this._paneInfo.bulkTagging ?
                  this._paneInfo.uris : [this._paneInfo.uri];
     let currentTags = this._paneInfo.bulkTagging ?
                         await this._getCommonTags() :
@@ -592,23 +609,28 @@ var gEditItemOverlay = {
       // Get all the bookmarks for the old tag, tag them with the new tag, and
       // untag them from the old tag.
       let oldTag = this._paneInfo.tag;
       this._paneInfo.tag = tag;
       let title = this._paneInfo.title;
       if (title == oldTag) {
         this._paneInfo.title = tag;
       }
-      await PlacesTransactions.RenameTag({ oldTag, tag }).transact();
+      let promise = PlacesTransactions.RenameTag({ oldTag, tag }).transact();
+      this.transactionPromises.push(promise.catch(Cu.reportError));
+      await promise;
       return;
     }
 
     this._mayUpdateFirstEditField("namePicker");
-    await PlacesTransactions.EditTitle({ guid: this._paneInfo.itemGuid,
-                                         title: this._namePicker.value }).transact();
+    let promise = PlacesTransactions.EditTitle({ guid: this._paneInfo.itemGuid,
+                                                 title: this._namePicker.value })
+                                    .transact();
+    this.transactionPromises.push(promise.catch(Cu.reportError));
+    await promise;
   },
 
   onLocationFieldChange() {
     if (this.readOnly || !this._paneInfo.isBookmark)
       return;
 
     let newURI;
     try {
@@ -617,30 +639,30 @@ var gEditItemOverlay = {
       // TODO: Bug 1089141 - Provide some feedback about the invalid url.
       return;
     }
 
     if (this._paneInfo.uri.equals(newURI))
       return;
 
     let guid = this._paneInfo.itemGuid;
-    PlacesTransactions.EditUrl({ guid, url: newURI })
-                      .transact().catch(Cu.reportError);
+    this.transactionPromises.push(PlacesTransactions.EditUrl({ guid, url: newURI })
+                                                    .transact().catch(Cu.reportError));
   },
 
   onKeywordFieldChange() {
     if (this.readOnly || !this._paneInfo.isBookmark)
       return;
 
     let oldKeyword = this._keyword;
     let keyword = this._keyword = this._keywordField.value;
     let postData = this._paneInfo.postData;
     let guid = this._paneInfo.itemGuid;
-    PlacesTransactions.EditKeyword({ guid, keyword, postData, oldKeyword })
-                      .transact().catch(Cu.reportError);
+    this.transactionPromises.push(PlacesTransactions.EditKeyword({ guid, keyword, postData, oldKeyword })
+                                                    .transact().catch(Cu.reportError));
   },
 
   toggleFolderTreeVisibility() {
     var expander = this._element("foldersExpander");
     var folderTreeRow = this._element("folderTreeRow");
     expander.classList.toggle("expander-up", folderTreeRow.collapsed);
     expander.classList.toggle("expander-down", !folderTreeRow.collapsed);
     if (!folderTreeRow.collapsed) {
@@ -712,20 +734,22 @@ var gEditItemOverlay = {
       setTimeout(() => this.toggleFolderTreeVisibility(), 100);
       return;
     }
 
     // Move the item
     let containerGuid = this._folderMenuList.selectedItem.folderGuid;
     if (this._paneInfo.parentGuid != containerGuid &&
         this._paneInfo.itemGuid != containerGuid) {
-      await PlacesTransactions.Move({
+      let promise = PlacesTransactions.Move({
         guid: this._paneInfo.itemGuid,
         newParentGuid: containerGuid,
       }).transact();
+      this.transactionPromises.push(promise.catch(Cu.reportError));
+      await promise;
 
       // Auto-show the bookmarks toolbar when adding / moving an item there.
       if (containerGuid == PlacesUtils.bookmarks.toolbarGuid) {
         Services.obs.notifyObservers(null, "autoshow-bookmarks-toolbar");
       }
     }
 
     // Update folder-tree selection
@@ -844,21 +868,23 @@ var gEditItemOverlay = {
       ip = new PlacesInsertionPoint({
         parentId: PlacesUtils.bookmarksMenuFolderId,
         parentGuid: PlacesUtils.bookmarks.menuGuid,
       });
     }
 
     // XXXmano: add a separate "New Folder" string at some point...
     let title = this._element("newFolderButton").label;
-    let guid = await PlacesTransactions.NewFolder({
+    let promise = PlacesTransactions.NewFolder({
       parentGuid: ip.guid,
       title,
       index: await ip.getIndex(),
-    }).transact().catch(Cu.reportError);
+    }).transact();
+    this.transactionPromises.push(promise.catch(Cu.reportError));
+    let guid = await promise;
 
     this._folderTree.focus();
     this._folderTree.selectItems([ip.guid]);
     PlacesUtils.asContainer(this._folderTree.selectedNode).containerOpen = true;
     this._folderTree.selectItems([guid]);
     this._folderTree.startEditing(this._folderTree.view.selection.currentIndex,
                                   this._folderTree.columns.getFirstColumn());
   },
--- a/browser/components/places/content/editBookmarkPanel.inc.xul
+++ b/browser/components/places/content/editBookmarkPanel.inc.xul
@@ -89,16 +89,17 @@
              control="editBMPanel_tagsField"/>
       <hbox flex="1" align="center">
         <textbox id="editBMPanel_tagsField"
                  type="autocomplete"
                  flex="1"
                  autocompletesearch="places-tag-autocomplete"
                  autocompletepopup="PopupAutoComplete"
                  completedefaultindex="true"
+                 completeselectedindex="true"
                  tabscrolling="true"
                  placeholder="&editBookmarkOverlay.tagsEmptyDesc.label;"
                  onchange="gEditItemOverlay.onTagsFieldChange();"/>
         <button id="editBMPanel_tagsSelectorExpander"
                 class="expander-down panel-button"
                 tooltiptext="&editBookmarkOverlay.tagsExpanderDown.tooltip;"
                 tooltiptextdown="&editBookmarkOverlay.tagsExpanderDown.tooltip;"
                 tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;"
--- a/toolkit/components/places/nsTaggingService.js
+++ b/toolkit/components/places/nsTaggingService.js
@@ -441,16 +441,17 @@ TagAutoCompleteSearch.prototype = {
         searchString = searchString.slice(m[0].length);
       }
     }
 
     // Create a new result to add eventual matches.  Note we need a result
     // regardless having matches.
     let result = Cc["@mozilla.org/autocomplete/simple-result;1"]
                    .createInstance(Ci.nsIAutoCompleteSimpleResult);
+    result.setDefaultIndex(0);
     result.setSearchString(searchString);
 
     let count = 0;
     if (!searchString.length) {
       this.notifyResult(result, count, listener, false);
       return;
     }