MD Settings: mouse movements should focus cr-action-menu items

ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js

Index: ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
diff --git a/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js b/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
index 9f58dc4f6cd9079f59c2ee05364b0f1f5aef5e46..0a107e79e8f16e3a8bc3e698b6b65ac6feecac63 100644
--- a/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
+++ b/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
@@ -32,6 +32,7 @@ Polymer({
 
   listeners: {
     'keydown': 'onKeyDown_',
+    'mousemove': 'onMouseMove_',
     'tap': 'onTap_',
   },
 
@@ -84,6 +85,21 @@ Polymer({
   },
 
   /**
+   * @param {!MouseEvent} e
+   * @private
+   */
+  onMouseMove_: function(e) {
+    var target = e.target;

[Dan Beam, 2017/04/05 03:29:21]

i'm still a little confused.  I don't think this is bulletproof because it makes
assumptions of what can be slotted, which is currently only guaranteed to have a
.dropdown-item class, not have shadow DOM (which effects what .target gives us).

i'd be more comfortable with something like:

var el;
for (var i = 0; this.contains(el); el = e.path[i++]) {
  if (this.options_.indexOf(el) >= 0) {
    el.focus();
    return;
  }
});

[scottchen, 2017/04/05 20:56:31]

Acknowledged the concern, I've modified the code to check path.

I had to change a couple things from your code:
- can't use this.contains(el), since e.path returns the shadowRoots in between
elements, and apparently $0.contains($0.shadowRoot) === false.

- this.options_ is a NodeList which doesn't have .indexOf() or .contains().
While I *could* convert it to an array and do indexOf, I thought just checking
el.classList.contains('dropdown-item') would be sufficient, since this.options_
= querySelectorAll('.dropdown-item') anyway, so it'd be equivalent.

+
+    if (target.classList.contains('dropdown-item') &&

[Dan Beam, 2017/04/04 23:41:13]

does this move if you move the mouse inside of a <paper-checkbox>, like in the
languages action menu?

[scottchen, 2017/04/04 23:48:45]

yes, it focuses the checkbox (ripple shown around the checkbox).

[Dan Beam, 2017/04/04 23:49:59]

don't you actually want it to focus the item, not the element inside the item?

[scottchen, 2017/04/05 00:08:19]

The top-most item *is* the <paper-checkbox>, which is the whole row item
including the label, and by default the behavior of focusing a paper-checkbox is
that the ripple shows around the checkbox. 

There's not an outer item-container outside the paper-checkbox:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/langua...

[Dan Beam, 2017/04/05 03:29:21]

ok

+        target != document.activeElement) {
+      target.focus();
+    } else {
+      this.focus();  // Blur option focus but keep up/down button working.

[Dan Beam, 2017/04/05 03:29:21]

why is this needed?  keydown should be fine as long as focus stays inside the
dialog.  i don't understand how that could change here...

[scottchen, 2017/04/05 20:56:31]

It's more about blurring the option focus. The comment is more of an explanation
of why we can't just do activeElement.blur().

Without this, if you hover over option 1 and move your mouse outside of the menu
area, you'd still be focused on option 1 - which debatably is still okay, I just
added this to blur from option 1 because:
- bug report says "mimic context menu", and context menu does this.
- I'd argue this is a better behavior anyway, it might cause confusion if you
move your mouse out and option 1 is still hovered. What if the user didn't know
and press space/enter/tab?

[scottchen, 2017/04/05 20:58:10]

Also (noting for other readers) we discussed offline this matches the behavior
of context menu across all platforms.

+    }
+  },
+
+  /**
    * @param {number} step -1 for getting previous option (up), 1 for getting
    *     next option (down).
    * @return {?Element} The next focusable option, taking into account
@@ -99,6 +115,10 @@ Polymer({
     var focusedIndex =
         Array.prototype.indexOf.call(this.options_, this.root.activeElement);
 
+    // Avoid off-by-one when nothing is focused and up is pressed.
+    if (focusedIndex === -1 && step === -1)
+      focusedIndex = 0;
+
     do {
       focusedIndex = (numOptions + focusedIndex + step) % numOptions;
       nextOption = this.options_[focusedIndex];