Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11308081:
cros: Account picker UI for public account. (Closed)
|
Created:
8 years, 1 month ago by xiyuan Modified:
8 years ago CC:
chromium-reviews, arv (Not doing code reviews), oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, Mattias Nissler (ping if slow), roma Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncros: Account picker UI for public account.
BUG=152918
TEST=Verify public accounts show up in account picker UI.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171419
Patch Set 1 #Patch Set 2 : rebased on top of 11419184 #
Total comments: 80
Patch Set 3 : rebase, 2nd step screen -> expand user pod #
Total comments: 6
Patch Set 4 : for comments in #3 #
Total comments: 5
Patch Set 5 : rebase, address comments in #4 #Patch Set 6 : reupload #Patch Set 7 : reupload #
Messages
Total messages: 22 (0 generated)
This CL depends on https://codereview.chromium.org/11419184/ to feed to public accounts in UserManager's user list. - The public account flag is passed to webui/js world via user dict's "publicAccount" property; - The user pod css/js is adjusted to show the pod with a managed icon and always show user name; - Upon activation of a public account pod, "chrome.send('launchPublicAccount', [this.email_])" is fired to call into SigninScreenHandler::HandleLaunchPublicAccount with the email of the public account; Screenshots of current status is attached to the bug.
+mnissler in case he is interested.
A few general comments: * Right now, when the user clicks on a public account user pod, the entire screen switches abruptly, replacing the pod row with just the public account pod in its expanded form. This is not how I interpreted the mocks. I thought the pod should expand in an animated fashion. Roma, can you comment on that? * Public account users are added and removed though policy. A policy refresh can happen at *any time*, including when the extended pod (aka PublicAccountSigninScreen) is being shown. The UI does get informed that the list of users changed. The PublicAccountSigninScreen should close itself and return to the pod row in this case. * What if the user decides not to click "Enter"? There seems to be no way to get back to the pod row. https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string... chrome/app/chromeos_strings.grdp:3489: <message name="IDS_LOGIN_PUBLIC_ACCOUNT_INFO" desc="Text for the hint on the public account sign-in panel."> The wording "public account sign-in panel" is inconsistent with the terms we normally use on the login screen. I suggest "public account user pod" instead. https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string... chrome/app/chromeos_strings.grdp:3490: This device is managed by (<ph name="DOMAIN">$1<ex>yourdomain.com</ex></ph>) and will be used by others after you. The brackets in the mock were meant to be part of the placeholder. Please remove them here. https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string... chrome/app/chromeos_strings.grdp:3492: <message name="IDS_LOGIN_PUBLIC_ACCOUNT_HINT" desc="Text for the bold hint on the public account sign-in panel."> s/public account sign-in panel/public account user pod/ as above Also, I think that rather than differentiating the two texts in the pod based on their font weight ("the hint" and "the bold hint"), you should differentiate them based on their function. How about: "Text shown in the public account user pod, informing the user that this is a public, managed account." and "Text shown in the public account user pod, reminding the user to log out." https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string... chrome/app/chromeos_strings.grdp:3495: <message name="IDS_LOGIN_PUBLIC_ACCOUNT_ENTER" desc="Text for the sign-in button in public account sign-in panel."> s/public account sign-in panel/public account user pod/ as above https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/login.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/login.js:42: login.PublicAccountSigninScreen.register(); Nit: Do we want alphabetical order for lines 40-42? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/oobe.css (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/oobe.css:125: html[oobe=new] #oobe.public-account-signin #inner-container { Nit: Alphabetic order (tpm-error-message already violates it, you might want to put that one in the correct order as well). https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/oobe.js:78: login.PublicAccountSigninScreen.register(); Nit: Do we want alphabetical order for lines 76-78? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.css (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:11: #public-account-avartar { The avatar size increases here from 160 x 160 in a regular pod to 220 x 220 in the expanded pod. This is not what the mocks show. The avatar should stay the same size. Also, it would be good to use style rules from user_pod_row.css where possible and only have a minimal set of overrides in this file, ensuring a more uniform visual appearance. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.html (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.html:2: <img id="public-account-avartar"> Spelling: It's "avatar", not "avartar" (here and elsewhere in the CL). https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.html:8: src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY"> As was pointed out in the bug, the mock uses the grayscale version of the asset here. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.html:12: <p id="public-account-hint" i18n-content="publicAccountHint"></p> In the grdp file, you called these two "the hint" and "the bold hint". It would be nice to synchronize the |id| attributes with their descriptions in the grdp. How about id="public-account-info" and id="public-account-reminder"? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:22: email_: undefined, Is this needed? If you try to access |this.email_| before it has been set, you will get |undefined|, even if you do not specify that explicitly. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.css (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:178: .pod:not(.need-password).focused input[type='password'] { How about you add |display: none| to the rule in 151 and then simplify and reverse this rule to: .pod.need-password.focused input[type='password'] { display: inline-block; } https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:240: right: 10px; Should this have a "left: auto" to go with the "right: 10px"? Also note that the badge position is different in the mock. This may be OK but please check with the UI people. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:328: } Did you accidentally add trailing whitespace here? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:251: if (this.user.publicAccount) { As described below, I think a separate |this.user.needsPassword| would be nice. Then you could split it like this: if (this.user.needsPassword) this.classList.add('need-password'); else this.classList.remove('need-password'); this.managedBadgeElement.hidden = !this.user.publicAccout; https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:278: // not currently signed in (i.e. not the lock screen) and is not a public Nit: "public account user" (for consistency throughout the code). https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:282: !this.user.publicAccount; This is a bit akin to user agent sniffing: What you *really* want to know is whether this user needs a password. What you check for instead is |publicAccount| which happens to imply passwordless login. Maybe we should add another property, |needsPassword|, and use that one here instead? |needsPassword| would tell you whether a GAIA login is needed. |publicAccount| would tell you furthermore whether this is a GAIA account. In the future, there may be accounts that have |needsPassword| = |false| without being public accounts. By adding a separate |needsPassword| property, we would be nicely prepared for this. If you agree with this idea, I can make the required C++-side changes to set |needsPassword| correctly. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:348: Oobe.showScreen({ I have not run the code but I would imagine that this switch to another screen is visually unpleasant. My understanding of the mock was that the pod should transform to its extended form smoothly (the additional info on the right should slide out). +roma for comment on that. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:353: imageUrl: this.imageElement.src The avatar is loaded asynchronously. It may happen that you copy the value of |this.imageElement.src| before the image has finished loading. You will need to make the extended user pod listen for asynchronous loads as well. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:422: this.showSigninUI(); Any idea why this line uses |this.showSigninUI()| instead of just setting |this.parentNode.activatedPod = this|? I think the latter should work just as well. If so, you can combine the if and else-if branches into one. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_template.html (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_template.html:10: <img class="managed-badge" src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY" The mock asks for a grayscale image. We do have an asset for that as well. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:658: email = gaia::SanitizeEmail(email); This is not used right now. Please remove. I will re-add it as required. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:660: // TODO(xiyuan): Wire this with real public account signin. Feel free to list me as the TODO owner here. I will start work on that ASAP.
I forgot to add comments for one file. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:108: } // namespace Nit: We always have two spaces before |namespace|. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:207: if (enterprise_domain.empty()) { This case would be an error - public accounts defined through policy but no managing domain. There is no have special code handling such an error in a particularly pretty way. Just feed the name to |GetStringFUTF16| as you would normally do and let it return a string with an empty domain inside.
lgtm with nits https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/login.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/login.js:42: login.PublicAccountSigninScreen.register(); On 2012/11/28 15:11:38, bartfab wrote: > Nit: Do we want alphabetical order for lines 40-42? I don't think that this is required. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.css (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:44: color: black; Is this really supposed to be black? As far a I remember we tend not to use black text on sign in screen. user pod name is color: #565656; https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:73: html[dir=ltr] #public-account-enter-button { nit: add extra lines between styles https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:76: html[dir=rtl] #public-account-enter-button { nit: same here https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:7: * Creates a new public account sign in screen. nit: at sign in screen https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:348: Oobe.showScreen({ On 2012/11/28 15:11:38, bartfab wrote: > I have not run the code but I would imagine that this switch to another screen > is visually unpleasant. My understanding of the mock was that the pod should > transform to its extended form smoothly (the additional info on the right should > slide out). > > +roma for comment on that. Yes, that would be the right thing to do but that would require more complicated refactoring as we would now have special types of user pods that are able to change their size. Let's leave this implementation for now otherwise it may conflict with https://code.google.com/p/chromium/issues/detail?id=122346 which will make it possible to change default size of user pods. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_template.html (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_template.html:10: <img class="managed-badge" src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY" nit: 80 chars limit https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:207: if (enterprise_domain.empty()) { On 2012/11/28 15:18:33, bartfab wrote: > This case would be an error - public accounts defined through policy but no > managing domain. There is no have special code handling such an error in a > particularly pretty way. Just feed the name to |GetStringFUTF16| as you would > normally do and let it return a string with an empty domain inside. Is this a possible situation: Chromebook stays at login screen, policy updates so it should show public account? There's a code that refreshes sign in screen on policy (preferences) update. That means that strings should be available too i.e. remove conditions?
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:207: if (enterprise_domain.empty()) { On 2012/11/28 18:57:34, Nikita Kostylev wrote: > On 2012/11/28 15:18:33, bartfab wrote: > > This case would be an error - public accounts defined through policy but no > > managing domain. There is no have special code handling such an error in a > > particularly pretty way. Just feed the name to |GetStringFUTF16| as you would > > normally do and let it return a string with an empty domain inside. > > Is this a possible situation: > > Chromebook stays at login screen, policy updates so it should show public > account? There's a code that refreshes sign in screen on policy (preferences) > update. That means that strings should be available too i.e. remove conditions? Yes, this is entirely possible. A policy refresh can happen any time, so user pods may appear and disappear while the login screen is visible. I know that the login screen correctly handles this (i.e. pods do get added and removed). It will be important to make sure that the strings are available for any newly created pods. We should also verify that this code handles enrollment correctly. On a virgin device, IsEnterpriseManaged() returns |false| and the enterprise domain is empty. If the device is now enrolled, IsEnterpriseManaged() starts returning |true| and there is an enterprise domain. IIRC, the login screen is redrawn from scratch after enrollment completes. I would expect the SigninScreenHandler to also get reinitialized at that point so that it picks up the new managed status - but it would be good to verify that the domain really is picked up correctly immediately after enrollment.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/login.html (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/login.html:71: <include src="screen_public_account_signin.html"> I already said in another CL that "public" account sounds strange. Maybe call them "gaia" accounts? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.css (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:19: border-left: 1px solid lightgray; Will this work with RTL correctly? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.css:71: position: absolute; I guess you can simplify a bit if you add 'right: 0' here and then 'right: auto; left: 0' in [dir=rtl] selector https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:22: email_: undefined, This is what roughly corresponds to a static member in OO languages (i.e., all PublicAccountSigninScreen instances will share a common this.email_). Is that really what you meant? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:35: $('public-account-avartar').src = data.imageUrl; avatar https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:353: imageUrl: this.imageElement.src On 2012/11/28 15:11:38, bartfab wrote: > The avatar is loaded asynchronously. It may happen that you copy the value of > |this.imageElement.src| before the image has finished loading. You will need to > make the extended user pod listen for asynchronous loads as well. 'src' should still be accessible while it loads, right? https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:422: this.showSigninUI(); On 2012/11/28 15:11:38, bartfab wrote: > Any idea why this line uses |this.showSigninUI()| instead of just setting > |this.parentNode.activatedPod = this|? I think the latter should work just as > well. If so, you can combine the if and else-if branches into one. Because if signingButton.hidden == false, that means clicking the pod should bring up the GAIA sign-in UI (this is the case for users with lost OAuth token).
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/login.html (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/login.html:71: <include src="screen_public_account_signin.html"> On 2012/11/28 21:50:39, Ivan Korotkov wrote: > I already said in another CL that "public" account sounds strange. Maybe call > them "gaia" accounts? Uh, sorry, I've been under misunderstanding about what public accounts are, please disregard this comment.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:7: * Creates a new public account sign in screen. On 2012/11/28 18:57:34, Nikita Kostylev wrote: > nit: at sign in screen How does this class "create a new public account at [the] sign in screen"? It creates a new screen, not a new account at the screen. IMHO, the original wording is correct here. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:22: email_: undefined, On 2012/11/28 21:50:39, Ivan Korotkov wrote: > This is what roughly corresponds to a static member in OO languages (i.e., all > PublicAccountSigninScreen instances will share a common this.email_). Is that > really what you meant? Well, it's only a static default. If you set this.email_ = "blah" on a specific object (as Xiyuan does in line 33), that value will stick on the object. So this line here does not break anything - but it is unnecessary as the default is |undefined| anyway. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:353: imageUrl: this.imageElement.src On 2012/11/28 21:50:39, Ivan Korotkov wrote: > On 2012/11/28 15:11:38, bartfab wrote: > > The avatar is loaded asynchronously. It may happen that you copy the value of > > |this.imageElement.src| before the image has finished loading. You will need > to > > make the extended user pod listen for asynchronous loads as well. > > 'src' should still be accessible while it loads, right? Sure, the |src| is accessible - but what does it point at? I have not inspected the backend code in detail but I would expect that |this.imageElement.src| points at some placeholder image and then gets reset to the actual image's URI once the image is loaded. If the public account sign-in screen copied the |src| value before that, it will never notice the change. https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:422: this.showSigninUI(); On 2012/11/28 21:50:39, Ivan Korotkov wrote: > On 2012/11/28 15:11:38, bartfab wrote: > > Any idea why this line uses |this.showSigninUI()| instead of just setting > > |this.parentNode.activatedPod = this|? I think the latter should work just as > > well. If so, you can combine the if and else-if branches into one. > > Because if signingButton.hidden == false, that means clicking the pod should > bring up the GAIA sign-in UI (this is the case for users with lost OAuth token). Thanks for clarifying.
https://chromiumcodereview.appspot.com/11308081/diff/4004/chrome/browser/ui/w... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://chromiumcodereview.appspot.com/11308081/diff/4004/chrome/browser/ui/w... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:207: if (enterprise_domain.empty()) { Nikita clarified what happens when the login screen is shown after enrollment completed: "SigninScreenHandler::Initialize is not called in that case but SendUseList is called." So reading the domain name SigninScreenHandler's constructor is not sufficient. If a device is freshly enrolled, the domain name will never make it to the UI otherwise. On 2012/11/28 19:19:11, bartfab wrote: > On 2012/11/28 18:57:34, Nikita Kostylev wrote: > > On 2012/11/28 15:18:33, bartfab wrote: > > > This case would be an error - public accounts defined through policy but no > > > managing domain. There is no have special code handling such an error in a > > > particularly pretty way. Just feed the name to |GetStringFUTF16| as you > would > > > normally do and let it return a string with an empty domain inside. > > > > Is this a possible situation: > > > > Chromebook stays at login screen, policy updates so it should show public > > account? There's a code that refreshes sign in screen on policy (preferences) > > update. That means that strings should be available too i.e. remove > conditions? > > Yes, this is entirely possible. A policy refresh can happen any time, so user > pods may appear and disappear while the login screen is visible. I know that the > login screen correctly handles this (i.e. pods do get added and removed). It > will be important to make sure that the strings are available for any newly > created pods. > > We should also verify that this code handles enrollment correctly. On a virgin > device, IsEnterpriseManaged() returns |false| and the enterprise domain is > empty. If the device is now enrolled, IsEnterpriseManaged() starts returning > |true| and there is an enterprise domain. IIRC, the login screen is redrawn from > scratch after enrollment completes. I would expect the SigninScreenHandler to > also get reinitialized at that point so that it picks up the new managed status > - but it would be good to verify that the domain really is picked up correctly > immediately after enrollment.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/screen_public_account_signin.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/screen_public_account_signin.js:7: * Creates a new public account sign in screen. On 2012/11/29 14:32:28, bartfab wrote: > On 2012/11/28 18:57:34, Nikita Kostylev wrote: > > nit: at sign in screen > > How does this class "create a new public account at [the] sign in screen"? It > creates a new screen, not a new account at the screen. IMHO, the original > wording is correct here. Right.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.js:353: imageUrl: this.imageElement.src On 2012/11/29 14:32:28, bartfab wrote: > On 2012/11/28 21:50:39, Ivan Korotkov wrote: > > On 2012/11/28 15:11:38, bartfab wrote: > > > The avatar is loaded asynchronously. It may happen that you copy the value > of > > > |this.imageElement.src| before the image has finished loading. You will need > > to > > > make the extended user pod listen for asynchronous loads as well. > > > > 'src' should still be accessible while it loads, right? > > Sure, the |src| is accessible - but what does it point at? I have not inspected > the backend code in detail but I would expect that |this.imageElement.src| > points at some placeholder image and then gets reset to the actual image's URI > once the image is loaded. If the public account sign-in screen copied the |src| > value before that, it will never notice the change. Right, I missed that.
CL refactored to expand user pod for 2nd step instead of going to a new step
screen.
Please take another look.
- JS: Derived a PublicAccountUserPod from UserPod.
This class handles public contains public account specific logics.
- JS: Update UserPod child element getter to use class name so that it's
easier to change the DOM template now;
- JS: Updated the click and focus handler of PodRow to be more generic
(handles deeper descedent elements) for click/focus event on the sub
controls of a UserPod;
- DOM: Update the user pod template:
- Normal user pod DOM is contained in a "main-pane" div now so that they
don't move around in "expanding" animation;
- Added an extra element section for public account side panes;
- CSS: Update rules for normal UserPod for newly added "main-pane";
- CSS: Added rules to public account user pod side pane elements;
Thanks for reviewing.
https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string...
File chrome/app/chromeos_strings.grdp (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:3489: <message
name="IDS_LOGIN_PUBLIC_ACCOUNT_INFO" desc="Text for the hint on the public
account sign-in panel.">
On 2012/11/28 15:11:38, bartfab wrote:
> The wording "public account sign-in panel" is inconsistent with the terms we
> normally use on the login screen. I suggest "public account user pod" instead.
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:3490: This device is managed by (<ph
name="DOMAIN">$1<ex>yourdomain.com</ex></ph>) and will be used by others after
you.
On 2012/11/28 15:11:38, bartfab wrote:
> The brackets in the mock were meant to be part of the placeholder. Please
remove
> them here.
Done.
And "managed" -> "owned" per mlchan
https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:3492: <message
name="IDS_LOGIN_PUBLIC_ACCOUNT_HINT" desc="Text for the bold hint on the public
account sign-in panel.">
On 2012/11/28 15:11:38, bartfab wrote:
> s/public account sign-in panel/public account user pod/ as above
>
> Also, I think that rather than differentiating the two texts in the pod based
on
> their font weight ("the hint" and "the bold hint"), you should differentiate
> them based on their function. How about:
>
> "Text shown in the public account user pod, informing the user that this is a
> public, managed account."
> and
> "Text shown in the public account user pod, reminding the user to log out."
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:3495: <message
name="IDS_LOGIN_PUBLIC_ACCOUNT_ENTER" desc="Text for the sign-in button in
public account sign-in panel.">
On 2012/11/28 15:11:38, bartfab wrote:
> s/public account sign-in panel/public account user pod/ as above
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/login.js (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/login.js:42:
login.PublicAccountSigninScreen.register();
PublicAccountSigninScreen is dead.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:125: html[oobe=new]
#oobe.public-account-signin #inner-container {
On 2012/11/28 15:11:38, bartfab wrote:
> Nit: Alphabetic order (tpm-error-message already violates it, you might want
to
> put that one in the correct order as well).
PublicAccountSigninScreen is dead now and this file is removed from this CL.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.js (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.js:78:
login.PublicAccountSigninScreen.register();
On 2012/11/28 15:11:38, bartfab wrote:
> Nit: Do we want alphabetical order for lines 76-78?
PublicAccountSigninScreen is dead and this file is no longer in this CL.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_public_account_signin.css
(right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:11:
#public-account-avartar {
On 2012/11/28 15:11:38, bartfab wrote:
> The avatar size increases here from 160 x 160 in a regular pod to 220 x 220 in
> the expanded pod. This is not what the mocks show. The avatar should stay the
> same size. Also, it would be good to use style rules from user_pod_row.css
where
> possible and only have a minimal set of overrides in this file, ensuring a
more
> uniform visual appearance.
CL refactored to expanding pod in-place and will use the same avatar image for
both states.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:19:
border-left: 1px solid lightgray;
On 2012/11/28 21:50:39, Ivan Korotkov wrote:
> Will this work with RTL correctly?
Yes because the divider div's width is 1px.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:44:
color: black;
On 2012/11/28 18:57:34, Nikita Kostylev wrote:
> Is this really supposed to be black?
> As far a I remember we tend not to use black text on sign in screen.
>
> user pod name is color: #565656;
I'll let the UX decide. The mock uses black.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:71:
position: absolute;
On 2012/11/28 21:50:39, Ivan Korotkov wrote:
> I guess you can simplify a bit if you add 'right: 0' here and then 'right:
auto;
> left: 0' in [dir=rtl] selector
Done in its new incarnation, i.e. as ".side-pane-container .enter-button".
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:73:
html[dir=ltr] #public-account-enter-button {
On 2012/11/28 18:57:34, Nikita Kostylev wrote:
> nit: add extra lines between styles
Removed now.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.css:76:
html[dir=rtl] #public-account-enter-button {
On 2012/11/28 18:57:34, Nikita Kostylev wrote:
> nit: same here
Done in its new incarnation, i.e. as rtl rule for ".side-pane-container
.enter-button".
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_public_account_signin.html
(right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.html:2:
<img id="public-account-avartar">
On 2012/11/28 15:11:38, bartfab wrote:
> Spelling: It's "avatar", not "avartar" (here and elsewhere in the CL).
New patch successfully avoided this word. :)
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.html:8:
src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY">
On 2012/11/28 15:11:38, bartfab wrote:
> As was pointed out in the bug, the mock uses the grayscale version of the
asset
> here.
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_public_account_signin.html:12: <p
id="public-account-hint" i18n-content="publicAccountHint"></p>
On 2012/11/28 15:11:38, bartfab wrote:
> In the grdp file, you called these two "the hint" and "the bold hint". It
would
> be nice to synchronize the |id| attributes with their descriptions in the
grdp.
> How about id="public-account-info" and id="public-account-reminder"?
Done in its new place.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/user_pod_row.css (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.css:178:
.pod:not(.need-password).focused input[type='password'] {
On 2012/11/28 15:11:38, bartfab wrote:
> How about you add |display: none| to the rule in 151 and then simplify and
> reverse this rule to:
>
> .pod.need-password.focused input[type='password'] {
> display: inline-block;
> }
Yep, it works. Changed as suggested.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.css:240: right: 10px;
On 2012/11/28 15:11:38, bartfab wrote:
> Should this have a "left: auto" to go with the "right: 10px"?
>
> Also note that the badge position is different in the mock. This may be OK but
> please check with the UI people.
The mock's pod is a bit different from the current style in use and the icon
size is 20px (41 physical pixels) in mock but the icon asset we have is 16px.
I'll leave this part until we finalize.
Also changed to to use background image instead of an img element now.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.css:328: }
On 2012/11/28 15:11:38, bartfab wrote:
> Did you accidentally add trailing whitespace here?
My vim automatically adds a new line break here.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.js:251: if
(this.user.publicAccount) {
On 2012/11/28 15:11:38, bartfab wrote:
> As described below, I think a separate |this.user.needsPassword| would be
nice.
> Then you could split it like this:
>
> if (this.user.needsPassword)
> this.classList.add('need-password');
> else
> this.classList.remove('need-password');
> this.managedBadgeElement.hidden = !this.user.publicAccout;
Created a new PublicAccountUserPod that derives from UserPod and all public
specific code is put together there.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.js:278: // not currently
signed in (i.e. not the lock screen) and is not a public
On 2012/11/28 15:11:38, bartfab wrote:
> Nit: "public account user" (for consistency throughout the code).
This change is reverted in new patch.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.js:282:
!this.user.publicAccount;
On 2012/11/28 15:11:38, bartfab wrote:
> This is a bit akin to user agent sniffing: What you *really* want to know is
> whether this user needs a password. What you check for instead is
> |publicAccount| which happens to imply passwordless login. Maybe we should add
> another property, |needsPassword|, and use that one here instead?
>
> |needsPassword| would tell you whether a GAIA login is needed.
> |publicAccount| would tell you furthermore whether this is a GAIA account.
>
> In the future, there may be accounts that have |needsPassword| = |false|
without
> being public accounts. By adding a separate |needsPassword| property, we would
> be nicely prepared for this. If you agree with this idea, I can make the
> required C++-side changes to set |needsPassword| correctly.
Reverted the change here. In new PublicAccountUserPod, it overrides this
property and always return false.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.js:348: Oobe.showScreen({
Refactored the CL to expand user pod instead of using a new screen. Hope it does
not make http://crbug.com/122346 too difficult.
For public account, a PublicAccountUserPod will be created. This pod would have
a "expanded" state where a div is shown at right hand side.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_row.js:353: imageUrl:
this.imageElement.src
New patch expand user pod in-place and will no longer have this problem.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/user_pod_template.html (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_template.html:10: <img
class="managed-badge" src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY"
On 2012/11/28 15:11:38, bartfab wrote:
> The mock asks for a grayscale image. We do have an asset for that as well.
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/user_pod_template.html:10: <img
class="managed-badge" src="chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY"
On 2012/11/28 18:57:34, Nikita Kostylev wrote:
> nit: 80 chars limit
Done.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:108: } //
namespace
On 2012/11/28 15:18:33, bartfab wrote:
> Nit: We always have two spaces before |namespace|.
Most of the file are having two spaces before "//" and one space before
"namespace".
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:207: if
(enterprise_domain.empty()) {
In new patch, the domain name is sent as part of user dict for public account
user.
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:658: email =
gaia::SanitizeEmail(email);
Removed
https://codereview.chromium.org/11308081/diff/4004/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:660: //
TODO(xiyuan): Wire this with real public account signin.
On 2012/11/28 15:11:38, bartfab wrote:
> Feel free to list me as the TODO owner here. I will start work on that ASAP.
Done.
lgtm https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.css (right): https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:337: url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_GRAY') 1x, You don't have to specify all these rules 1x/2x rules manually as resources will be parsed and rules inserted. Just url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_GRAY') would work. https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:423: .pod.public-account.expanded { When public account user pod is in such expanded state will keyboard shortcut navigation between pods work (Left/Right)? Will it reset public account pod state to original one? I think it should.
https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.css (right): https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:425: width: 400px; User pod row width is 1100px wide and we allow 5 user pods at a time. Will 4 pods + public account user pod in expanded state fit in 1100px?
https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... File chrome/browser/resources/chromeos/login/user_pod_row.css (right): https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:337: url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_GRAY') 1x, On 2012/12/04 08:41:26, Nikita Kostylev wrote: > You don't have to specify all these rules 1x/2x rules manually as resources will > be parsed and rules inserted. > > Just url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_GRAY') would work. Done. Thanks for the tip. https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:423: .pod.public-account.expanded { On 2012/12/04 08:41:26, Nikita Kostylev wrote: > When public account user pod is in such expanded state will keyboard shortcut > navigation between pods work (Left/Right)? > > Will it reset public account pod state to original one? I think it should. Tab/Shift+Tab/Left/Right keys are working and navigate away from an expanded pod would restore it to original state . https://codereview.chromium.org/11308081/diff/3022/chrome/browser/resources/c... chrome/browser/resources/chromeos/login/user_pod_row.css:425: width: 400px; On 2012/12/04 09:28:14, Nikita Kostylev wrote: > User pod row width is 1100px wide and we allow 5 user pods at a time. > > Will 4 pods + public account user pod in expanded state fit in 1100px? 4 pods + expaned public pod does not fit in. However, because we are using "display:-webkit-box" and "overflow:hidden" on the pod-row, the right-most pod would be animated away. And when focus move out of the public pod to cause it to collapse, the right-most pod will be animated back in.
bartfab and ivankr, any comments?
LGTM. Thanks for making the UI animated. I just applied your patch locally - it looks great. https://chromiumcodereview.appspot.com/11308081/diff/11005/chrome/browser/res... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://chromiumcodereview.appspot.com/11308081/diff/11005/chrome/browser/res... chrome/browser/resources/chromeos/login/user_pod_row.js:173: return this.querySelector('.signed-in-indicator'); Nice drive-by clean-up in these methods. https://chromiumcodereview.appspot.com/11308081/diff/11005/chrome/browser/res... chrome/browser/resources/chromeos/login/user_pod_row.js:443: if (expanded) { As I was just told in another code review, webkit now has: classList.toggle(string, bool);
LGTM with nit https://codereview.chromium.org/11308081/diff/11005/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/11005/chrome/browser/resources/... chrome/browser/resources/chromeos/login/user_pod_row.js:451: this.removeEventListener('webkitTransitionEnd', f); I think it's not gonna work since you add f.bind(...), and remove just f. So you'll have to use some kind of var self = this instead of bind(this).
https://codereview.chromium.org/11308081/diff/11005/chrome/browser/resources/... File chrome/browser/resources/chromeos/login/user_pod_row.js (right): https://codereview.chromium.org/11308081/diff/11005/chrome/browser/resources/... chrome/browser/resources/chromeos/login/user_pod_row.js:443: if (expanded) { On 2012/12/05 18:49:51, bartfab wrote: > As I was just told in another code review, webkit now has: > > classList.toggle(string, bool); Good to know. Thanks for the tip. https://codereview.chromium.org/11308081/diff/11005/chrome/browser/resources/... chrome/browser/resources/chromeos/login/user_pod_row.js:451: this.removeEventListener('webkitTransitionEnd', f); On 2012/12/05 18:57:33, Ivan Korotkov wrote: > I think it's not gonna work since you add f.bind(...), and remove just f. So > you'll have to use some kind of var self = this instead of bind(this). You are right. I missed that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/11308081/31007
Message was sent while issue was closed.
Change committed as 171419 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
