DevTools: Use real focus in TreeOutline

third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js

Index: third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js
diff --git a/third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js b/third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js
index 99e46546a0beff60193852d0a43a369db5af30a7..525097edd0e7f007a36e2df58d46e5c9e6b425b5 100644
--- a/third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js
+++ b/third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js
@@ -37,6 +37,7 @@ UI.TreeOutline = class extends Common.Object {
     super();
     this._createRootElement();
 
+    /** @type {?UI.TreeElement} */
     this.selectedTreeElement = null;
     this.expandTreeElementsWhenArrowing = false;
     /** @type {?function(!UI.TreeElement, !UI.TreeElement):number} */
@@ -44,22 +45,10 @@ UI.TreeOutline = class extends Common.Object {
 
     this.contentElement = this._rootElement._childrenListNode;
     this.contentElement.addEventListener('keydown', this._treeKeyDown.bind(this), true);
-    this.contentElement.addEventListener('focus', setFocused.bind(this, true), false);
-    this.contentElement.addEventListener('blur', setFocused.bind(this, false), false);
 
-    this.setFocusable(!nonFocusable);
+    this._focusable = !nonFocusable;
 
     this.element = this.contentElement;
-
-    /**
-     * @param {boolean} isFocused
-     * @this {UI.TreeOutline}
-     */
-    function setFocused(isFocused) {
-      this._focused = isFocused;
-      if (this.selectedTreeElement)
-        this.selectedTreeElement._setFocused(this._focused);
-    }
   }
 
   _createRootElement() {
@@ -142,18 +131,11 @@ UI.TreeOutline = class extends Common.Object {
     this._comparator = comparator;
   }
 
-  /**
-   * @param {boolean} focusable
-   */
-  setFocusable(focusable) {
-    if (focusable)
-      this.contentElement.setAttribute('tabIndex', 0);
-    else
-      this.contentElement.removeAttribute('tabIndex');
-  }
-
   focus() {
-    this.contentElement.focus();
+    if (this.selectedTreeElement)
+      this.selectedTreeElement.listItemElement.focus();
+    else
+      this.contentElement.focus();

[pfeldman, 2017/01/21 04:57:34]

There is no tabIndex on contentElement, is it focusable?

[einbinder, 2017/01/21 06:20:32]

Ah, it isn't. I think its ok for empty TreeOutlines to not be focusable.

   }
 
   /**
@@ -164,6 +146,8 @@ UI.TreeOutline = class extends Common.Object {
       console.error('Binding element for the second time: ' + new Error().stack);
     element.treeOutline = this;
     element.onbind();
+    if (this._focusable)
+      element.tabIndex = -1;

[pfeldman, 2017/01/21 04:57:34]

element does not have tabIndex property. use setAttribute for consistency and
compilation-friendliness

[einbinder, 2017/01/21 06:20:32]

Ok!

   }
 
   /**
@@ -176,6 +160,7 @@ UI.TreeOutline = class extends Common.Object {
     element.deselect();
     element.onunbind();
     element.treeOutline = null;
+    element.listItemElement.removeAttribute('tabIndex');
   }
 
   /**
@@ -212,10 +197,8 @@ UI.TreeOutline = class extends Common.Object {
    * @param {!Event} event
    */
   _treeKeyDown(event) {
-    if (event.target !== this.contentElement)
-      return;
-
-    if (!this.selectedTreeElement || event.shiftKey || event.metaKey || event.ctrlKey)
+    if (!this.selectedTreeElement || event.target !== this.selectedTreeElement.listItemElement || event.shiftKey ||

[pfeldman, 2017/01/21 04:57:34]

listItemElement can have focusable children, if listeners don't consume their
event, you should execute it (inplace editing).

[einbinder, 2017/01/21 06:20:31]

This was specifically to guard against that scenario. If you have an inplace
editor, we don't want left and right arrow to move you in the treeoutline.

+        event.metaKey || event.ctrlKey)
       return;
 
     var handled = false;
@@ -276,8 +259,11 @@ UI.TreeOutline.Events = {
  * @unrestricted
  */
 UI.TreeOutlineInShadow = class extends UI.TreeOutline {
-  constructor() {
-    super();
+  /**
+   * @param {boolean=} nonFocusable
+   */
+  constructor(nonFocusable) {
+    super(nonFocusable);
     this.contentElement.classList.add('tree-outline');
 
     // Redefine element to the external one.
@@ -671,13 +657,6 @@ UI.TreeElement = class {
       this._trailingIconsElement.appendChild(icon);
   }
 
-  /**
-   * @param {boolean} focused
-   */
-  _setFocused(focused) {
-    this._focused = focused;
-    this._listItemNode.classList.toggle('force-white-icons', focused);
-  }
 
   /**
    * @return {string}
@@ -1001,15 +980,13 @@ UI.TreeElement = class {
 
     this.selected = true;
 
+    this.treeOutline.selectedTreeElement = this;
+    if (this.treeOutline._focusable)
+      this._listItemNode.tabIndex = 0;
     if (!omitFocus)
       this.treeOutline.focus();

[pfeldman, 2017/01/21 04:57:34]

Why going into treeOutline just to focus the node you have here?

[einbinder, 2017/01/21 06:20:32]

No great reason.

 
-    // Focusing on another node may detach "this" from tree.
-    if (!this.treeOutline)

[pfeldman, 2017/01/21 04:57:34]

I'm pretty sure this was happening :(, so removing the check is risky.

[einbinder, 2017/01/21 06:20:32]

I don't see how this can happen, it just calls focus. On itself. Nothing
overrides TreeOutline focus anymore.
But I agree that focusing the TreeElement directly will be less risky.

-      return false;
-    this.treeOutline.selectedTreeElement = this;
     this._listItemNode.classList.add('selected');
-    this._setFocused(this.treeOutline._focused);
     this.treeOutline.dispatchEventToListeners(UI.TreeOutline.Events.ElementSelected, this);
     return this.onselect(selectedByUser);
   }
@@ -1032,7 +1009,7 @@ UI.TreeElement = class {
     this.selected = false;
     this.treeOutline.selectedTreeElement = null;
     this._listItemNode.classList.remove('selected');
-    this._setFocused(false);
+    this._listItemNode.tabIndex = -1;
   }
 
   _populateIfNeeded() {