Bug 1516763 - Remove the feature to set a direction on the "richlistbox" element. r=NeilDeakin
authorTimothy Guan-tin Chien <timdream@gmail.com>
Sat, 29 Dec 2018 13:05:04 +0000
changeset 452106 a62a47488906d8cb8d793a53e89a7fe66246d602
parent 452105 51b648c8884895aa47c40cb241cf60eea1730ff1
child 452107 0b47b25aa76748849585085d121d211cd173ae5a
child 452146 909a36b5155fd84d7ab1b7f987f751362543187f
push id110810
push userpaolo.mozmail@amadzone.org
push dateSat, 29 Dec 2018 18:30:17 +0000
treeherdermozilla-inbound@a62a47488906 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersNeilDeakin
bugs1516763, 490178
milestone66.0a1
first release with
nightly linux32
a62a47488906 / 66.0a1 / 20181229214252 / files
nightly linux64
a62a47488906 / 66.0a1 / 20181229214252 / files
nightly mac
a62a47488906 / 66.0a1 / 20181229214252 / files
nightly win32
a62a47488906 / 66.0a1 / 20181229214252 / files
nightly win64
a62a47488906 / 66.0a1 / 20181229214252 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1516763 - Remove the feature to set a direction on the "richlistbox" element. r=NeilDeakin This reverts the change implemented in bug 490178 in preparation for removing the inner scrollbox of the "richlistbox" binding.
toolkit/content/tests/chrome/chrome.ini
toolkit/content/tests/chrome/test_richlist_direction.xul
toolkit/content/tests/chrome/test_richlistbox.xul
toolkit/content/widgets/richlistbox.xml
--- a/toolkit/content/tests/chrome/chrome.ini
+++ b/toolkit/content/tests/chrome/chrome.ini
@@ -167,17 +167,17 @@ skip-if = (verify && (os == 'win'))
 [test_position.xul]
 [test_preferences.xul]
 [test_preferences_beforeaccept.xul]
 support-files = window_preferences_beforeaccept.xul
 [test_preferences_onsyncfrompreference.xul]
 support-files = window_preferences_onsyncfrompreference.xul
 [test_props.xul]
 [test_radio.xul]
-[test_richlist_direction.xul]
+[test_richlistbox.xul]
 [test_righttoleft.xul]
 [test_screenPersistence.xul]
 [test_scrollbar.xul]
 [test_showcaret.xul]
 [test_subframe_origin.xul]
 [test_tabbox.xul]
 [test_tabindex.xul]
 [test_textbox_dictionary.xul]
rename from toolkit/content/tests/chrome/test_richlist_direction.xul
rename to toolkit/content/tests/chrome/test_richlistbox.xul
--- a/toolkit/content/tests/chrome/test_richlist_direction.xul
+++ b/toolkit/content/tests/chrome/test_richlistbox.xul
@@ -37,102 +37,65 @@ function test_richlistbox()
   do {
     item = richListBox.appendItem("Test", "");
     item.height = item.minHeight = item.maxHeight = Math.floor(height / 4);
   } while (item.getBoundingClientRect().bottom < (height * 2))
   richListBox.appendItem("Test", "");
   richListBox.firstChild.nextSibling.id = "list-box-first";
   richListBox.lastChild.previousSibling.id = "list-box-last";
 
-  // direction = "reverse", the values here are backwards due to the fact that
-  // richlistboxes respond differently when a user initiates a selection
-  richListBox.dir = "reverse";
   var count = richListBox.itemCount;
   richListBox.focus();
-  richListBox.selectedIndex = count - 1;
-  sendKey("DOWN");
-  is(richListBox.currentIndex, count - 2, "Selection should move to the next item");
-  sendKey("UP");
-  is(richListBox.currentIndex, count - 1, "Selection should move to the previous item");
-  sendKey("END");
-  is(richListBox.currentIndex, 0, "Selection should move to the last item");
-  sendKey("HOME");
-  is(richListBox.currentIndex, count - 1, "Selection should move to the first item");
-  var currentIndex = richListBox.currentIndex;
-  var index = getScrollIndexAmount(-1);
-  sendKey("PAGE_DOWN");
-  is(richListBox.currentIndex, index, "Selection should move to one page down");
-  ok(richListBox.currentIndex < currentIndex, "Selection should move downwards");
-  sendKey("END");
-  currentIndex = richListBox.currentIndex;
-  index = getScrollIndexAmount(1);
-  sendKey("PAGE_UP");
-  is(richListBox.currentIndex, index, "Selection should move to one page up");
-  ok(richListBox.currentIndex > currentIndex, "Selection should move upwards");
-  richListBox.selectedItem = richListBox.lastChild;
-  richListBox.focus();
-  synthesizeKey("KEY_ArrowDown", {shiftKey: true}, window);
-  let items = [richListBox.selectedItems[0],
-               richListBox.selectedItems[1]];
-  is(items[0], richListBox.lastChild, "The last element should still be selected");
-  is(items[1], richListBox.lastChild.previousSibling, "Both elements should now be selected");
-  richListBox.clearSelection();
-  richListBox.selectedItem = richListBox.lastChild;
-  sendMouseEvent({type: "click", shiftKey: true, clickCount: 1},
-                 "list-box-last",
-                 window);
-  items = [richListBox.selectedItems[0],
-           richListBox.selectedItems[1]];
-  is(items[0], richListBox.lastChild, "The last element should still be selected");
-  is(items[1], richListBox.lastChild.previousSibling, "Both elements should now be selected");
-  richListBox.addEventListener("keypress", function(aEvent) {
-    richListBox.removeEventListener("keypress", arguments.callee, true);
-    aEvent.preventDefault();
-  }, true);
-  richListBox.selectedIndex = 1;
-  sendKey("HOME");
-  is(richListBox.selectedIndex, 1, "A stopped event should return indexing to normal");
 
-  // direction = "normal"
-  richListBox.dir = "normal";
-  richListBox.selectedIndex = 0;
-  sendKey("DOWN");
-  is(richListBox.currentIndex, 1, "Selection should move to the next item");
-  sendKey("UP");
-  is(richListBox.currentIndex, 0, "Selection should move to the previous item");
-  sendKey("END");
-  is(richListBox.currentIndex, count - 1, "Selection should move to the last item");
-  sendKey("HOME");
-  is(richListBox.currentIndex, 0, "Selection should move to the first item");
-  var currentIndex = richListBox.currentIndex;
-  var index = richListBox.scrollOnePage(1);
-  sendKey("PAGE_DOWN");
-  is(richListBox.currentIndex, index, "Selection should move to one page down");
-  ok(richListBox.currentIndex > currentIndex, "Selection should move downwards");
-  sendKey("END");
-  currentIndex = richListBox.currentIndex;
-  index = richListBox.scrollOnePage(-1) + richListBox.currentIndex;
-  sendKey("PAGE_UP");
-  is(richListBox.currentIndex, index, "Selection should move to one page up");
-  ok(richListBox.currentIndex < currentIndex, "Selection should move upwards");
-  richListBox.selectedItem = richListBox.firstChild;
-  richListBox.focus();
-  synthesizeKey("KEY_ArrowDown", {shiftKey: true}, window);
-  items = [richListBox.selectedItems[0],
-           richListBox.selectedItems[1]];
-  is(items[0], richListBox.firstChild, "The last element should still be selected");
-  is(items[1], richListBox.firstChild.nextSibling, "Both elements should now be selected");
-  richListBox.clearSelection();
-  richListBox.selectedItem = richListBox.firstChild;
-  sendMouseEvent({type: "click", shiftKey: true, clickCount: 1},
-                 "list-box-first",
-                 window);
-  items = [richListBox.selectedItems[0],
-           richListBox.selectedItems[1]];
-  is(items[0], richListBox.firstChild, "The last element should still be selected");
-  is(items[1], richListBox.firstChild.nextSibling, "Both elements should now be selected");
+  // Test that dir="reverse" is ignored and behaves the same as dir="normal".
+  for (let dir of ["reverse", "normal"]) {
+    richListBox.dir = dir;
+    richListBox.selectedIndex = 0;
+    sendKey("DOWN");
+    is(richListBox.currentIndex, 1, "Selection should move to the next item");
+    sendKey("UP");
+    is(richListBox.currentIndex, 0, "Selection should move to the previous item");
+    sendKey("END");
+    is(richListBox.currentIndex, count - 1, "Selection should move to the last item");
+    sendKey("HOME");
+    is(richListBox.currentIndex, 0, "Selection should move to the first item");
+    var currentIndex = richListBox.currentIndex;
+    var index = richListBox.scrollOnePage(1);
+    sendKey("PAGE_DOWN");
+    is(richListBox.currentIndex, index, "Selection should move to one page down");
+    ok(richListBox.currentIndex > currentIndex, "Selection should move downwards");
+    sendKey("END");
+    currentIndex = richListBox.currentIndex;
+    index = richListBox.scrollOnePage(-1) + richListBox.currentIndex;
+    sendKey("PAGE_UP");
+    is(richListBox.currentIndex, index, "Selection should move to one page up");
+    ok(richListBox.currentIndex < currentIndex, "Selection should move upwards");
+    richListBox.selectedItem = richListBox.firstChild;
+    richListBox.focus();
+    synthesizeKey("KEY_ArrowDown", {shiftKey: true}, window);
+    items = [richListBox.selectedItems[0],
+             richListBox.selectedItems[1]];
+    is(items[0], richListBox.firstChild, "The last element should still be selected");
+    is(items[1], richListBox.firstChild.nextSibling, "Both elements should now be selected");
+    richListBox.clearSelection();
+    richListBox.selectedItem = richListBox.firstChild;
+    sendMouseEvent({type: "click", shiftKey: true, clickCount: 1},
+                   "list-box-first",
+                   window);
+    items = [richListBox.selectedItems[0],
+             richListBox.selectedItems[1]];
+    is(items[0], richListBox.firstChild, "The last element should still be selected");
+    is(items[1], richListBox.firstChild.nextSibling, "Both elements should now be selected");
+    richListBox.addEventListener("keypress", function(aEvent) {
+      richListBox.removeEventListener("keypress", arguments.callee, true);
+      aEvent.preventDefault();
+    }, true);
+    richListBox.selectedIndex = 1;
+    sendKey("HOME");
+    is(richListBox.selectedIndex, 1, "A stopped event should return indexing to normal");
+  }
   SimpleTest.finish();
 }
 
 ]]>
 </script>
 
 </window>
--- a/toolkit/content/widgets/richlistbox.xml
+++ b/toolkit/content/widgets/richlistbox.xml
@@ -59,28 +59,23 @@
             this.dispatchEvent(event);
 
             // always call this (allows a commandupdater without controller)
             document.commandDispatcher.updateCommands("richlistbox-select");
           ]]>
         </body>
       </method>
 
-      <!-- We override base-listbox here because those methods don't take dir
-           into account on listbox (which doesn't support dir yet) -->
       <method name="getNextItem">
         <parameter name="aStartItem"/>
         <parameter name="aDelta"/>
         <body>
         <![CDATA[
-          var prop = this.dir == "reverse" && this._mayReverse ?
-                                                "previousSibling" :
-                                                "nextSibling";
           while (aStartItem) {
-            aStartItem = aStartItem[prop];
+            aStartItem = aStartItem.nextSibling;
             if (aStartItem && aStartItem.localName == "richlistitem" &&
                 (!this._userSelecting || this._canUserSelect(aStartItem))) {
               --aDelta;
               if (aDelta == 0)
                 return aStartItem;
             }
           }
           return null;
@@ -88,21 +83,18 @@
         </body>
       </method>
 
       <method name="getPreviousItem">
         <parameter name="aStartItem"/>
         <parameter name="aDelta"/>
         <body>
         <![CDATA[
-          var prop = this.dir == "reverse" && this._mayReverse ?
-                                                "nextSibling" :
-                                                "previousSibling";
           while (aStartItem) {
-            aStartItem = aStartItem[prop];
+            aStartItem = aStartItem.previousSibling;
             if (aStartItem && aStartItem.localName == "richlistitem" &&
                 (!this._userSelecting || this._canUserSelect(aStartItem))) {
               --aDelta;
               if (aDelta == 0)
                 return aStartItem;
             }
           }
           return null;
@@ -575,19 +567,16 @@
         </body>
       </method>
 
       <property name="itemChildren" readonly="true">
         <getter>
           <![CDATA[
             let children = Array.from(this.children)
                                 .filter(node => node.localName == "richlistitem");
-            if (this.dir == "reverse" && this._mayReverse) {
-              children.reverse();
-            }
             return children;
           ]]>
         </getter>
       </property>
 
       <method name="_refreshSelection">
         <body>
           <![CDATA[
@@ -618,19 +607,17 @@
                 if (this.selType != "multiple" && this.selectedCount == 0)
                   this.selectedItem = currentItem;
 
                 if (this._scrollbox.boxObject.height) {
                   this.ensureElementIsVisible(currentItem);
                 } else {
                   // XXX hack around a bug in ensureElementIsVisible as it will
                   // scroll beyond the last element, bug 493645.
-                  var previousElement = this.dir == "reverse" ? currentItem.nextElementSibling :
-                                                                currentItem.previousElementSibling;
-                  this.ensureElementIsVisible(previousElement);
+                  this.ensureElementIsVisible(currentItem.previousElementSibling);
                 }
               }
               this._suppressOnSelect = suppressSelect;
               // XXX actually it's just a refresh, but at least
               // the Extensions manager expects this:
               this._fireOnSelect();
               return;
             }
@@ -739,20 +726,18 @@
 
       <method name="_moveByOffsetFromUserEvent">
         <parameter name="aOffset"/>
         <parameter name="aEvent"/>
         <body>
         <![CDATA[
           if (!aEvent.defaultPrevented) {
             this._userSelecting = true;
-            this._mayReverse = true;
             this.moveByOffset(aOffset, !aEvent.ctrlKey, aEvent.shiftKey);
             this._userSelecting = false;
-            this._mayReverse = false;
             aEvent.preventDefault();
           }
         ]]>
         </body>
       </method>
 
       <method name="_canUserSelect">
         <parameter name="aItem"/>
@@ -795,17 +780,16 @@
         </body>
       </method>
 
       <field name="_currentIndex">null</field>
       <field name="_lastKeyTime">0</field>
       <field name="_incrementalString">""</field>
       <field name="_suppressOnSelect">false</field>
       <field name="_userSelecting">false</field>
-      <field name="_mayReverse">false</field>
       <field name="_selectTimeout">null</field>
       <field name="_currentItem">null</field>
       <field name="_selectionStart">null</field>
 
       <!-- For backwards-compatibility and for convenience.
         Use ensureElementIsVisible instead -->
       <method name="ensureSelectedElementIsVisible">
         <body>
@@ -823,46 +807,38 @@
 
       <handler event="keypress" keycode="VK_DOWN" modifiers="control shift any"
                action="this._moveByOffsetFromUserEvent(1, event);"
                group="system"/>
 
       <handler event="keypress" keycode="VK_HOME" modifiers="control shift any"
                group="system">
         <![CDATA[
-          this._mayReverse = true;
           this._moveByOffsetFromUserEvent(-this.currentIndex, event);
-          this._mayReverse = false;
         ]]>
       </handler>
 
       <handler event="keypress" keycode="VK_END" modifiers="control shift any"
                group="system">
         <![CDATA[
-          this._mayReverse = true;
           this._moveByOffsetFromUserEvent(this.getRowCount() - this.currentIndex - 1, event);
-          this._mayReverse = false;
         ]]>
       </handler>
 
       <handler event="keypress" keycode="VK_PAGE_UP" modifiers="control shift any"
                group="system">
         <![CDATA[
-          this._mayReverse = true;
           this._moveByOffsetFromUserEvent(this.scrollOnePage(-1), event);
-          this._mayReverse = false;
         ]]>
       </handler>
 
       <handler event="keypress" keycode="VK_PAGE_DOWN" modifiers="control shift any"
                group="system">
         <![CDATA[
-          this._mayReverse = true;
           this._moveByOffsetFromUserEvent(this.scrollOnePage(1), event);
-          this._mayReverse = false;
         ]]>
       </handler>
 
       <handler event="keypress" key=" " modifiers="control" phase="target">
         <![CDATA[
           if (this.currentItem && this.selType == "multiple")
             this.toggleItemSelection(this.currentItem);
         ]]>