MD Settings: Fix subpage navigation focus for bluetooth+internet

chrome/browser/resources/settings/internet_page/internet_detail_page.js

Index: chrome/browser/resources/settings/internet_page/internet_detail_page.js
diff --git a/chrome/browser/resources/settings/internet_page/internet_detail_page.js b/chrome/browser/resources/settings/internet_page/internet_detail_page.js
index ef74fd58e9a3f81d343047e79d349016b95633d8..4a6677f6cb427451df97d1e9dde27b8382bf3026 100644
--- a/chrome/browser/resources/settings/internet_page/internet_detail_page.js
+++ b/chrome/browser/resources/settings/internet_page/internet_detail_page.js
@@ -144,6 +144,9 @@ Polymer({
    */
   networksChangedListener_: null,
 
+  /** @private {boolean} */
+  didSetFocus_: false,
+
   /**
    * settings.RouteObserverBehavior
    * @param {!settings.Route} route
@@ -180,10 +183,22 @@ Polymer({
       ConnectionState: CrOnc.ConnectionState.NOT_CONNECTED,
       Name: {Active: name},
     };
+    this.didSetFocus_ = false;
     this.getNetworkDetails_();
   },
 
   /** @private */
+  maybeSetFocus_: function() {
+    if (this.didSetFocus_)
+      return;
+    this.didSetFocus_ = true;
+    var button = this.$$('#buttonDiv .primary-button:not([hidden])');

[dpapad, 2017/04/26 00:22:38]

Nit (optional): You can also use a single selector as follows.

var button = this.$$(
    '#buttonDiv .primary-button:not([hidden]), ' +
    '#buttonDiv .secondary-button:not([hidden])'); 

assert(button); // Expecting at least one button to always be showing.
button.focus();

[stevenjb, 2017/04/26 17:53:36]

Does that perform the first query and then the second one? We want a primary
button if not hidden and a secondary button only if there is no visible primary.
(The primary buttons are not necessarily first in the DOM ordering). The
documentation for querySelector says 'depth-first pre-order traversal' with no
information about how multiple selectors work.

I will add the assert though.

[dpapad, 2017/04/26 18:14:49]

Hm, I think it returns the 1st item that matches either of the two conditions,
so I understand now that might not be equivalent. Please ignore.

+    if (!button)
+      button = this.$$('#buttonDiv .secondary-button:not([hidden])');
+    button.focus();
+  },
+
+  /** @private */
   close_: function() {
     // Delay navigating until the next render frame to allow other subpages to
     // load first.
@@ -285,6 +300,7 @@ Polymer({
       return;
     }
     this.networkProperties = properties;
+    this.maybeSetFocus_();

[dpapad, 2017/04/26 00:22:38]

I missed this file completely in previous iteration. Couple of questions:

1) The changes in this file seem to not be related with the "exiting a subpage"
case. Instead here it tries to focus something when we are entering a subpage.
Is my understanding correct?

Also why do we need to call focus related code in getPropertiesCallback_ and
getStateCallback_?

Per my understanding, when we set |networkProperties| the observer
networkPropertiesChanged_ is fired, which in turn causes some kind of page
transition. Can we move the focus() call inside |networkPropertiesChanged_|
instead? The we might not need the "maybeSetFocus()" logic either.

[stevenjb, 2017/04/26 17:53:36]

1. Which buttons are visible may change after we set |this.networkProperties|.
2. We still need didSetFocus_ because this will get called if the network
properties change and we don't want to change focus later (the user may have
changed focus themselves at that point).

Moving this to networkPropertiesChanged_ however makes sense so I will do that.

   },
 
   /**
@@ -305,6 +321,7 @@ Polymer({
       Connectable: state.Connectable,
       ConnectionState: state.ConnectionState,
     };
+    this.maybeSetFocus_();
   },
 
   /**
@@ -549,7 +566,8 @@ Polymer({
     this.networkingPrivate.startActivate(this.guid);
   },
 
-  /** @const {string} */ CR_EXPAND_BUTTON_TAG: 'CR-EXPAND-BUTTON',
+  /** @const {string} */
+  CR_EXPAND_BUTTON_TAG: 'CR-EXPAND-BUTTON',
 
   /**
    * @param {Event} event