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

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

 // Copyright 2016 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.
 
 Polymer({
   is: 'cr-action-menu',
   extends: 'dialog',
 
   /**
    * List of all options in this action menu.
@@ skipping 14 matching lines @@
    * @private {?Function}
    */
   boundClose_: null,
 
   hostAttributes: {
     tabindex: 0,
   },
 
   listeners: {
     'keydown': 'onKeyDown_',
+    'mousemove': 'onMouseMove_',
     'tap': 'onTap_',
   },
 
   /** override */
   attached: function() {
     this.options_ = this.querySelectorAll('.dropdown-item');
   },
 
   /** override */
   detached: function() {
@@ skipping 32 matching lines @@
       return;
 
     var nextOption = this.getNextOption_(e.key == 'ArrowDown' ? 1 : -1);
     if (nextOption)
       nextOption.focus();
 
     e.preventDefault();
   },
 
   /**
+   * @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
    *     disabled/hidden attributes, or null if no focusable option exists.
    * @private
    */
   getNextOption_: function(step) {
     // Using a counter to ensure no infinite loop occurs if all elements are
     // hidden/disabled.
     var counter = 0;
     var nextOption = null;
     var numOptions = this.options_.length;
     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];
       if (nextOption.disabled || nextOption.hidden)
         nextOption = null;
       counter++;
     } while (!nextOption && counter < numOptions);
 
     return nextOption;
   },
@@ skipping 39 matching lines @@
     // Attempt to show the menu starting from the top of the rectangle and
     // extending downwards. If that does not fit within the window, fallback to
     // starting from the bottom and extending upwards.
     var top = rect.top + this.offsetHeight <= window.innerHeight ? rect.top :
                                                                    rect.bottom -
             this.offsetHeight - Math.max(rect.bottom - window.innerHeight, 0);
 
     this.style.top = top + 'px';
   },
 });