MD Settings: adjust iron-list focus row behaviors.

chrome/browser/resources/settings/focus_row_behavior.js

Index: chrome/browser/resources/settings/focus_row_behavior.js
diff --git a/chrome/browser/resources/settings/focus_row_behavior.js b/chrome/browser/resources/settings/focus_row_behavior.js
new file mode 100644
index 0000000000000000000000000000000000000000..21d259e90e9dbbb473e2888e608b614a2cc5fb86
--- /dev/null
+++ b/chrome/browser/resources/settings/focus_row_behavior.js
@@ -0,0 +1,169 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @param {{lastFocused: Object}} listItem
+ * @constructor
+ * @implements {cr.ui.FocusRow.Delegate}
+ */
+function FocusRowDelegate(listItem) {
+  this.listItem = listItem;
+}
+
+FocusRowDelegate.prototype = {
+  /**
+   * @override
+   * @param {!cr.ui.FocusRow} row
+   * @param {!Event} e
+   */
+  onFocus: function(row, e) {
+    this.listItem.lastFocused = e.path[0];
+  },
+
+  /**
+   * @override
+   * @param {!cr.ui.FocusRow} row The row that detected a keydown.
+   * @param {!Event} e
+   * @return {boolean} Whether the event was handled.
+   */
+  onKeydown: function(row, e) {
+    // Prevent iron-list from changing the focus on enter.
+    if (e.key == 'Enter')
+      e.stopPropagation();
+
+    return false;
+  },
+};
+
+/**
+ * @param {!Element} root
+ * @param {?Element} boundary
+ * @param {cr.ui.FocusRow.Delegate} delegate
+ * @constructor
+ * @extends {cr.ui.FocusRow}
+ */
+function VirtualFocusRow(root, boundary, delegate) {

[hcarmona, 2017/03/16 21:31:54]

boundary is not used. Can we get rid of it and add later if we need it?

[Optional] You can change the next line to:
cr.ui.FocusRow.call(this, root, /* boundary */ null, delegate) so that the
meaning isn't lost.

[scottchen, 2017/03/17 21:26:58]

Good idea.

+  cr.ui.FocusRow.call(this, root, boundary, delegate);
+}
+
+VirtualFocusRow.prototype = {
+  __proto__: cr.ui.FocusRow.prototype
+};
+
+
+/**
+ * Any element that is being used as an iron-list row item can extend this
+ * behavior, which encapsulates focus controls of mouse and keyboards.
+ * To use this behavior:
+ *    - The parent element should pass a "last-focused" attribute double-bound
+ *      to the row items, to track the last-focused element across rows.
+ *    - There must be a container in the extending element with the
+ *      [focus-row-container] attribute that contains all focusable controls.
+ *    - On each of the focusable controls, there must be a [focus-row-control]
+ *      attribute, and a [type=] attribute unique for each type of control.
+ *
+ * @polymerBehavior
+ */
+var FocusRowBehavior = {
+  properties: {
+    /** @private {VirtualFocusRow} */
+    row_: Object,
+
+    /** @private {boolean} */
+    mouseFocused_: Boolean,
+
+    /** @type {Element} */
+    lastFocused: {
+      type: Object,
+      notify: true,
+    },
+
+    /** @type {number} */
+    ironListTabIndex: {

[hcarmona, 2017/03/16 21:31:54]

This seems a bit redundant, can we use |tabindex| to avoid having two properties
that are the same?

[scottchen, 2017/03/17 21:26:58]

They're actually not always the same since the template only does a one-way
binding on both attributes, and the mechanism actually make use of this fact.
For example, when a control within row-one is focused, row-one has tabIndex = -1
(so that it can't be focused), and ironListTabIndex = 0 indicating that the row
is "active", as in someone inside of it is focused. 

[hcarmona, 2017/03/17 23:55:50]

SG, can we add a quick comment to explain that here?

[scottchen, 2017/03/18 01:05:47]

Done.

+      type: Number,
+      observer: 'ironListTabIndexChanged_',
+    },
+  },
+
+  /** @override */
+  attached: function() {
+    this.classList.add('no-outline');
+
+    Polymer.RenderStatus.afterNextRender(this, function() {
+      var rowContainer = this.root.querySelector('[focus-row-container]');
+      assert(!!rowContainer);
+      this.row_ =
+          new VirtualFocusRow(rowContainer, null, new FocusRowDelegate(this));
+      this.row_.makeActive(this.ironListTabIndex == 0);
+      this.addItems_();

[hcarmona, 2017/03/16 21:31:54]

Should addItems_ be called before makeActive?

[scottchen, 2017/03/17 21:26:58]

Either way is fine, row_.makeActive() and row_.addItem() both have checks
built-in such that it's safe to invoke in either order.

See: 
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/focus_row.js...

(Btw, either one will make one more 1-op call per element, so both are O(2n) ~=
O(n).)

[hcarmona, 2017/03/17 23:55:50]

Acknowledged.

+
+      // Adding listeners asynchronously to reduce blocking time, since this
+      // behavior will be used by items in potentially long lists.
+      this.listen(this, 'focus', 'onFocus_');
+      this.listen(this, 'dom-change', 'onDomChange_');

[hcarmona, 2017/03/16 21:31:54]

Can this just call addItems_ directly?

[scottchen, 2017/03/17 21:26:58]

Yep!

+      this.listen(this, 'mousedown', 'onMouseDown_');
+      this.listen(this, 'blur', 'onBlur_');
+    });

[hcarmona, 2017/03/16 21:31:54]

I think you're missing a .bind(this) on your function

[scottchen, 2017/03/17 21:26:58]

"this" is provided as the first element of the afterNextRender method signature
(line 93 above). see: https://github.com/Polymer/polymer/issues/4104

[hcarmona, 2017/03/17 23:55:50]

Acknowledged.

+  },
+
+  /** @override */
+  detached: function() {
+    this.unlisten(this, 'focus', 'onFocus_');
+    this.unlisten(this, 'dom-change', 'onDomChange_');
+    this.unlisten(this, 'mousedown', 'onMouseDown_');
+    this.unlisten(this, 'blur', 'onBlur_');
+    if (this.row_)
+      this.row_.destroy();
+  },
+
+  /** @private */
+  addItems_: function() {
+    if (this.row_) {
+      this.row_.destroy();
+
+      var controls = this.root.querySelectorAll('[focus-row-control]');
+      for (var i = 0; i < controls.length; i++) {
+        this.row_.addItem(
+            controls[i].getAttribute('type'),
+            /** @type {HTMLElement} */ (controls[i]));
+      }
+    }
+  },
+
+  /** @private */
+  onFocus_: function() {
+    if (this.mouseFocused_) {
+      this.mouseFocused_ = false;  // Consume and reset flag.
+      return;
+    }
+
+    if (this.lastFocused)
+      this.row_.getEquivalentElement(this.lastFocused).focus();
+    else
+      this.row_.getFirstFocusable().focus();
+
+    this.tabIndex = -1;

[hcarmona, 2017/03/16 21:31:54]

Why are we setting the tabindex to -1 here?
Do we ever set it back to the original value?

[scottchen, 2017/03/17 21:26:58]

It's to make the row itself back to an "unselectable" state, once it forwards
the focus to its children, so that you can't shift-tab back to the row itself.

[hcarmona, 2017/03/17 23:55:50]

What happens if you tab away, can it be focused again later?

[scottchen, 2017/03/18 01:05:46]

Yeah, since now the child's tabindex would be 0.

+  },
+
+  /** @private */
+  ironListTabIndexChanged_: function() {
+    if (this.row_)
+      this.row_.makeActive(this.ironListTabIndex == 0);
+  },
+
+  /** @private */
+  onDomChange_: function() {
+    this.addItems_();
+  },
+
+  /** @private */
+  onMouseDown_: function() {
+    this.mouseFocused_ = true;  // Set flag to not do any control-focusing.
+  },
+
+  /** @private */
+  onBlur_: function() {
+    this.mouseFocused_ = false;  // Reset flag since it's not active anymore.
+  }
+};