MD Settings: Restore focus after closing various dialogs.

MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page
 - languages-page

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

Review-Url: https://codereview.chromium.org/2793463002
Cr-Commit-Position: refs/heads/master@{#461616}
Committed: https://chromium.googlesource.com/chromium/src/+/79d1fe73c50b7a5f0ef25daed2a637710cb7e440

[dpapad, 2017-03-31 00:19:09]

Description was changed from

==========
MD Settings: Restore focus after closing dialogs, part 1.

BUG=
==========

to

==========
MD Settings: Restore focus after closing dialogs, part 1.

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

[dpapad, 2017-03-31 00:20:43]

Description was changed from

==========
MD Settings: Restore focus after closing dialogs, part 1.

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

to

==========
MD Settings: Restore focus after closing dialogs, part 1.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page

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

[dpapad, 2017-03-31 00:21:47]

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

[commit-bot: I haz the power, 2017-03-31 00:22:48]

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

[dpapad, 2017-03-31 00:45:27]

Description was changed from

==========
MD Settings: Restore focus after closing dialogs, part 1.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page

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

to

==========
MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page

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

[dpapad, 2017-03-31 01:23:56]

Description was changed from

==========
MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page

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

to

==========
MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page
 - languages-page

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

[dpapad, 2017-03-31 01:24:48]

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

[commit-bot: I haz the power, 2017-03-31 01:25:25]

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

[dpapad, 2017-03-31 01:43:41]

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

[dpapad, 2017-03-31 01:43:41]

I have split the overall task to multiple CL. This CL holds the easiest cases
together (so that I don't end up creating too many CLs unnecessarily).

[commit-bot: I haz the power, 2017-03-31 02:43:25]

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

[commit-bot: I haz the power, 2017-03-31 02:43:26]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-04-03 15:39:50]

lgtm if the below comments are easily addressed.

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:311:
this.$$('#disconnectButton').focus();
Hmm... this is interesting to me. Usually after a successful signout, the
disconnect dialog is no longer shown. Perhaps we should focus the sign-in dialog
instead.

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:62:
this.$$('#resetProfile button[is=paper-icon-button-light').focus();
Are we very confident this won't be null? Also I think the selector is missing a
close bracket ]

[dpapad, 2017-04-03 19:23:06]

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:311:
this.$$('#disconnectButton').focus();
On 2017/04/03 at 15:39:50, tommycli wrote:
> Hmm... this is interesting to me. Usually after a successful signout, the
disconnect dialog is no longer shown. Perhaps we should focus the sign-in dialog
instead.

I thought about this and settled to the current approach with the following
rationale:

The main goal of this patch is to ensure that focus is not completely lost when
the dialog is closed (without this CL it goes to the browser's home button next
to the omnibox). Focusing the "sign out" button even though it is being dom-ifed
out shortly after, achieves the goal of rescuing the focus. Even though the
"sign in" button is not focused, when the user presses Tab the focus goes to the
next element, which is the "learn more" link. Given that the "sign in" button is
added to the DOM after a series of async operations, so it is not trivial to get
a reference to it, I would prefer to addressing this case separately (but I
don't think it should be a blocker). WDYT?

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:62:
this.$$('#resetProfile button[is=paper-icon-button-light').focus();
On 2017/04/03 at 15:39:50, tommycli wrote:
> Are we very confident this won't be null? Also I think the selector is missing
a close bracket ]


Fixed closing bracket. Regarding whether it can be null, the button is not
within a dom-if, so it is always present on the page.

[tommycli, 2017-04-03 19:30:52]

lgtm. Below comment is optional.

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/people_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/people_page.js:311:
this.$$('#disconnectButton').focus();
On 2017/04/03 19:23:05, dpapad wrote:
> On 2017/04/03 at 15:39:50, tommycli wrote:
> > Hmm... this is interesting to me. Usually after a successful signout, the
> disconnect dialog is no longer shown. Perhaps we should focus the sign-in
dialog
> instead.
> 
> I thought about this and settled to the current approach with the following
> rationale:
> 
> The main goal of this patch is to ensure that focus is not completely lost
when
> the dialog is closed (without this CL it goes to the browser's home button
next
> to the omnibox). Focusing the "sign out" button even though it is being
dom-ifed
> out shortly after, achieves the goal of rescuing the focus. Even though the
> "sign in" button is not focused, when the user presses Tab the focus goes to
the
> next element, which is the "learn more" link. Given that the "sign in" button
is
> added to the DOM after a series of async operations, so it is not trivial to
get
> a reference to it, I would prefer to addressing this case separately (but I
> don't think it should be a blocker). WDYT?

I'm fine with addressing it separately. And yeah, it shouldn't block this.

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:62:
this.$$('#resetProfile button[is=paper-icon-button-light').focus();
On 2017/04/03 19:23:05, dpapad wrote:
> On 2017/04/03 at 15:39:50, tommycli wrote:
> > Are we very confident this won't be null? Also I think the selector is
missing
> a close bracket ]
> 
> 
> Fixed closing bracket. Regarding whether it can be null, the button is not
> within a dom-if, so it is always present on the page.

Acknowledged. I would probably still recommend giving it an ID to have it be
explicitly addressed (and so we can use the this.$.foo syntax, so future readers
will know it can never be null...)

[dpapad, 2017-04-04 00:03:16]

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/2793463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:62:
this.$$('#resetProfile button[is=paper-icon-button-light').focus();
On 2017/04/03 at 19:30:51, tommycli wrote:
> On 2017/04/03 19:23:05, dpapad wrote:
> > On 2017/04/03 at 15:39:50, tommycli wrote:
> > > Are we very confident this won't be null? Also I think the selector is
missing
> > a close bracket ]
> > 
> > 
> > Fixed closing bracket. Regarding whether it can be null, the button is not
> > within a dom-if, so it is always present on the page.
> 
> Acknowledged. I would probably still recommend giving it an ID to have it be
explicitly addressed (and so we can use the this.$.foo syntax, so future readers
will know it can never be null...)

Done.

[dpapad, 2017-04-04 00:03:32]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-04-04 00:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2793463002/#ps60001
(title: "Nits.")

[commit-bot: I haz the power, 2017-04-04 00:03:57]

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-04 02:46:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491264212219800,
"parent_rev": "c641a534451545e3124eacbd7f711dec84f3daed", "commit_rev":
"79d1fe73c50b7a5f0ef25daed2a637710cb7e440"}

[commit-bot: I haz the power, 2017-04-04 02:47:19]

Description was changed from

==========
MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page
 - languages-page

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

to

==========
MD Settings: Restore focus after closing various dialogs.

This CL addresses dialogs in
 - people-page
 - privacy-page (but not site settings)
 - reset-page
 - search-engines-page
 - languages-page

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

Review-Url: https://codereview.chromium.org/2793463002
Cr-Commit-Position: refs/heads/master@{#461616}
Committed:
https://chromium.googlesource.com/chromium/src/+/79d1fe73c50b7a5f0ef25daed2a6...
==========

[commit-bot: I haz the power, 2017-04-04 02:47:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/79d1fe73c50b7a5f0ef25daed2a6...