md-settings: wrap lock screen dialogs in dom-if.

md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests. This also fixes issue where lock screen page flickers when pressing back button on people page, after canceling a password authentication.

TEST=manual, browser_test --gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092, 728394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2893053002
Cr-Commit-Position: refs/heads/master@{#476554}
Committed: https://chromium.googlesource.com/chromium/src/+/54d7eb0b3a09f39e9db65452d72209bee09fd006

[sammiequon, 2017-05-19 18:52:34]

Description was changed from

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092
==========

to

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[sammiequon, 2017-05-19 18:55:20]

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

[commit-bot: I haz the power, 2017-05-19 18:56:13]

Dry run: 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-05-19 21:40:05]

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

[commit-bot: I haz the power, 2017-05-19 21:40:06]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-06-01 02:47:06]

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

[commit-bot: I haz the power, 2017-06-01 02:47:32]

Dry run: 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-06-01 04:11:38]

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

[commit-bot: I haz the power, 2017-06-01 04:11:38]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-06-01 16:18:31]

Patchset #1 (id:1) has been deleted

[sammiequon, 2017-06-01 16:23:32]

Description was changed from

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests. This also fixes issue where lock screen page
flickers when pressing back button on people page, after canceling a password
authentication.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092,728394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[sammiequon, 2017-06-01 16:23:32]

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

[sammiequon, 2017-06-01 16:23:56]

stevenjb@ - Please take a look. Thanks!

[stevenjb, 2017-06-01 17:28:23]

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

[stevenjb, 2017-06-01 17:28:24]

+dpapad@ who is the authority on cr-dialog

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/fingerprint_list.js:155:
this.onSetupFingerprintDialogClose_.bind(this));
Instead of this, does this work in the HTML for the dialog:

on-close="[[onSetupFingerprintDialogClose_]]"

?

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:223: }.bind(this));
Same here, can we use an on-close binding (and below)? (You'll need to define
these inline functions as methods, but I think that is better anyway).

[dpapad, 2017-06-01 17:48:44]

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/fingerprint_list.js:155:
this.onSetupFingerprintDialogClose_.bind(this));
On 2017/06/01 at 17:28:24, stevenjb wrote:
> Instead of this, does this work in the HTML for the dialog:
> 
> on-close="[[onSetupFingerprintDialogClose_]]"
> 
> ?

+1. We've been following the on-close= approach in the majority of cases.

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:237:
dialog.addEventListener('close', function() {
Same here, let's use on-close=, instead of calling addEventListener in JS.

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
this.$.passwordInput.focus();
I don't understand why this is needed at all.
 - <dialog> supports |autofocus|,
 - password_prompt_dialog.html already uses |autofocus|
 - AFAIK paper-input understands |autofocus| (we already use it in other places
like
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search....

This was added at https://codereview.chromium.org/2841313002, which claims that
the observed focus was not as expected. Feel free to experiment with removing
this focus() call in a separate CL. I just point it out here, because it seems
unnecessary.

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:104:
this.$.pinKeyboard.focus();
Similar question about |autofocus| here. Can we add it in the HTML and remove
this call?

[sammiequon, 2017-06-01 20:20:17]

Patchset #2 (id:40001) has been deleted

[sammiequon, 2017-06-01 20:21:10]

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/fingerprint_list.js:155:
this.onSetupFingerprintDialogClose_.bind(this));
On 2017/06/01 17:48:44, dpapad wrote:
> On 2017/06/01 at 17:28:24, stevenjb wrote:
> > Instead of this, does this work in the HTML for the dialog:
> > 
> > on-close="[[onSetupFingerprintDialogClose_]]"
> > 
> > ?
> 
> +1. We've been following the on-close= approach in the majority of cases.

Done.

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:223: }.bind(this));
On 2017/06/01 17:28:24, stevenjb wrote:
> Same here, can we use an on-close binding (and below)? (You'll need to define
> these inline functions as methods, but I think that is better anyway).
> 

Done.

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:237:
dialog.addEventListener('close', function() {
On 2017/06/01 17:48:44, dpapad wrote:
> Same here, let's use on-close=, instead of calling addEventListener in JS.

Done.

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
this.$.passwordInput.focus();
On 2017/06/01 17:48:44, dpapad wrote:
> I don't understand why this is needed at all.
>  - <dialog> supports |autofocus|,
>  - password_prompt_dialog.html already uses |autofocus|
>  - AFAIK paper-input understands |autofocus| (we already use it in other
places
> like
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search....
> 
> This was added at https://codereview.chromium.org/2841313002, which claims
that
> the observed focus was not as expected. Feel free to experiment with removing
> this focus() call in a separate CL. I just point it out here, because it seems
> unnecessary.

I tried removing this but it does not give the expected behavior. Actually I had
to remove |autofocus| as well after moving the dialog into a dom-if to get it
working as expected. Perhaps it's because this dialog does not come up on click
like the other ones?

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/setup_pin_dialog.js:104:
this.$.pinKeyboard.focus();
On 2017/06/01 17:48:44, dpapad wrote:
> Similar question about |autofocus| here. Can we add it in the HTML and remove
> this call?

Done.

[dpapad, 2017-06-01 21:44:53]

+dbeam FYI (see autofocus comment below).

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
this.$.passwordInput.focus();
> I tried removing this but it does not give the expected behavior. Actually I
had to remove |autofocus| as well after moving the dialog into a dom-if to get
it working as expected. Perhaps it's because this dialog does not come up on
click like the other ones?

Thanks for trying.  The logic for focusing an element when a dialog opens is at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML....
@dbeam is more familiar with it.
I am curious to eventually find out why autofocus-ing a paper-input in this
dialog, since at first glance there is nothing special about it (other dialogs
within dom-if seem to work).

[dpapad, 2017-06-01 21:45:05]

LGTM

[stevenjb, 2017-06-01 21:49:09]

lgtm

[sammiequon, 2017-06-01 21:58:21]

Thanks!

[sammiequon, 2017-06-01 22:02:33]

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

https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
this.$.passwordInput.focus();
On 2017/06/01 21:44:53, dpapad wrote:
> > I tried removing this but it does not give the expected behavior. Actually I
> had to remove |autofocus| as well after moving the dialog into a dom-if to get
> it working as expected. Perhaps it's because this dialog does not come up on
> click like the other ones?
> 
> Thanks for trying.  The logic for focusing an element when a dialog opens is
at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML....
> @dbeam is more familiar with it.
> I am curious to eventually find out why autofocus-ing a paper-input in this
> dialog, since at first glance there is nothing special about it (other dialogs
> within dom-if seem to work).

Actually, it's not that autofocus doesn't work, its only doesn't work on the
first try. I think it's because the dialog gets opened as soon as lock screen is
attached or routed to so some things might not be ready. Would putting the async
where the dialog dom-if boolean be a viable solution? (and putting autofocus
back in the paper-input)

this.async( function() { this.showPasswordPromptDialog = true; }).bind(this);   
 in lock_screen.js

WDYT?

[dpapad, 2017-06-01 22:25:24]

On 2017/06/01 at 22:02:33, sammiequon wrote:
>
https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/people_page/password_prompt_dialog.js
(right):
> 
>
https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
this.$.passwordInput.focus();
> On 2017/06/01 21:44:53, dpapad wrote:
> > > I tried removing this but it does not give the expected behavior. Actually
I
> > had to remove |autofocus| as well after moving the dialog into a dom-if to
get
> > it working as expected. Perhaps it's because this dialog does not come up on
> > click like the other ones?
> > 
> > Thanks for trying.  The logic for focusing an element when a dialog opens is
at
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML....
> > @dbeam is more familiar with it.
> > I am curious to eventually find out why autofocus-ing a paper-input in this
> > dialog, since at first glance there is nothing special about it (other
dialogs
> > within dom-if seem to work).
> 
> Actually, it's not that autofocus doesn't work, its only doesn't work on the
first try. I think it's because the dialog gets opened as soon as lock screen is
attached or routed to so some things might not be ready. Would putting the async
where the dialog dom-if boolean be a viable solution? (and putting autofocus
back in the paper-input)
> 
> this.async( function() { this.showPasswordPromptDialog = true; }).bind(this); 
   in lock_screen.js
> 
> WDYT?

Are you saying that when the dialog is attached, the parent of the dialog
(lock_screen.html) is still not visible (still animating), and therefore somehow
the autofocus is ignored? It sounds plausible. I would still not change it as
part of this CL.

[sammiequon, 2017-06-01 22:44:37]

On 2017/06/01 22:25:24, dpapad wrote:
> On 2017/06/01 at 22:02:33, sammiequon wrote:
> >
>
https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
> > File chrome/browser/resources/settings/people_page/password_prompt_dialog.js
> (right):
> > 
> >
>
https://codereview.chromium.org/2893053002/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/people_page/password_prompt_dialog.js:103:
> this.$.passwordInput.focus();
> > On 2017/06/01 21:44:53, dpapad wrote:
> > > > I tried removing this but it does not give the expected behavior.
Actually
> I
> > > had to remove |autofocus| as well after moving the dialog into a dom-if to
> get
> > > it working as expected. Perhaps it's because this dialog does not come up
on
> > > click like the other ones?
> > > 
> > > Thanks for trying.  The logic for focusing an element when a dialog opens
is
> at
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML....
> > > @dbeam is more familiar with it.
> > > I am curious to eventually find out why autofocus-ing a paper-input in
this
> > > dialog, since at first glance there is nothing special about it (other
> dialogs
> > > within dom-if seem to work).
> > 
> > Actually, it's not that autofocus doesn't work, its only doesn't work on the
> first try. I think it's because the dialog gets opened as soon as lock screen
is
> attached or routed to so some things might not be ready. Would putting the
async
> where the dialog dom-if boolean be a viable solution? (and putting autofocus
> back in the paper-input)
> > 
> > this.async( function() { this.showPasswordPromptDialog = true;
}).bind(this); 
>    in lock_screen.js
> > 
> > WDYT?
> 
> Are you saying that when the dialog is attached, the parent of the dialog
> (lock_screen.html) is still not visible (still animating), and therefore
somehow
> the autofocus is ignored? It sounds plausible. I would still not change it as
> part of this CL.

Yes, that was my kind of my hypothesis. I filed a bug. I'll commit this CL as is
then, thanks!

[sammiequon, 2017-06-01 22:44:46]

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

[commit-bot: I haz the power, 2017-06-01 22:45:28]

Dry run: 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-06-02 01:55:38]

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

[commit-bot: I haz the power, 2017-06-02 01:55:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sammiequon, 2017-06-02 02:18:43]

The CQ bit was checked by sammiequon@chromium.org

[commit-bot: I haz the power, 2017-06-02 02:19:48]

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-06-02 03:45:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496369923472450,
"parent_rev": "595784482507ade80774fe371effc58987f8a30e", "commit_rev":
"54d7eb0b3a09f39e9db65452d72209bee09fd006"}

[commit-bot: I haz the power, 2017-06-02 03:46:11]

Description was changed from

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests. This also fixes issue where lock screen page
flickers when pressing back button on people page, after canceling a password
authentication.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092,728394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
md-settings: wrap lock screen dialogs in dom-if.

Also had to modify some tests. This also fixes issue where lock screen page
flickers when pressing back button on people page, after canceling a password
authentication.

TEST=manual, browser_test
--gtest_filter="CrSettingsPeoplePageLockScreenTest.*:CrSettingsPeoplePageSetupPinDialogTest.*:CrSettingsFingerprintListTest.*"
BUG=723092,728394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2893053002
Cr-Commit-Position: refs/heads/master@{#476554}
Committed:
https://chromium.googlesource.com/chromium/src/+/54d7eb0b3a09f39e9db65452d722...
==========

[commit-bot: I haz the power, 2017-06-02 03:46:12]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/54d7eb0b3a09f39e9db65452d722...