DevTools: Use real focus in TreeOutline

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

 /*
  * Copyright (C) 2007 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 19 matching lines @@
  * @unrestricted
  */
 UI.TreeOutline = class extends Common.Object {
   /**
    * @param {boolean=} nonFocusable
    */
   constructor(nonFocusable) {
     super();
     this._createRootElement();
 
+    /** @type {?UI.TreeElement} */
     this.selectedTreeElement = null;
     this.expandTreeElementsWhenArrowing = false;
     /** @type {?function(!UI.TreeElement, !UI.TreeElement):number} */
     this._comparator = null;
 
     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() {
     this._rootElement = new UI.TreeElement();
     this._rootElement.treeOutline = this;
     this._rootElement.root = true;
     this._rootElement.selectable = false;
     this._rootElement.expanded = true;
     this._rootElement._childrenListNode.classList.remove('children');
   }
@@ skipping 62 matching lines @@
     return event ? this.treeElementFromPoint(event.pageX, event.pageY) : null;
   }
 
   /**
    * @param {?function(!UI.TreeElement, !UI.TreeElement):number} comparator
    */
   setComparator(comparator) {
     this._comparator = comparator;
   }
 
-  /**
-   * @param {boolean} focusable
-   */
-  setFocusable(focusable) {
-    if (focusable)
-      this.contentElement.setAttribute('tabIndex', 0);
+  focus() {
+    if (this.selectedTreeElement)
+      this.selectedTreeElement.listItemElement.focus();
     else
-      this.contentElement.removeAttribute('tabIndex');
-  }
-
-  focus() {
-    this.contentElement.focus();
+      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.

   }
 
   /**
    * @param {!UI.TreeElement} element
    */
   _bindTreeElement(element) {
     if (element.treeOutline)
       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!

   }
 
   /**
    * @param {!UI.TreeElement} element
    */
   _unbindTreeElement(element) {
     if (!element.treeOutline)
       console.error('Unbinding element that was not bound: ' + new Error().stack);
 
     element.deselect();
     element.onunbind();
     element.treeOutline = null;
+    element.listItemElement.removeAttribute('tabIndex');
   }
 
   /**
    * @return {boolean}
    */
   selectPrevious() {
     var nextSelectedElement = this.selectedTreeElement.traversePreviousTreeElement(true);
     while (nextSelectedElement && !nextSelectedElement.selectable)
       nextSelectedElement = nextSelectedElement.traversePreviousTreeElement(!this.expandTreeElementsWhenArrowing);
     if (nextSelectedElement) {
@@ skipping 16 matching lines @@
       nextSelectedElement.select(false, true);
       return true;
     }
     return false;
   }
 
   /**
    * @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;
     if (event.key === 'ArrowUp' && !event.altKey) {
       handled = this.selectPrevious();
     } else if (event.key === 'ArrowDown' && !event.altKey) {
       handled = this.selectNext();
     } else if (event.key === 'ArrowLeft') {
       handled = this.selectedTreeElement.collapseOrAscend(event.altKey);
     } else if (event.key === 'ArrowRight') {
@@ skipping 40 matching lines @@
   ElementAttached: Symbol('ElementAttached'),
   ElementExpanded: Symbol('ElementExpanded'),
   ElementCollapsed: Symbol('ElementCollapsed'),
   ElementSelected: Symbol('ElementSelected')
 };
 
 /**
  * @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.
     this.element = createElement('div');
     this._shadowRoot = UI.createShadowRootWithCoreStyles(this.element, 'ui/treeoutline.css');
     this._disclosureElement = this._shadowRoot.createChild('div', 'tree-outline-disclosure');
     this._disclosureElement.appendChild(this.contentElement);
     this._renderSelection = true;
   }
 
@@ skipping 373 matching lines @@
       this._trailingIconsElement = createElementWithClass('div', 'trailing-icons');
       this._trailingIconsElement.classList.add('icons-container');
       this._listItemNode.appendChild(this._trailingIconsElement);
       this._ensureSelection();
     }
     this._trailingIconsElement.removeChildren();
     for (var icon of icons)
       this._trailingIconsElement.appendChild(icon);
   }
 
-  /**
-   * @param {boolean} focused
-   */
-  _setFocused(focused) {
-    this._focused = focused;
-    this._listItemNode.classList.toggle('force-white-icons', focused);
-  }
 
   /**
    * @return {string}
    */
   get tooltip() {
     return this._tooltip || '';
   }
 
   /**
    * @param {string} x
@@ skipping 303 matching lines @@
 
     if (this.treeOutline.selectedTreeElement)
       this.treeOutline.selectedTreeElement.deselect();
     this.treeOutline.selectedTreeElement = null;
 
     if (this.treeOutline._rootElement === this)
       return false;
 
     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);
   }
 
   /**
    * @param {boolean=} omitFocus
    */
   revealAndSelect(omitFocus) {
     this.reveal(true);
     this.select(omitFocus);
   }
 
   /**
    * @param {boolean=} supressOnDeselect
    */
   deselect(supressOnDeselect) {
     if (!this.treeOutline || this.treeOutline.selectedTreeElement !== this || !this.selected)
       return;
 
     this.selected = false;
     this.treeOutline.selectedTreeElement = null;
     this._listItemNode.classList.remove('selected');
-    this._setFocused(false);
+    this._listItemNode.tabIndex = -1;
   }
 
   _populateIfNeeded() {
     if (this.treeOutline && this._expandable && !this._children) {
       this._children = [];
       this.onpopulate();
     }
   }
 
   onpopulate() {
@@ skipping 131 matching lines @@
     var paddingLeftValue = window.getComputedStyle(this._listItemNode).paddingLeft;
     console.assert(paddingLeftValue.endsWith('px'));
     var computedLeftPadding = parseFloat(paddingLeftValue);
     var left = this._listItemNode.totalOffsetLeft() + computedLeftPadding;
     return event.pageX >= left && event.pageX <= left + UI.TreeElement._ArrowToggleWidth && this._expandable;
   }
 };
 
 /** @const */
 UI.TreeElement._ArrowToggleWidth = 10;