Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox". r=mak

This uses the same event handling as the "listbox" and "listitem-checkbox" bindings that are scheduled for removal, and copies the required styles locally.

There is no need to preserve the scroll position explicitly anymore, because "richlistbox" handles scrolling like regular elements, and we don't want to persist the position when the selector is closed and reopened.

MozReview-Commit-ID: 4gYhwlprPN7

--HG--
extra : source : a92f9683c6cdaeb5541b85eb77adf46448716a7b

browser/base/content/browser.css

@@ -849,11 +849,6 @@ toolbarspring {
   }
 }
 
-#editBMPanel_tagsSelector {
-  /* override default listbox width from xul.css */
-  width: auto;
-}
-
 menupopup[emptyplacesresult="true"] > .hide-if-empty-places-result {
   display: none;
 }

browser/components/places/content/editBookmark.js

@@ -747,8 +747,6 @@ var gEditItemOverlay = {
     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;
@@ -759,24 +757,22 @@ var gEditItemOverlay = {
 
     let tagsInField = this._getTagsArrayFromTagsInputField();
     let allTags = PlacesUtils.tagging.allTags;
-    for (let tag of allTags) {
-      let elt = document.createElement("listitem");
-      elt.setAttribute("type", "checkbox");
-      elt.setAttribute("label", tag);
+    let fragment = document.createDocumentFragment();
+    for (var i = 0; i < allTags.length; i++) {
+      let tag = allTags[i];
+      let elt = document.createElement("richlistitem");
+      elt.appendChild(document.createElement("image"));
+      let label = document.createElement("label");
+      label.setAttribute("value", tag);
+      elt.appendChild(label);
       if (tagsInField.includes(tag))
         elt.setAttribute("checked", "true");
-      tagsSelector.appendChild(elt);
+      fragment.appendChild(elt);
       if (selectedTag === tag)
-        selectedIndex = tagsSelector.getIndexOfItem(elt);
+        selectedIndex = i;
     }
+    tagsSelector.appendChild(fragment);
 
-    // 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;
@@ -796,12 +792,17 @@ var gEditItemOverlay = {
       this._rebuildTagsSelectorList();
 
       // This is a no-op if we've added the listener.
-      tagsSelector.addEventListener("CheckboxStateChange", this);
+      tagsSelector.addEventListener("mousedown", this);
+      tagsSelector.addEventListener("keypress", this);
     } else {
       expander.className = "expander-down";
       expander.setAttribute("tooltiptext",
                             expander.getAttribute("tooltiptextdown"));
       tagsSelectorRow.collapsed = true;
+
+      // This is a no-op if we've removed the listener.
+      tagsSelector.removeEventListener("mousedown", this);
+      tagsSelector.removeEventListener("keypress", this);
     }
   },
 
@@ -845,25 +846,24 @@ var gEditItemOverlay = {
   },
 
   // EventListener
-  handleEvent(aEvent) {
-    switch (aEvent.type) {
-    case "CheckboxStateChange":
-      // Update the tags field when items are checked/unchecked in the listbox
-      let tags = this._getTagsArrayFromTagsInputField();
-      let tagCheckbox = aEvent.target;
-
-      let curTagIndex = tags.indexOf(tagCheckbox.label);
-      let tagsSelector = this._element("tagsSelector");
-      tagsSelector.selectedItem = tagCheckbox;
-
-      if (tagCheckbox.checked) {
-        if (curTagIndex == -1)
-          tags.push(tagCheckbox.label);
-      } else if (curTagIndex != -1) {
-        tags.splice(curTagIndex, 1);
+  handleEvent(event) {
+    switch (event.type) {
+    case "mousedown":
+      if (event.button == 0) {
+        // Make sure the event is triggered on an item and not the empty space.
+        let item = event.target.closest("richlistbox,richlistitem");
+        if (item.localName == "richlistitem") {
+          this.toggleItemCheckbox(item);
+        }
+      }
+      break;
+    case "keypress":
+      if (event.key == " ") {
+        let item = event.target.currentItem;
+        if (item) {
+          this.toggleItemCheckbox(item);
+        }
       }
-      this._element("tagsField").value = tags.join(", ");
-      this._updateTags();
       break;
     case "unload":
       this.uninitPanel(false);
@@ -871,6 +871,27 @@ var gEditItemOverlay = {
     }
   },
 
+  toggleItemCheckbox(item) {
+    // Update the tags field when items are checked/unchecked in the listbox
+    let tags = this._getTagsArrayFromTagsInputField();
+
+    let curTagIndex = tags.indexOf(item.label);
+    let tagsSelector = this._element("tagsSelector");
+    tagsSelector.selectedItem = item;
+
+    if (!item.hasAttribute("checked")) {
+      item.setAttribute("checked", "true");
+      if (curTagIndex == -1)
+        tags.push(item.label);
+    } else {
+      item.removeAttribute("checked");
+      if (curTagIndex != -1)
+        tags.splice(curTagIndex, 1);
+    }
+    this._element("tagsField").value = tags.join(", ");
+    this._updateTags();
+  },
+
   _initTagsField() {
     let tags;
     if (this._paneInfo.isURI)

browser/components/places/content/editBookmarkPanel.inc.xul

@@ -107,9 +107,10 @@
     </vbox>
 
     <vbox id="editBMPanel_tagsSelectorRow"
-         collapsed="true">
-      <listbox id="editBMPanel_tagsSelector"
-               height="150"/>
+          collapsed="true">
+      <richlistbox id="editBMPanel_tagsSelector"
+                   styled="true"
+                   height="150"/>
     </vbox>
 
     <vbox id="editBMPanel_keywordRow"

browser/components/places/tests/browser/browser_bug631374_tags_selector_scroll.js

@@ -5,6 +5,15 @@
 
 const TEST_URL = "about:buildconfig";
 
+function scrolledIntoView(item, parentItem) {
+  let itemRect = item.getBoundingClientRect();
+  let parentItemRect = parentItem.getBoundingClientRect();
+  let pointInView = y => parentItemRect.top < y && y < parentItemRect.bottom;
+
+  // Partially visible items are also considered visible.
+  return pointInView(itemRect.top) || pointInView(itemRect.bottom);
+}
+
 add_task(async function() {
   await PlacesUtils.bookmarks.eraseEverything();
   let tags = ["a", "b", "c", "d", "e", "f", "g",
@@ -63,32 +72,30 @@ add_task(async function() {
     isnot(listItem, null, "Valid listItem found");
 
     tagsSelector.ensureElementIsVisible(listItem);
-    let visibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
+    let scrollTop = tagsSelector.scrollTop;
 
-    ok(listItem.checked, "Item is checked " + i);
+    ok(listItem.hasAttribute("checked"), "Item is checked " + i);
     let selectedTag = listItem.label;
 
     // Uncheck the tag.
     let promiseNotification = PlacesTestUtils.waitForNotification(
       "onItemChanged", (id, property) => property == "tags");
-    listItem.checked = false;
+    EventUtils.synthesizeMouseAtCenter(listItem.firstChild, {});
     await promiseNotification;
-    is(visibleIndex, tagsSelector.getIndexOfFirstVisibleRow(),
-       "Scroll position did not change");
+    is(scrollTop, tagsSelector.scrollTop, "Scroll position did not change");
 
     // The listbox is rebuilt, so we have to get the new element.
     let newItem = tagsSelector.selectedItem;
     isnot(newItem, null, "Valid new listItem found");
-    ok(!newItem.checked, "New listItem is unchecked " + i);
+    ok(!newItem.hasAttribute("checked"), "New listItem is unchecked " + i);
     is(newItem.label, selectedTag, "Correct tag is still selected");
 
     // Check the tag.
     promiseNotification = PlacesTestUtils.waitForNotification(
       "onItemChanged", (id, property) => property == "tags");
-    newItem.checked = true;
+    EventUtils.synthesizeMouseAtCenter(newItem.firstChild, {});
     await promiseNotification;
-    is(visibleIndex, tagsSelector.getIndexOfFirstVisibleRow(),
-       "Scroll position did not change");
+    is(scrollTop, tagsSelector.scrollTop, "Scroll position did not change");
   }
 
   // Remove the second bookmark, then nuke some of the tags.
@@ -101,28 +108,24 @@ add_task(async function() {
     isnot(listItem, null, "Valid listItem found");
 
     tagsSelector.ensureElementIsVisible(listItem);
-    let firstVisibleTag = tags[tagsSelector.getIndexOfFirstVisibleRow()];
+    let items = [...tagsSelector.children];
+    let topTag = items.find(e => scrolledIntoView(e, tagsSelector)).label;
 
-    ok(listItem.checked, "Item is checked " + i);
+    ok(listItem.hasAttribute("checked"), "Item is checked " + i);
 
     // Uncheck the tag.
     let promiseNotification = PlacesTestUtils.waitForNotification(
       "onItemChanged", (id, property) => property == "tags");
-    listItem.checked = false;
+    EventUtils.synthesizeMouseAtCenter(listItem.firstChild, {});
     await promiseNotification;
 
-    // Ensure the first visible tag is still visible in the list.
-    let firstVisibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
-    let lastVisibleIndex = firstVisibleIndex + tagsSelector.getNumberOfVisibleRows() - 1;
-    let expectedTagIndex = tags.indexOf(firstVisibleTag);
-    ok(expectedTagIndex >= firstVisibleIndex &&
-       expectedTagIndex <= lastVisibleIndex,
-       "Scroll position is correct");
-
     // The listbox is rebuilt, so we have to get the new element.
+    let topItem = [...tagsSelector.children].find(e => e.label == topTag);
+    ok(scrolledIntoView(topItem, tagsSelector), "Scroll position is correct");
+
     let newItem = tagsSelector.selectedItem;
     isnot(newItem, null, "Valid new listItem found");
-    ok(newItem.checked, "New listItem is checked " + i);
+    ok(newItem.hasAttribute("checked"), "New listItem is checked " + i);
     is(tagsSelector.selectedItem.label,
        tags[Math.min(i + 1, tags.length - 2)],
        "The next tag is now selected");

browser/components/places/tests/browser/browser_editBookmark_tags_liveUpdate.js

@@ -9,7 +9,7 @@ function checkTagsSelector(aAvailableTags, aCheckedTags) {
       "Found expected number of tags in the tags selector");
 
   Array.prototype.forEach.call(children, function(aChild) {
-    let tag = aChild.getAttribute("label");
+    let tag = aChild.querySelector("label").getAttribute("value");
     ok(true, "Found tag '" + tag + "' in the selector");
     ok(aAvailableTags.includes(tag), "Found expected tag");
     let checked = aChild.getAttribute("checked") == "true";

browser/themes/linux/places/editBookmark.css

@@ -58,6 +58,14 @@
   visibility: collapse;
 }
 
+#editBMPanel_tagsSelector > richlistitem > image {
+  -moz-appearance: checkbox;
+  -moz-box-align: center;
+  margin: 0 2px;
+  min-width: 13px;
+  min-height: 13px;
+}
+
 
 /* Bookmark panel dropdown menu items */
 #editBMPanel_folderMenuList[selectedIndex="0"],

browser/themes/osx/places/editBookmark.css

@@ -60,6 +60,20 @@
   visibility: collapse;
 }
 
+#editBMPanel_tagsSelector > richlistitem > image {
+  -moz-appearance: checkbox;
+  -moz-box-align: center;
+  margin: 0px 2px;
+  border: 1px solid -moz-DialogText;
+  min-width: 13px;
+  min-height: 13px;
+  background: -moz-Field no-repeat 50% 50%;
+}
+
+#editBMPanel_tagsSelector > richlistitem[checked="true"] > image {
+  background-image: url("chrome://global/skin/checkbox/cbox-check.gif");
+}
+
 
 /* ----- BOOKMARK PANEL DROPDOWN MENU ITEMS ----- */
 

browser/themes/windows/places/editBookmark.css

@@ -63,6 +63,16 @@
   visibility: collapse;
 }
 
+#editBMPanel_tagsSelector > richlistitem > image {
+  -moz-appearance: checkbox;
+  -moz-box-align: center;
+  margin: 0px 2px;
+  border: 1px solid -moz-DialogText;
+  min-width: 13px;
+  min-height: 13px;
+  background: -moz-Field no-repeat 50% 50%;
+}
+
 
 /* ::::: bookmark panel dropdown icons ::::: */