Bug 843187 - Variables view: going down through the properties via keyboard is really broken, r=vporof

browser/devtools/debugger/test/browser_dbg_propertyview-data.js

@@ -653,12 +653,12 @@ function testKeyboardAccessibility(callback) {
                 "The 0 item should be focused now.");
 
               EventUtils.sendKey("END", gDebugger);
-              is(gVariablesView.getFocusedItem().name, "foo",
+              is(gVariablesView.getFocusedItem().name, "bar",
-                "The foo item should be focused now.");
+                "The bar item should be focused now.");
 
               EventUtils.sendKey("DOWN", gDebugger);
               is(gVariablesView.getFocusedItem().name, "bar",
-                "The bar item should be focused now.");
+                "The bar item should still be focused now.");
 
               EventUtils.sendKey("UP", gDebugger);
               is(gVariablesView.getFocusedItem().name, "foo",
@@ -669,10 +669,14 @@ function testKeyboardAccessibility(callback) {
                 "The foo item should still be focused now.");
 
               EventUtils.sendKey("PAGE_DOWN", gDebugger);
-              is(gVariablesView.getFocusedItem().name, "foo",
+              is(gVariablesView.getFocusedItem().name, "bar",
-                "The foo item should still be focused now.");
+                "The bar item should be focused now.");
 
               EventUtils.sendKey("PAGE_UP", gDebugger);
+              is(gVariablesView.getFocusedItem().name, "someProp7",
+                "The someProp7 item should be focused now.");
+
+              EventUtils.sendKey("UP", gDebugger);
               is(gVariablesView.getFocusedItem().name, "__proto__",
                 "The __proto__ item should be focused now.");
 
@@ -684,10 +688,6 @@ function testKeyboardAccessibility(callback) {
               is(gVariablesView.getFocusedItem().name, "get",
                 "The get item should be focused now.");
 
-              EventUtils.sendKey("UP", gDebugger);
-              is(gVariablesView.getFocusedItem().name, "p8",
-                "The p8 item should be focused now.");
-
               EventUtils.sendKey("HOME", gDebugger);
               is(gVariablesView.getFocusedItem().name, "someProp0",
                 "The someProp0 item should be focused now.");
@@ -828,6 +828,18 @@ function testKeyboardAccessibility(callback) {
               is(gVariablesView.getFocusedItem().expanded, false,
                 "The top-level __proto__ item should not be expanded.");
 
+              EventUtils.sendKey("END", gDebugger);
+              is(gVariablesView.getFocusedItem().name, "foo",
+                "The foo scope should be focused.");
+
+              EventUtils.sendKey("PAGE_UP", gDebugger);
+              is(gVariablesView.getFocusedItem().name, "__proto__",
+                "The __proto__ property should be focused.");
+
+              EventUtils.sendKey("PAGE_DOWN", gDebugger);
+              is(gVariablesView.getFocusedItem().name, "foo",
+                "The foo scope should be focused.");
+
               executeSoon(callback);
             });
           });

browser/devtools/shared/widgets/VariablesView.jsm

@@ -52,6 +52,7 @@ const STR = Services.strings.createBundle(DBG_STRINGS_URI);
  */
 this.VariablesView = function VariablesView(aParentNode, aFlags = {}) {
   this._store = new Map();
+  this._items = [];
   this._itemsByElement = new WeakMap();
   this._prevHierarchy = new Map();
   this._currHierarchy = new Map();
@@ -103,6 +104,7 @@ VariablesView.prototype = {
 
     let scope = new Scope(this, aName);
     this._store.set(scope.id, scope);
+    this._items.push(scope);
     this._currHierarchy.set(aName, scope);
     this._itemsByElement.set(scope._target, scope);
     scope.header = !!aName;
@@ -135,6 +137,7 @@ VariablesView.prototype = {
     }
 
     this._store.clear();
+    this._items.length = 0;
     this._itemsByElement.clear();
 
     this._appendEmptyNotice();
@@ -161,6 +164,7 @@ VariablesView.prototype = {
     let currList = this._list = this.document.createElement("scrollbox");
 
     this._store.clear();
+    this._items.length = 0;
     this._itemsByElement.clear();
 
     this._emptyTimeout = this.window.setTimeout(function() {
@@ -529,64 +533,73 @@ VariablesView.prototype = {
   },
 
   /**
-   * Focuses the first visible variable or property in this container.
+   * Find the first item in the tree of visible items in this container that
+   * matches the predicate. Searches in visual order (the order seen by the
+   * user). Descends into each scope to check the scope and its children.
+   *
+   * @param function aPredicate
+   *        A function that returns true when a match is found.
+   * @return Scope | Variable | Property
+   *         The first visible scope, variable or property, or null if nothing
+   *         is found.
+   */
+  _findInVisibleItems: function VV__findInVisibleItems(aPredicate) {
+    for (let scope of this._items) {
+      let result = scope._findInVisibleItems(aPredicate);
+      if (result) {
+        return result;
+      }
+    }
+    return null;
+  },
+
+  /**
+   * Find the last item in the tree of visible items in this container that
+   * matches the predicate. Searches in reverse visual order (opposite of the
+   * order seen by the user). Descends into each scope to check the scope and
+   * its children.
+   *
+   * @param function aPredicate
+   *        A function that returns true when a match is found.
+   * @return Scope | Variable | Property
+   *         The last visible scope, variable or property, or null if nothing
+   *         is found.
+   */
+  _findInVisibleItemsReverse: function VV__findInVisibleItemsReverse(aPredicate) {
+    for (let i = this._items.length - 1; i >= 0; i--) {
+      let scope = this._items[i];
+      let result = scope._findInVisibleItemsReverse(aPredicate);
+      if (result) {
+        return result;
+      }
+    }
+    return null;
+  },
+
+  /**
+   * Focuses the first visible scope, variable, or property in this container.
    */
   focusFirstVisibleNode: function VV_focusFirstVisibleNode() {
-    let property, variable, scope;
+    let focusableItem = this._findInVisibleItems(item => item.focusable);
 
-    for (let [, item] of this._currHierarchy) {
+    if (focusableItem) {
-      if (!item.focusable) {
+      this._focusItem(focusableItem);
-        continue;
-      }
-      if (item instanceof Property) {
-        property = item;
-        break;
-      } else if (item instanceof Variable) {
-        variable = item;
-        break;
-      } else if (item instanceof Scope) {
-        scope = item;
-        break;
-      }
-    }
-    if (scope) {
-      this._focusItem(scope);
-    } else if (variable) {
-      this._focusItem(variable);
-    } else if (property) {
-      this._focusItem(property);
     }
     this._parent.scrollTop = 0;
     this._parent.scrollLeft = 0;
   },
 
   /**
-   * Focuses the last visible variable or property in this container.
+   * Focuses the last visible scope, variable, or property in this container.
    */
   focusLastVisibleNode: function VV_focusLastVisibleNode() {
-    let property, variable, scope;
+    let focusableItem = this._findInVisibleItemsReverse(item => item.focusable);
 
-    for (let [, item] of this._currHierarchy) {
+    if (focusableItem) {
-      if (!item.focusable) {
+      this._focusItem(focusableItem);
-        continue;
     }
-      if (item instanceof Property) {
-        property = item;
-      } else if (item instanceof Variable) {
-        variable = item;
-      } else if (item instanceof Scope) {
-        scope = item;
-      }
-    }
-    if (property && (!variable || property.isDescendantOf(variable))) {
-      this._focusItem(property);
-    } else if (variable && (!scope || variable.isDescendantOf(scope))) {
-      this._focusItem(variable);
-    } else if (scope) {
-      this._focusItem(scope);
     this._parent.scrollTop = this._parent.scrollHeight;
     this._parent.scrollLeft = 0;
-    }
   },
 
   /**
@@ -888,6 +901,7 @@ VariablesView.prototype = {
   _window: null,
 
   _store: null,
+  _items: null,
   _prevHierarchy: null,
   _currHierarchy: null,
   _enumVisible: true,
@@ -1082,6 +1096,8 @@ function Scope(aView, aName, aFlags = {}) {
   this.separatorStr = aView.separatorStr;
 
   this._store = new Map();
+  this._enumItems = [];
+  this._nonEnumItems = [];
   this._init(aName.trim(), aFlags);
 }
 
@@ -1785,6 +1801,89 @@ Scope.prototype = {
     return null;
   },
 
+  /**
+   * Find the first item in the tree of visible items in this item that matches
+   * the predicate. Searches in visual order (the order seen by the user).
+   * Tests itself, then descends into first the enumerable children and then
+   * the non-enumerable children (since they are presented in separate groups).
+   *
+   * @param function aPredicate
+   *        A function that returns true when a match is found.
+   * @return Scope | Variable | Property
+   *         The first visible scope, variable or property, or null if nothing
+   *         is found.
+   */
+  _findInVisibleItems: function S__findInVisibleItems(aPredicate) {
+    if (aPredicate(this)) {
+      return this;
+    }
+
+    if (this._isExpanded) {
+      if (this._variablesView._enumVisible) {
+        for (let item of this._enumItems) {
+          let result = item._findInVisibleItems(aPredicate);
+          if (result) {
+            return result;
+          }
+        }
+      }
+
+      if (this._variablesView._nonEnumVisible) {
+        for (let item of this._nonEnumItems) {
+          let result = item._findInVisibleItems(aPredicate);
+          if (result) {
+            return result;
+          }
+        }
+      }
+    }
+
+    return null;
+  },
+
+  /**
+   * Find the last item in the tree of visible items in this item that matches
+   * the predicate. Searches in reverse visual order (opposite of the order
+   * seen by the user). Descends into first the non-enumerable children, then
+   * the enumerable children (since they are presented in separate groups), and
+   * finally tests itself.
+   *
+   * @param function aPredicate
+   *        A function that returns true when a match is found.
+   * @return Scope | Variable | Property
+   *         The last visible scope, variable or property, or null if nothing
+   *         is found.
+   */
+  _findInVisibleItemsReverse: function S__findInVisibleItemsReverse(aPredicate) {
+    if (this._isExpanded) {
+      if (this._variablesView._nonEnumVisible) {
+        for (let i = this._nonEnumItems.length - 1; i >= 0; i--) {
+          let item = this._nonEnumItems[i];
+          let result = item._findInVisibleItemsReverse(aPredicate);
+          if (result) {
+            return result;
+          }
+        }
+      }
+
+      if (this._variablesView._enumVisible) {
+        for (let i = this._enumItems.length - 1; i >= 0; i--) {
+          let item = this._enumItems[i];
+          let result = item._findInVisibleItemsReverse(aPredicate);
+          if (result) {
+            return result;
+          }
+        }
+      }
+    }
+
+    if (aPredicate(this)) {
+      return this;
+    }
+
+    return null;
+  },
+
   /**
    * Gets top level variables view instance.
    * @return VariablesView
@@ -1854,7 +1953,9 @@ Scope.prototype = {
   _name: null,
   _title: null,
   _enum: null,
+  _enumItems: null,
   _nonenum: null,
+  _nonEnumItems: null,
   _throbber: null
 };
 
@@ -2167,8 +2268,10 @@ ViewHelpers.create({ constructor: Variable, proto: Scope.prototype }, {
         this._nameString == "this" ||
         this._nameString == "<exception>") {
       this.ownerView._lazyAppend(aImmediateFlag, true, this._target);
+      this.ownerView._enumItems.push(this);
     } else {
       this.ownerView._lazyAppend(aImmediateFlag, false, this._target);
+      this.ownerView._nonEnumItems.push(this);
     }
   },
 
@@ -2674,8 +2777,10 @@ ViewHelpers.create({ constructor: Property, proto: Variable.prototype }, {
   _onInit: function P__onInit(aImmediateFlag) {
     if (this._initialDescriptor.enumerable) {
       this.ownerView._lazyAppend(aImmediateFlag, true, this._target);
+      this.ownerView._enumItems.push(this);
     } else {
       this.ownerView._lazyAppend(aImmediateFlag, false, this._target);
+      this.ownerView._nonEnumItems.push(this);
     }
   }
 });