MD Settings: Users: Fix add person validation and dialog focus

MD Settings: Users: Fix add person validation and dialog focus

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

Review-Url: https://codereview.chromium.org/2805383002
Cr-Commit-Position: refs/heads/master@{#463506}
Committed: https://chromium.googlesource.com/chromium/src/+/4cd1f352db04e0325fa1929034af54359f80ebff

[stevenjb, 2017-04-07 23:35:23]

Description was changed from

==========
MD Settings: Users: Fix add person validation and dialog focus

BUG=708979
==========

to

==========
MD Settings: Users: Fix add person validation and dialog focus

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

[stevenjb, 2017-04-07 23:36:58]

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

[dpapad, 2017-04-10 17:39:40]

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_add_user_dialog.html
(right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23:
on-input="validate_" auto-validate">
What does auto-validate do for us? Is it necessary?

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_page.html (right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_page.html:73:
<settings-users-add-user-dialog id="addUserDialog"
Just FYI, as an alternative to clearing the dialog's state in open(), you could
also use a  <template is="dom-if" restamp> around the dialog to ensure that no
information is leaking between dialog invocations. Then you would not have to
worry about resetting any state, which might be a bit more robust and better for
performance anyway (why render a settings-users-add-user-dialog even in cases
when it will not be shown to the user)?

[stevenjb, 2017-04-10 22:02:01]

PTAL

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_add_user_dialog.html
(right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_add_user_dialog.html:23:
on-input="validate_" auto-validate">
On 2017/04/10 17:39:39, dpapad wrote:
> What does auto-validate do for us? Is it necessary?

Somehow I thought on-input was a paper-input thing. Fixed.

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_page.html (right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_page.html:73:
<settings-users-add-user-dialog id="addUserDialog"
On 2017/04/10 17:39:39, dpapad wrote:
> Just FYI, as an alternative to clearing the dialog's state in open(), you
could
> also use a  <template is="dom-if" restamp> around the dialog to ensure that no
> information is leaking between dialog invocations. Then you would not have to
> worry about resetting any state, which might be a bit more robust and better
for
> performance anyway (why render a settings-users-add-user-dialog even in cases
> when it will not be shown to the user)?

The problem with that technique is that it requires using async to open the
dialog after setting the comtrol property, which is fine and all but a bit more
code to maintain.

Also, I personally prefer using data binding anyway rather than relying on
explicitly setting the disabled state; it makes it clear when inspecting the
HTML that the control may be enabled/disabled.

[tommycli, 2017-04-10 22:18:09]

lgtm with some side commentary about the auto open dialogs below

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_page.html (right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_page.html:73:
<settings-users-add-user-dialog id="addUserDialog"
On 2017/04/10 22:02:01, stevenjb wrote:
> On 2017/04/10 17:39:39, dpapad wrote:
> > Just FYI, as an alternative to clearing the dialog's state in open(), you
> could
> > also use a  <template is="dom-if" restamp> around the dialog to ensure that
no
> > information is leaking between dialog invocations. Then you would not have
to
> > worry about resetting any state, which might be a bit more robust and better
> for
> > performance anyway (why render a settings-users-add-user-dialog even in
cases
> > when it will not be shown to the user)?
> 
> The problem with that technique is that it requires using async to open the
> dialog after setting the comtrol property, which is fine and all but a bit
more
> code to maintain.
> 
> Also, I personally prefer using data binding anyway rather than relying on
> explicitly setting the disabled state; it makes it clear when inspecting the
> HTML that the control may be enabled/disabled.

Hey, the pattern dpapad used elsewhere is to put the "showModal" call in the
attached lifecycle call:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/clear_...

I actually do agree with dpapad that having "auto-open" dialogs that are
restamped is the cleanest solution. (And I have tried about 3 other variants)

[stevenjb, 2017-04-10 22:44:00]

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_page.html (right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_page.html:73:
<settings-users-add-user-dialog id="addUserDialog"
On 2017/04/10 22:18:09, tommycli wrote:
> On 2017/04/10 22:02:01, stevenjb wrote:
> > On 2017/04/10 17:39:39, dpapad wrote:
> > > Just FYI, as an alternative to clearing the dialog's state in open(), you
> > could
> > > also use a  <template is="dom-if" restamp> around the dialog to ensure
that
> no
> > > information is leaking between dialog invocations. Then you would not have
> to
> > > worry about resetting any state, which might be a bit more robust and
better
> > for
> > > performance anyway (why render a settings-users-add-user-dialog even in
> cases
> > > when it will not be shown to the user)?
> > 
> > The problem with that technique is that it requires using async to open the
> > dialog after setting the comtrol property, which is fine and all but a bit
> more
> > code to maintain.
> > 
> > Also, I personally prefer using data binding anyway rather than relying on
> > explicitly setting the disabled state; it makes it clear when inspecting the
> > HTML that the control may be enabled/disabled.
> 
> Hey, the pattern dpapad used elsewhere is to put the "showModal" call in the
> attached lifecycle call:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/clear_...
> 
> I actually do agree with dpapad that having "auto-open" dialogs that are
> restamped is the cleanest solution. (And I have tried about 3 other variants)

That example is a little odd, there we have a route to the dialog and show it in
currentRouteChanged:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac...

Does the same pattern work with just setting 'showFoo_ = true' anywhere? Do we
have good examples of that?

I'd prefer to get this in and re-factor it later, but it's a P2 so if we have to
merge after the branch that's not a big deal either (I'm deep in Internet
settings right now).

[dpapad, 2017-04-11 00:03:55]

LGTM with question.

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/users_page.html (right):

https://codereview.chromium.org/2805383002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/users_page.html:73:
<settings-users-add-user-dialog id="addUserDialog"
> Does the same pattern work with just setting 'showFoo_ = true' anywhere? Do we
have good examples of that?

Yes, here is another example, for a dialog that does not have a route,
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta....
Most of our dialogs actually use this pattern (boolean + restamp + calling
showModal() inside attached()).

> 
> I'd prefer to get this in and re-factor it later, but it's a P2 so if we have
to merge after the branch that's not a big deal either (I'm deep in Internet
settings right now).

That's fine by me.

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

https://codereview.chromium.org/2805383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.html:31:
disabled$="[[!isValid_]]">
Probably "$=" not needed here, "=" should be sufficient.

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

https://codereview.chromium.org/2805383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.js:66: //
May be submitted by the Enter key even if the input value is invalid.
@Tommy: Is this possible anymore, now that cr-dialog itself handles the "Enter"
key? If not, we can avoid unnecessary extra validation call, and also
validate_() method can be simplified (no need to return a value anymore).

[stevenjb, 2017-04-11 00:44:23]

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

https://codereview.chromium.org/2805383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.html:31:
disabled$="[[!isValid_]]">
On 2017/04/11 00:03:55, dpapad wrote:
> Probably "$=" not needed here, "=" should be sufficient.
Either works, Polymer maps it to an attribute, and we use a mix in Settings. I
guess the advantage of $ is that it can be used as an attribute in CSS also.

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

https://codereview.chromium.org/2805383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/users_add_user_dialog.js:66: //
May be submitted by the Enter key even if the input value is invalid.
On 2017/04/11 00:03:55, dpapad wrote:
> @Tommy: Is this possible anymore, now that cr-dialog itself handles the
"Enter"
> key? If not, we can avoid unnecessary extra validation call, and also
> validate_() method can be simplified (no need to return a value anymore).

Do let me know about this, I'll take that into consideration when we clean this
up with other cros dialogs.

[stevenjb, 2017-04-11 00:44:26]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-04-11 00:45: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-11 02:52:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491871466827800,
"parent_rev": "70b823c83eccf676efd3770bfc8c297e8aff95a4", "commit_rev":
"4cd1f352db04e0325fa1929034af54359f80ebff"}

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

Description was changed from

==========
MD Settings: Users: Fix add person validation and dialog focus

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

to

==========
MD Settings: Users: Fix add person validation and dialog focus

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

Review-Url: https://codereview.chromium.org/2805383002
Cr-Commit-Position: refs/heads/master@{#463506}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cd1f352db04e0325fa1929034af...
==========

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

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