MD User Manager: Supervised user checkbox is always displaying

MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9a4c0a69d113eda53a86c4cd3013632f82f88a65
Cr-Commit-Position: refs/heads/master@{#391417}

[Moe, 2016-05-02 18:55:56]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

Signed-off-by: mahmadi <mahmadi@chromium.org>

BUG=563722
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

Signed-off-by: mahmadi <mahmadi@chromium.org>

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

[Moe, 2016-05-02 18:57:08]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

Signed-off-by: mahmadi <mahmadi@chromium.org>

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

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

BUG=563722
==========

[Moe, 2016-05-02 18:57:08]

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

[Moe, 2016-05-02 18:57:35]

tommy, please review this patch.

[tommycli, 2016-05-02 19:10:13]

moe: Thanks!

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:110:
#supervised-user-container #learn-more,
instead of this, can you use

-webkit-padding-start on #supervised-user-container? Then you wouldn't need to
apply this rule to both ids

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:119:
#supervised-user-container paper-dropdown-menu {
What was the purpose of moving these rules to the more specific selector? Just
to be more specific? (Not saying to reverse this change, I'm just curious)

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:214: <div
id="custodians">
If you can use -webkit-padding-start, you can get rid of this div

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:135: * pushed by the
browser as well as the noSignedInUserMessage i18n string.
Would this comment be more readable as?

Handles tap events from:
 - links within dynamic warning/error messages pushed from the browser
 - the 'noSignedInUserMessage' i18n string

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:147:
event.preventDefault();
You could move this line outside the if blocks

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:386:
assertFalse(!!createProfileElement.$$('paper-dropdown-menu'));
you will also need an

assertTrue(fooElement.hidden) test, since the above line only tests that the
element exists.

[Moe, 2016-05-02 20:24:31]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

BUG=563722
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

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

[Moe, 2016-05-02 20:25:30]

Patchset #2 (id:20001) has been deleted

[Moe, 2016-05-02 20:28:19]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

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

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
==========

[Moe, 2016-05-02 20:28:51]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-05-02 20:30:44]

Patchset #2 (id:40001) has been deleted

[Moe, 2016-05-02 20:32:54]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
==========

[Moe, 2016-05-02 20:34:57]

Thank you tommy.

I also snuck in a fix for the ripple effect of the dropdown menu which was
related to missing padding. I simply had to remove:

paper-dropdown-menu {
  --paper-input-container: {
    padding: 0;
  };
}

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:110:
#supervised-user-container #learn-more,
On 2016/05/02 19:10:12, tommycli wrote:
> instead of this, can you use
> 
> -webkit-padding-start on #supervised-user-container? Then you wouldn't need to
> apply this rule to both ids

Done.

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:119:
#supervised-user-container paper-dropdown-menu {
On 2016/05/02 19:10:13, tommycli wrote:
> What was the purpose of moving these rules to the more specific selector? Just
> to be more specific? (Not saying to reverse this change, I'm just curious)

It's more for readability and personal preference. When glancing at these it,
you'd immediately know where they appear in the html rather having to search for
them and go back and forth between html and css.

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:214: <div
id="custodians">
On 2016/05/02 19:10:13, tommycli wrote:
> If you can use -webkit-padding-start, you can get rid of this div

Done. However, removing this this would require adding another div around <a> so
the dropdown would go to the next line.

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:135: * pushed by the
browser as well as the noSignedInUserMessage i18n string.
On 2016/05/02 19:10:13, tommycli wrote:
> Would this comment be more readable as?
> 
> Handles tap events from:
>  - links within dynamic warning/error messages pushed from the browser
>  - the 'noSignedInUserMessage' i18n string

Done.

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:147:
event.preventDefault();
On 2016/05/02 19:10:13, tommycli wrote:
> You could move this line outside the if blocks

This handler handles all tap events on the page. We only want to call
preventDefault when the tap is coming from one of these elements. I can wrap the
if-elseif chain inside another if condition that checks if the source of the
event is a link and move this line to the outer if condition. But I don't think
that's easier to read.

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:386:
assertFalse(!!createProfileElement.$$('paper-dropdown-menu'));
On 2016/05/02 19:10:13, tommycli wrote:
> you will also need an
> 
> assertTrue(fooElement.hidden) test, since the above line only tests that the
> element exists.

This line is asserting that the dropdown does NOT exist. which is the expected
behavior.

[Moe, 2016-05-02 20:56:58]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-05-02 21:57:49]

lgtm thanks!

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:147:
event.preventDefault();
On 2016/05/02 20:34:57, Moe wrote:
> On 2016/05/02 19:10:13, tommycli wrote:
> > You could move this line outside the if blocks
> 
> This handler handles all tap events on the page. We only want to call
> preventDefault when the tap is coming from one of these elements. I can wrap
the
> if-elseif chain inside another if condition that checks if the source of the
> event is a link and move this line to the outer if condition. But I don't
think
> that's easier to read.

You are correct. My mistake :)

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/1939933002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:386:
assertFalse(!!createProfileElement.$$('paper-dropdown-menu'));
On 2016/05/02 20:34:57, Moe wrote:
> On 2016/05/02 19:10:13, tommycli wrote:
> > you will also need an
> > 
> > assertTrue(fooElement.hidden) test, since the above line only tests that the
> > element exists.
> 
> This line is asserting that the dropdown does NOT exist. which is the expected
> behavior.

Ah you're right. thanks

[Moe, 2016-05-03 15:14:10]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-05-03 15:14:10]

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

[Moe, 2016-05-03 15:15:03]

thank you tommy.

demetrios, Please review this CL.

[Moe, 2016-05-03 15:16:15]

mahmadi@chromium.org changed reviewers:
+ rogerta@chromium.org

[Moe, 2016-05-03 15:18:06]

Roger, only adding a string to:
c/b/ui/webui/signin/signin_create_profile_handler.cc

[Roger Tawa OOO till Jul 10th, 2016-05-03 15:43:50]

lgtm

[dpapad, 2016-05-03 17:30:26]

Could you modify the CL description to be more friendly for git history and
other tools? For example as it is now it is getting trimmed by the codereview
UI. Specifically something as follows

MD User Manager: Short sentence here that can fit in one line.

Longer multi-line description here with more details about this CL.
....
....

https://codereview.chromium.org/1939933002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12798: +        <message
name="IDS_PROFILES_CREATE_SUPERVISED_NO_SIGNED_IN_USER_TEXT" desc="Informative
text that displays below the 'Supervised user' checkbox in the create-profile
dialog when there are no signed in users to choose as the custodian.">
Nit: s/that displays/displayed

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:210: <paper-menu
class="dropdown-content"
Could this paper-menu be swapped with the lighter weight paper-listbox? See
related discussion at crbug.com/601214

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:130:
this.NO_USER_SELECTED = NO_USER_SELECTED;
Nit: Can this be moved in the properties {} section? See similar example where
an enum is mirrored such that it can be used in HTML,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:143: if (element.id
== 'supervised-user-import-existing') {
I can't find any element with this ID in the HTML? Can you point me to where it
is defined? Is it being added dynamically? If so, can you add a comment
explaining where the ID comes from?

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:146: } else if
(element.id == 'sign-in-to-chrome') {
Same here.

[Moe, 2016-05-03 19:31:55]

Description was changed from

==========
Supervised user checkbox is always displaying. When there are no signed in users
available, a message containing a link to a HC article is displayed instead of
the dropdown.

It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC
article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-05-03 20:30:10]

https://codereview.chromium.org/1939933002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12798: +        <message
name="IDS_PROFILES_CREATE_SUPERVISED_NO_SIGNED_IN_USER_TEXT" desc="Informative
text that displays below the 'Supervised user' checkbox in the create-profile
dialog when there are no signed in users to choose as the custodian.">
On 2016/05/03 17:30:26, dpapad wrote:
> Nit: s/that displays/displayed

Done.

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:210: <paper-menu
class="dropdown-content"
On 2016/05/03 17:30:26, dpapad wrote:
> Could this paper-menu be swapped with the lighter weight paper-listbox? See
> related discussion at crbug.com/601214

yep. Done!

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:143: if (element.id
== 'supervised-user-import-existing') {
On 2016/05/03 17:30:26, dpapad wrote:
> I can't find any element with this ID in the HTML? Can you point me to where
it
> is defined? Is it being added dynamically? If so, can you add a comment
> explaining where the ID comes from?

Yes. These are both dynamically added. The comment above the function explains
this.

https://codereview.chromium.org/1939933002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:146: } else if
(element.id == 'sign-in-to-chrome') {
On 2016/05/03 17:30:26, dpapad wrote:
> Same here.

Same as above.

[dpapad, 2016-05-03 20:40:29]

lgtm

https://codereview.chromium.org/1939933002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/1939933002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:149:
selector.selected = 0;
Nit (optional): Use selectIndex(0) instead of assigning directly?

[Moe, 2016-05-03 22:59:25]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-05-03 22:59:26]

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

[commit-bot: I haz the power, 2016-05-03 23:00:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939933002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939933002/120001

[commit-bot: I haz the power, 2016-05-04 00:09:34]

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

[commit-bot: I haz the power, 2016-05-04 00:09:36]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-05-04 00:09:54]

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

[Moe, 2016-05-04 01:48:18]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2016-05-04 01:49:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939933002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939933002/120001

[commit-bot: I haz the power, 2016-05-04 02:16:20]

Description was changed from

==========
MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC
article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC
article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-04 02:16:22]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-05-04 02:18:51]

Description was changed from

==========
MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC
article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD User Manager: Supervised user checkbox is always displaying.

When there are no signed in users available, a message containing a link to a HC
article is displayed instead of the dropdown.
It also fixes the bug related to the ripple effect on the dropdown menu.

BUG=563722, 605087
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9a4c0a69d113eda53a86c4cd3013632f82f88a65
Cr-Commit-Position: refs/heads/master@{#391417}
==========

[commit-bot: I haz the power, 2016-05-04 02:18:53]

Patchset 5 (id:??) landed as
https://crrev.com/9a4c0a69d113eda53a86c4cd3013632f82f88a65
Cr-Commit-Position: refs/heads/master@{#391417}