Bug 631374 - Adding or removing tags in the selector listbox always scrolls to top.
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 05 May 2011 13:14:21 +0200
changeset 68847 96cbb26bed194d0970a2f8982328d57dea288b2c
parent 68846 3e5d2b7831d0a85c079eb99d7ac0e5460386d74f
child 68848 f1f0632c172f9efbdb9a3728530b2ee4f1111524
push id169
push usermak77@bonardo.net
push dateThu, 05 May 2011 11:21:26 +0000
bugs631374
milestone6.0a1
Bug 631374 - Adding or removing tags in the selector listbox always scrolls to top. r=dietrich
browser/components/places/content/editBookmarkOverlay.js
browser/components/places/tests/chrome/Makefile.in
browser/components/places/tests/chrome/test_bug631374_tags_selector_scroll.xul
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/tests/unit/test_onItemChanged_tags.js
--- a/browser/components/places/content/editBookmarkOverlay.js
+++ b/browser/components/places/content/editBookmarkOverlay.js
@@ -798,30 +798,50 @@ var gEditItemOverlay = {
   },
 
   _rebuildTagsSelectorList: function EIO__rebuildTagsSelectorList() {
     var tagsSelector = this._element("tagsSelector");
     var tagsSelectorRow = this._element("tagsSelectorRow");
     if (tagsSelectorRow.collapsed)
       return;
 
+    // Save the current scroll position and restore it after the rebuild.
+    let firstIndex = tagsSelector.getIndexOfFirstVisibleRow();
+    let selectedIndex = tagsSelector.selectedIndex;
+    let selectedTag = selectedIndex >= 0 ? tagsSelector.selectedItem.label
+                                         : null;
+
     while (tagsSelector.hasChildNodes())
       tagsSelector.removeChild(tagsSelector.lastChild);
 
     var tagsInField = this._getTagsArrayFromTagField();
     var allTags = PlacesUtils.tagging.allTags;
     for (var i = 0; i < allTags.length; i++) {
       var tag = allTags[i];
       var elt = document.createElement("listitem");
       elt.setAttribute("type", "checkbox");
       elt.setAttribute("label", tag);
       if (tagsInField.indexOf(tag) != -1)
         elt.setAttribute("checked", "true");
+      tagsSelector.appendChild(elt);
+      if (selectedTag === tag)
+        selectedIndex = tagsSelector.getIndexOfItem(elt);
+    }
 
-      tagsSelector.appendChild(elt);
+    // Restore position.
+    // The listbox allows to scroll only if the required offset doesn't
+    // overflow its capacity, thus need to adjust the index for removals.
+    firstIndex =
+      Math.min(firstIndex,
+               tagsSelector.itemCount - tagsSelector.getNumberOfVisibleRows());
+    tagsSelector.scrollToIndex(firstIndex);
+    if (selectedIndex >= 0 && tagsSelector.itemCount > 0) {
+      selectedIndex = Math.min(selectedIndex, tagsSelector.itemCount - 1);
+      tagsSelector.selectedIndex = selectedIndex;
+      tagsSelector.ensureIndexIsVisible(selectedIndex);
     }
   },
 
   toggleTagsSelector: function EIO_toggleTagsSelector() {
     var tagsSelector = this._element("tagsSelector");
     var tagsSelectorRow = this._element("tagsSelectorRow");
     var expander = this._element("tagsSelectorExpander");
     if (tagsSelectorRow.collapsed) {
--- a/browser/components/places/tests/chrome/Makefile.in
+++ b/browser/components/places/tests/chrome/Makefile.in
@@ -47,12 +47,13 @@ include $(topsrcdir)/config/rules.mk
 	test_treeview_date.xul \
 	test_bug485100-change-case-loses-tag.xul \
 	test_bug427633_no_newfolder_if_noip.xul \
 	test_0_multiple_left_pane.xul \
 	test_0_bug510634.xul \
 	test_bug549192.xul \
 	test_bug549491.xul \
 	test_editBookmarkOverlay_tags_liveUpdate.xul \
+	test_bug631374_tags_selector_scroll.xul \
 	$(NULL)
 
 libs:: $(_CHROME_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/chrome/test_bug631374_tags_selector_scroll.xul
@@ -0,0 +1,156 @@
+<?xml version="1.0"?>
+
+<!-- Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"
+                 type="text/css"?>
+
+<?xml-stylesheet href="chrome://browser/skin/places/editBookmarkOverlay.css"?>
+<?xml-stylesheet href="chrome://browser/content/places/places.css"?>
+<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>
+
+<?xul-overlay href="chrome://browser/content/places/placesOverlay.xul"?>
+<?xul-overlay href="chrome://browser/content/places/editBookmarkOverlay.xul"?>
+
+<!DOCTYPE window [
+  <!ENTITY % editBookmarkOverlayDTD SYSTEM "chrome://browser/locale/places/editBookmarkOverlay.dtd">
+  %editBookmarkOverlayDTD;
+]>
+
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        title="Bug 631374 - Editing tags in the selector scrolls up the listbox"
+        onload="runTest();">
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/MochiKit/packed.js" />
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js" />
+  <script type="application/javascript"
+          src="chrome://browser/content/places/editBookmarkOverlay.js"/>
+
+  <body xmlns="http://www.w3.org/1999/xhtml" />
+
+  <vbox id="editBookmarkPanelContent"/>
+
+  <script type="application/javascript">
+  <![CDATA[
+
+     /**
+      * This test checks that editing tags doesn't scroll the tags selector
+      * listbox to wrong positions.
+      */
+
+    function runTest() {
+      SimpleTest.waitForExplicitFinish();
+
+      Components.utils.import("resource://gre/modules/NetUtil.jsm");
+
+      let tags = ["a", "b", "c", "d", "e", "f", "g",
+                  "h", "i", "l", "m", "n", "o", "p"];
+
+      // Add a bookmark and tag it.
+      let uri1 = NetUtil.newURI("http://www1.mozilla.org/");
+      let id1 = PlacesUtils.bookmarks.insertBookmark(
+        PlacesUtils.toolbarFolderId, uri1,
+        PlacesUtils.bookmarks.DEFAULT_INDEX, "mozilla"
+      );
+      PlacesUtils.tagging.tagURI(uri1, tags);
+
+      // Add a second bookmark so that tags won't disappear when unchecked.
+      let uri2 = NetUtil.newURI("http://www2.mozilla.org/");
+      let id2 = PlacesUtils.bookmarks.insertBookmark(
+        PlacesUtils.toolbarFolderId, uri2,
+        PlacesUtils.bookmarks.DEFAULT_INDEX, "mozilla"
+      );
+      PlacesUtils.tagging.tagURI(uri2, tags);
+
+      // Init panel.
+      ok(gEditItemOverlay, "gEditItemOverlay is in context");
+      gEditItemOverlay.initPanel(id1);
+      ok(gEditItemOverlay._initialized, "gEditItemOverlay is initialized");
+
+      // Wait for the tags selector to be open.
+      let tagsSelectorRow = document.getElementById("editBMPanel_tagsSelectorRow");
+      tagsSelectorRow.addEventListener("DOMAttrModified", function (event) {
+        tagsSelectorRow.removeEventListener("DOMAttrModified", arguments.callee, false);
+        SimpleTest.executeSoon(function () {
+          let tagsSelector = document.getElementById("editBMPanel_tagsSelector");
+
+          // Go by two so there is some untouched tag in the middle.
+          for (let i = 8; i < tags.length; i += 2) {
+            tagsSelector.selectedIndex = i;
+            let listItem = tagsSelector.selectedItem;
+            SimpleTest.isnot(listItem, null, "Valid listItem found");
+
+            tagsSelector.ensureElementIsVisible(listItem);
+            let visibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
+
+            SimpleTest.ok(listItem.checked, "Item is checked " + i);
+            let selectedTag = listItem.label;
+
+            // Uncheck the tag.
+            listItem.checked = false;
+            SimpleTest.is(visibleIndex,
+                          tagsSelector.getIndexOfFirstVisibleRow(),
+                          "Scroll position did not change");
+
+            // The listbox is rebuilt, so we have to get the new element.
+            let newItem = tagsSelector.selectedItem;
+            SimpleTest.isnot(newItem, null, "Valid new listItem found");
+            SimpleTest.ok(!newItem.checked, "New listItem is unchecked " + i);
+            SimpleTest.is(newItem.label, selectedTag,
+                          "Correct tag is still selected");
+
+            // Check the tag.
+            newItem.checked = true;
+            SimpleTest.is(visibleIndex,
+                          tagsSelector.getIndexOfFirstVisibleRow(),
+                          "Scroll position did not change");
+          }
+
+          // Remove the second bookmark, then nuke some of the tags.
+          PlacesUtils.bookmarks.removeItem(id2);
+
+          // Doing this backwords tests more interesting paths.
+          for (let i = tags.length - 1; i >= 0 ; i -= 2) {
+            tagsSelector.selectedIndex = i;
+            let listItem = tagsSelector.selectedItem;
+            SimpleTest.isnot(listItem, null, "Valid listItem found");
+
+            tagsSelector.ensureElementIsVisible(listItem);
+            let visibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
+
+            SimpleTest.ok(listItem.checked, "Item is checked " + i);
+            let selectedTag = listItem.label;
+
+            // Uncheck the tag.
+            listItem.checked = false;
+            SimpleTest.is(tagsSelector.getIndexOfFirstVisibleRow(),
+                          Math.max(visibleIndex -1, 0),
+                          "Scroll position is correct");
+
+            // The listbox is rebuilt, so we have to get the new element.
+            let newItem = tagsSelector.selectedItem;
+            SimpleTest.isnot(newItem, null, "Valid new listItem found");
+            SimpleTest.ok(newItem.checked, "New listItem is checked " + i);
+            SimpleTest.is(tagsSelector.selectedItem.label,
+                          tags[Math.min(i + 1, tags.length - 2)],
+                          "The next tag is now selected");
+          }
+
+          // Cleanup.
+          PlacesUtils.bookmarks.removeItem(id1);
+
+          SimpleTest.finish();
+        });
+      }, false);
+
+      // Open the tags selector.
+      document.getElementById("editBMPanel_tagsSelectorExpander").doCommand();
+    }
+  ]]>
+  </script>
+
+</window>
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -141,17 +141,26 @@ public:
       (void)stmt->ExecuteAsync(this, getter_AddRefs(pendingStmt));
     }
   }
 
   NS_IMETHOD HandleResult(mozIStorageResultSet* aResultSet)
   {
     nsCOMPtr<mozIStorageRow> row;
     while (NS_SUCCEEDED(aResultSet->GetNextRow(getter_AddRefs(row))) && row) {
-      nsresult rv = row->GetInt64(0, &mData.itemId);
+      // Skip tags, for the use-cases of this async getter they are useless.
+      PRInt64 grandParentId, tagsFolderId;
+      nsresult rv = row->GetInt64(1, &grandParentId);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = mBookmarksSvc->GetTagsFolder(&tagsFolderId);
+      NS_ENSURE_SUCCESS(rv, rv);
+      if (grandParentId == tagsFolderId) {
+        continue;
+      }
+      rv = row->GetInt64(0, &mData.itemId);
       NS_ENSURE_SUCCESS(rv, rv);
       if (mCallback) {
         ((*mBookmarksSvc).*mCallback)(mData);
       }
     }
     return NS_OK;
   }
 
@@ -297,18 +306,19 @@ nsNavBookmarks::GetStatement(const nsCOM
 {
   if (mShuttingDown)
     return nsnull;
 
   // Double ordering covers possible lastModified ties, that could happen when
   // importing, syncing or due to extensions.
   // Note: not using a JOIN is cheaper in this case.
   RETURN_IF_STMT(mDBFindURIBookmarks, NS_LITERAL_CSTRING(
-    "SELECT b.id "
+    "SELECT b.id, t.parent "
     "FROM moz_bookmarks b "
+    "JOIN moz_bookmarks t on t.id = b.parent "
     "WHERE b.fk = (SELECT id FROM moz_places WHERE url = :page_url) "
     "ORDER BY b.lastModified DESC, b.id DESC "));
 
   // Select all children of a given folder, sorted by position.
   // This is a LEFT JOIN because not all bookmarks types have a place.
   // We construct a result where the first columns exactly match those returned
   // by mDBGetURLPageInfo, and additionally contains columns for position,
   // item_child, and folder_child from moz_bookmarks.
@@ -935,17 +945,17 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
   // bookmark-folder result nodes which contain a bookmark for the new
   // bookmark's url
   PRInt64 grandParentId;
   rv = GetFolderIdForItem(aFolder, &grandParentId);
   NS_ENSURE_SUCCESS(rv, rv);
   if (grandParentId == mTagsRoot) {
     // query for all bookmarks for that URI, notify for each
     nsTArray<PRInt64> bookmarks;
-    rv = GetBookmarkIdsForURITArray(aURI, bookmarks);
+    rv = GetBookmarkIdsForURITArray(aURI, bookmarks, true);
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (bookmarks.Length()) {
       for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
         // Don't notify to the same tag entry we just added.
         if (bookmarks[i] == *aNewBookmarkId) {
           continue;
         }
@@ -1068,17 +1078,17 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
 
   if (isTagEntry) {
     // Get all bookmarks pointing to the same uri as this tag entry and
     // notify them that tags changed.
     nsCOMPtr<nsIURI> uri;
     rv = NS_NewURI(getter_AddRefs(uri), spec);
     NS_ENSURE_SUCCESS(rv, rv);
     nsTArray<PRInt64> bookmarks;
-    rv = GetBookmarkIdsForURITArray(uri, bookmarks);
+    rv = GetBookmarkIdsForURITArray(uri, bookmarks, true);
     NS_ENSURE_SUCCESS(rv, rv);
 
     for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
       NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                        nsINavBookmarkObserver,
                        OnItemChanged(bookmarks[i],
                                      NS_LITERAL_CSTRING("tags"), PR_FALSE,
                                      EmptyCString(), 0, TYPE_BOOKMARK));
@@ -1654,17 +1664,17 @@ nsNavBookmarks::RemoveFolderChildren(PRI
       // bookmark's url.
 
       if (child.grandParentId == mTagsRoot) {
         nsCOMPtr<nsIURI> uri;
         rv = NS_NewURI(getter_AddRefs(uri), child.url);
         NS_ENSURE_SUCCESS(rv, rv);
 
         nsTArray<PRInt64> bookmarks;
-        rv = GetBookmarkIdsForURITArray(uri, bookmarks);
+        rv = GetBookmarkIdsForURITArray(uri, bookmarks, true);
         NS_ENSURE_SUCCESS(rv, rv);
 
         if (bookmarks.Length()) {
           for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
             NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                              nsINavBookmarkObserver,
                              OnItemChanged(bookmarks[i],
                                            NS_LITERAL_CSTRING("tags"), PR_FALSE,
@@ -2544,26 +2554,36 @@ nsNavBookmarks::GetFolderIdForItem(PRInt
   // this should not happen, but see bug #400448 for details
   NS_ENSURE_TRUE(aItemId != *aFolderId, NS_ERROR_UNEXPECTED);
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI* aURI,
-                                           nsTArray<PRInt64>& aResult)
+                                           nsTArray<PRInt64>& aResult,
+                                           bool aSkipTags)
 {
   NS_ENSURE_ARG(aURI);
 
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBFindURIBookmarks);
   nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), aURI);
   NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool more;
   while (NS_SUCCEEDED((rv = stmt->ExecuteStep(&more))) && more) {
+    if (aSkipTags) {
+      // Skip tags, for the use-cases of this async getter they are useless.
+      PRInt64 grandParentId;
+      nsresult rv = stmt->GetInt64(1, &grandParentId);
+      NS_ENSURE_SUCCESS(rv, rv);
+      if (grandParentId == mTagsRoot) {
+        continue;
+      }
+    }
     PRInt64 bookmarkId;
     rv = stmt->GetInt64(kFindBookmarksIndex_ID, &bookmarkId);
     NS_ENSURE_SUCCESS(rv, rv);
     NS_ENSURE_TRUE(aResult.AppendElement(bookmarkId), NS_ERROR_OUT_OF_MEMORY);
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
@@ -2578,17 +2598,18 @@ nsNavBookmarks::GetBookmarkIdsForURI(nsI
   NS_ENSURE_ARG_POINTER(aCount);
   NS_ENSURE_ARG_POINTER(aBookmarks);
 
   *aCount = 0;
   *aBookmarks = nsnull;
   nsTArray<PRInt64> bookmarks;
 
   // Get the information from the DB as a TArray
-  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks);
+  // TODO (bug 653816): make this API skip tags by default.
+  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks, false);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Copy the results into a new array for output
   if (bookmarks.Length()) {
     *aBookmarks =
       static_cast<PRInt64*>(nsMemory::Alloc(sizeof(PRInt64) * bookmarks.Length()));
     if (!*aBookmarks)
       return NS_ERROR_OUT_OF_MEMORY;
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -381,19 +381,27 @@ private:
                               PRTime aLastModified,
                               const nsAString& aServiceContractId,
                               PRInt64* _retval);
 
   /**
    * TArray version of getBookmarksIdForURI for ease of use in C++ code.
    * Pass in a reference to a TArray; it will get filled with the
    * resulting list of bookmark IDs.
+   *
+   * @param aURI
+   *        URI to get bookmarks for.
+   * @param aResult
+   *        Array of bookmark ids.
+   * @param aSkipTags
+   *        If true ids of tags-as-bookmarks entries will be excluded.
    */
   nsresult GetBookmarkIdsForURITArray(nsIURI* aURI,
-                                      nsTArray<PRInt64>& aResult);
+                                      nsTArray<PRInt64>& aResult,
+                                      bool aSkipTags);
 
   PRInt64 RecursiveFindRedirectedBookmark(PRInt64 aPlaceId);
 
   /**
    *  You should always use this getter and never use directly the nsCOMPtr.
    */
   mozIStorageStatement* GetStatement(const nsCOMPtr<mozIStorageStatement>& aStmt);
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_onItemChanged_tags.js
@@ -0,0 +1,53 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This test checks that changing a tag for a bookmark with multiple tags
+// notifies OnItemChanged("tags") only once, and not once per tag.
+
+function run_test() {
+  do_test_pending();
+
+  let tags = ["a", "b", "c"];
+  let uri = NetUtil.newURI("http://1.moz.org/");
+
+  let id = PlacesUtils.bookmarks.insertBookmark(
+    PlacesUtils.unfiledBookmarksFolderId, uri,
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 1"
+  );
+  PlacesUtils.tagging.tagURI(uri, tags);
+
+  let bookmarksObserver = {
+    QueryInterface: XPCOMUtils.generateQI([
+      Ci.nsINavBookmarkObserver
+    ]),
+
+    _changedCount: 0,
+    onItemChanged: function (aItemId, aProperty, aIsAnnotationProperty, aValue,
+                             aLastModified, aItemType) {
+      if (aProperty == "tags") {
+        do_check_eq(aItemId, id);
+        this._changedCount++;
+      }
+    },
+
+    onItemRemoved: function (aItemId, aParentId, aIndex, aItemType) {
+      if (aItemId == id) {
+        PlacesUtils.bookmarks.removeObserver(this);
+        do_check_eq(this._changedCount, 2);
+        do_test_finished();
+      }
+    },
+
+    onItemAdded: function () {},
+    onBeginUpdateBatch: function () {},
+    onEndUpdateBatch: function () {},
+    onBeforeItemRemoved: function () {},
+    onItemVisited: function () {},
+    onItemMoved: function () {},
+  };
+  PlacesUtils.bookmarks.addObserver(bookmarksObserver, false);
+
+  PlacesUtils.tagging.tagURI(uri, ["d"]);
+  PlacesUtils.tagging.tagURI(uri, ["e"]);
+  PlacesUtils.bookmarks.removeItem(id);
+}