MD Settings: Add mechanism to restore focus after subpage exiting.

MD Settings: Add mechanism to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2805363002
Cr-Commit-Position: refs/heads/master@{#463502}
Committed: https://chromium.googlesource.com/chromium/src/+/5c96df5fc329e084e0d7752bf23f8a12e6acd344

[dpapad, 2017-04-07 22:25:08]

Description was changed from

==========
MD Settings; 2nd Proposal on how to restore focus after subpage exiting.

BUG=709586
==========

to

==========
MD Settings; 2nd Proposal on how to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-04-07 22:43:06]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 22:43:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-04-07 22:46:44]

Patchset #2 (id:20001) has been deleted

[dpapad, 2017-04-07 22:47:17]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 22:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-04-07 23:18:37]

Patchset #3 (id:60001) has been deleted

[dpapad, 2017-04-08 00:52:59]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-08 00:53:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-04-08 01:36:36]

Description was changed from

==========
MD Settings; 2nd Proposal on how to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: 2nd Proposal on how to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-04-08 01:36:48]

Description was changed from

==========
MD Settings: 2nd Proposal on how to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Add mechanism to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-04-08 01:40:14]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org, tommycli@chromium.org

[dpapad, 2017-04-08 01:40:15]

Thanks for the discussion. It helped coming up with an elegant (I think)
solution.

https://codereview.chromium.org/2805363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66:
if (e.detail.item != this.querySelector('neon-animatable'))
Note to self: Perhaps querySelector() call is unnecessary here. Instead
e.detail.item.tagName == 'NEON-ANIMATABLE'
might be equivalent. Will verify that on Monday.

[commit-bot: I haz the power, 2017-04-08 01:54:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-08 01:54:35]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-04-10 19:11:27]

https://codereview.chromium.org/2805363002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66:
if (e.detail.item != this.querySelector('neon-animatable'))
On 2017/04/08 at 01:40:15, dpapad wrote:
> Note to self: Perhaps querySelector() call is unnecessary here. Instead
> e.detail.item.tagName == 'NEON-ANIMATABLE'
> might be equivalent. Will verify that on Monday.

Done.

[tommycli, 2017-04-10 22:14:37]

lgtm thanks!

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_animated_pages_test.js (right):

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_animated_pages_test.js:12:
<neon-animatable route-path=${settings.Route.SEARCH_ENGINES.path}>
that's interesting... does this need double quotes around the interpolated
string?

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_animated_pages_test.js:24: var trigger
= document.body.querySelector('#subpage-trigger');
i'm assuming you put it in its own variable so you could do
assertTrue(!!trigger) right?

[dpapad, 2017-04-10 23:52:28]

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_animated_pages_test.js (right):

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_animated_pages_test.js:12:
<neon-animatable route-path=${settings.Route.SEARCH_ENGINES.path}>
On 2017/04/10 at 22:14:36, tommycli wrote:
> that's interesting... does this need double quotes around the interpolated
string?

Added double quotes. They are not needed, but added them just for consistency
with the prod code.

https://codereview.chromium.org/2805363002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_animated_pages_test.js:24: var trigger
= document.body.querySelector('#subpage-trigger');
On 2017/04/10 at 22:14:36, tommycli wrote:
> i'm assuming you put it in its own variable so you could do
assertTrue(!!trigger) right?

Done.

[Dan Beam, 2017-04-11 00:06:07]

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66:
if (e.detail.item.tagName != 'NEON-ANIMATABLE')
nit: combine with above if

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:69:
var querySelector = this.focusConfig.get(this.lastOldRoute_.path);
nit: the argument passed to querySelector is just a "selector", so I think this
var should be named "selector" or at least not have a "query"

https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector
https://www.w3.org/TR/selectors-api/#interface-definitions
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Conta...

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:77:
var trigger = assert(this.querySelector(querySelector));
nit: this.querySelector(selector).focus();

(no assert() needed if it will just blow up naturally)

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:25:
'#passwordManagerButton .subpage-arrow');
note: this is also a thing

this.focusConfig_ = new Map([
  [settings.Route.AUTOFILL.path, '#autofillManagerButton .subpage-arrow'],
  ...
]);

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:19:
var FocusConfig;
still needed?

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
lastOldRoute_: null,
nit: just previousRoute_?

[Dan Beam, 2017-04-11 00:06:32]

eh, guess those are nits, so lgtm

[dpapad, 2017-04-11 00:18:33]

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66:
if (e.detail.item.tagName != 'NEON-ANIMATABLE')
On 2017/04/11 at 00:06:07, Dan Beam wrote:
> nit: combine with above if

Done.

https://codereview.chromium.org/2805363002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:77:
var trigger = assert(this.querySelector(querySelector));
On 2017/04/11 at 00:06:07, Dan Beam wrote:
> nit: this.querySelector(selector).focus();
> 
> (no assert() needed if it will just blow up naturally)

Done. Agreed. An assert() is more useful when it is accommodated by a helpful
error message. Otherwise just letting the code blow up is just as good.

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:25:
'#passwordManagerButton .subpage-arrow');
On 2017/04/11 at 00:06:07, Dan Beam wrote:
> note: this is also a thing
> 
> this.focusConfig_ = new Map([
>   [settings.Route.AUTOFILL.path, '#autofillManagerButton .subpage-arrow'],
>   ...
> ]);

Acknowledged. I actually used that first, and then changed it to the current
version. Creating N+1 transient arrays seemed unnecessary (N is the number of
entries), when I can use set() instead.

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:19:
var FocusConfig;
On 2017/04/11 at 00:06:07, Dan Beam wrote:
> still needed?

No. Removed.

https://codereview.chromium.org/2805363002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
lastOldRoute_: null,
On 2017/04/11 at 00:06:07, Dan Beam wrote:
> nit: just previousRoute_?

Done.

[dpapad, 2017-04-11 00:56:27]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-04-11 00:56:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2805363002/#ps160001
(title: "Address @dbeam comments.")

[commit-bot: I haz the power, 2017-04-11 00:56:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 02:42:24]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491872187926780,
"parent_rev": "fa4c8940a8d8d6a84c60bc1f108b1d6a86ef3437", "commit_rev":
"5c96df5fc329e084e0d7752bf23f8a12e6acd344"}

[commit-bot: I haz the power, 2017-04-11 02:43:43]

Description was changed from

==========
MD Settings: Add mechanism to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Add mechanism to restore focus after subpage exiting.

BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2805363002
Cr-Commit-Position: refs/heads/master@{#463502}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c96df5fc329e084e0d7752bf23f...
==========

[commit-bot: I haz the power, 2017-04-11 02:43:45]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/5c96df5fc329e084e0d7752bf23f...