MD Settings: CrOS: Focus elements on dialog close

MD Settings: CrOS: Focus elements on dialog close

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

Review-Url: https://codereview.chromium.org/2817803002
Cr-Commit-Position: refs/heads/master@{#464291}
Committed: https://chromium.googlesource.com/chromium/src/+/2e00979d16dd569963846451ce467626b1e86a04

[stevenjb, 2017-04-12 23:06:07]

Description was changed from

==========
MD Settings: CrOS: Focus elements on dialog close

BUG=709148
==========

to

==========
MD Settings: CrOS: Focus elements on dialog close

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

[stevenjb, 2017-04-12 23:40:43]

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

[dpapad, 2017-04-13 00:16:41]

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.js:308:
this.$$('#driveCache button').focus();
The button already has an ID, 'deleteButton'. Can we use that instead?

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_siminfo.html
(right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.html:70:
<paper-button id="unlockPinButton" on-tap="onUnlockPinTap_">
Is the "Button" suffix necessary for disambiguation? If not, consider removing
it (I believe that is the norm in the rest of the Settings codebase).

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_siminfo.js (right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.js:226:
this.$.unlockPuk.value = '';
How about initializing those three values before calling showModal()? Setting
them after the dialog is shown could cause flickering.

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.js:353:
onUnlockPinDialogClose__: function() {
Typo: There are two underscores here, which makes this mismatch the HTML code
binding.

Could you verify that all the focus() calls target the correct element manually
(given that we don't add tests for those)?

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

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.js:43:
attached: function() {
I could not find any callers of open() in this CL that got updated. Who called
this method?

Also, nit: Add @override for extra compiler coverage.

[stevenjb, 2017-04-13 00:38:19]

Patchset #3 (id:40001) has been deleted

[stevenjb, 2017-04-13 00:39:09]

PTAL

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/storage.js:308:
this.$$('#driveCache button').focus();
On 2017/04/13 00:16:40, dpapad wrote:
> The button already has an ID, 'deleteButton'. Can we use that instead?

Oops, missed that, thanks for catching. Done.

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_siminfo.html
(right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.html:70:
<paper-button id="unlockPinButton" on-tap="onUnlockPinTap_">
On 2017/04/13 00:16:40, dpapad wrote:
> Is the "Button" suffix necessary for disambiguation? If not, consider removing
> it (I believe that is the norm in the rest of the Settings codebase).

There is already a paper-input "unlockPin". I'd rename that but was trying ot
keep this simple.

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_siminfo.js (right):

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.js:226:
this.$.unlockPuk.value = '';
On 2017/04/13 00:16:40, dpapad wrote:
> How about initializing those three values before calling showModal()? Setting
> them after the dialog is shown could cause flickering.

Done.

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_siminfo.js:353:
onUnlockPinDialogClose__: function() {
On 2017/04/13 00:16:40, dpapad wrote:
> Typo: There are two underscores here, which makes this mismatch the HTML code
> binding.
> 
> Could you verify that all the focus() calls target the correct element
manually
> (given that we don't add tests for those)?

Thanks. Sadly these are extremely difficult to test correctly. I tested the
others, but included these optimistically. I'll be testing this along with other
Cellular related UI post branch.

Fixed.

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

https://codereview.chromium.org/2817803002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.js:43:
attached: function() {
On 2017/04/13 00:16:40, dpapad wrote:
> I could not find any callers of open() in this CL that got updated. Who called
> this method?
> 
> Also, nit: Add @override for extra compiler coverage.

Ugh, too many balls in the air. I started to convert these to dom-if based
dialogs but decided to postpone that.

[dpapad, 2017-04-13 00:55:46]

LGTM

[stevenjb, 2017-04-13 01:01:02]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-04-13 01:02:21]

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-13 01:08:52]

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

[commit-bot: I haz the power, 2017-04-13 01:08:53]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2017-04-13 02:50:13]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-04-13 02:50:13]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2817803002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-13 02:51:15]

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-13 04:12:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492051813213800,
"parent_rev": "343665e7bad5f7f1b01b8bfec116cff4194766d8", "commit_rev":
"2e00979d16dd569963846451ce467626b1e86a04"}

[commit-bot: I haz the power, 2017-04-13 04:13:55]

Description was changed from

==========
MD Settings: CrOS: Focus elements on dialog close

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

to

==========
MD Settings: CrOS: Focus elements on dialog close

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

Review-Url: https://codereview.chromium.org/2817803002
Cr-Commit-Position: refs/heads/master@{#464291}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e00979d16dd569963846451ce46...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2e00979d16dd569963846451ce46...