Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2027683003:
Pin keyboard moved to under the user profile. (Closed)
|
Created:
4 years, 6 months ago by sammiequon Modified:
4 years, 5 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPin keyboard moved to under the user profile.
First iteration of transition keypad on lock screen.
BUG=612221
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/d2bacfefb599942fb56c2d0d5c9ce623eeb29150
Cr-Commit-Position: refs/heads/master@{#402017}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Made transition smoother. #
Total comments: 11
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 21
Patch Set 4 : Rebase. #Patch Set 5 : Rebased. #Patch Set 6 : Fixed patch set 3 errors. #
Total comments: 26
Patch Set 7 : Rebase. #Patch Set 8 : Fixed patch set 6 errors. #
Total comments: 12
Patch Set 9 : Fixed patch set 8 errors. #
Total comments: 11
Patch Set 10 : Fixed patch set 9 errors. #Patch Set 11 : Fixed patch set 10 errors. #
Total comments: 2
Patch Set 12 : Rebased. #
Total comments: 17
Patch Set 13 : Fixed patch set 12 errors. #Patch Set 14 : Added a comment. #Messages
Total messages: 36 (11 generated)
Description was changed from ========== Pin keyboard moved to under the user profile. Fixed some javascript styling. One more javascript style change. Fixed multiple css/js styling issues. First iteration of transition keypad on lock screen. BUG=612221 ========== to ========== Pin keyboard moved to under the user profile. Fixed some javascript styling. One more javascript style change. Fixed multiple css/js styling issues. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Pin keyboard moved to under the user profile. Fixed some javascript styling. One more javascript style change. Fixed multiple css/js styling issues. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard moved to under the user profile. Fixed some javascript styling. One more javascript style change. Fixed multiple css/js styling issues. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/05/31 18:58:55, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@chromium.org
Please also tidy up the CL description. https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/lock.js:25: var element1 = $('account-picker'); Why is this in a setTimeout? onPinLoaded is getting called after a delay. What about activating a class instead that enables these values? Please use more descriptive variable names (ie, element1 => accountPicker). https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.css (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.css:7: Is there a reason to move the css into it's own file? md-settings has stylesheets inline in HTML files, login/lock has them in separate files. However, linked stylesheets (used here) are deprecated in polymer. https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:18: <div class="digit-button"> Let's pull the pin keyboard changes out into a separate CL. https://codereview.chromium.org/2027683003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:145: source->AddBoolean(kShowPinKey, true); We don't want to show the keyboard by default, so keep this as false. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/scr... File ui/login/account_picker/screen_account_picker.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/scr... ui/login/account_picker/screen_account_picker.css:8: -webkit-transition: margin 500ms ease-in-out; Can we just use transition instead of -webkit-transition? https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:68: height: 160px; All CSS properties should be in alphabetical order. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:93: height: 100%; nit: alphabetical ordering https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:125: display: inline-block; nit: alphabetical ordering https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:136: display: block; Is this change needed? https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:196: } Is this permanently hiding the password input field? The PIN keyboard will not always be active, so the user pods should look like they do now if the PIN keyboard is not enabled. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:59: var POD_ROW_PADDING = 0; Have you tested this with multiple pods? We probably want to keep the padding. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:158: Remove extraneous lines. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:777: chrome.send('authenticateUser', [this.user.username, pin]); Drop this change, it is handled in a different CL. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2435: // TODO: hardcode this.user.username Remove TODO? https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2441: pinKeyboard[0].addEventListener('submit', Can we add the event listener directly to the |userPod| instance and avoid doing a DOM query? https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.css File ui/login/screen_container.css (left): https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.c... ui/login/screen_container.css:46: position: relative; Do we need to remove this? Please verify layout works as expected w/o the PIN keyboard. https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.css File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.c... ui/login/screen_container.css:95: -webkit-transition: transform 500ms ease-in-out; Are we sure this value should be changed? Existing transitions should probably be the same.
Description was changed from ========== Pin keyboard moved to under the user profile. Fixed some javascript styling. One more javascript style change. Fixed multiple css/js styling issues. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard moved to under the user profile. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/lock.js:25: var element1 = $('account-picker'); On 2016/05/31 19:24:51, jdufault wrote: > Why is this in a setTimeout? onPinLoaded is getting called after a delay. > > What about activating a class instead that enables these values? > > Please use more descriptive variable names (ie, element1 => accountPicker). Done. https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.css (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.css:7: On 2016/05/31 19:24:51, jdufault wrote: > Is there a reason to move the css into it's own file? > > md-settings has stylesheets inline in HTML files, login/lock has them in > separate files. However, linked stylesheets (used here) are deprecated in > polymer. Done. Reverted back to inline css. https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:18: <div class="digit-button"> On 2016/05/31 19:24:51, jdufault wrote: > Let's pull the pin keyboard changes out into a separate CL. Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/scr... File ui/login/account_picker/screen_account_picker.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/scr... ui/login/account_picker/screen_account_picker.css:8: -webkit-transition: margin 500ms ease-in-out; On 2016/05/31 19:24:51, jdufault wrote: > Can we just use transition instead of -webkit-transition? Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:68: height: 160px; On 2016/05/31 19:24:51, jdufault wrote: > All CSS properties should be in alphabetical order. Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:93: height: 100%; On 2016/05/31 19:24:52, jdufault wrote: > nit: alphabetical ordering Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:125: display: inline-block; On 2016/05/31 19:24:52, jdufault wrote: > nit: alphabetical ordering Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:136: display: block; On 2016/05/31 19:24:52, jdufault wrote: > Is this change needed? Nope. Reverted. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.css:196: } On 2016/05/31 19:24:51, jdufault wrote: > Is this permanently hiding the password input field? > > The PIN keyboard will not always be active, so the user pods should look like > they do now if the PIN keyboard is not enabled. Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:158: On 2016/05/31 19:24:52, jdufault wrote: > Remove extraneous lines. Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2435: // TODO: hardcode this.user.username On 2016/05/31 19:24:52, jdufault wrote: > Remove TODO? Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:2441: pinKeyboard[0].addEventListener('submit', On 2016/05/31 19:24:52, jdufault wrote: > Can we add the event listener directly to the |userPod| instance and avoid doing > a DOM query? Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.css File ui/login/screen_container.css (left): https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.c... ui/login/screen_container.css:46: position: relative; On 2016/05/31 19:24:52, jdufault wrote: > Do we need to remove this? Please verify layout works as expected w/o the PIN > keyboard. Done. https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.css File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/1/ui/login/screen_container.c... ui/login/screen_container.css:95: -webkit-transition: transform 500ms ease-in-out; On 2016/05/31 19:24:52, jdufault wrote: > Are we sure this value should be changed? Existing transitions should probably > be the same. Done.
https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:27: var elems = [authContainer, pinContainer, signIn, pod, userImage]; Can you make showPinContainer take a UserPod instance, and then fetch all of these sub-elements from that UserPod? That way, when we add support for multiple users this code will just work. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:30: // overwritte its values. nit: overwrite https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:38: elems[idx].classList.add('pin-enabled'); Since you're typing elems[idx] 4 times, I think it's worthwhile to store it in a variable. var elem = elems[idx]; https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:43: height: 21%; For this entire CL, can we use margin/offset with pixel values instead of using % values? https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:61: width: 34%; Do the buttons change sizes? Can we define the width in terms of pixels? https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:108: <div id="root"> Revert change. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:114: <div class="row keyboard"> Revert change. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:157: </div> Revert change. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:33: top: 0; Remove this if the CSS approach doesn't work. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:43: top: -200px; Does adding !important allow you to remove the JS that sets this? https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:86: } ? Make sure you test in a RTL locale. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:748: if(this.pinKeyboard) { nit: if ( https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:750: this.handlePinSubmitted_.bind(this)); nit: 4 space indent https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:763: // // the pin keyboard is not present on the md user manager. Remove this code https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:786: chrome.send('authenticateUser', [this.user.username, pin]); Revert this change.
https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:748: if(this.pinKeyboard) { nit: if ( https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:750: this.handlePinSubmitted_.bind(this)); nit: 4 space indent https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:786: chrome.send('authenticateUser', [this.user.username, pin]); Revert this change.
https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:30: // overwritte its values. On 2016/06/14 22:50:33, jdufault wrote: > nit: overwrite Done. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:38: elems[idx].classList.add('pin-enabled'); On 2016/06/14 22:50:33, jdufault wrote: > Since you're typing elems[idx] 4 times, I think it's worthwhile to store it in a > variable. > > var elem = elems[idx]; Done. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:108: <div id="root"> On 2016/06/14 22:50:33, jdufault wrote: > Revert change. Done. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:114: <div class="row keyboard"> On 2016/06/14 22:50:33, jdufault wrote: > Revert change. Done. https://codereview.chromium.org/2027683003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:157: </div> On 2016/06/14 22:50:33, jdufault wrote: > Revert change. Done. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:86: } On 2016/06/14 22:50:33, jdufault wrote: > ? > > Make sure you test in a RTL locale. Done. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:748: if(this.pinKeyboard) { On 2016/06/14 22:50:33, jdufault wrote: > nit: if ( Done. https://codereview.chromium.org/2027683003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:750: this.handlePinSubmitted_.bind(this)); On 2016/06/14 22:50:34, jdufault wrote: > nit: 4 space indent Done.
https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:20: // TODO(sammiequon): Remove this method once showPinContainerUser works. nit: indent nit: newline before the comment nit: Please adjust the comment; ie, what is showPinContainerUser? https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:30: var currElem = elems[idx]; Typically we don't abbreviate, so either currentElement or just element. Then elems -> elements https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:36: currElem.classList.add('pin-disabled'); flip-flop the order here so pin-enabled is always the first statement in the two if bodies. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:42: var showPinContainer = function(visible, userPod) { Only one of these definitions should be committed. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:47: var userImage = userPod.imageElement; I don't think the local variables give us much in terms of readability; how about just adding the variable values directly to the array? https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:29: width: 256px; Is this needed? https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:34: width: 256px; Is this needed / can it be driven by #container-constrained-width? https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:38: height: 46px; ditto https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: .row.keyboard paper-button { Why is this change needed? https://codereview.chromium.org/2027683003/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:175: .auth-container.pin-enabled { Can the user still access this element by tabbing to it if the PIN is disabled? https://codereview.chromium.org/2027683003/diff/60001/ui/login/screen_contain... File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/60001/ui/login/screen_contain... ui/login/screen_container.css:34: opacity: 0; Is this needed? It should get this value from #pin-container, right?
https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:20: // TODO(sammiequon): Remove this method once showPinContainerUser works. On 2016/06/15 22:20:39, jdufault wrote: > nit: indent > nit: newline before the comment > nit: Please adjust the comment; ie, what is showPinContainerUser? Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:30: var currElem = elems[idx]; On 2016/06/15 22:20:38, jdufault wrote: > Typically we don't abbreviate, so either currentElement or just element. Then > elems -> elements Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:36: currElem.classList.add('pin-disabled'); On 2016/06/15 22:20:39, jdufault wrote: > flip-flop the order here so pin-enabled is always the first statement in the two > if bodies. Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:47: var userImage = userPod.imageElement; On 2016/06/15 22:20:39, jdufault wrote: > I don't think the local variables give us much in terms of readability; how > about just adding the variable values directly to the array? Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:29: width: 256px; On 2016/06/15 22:20:39, jdufault wrote: > Is this needed? Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:34: width: 256px; On 2016/06/15 22:20:39, jdufault wrote: > Is this needed / can it be driven by #container-constrained-width? Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:38: height: 46px; On 2016/06/15 22:20:39, jdufault wrote: > ditto Done. https://codereview.chromium.org/2027683003/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:56: .row.keyboard paper-button { On 2016/06/15 22:20:39, jdufault wrote: > Why is this change needed? Done. https://codereview.chromium.org/2027683003/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:175: .auth-container.pin-enabled { On 2016/06/15 22:20:39, jdufault wrote: > Can the user still access this element by tabbing to it if the PIN is disabled? Done. https://codereview.chromium.org/2027683003/diff/60001/ui/login/screen_contain... File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/60001/ui/login/screen_contain... ui/login/screen_container.css:34: opacity: 0; On 2016/06/15 22:20:39, jdufault wrote: > Is this needed? It should get this value from #pin-container, right? Done.
https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:16: function showPinKeyboardAsync(userPod, visible) { Please rename this function now that it can also hide the pin keyboard. https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:19: var setPinVisibility = function(userPod, visible) { If the |userPod| and |visible| params are removed from this function, then the |onPinLoaded| function could be removed. https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:21: userPod.signInElement, userPod, userPod.pinContainer]; Make userPod first element in the array? https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:29: else { nit: } else { https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:52: setPinVisibility(userPod, true); true should be |visible|, if you call showPinKeyboardAsync for the first time with |visible|=false then this will fade the keyboard in. You could also skip loading if |visible|=false. Up to you. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:8: login.createScreen('AccountPickerScreen', 'account-picker', function() { Restore newline https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:197: /** Restore newline https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:206: if(pod) nit: if ( https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:207: showPinKeyboardAsync(pod, true); When is there going to be a username with showPin=true but no pod? https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:221: I'd put loadPinKeyboardIfNeeded at the end of the function because the other statements are all directly manipulating DOM. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:43: .pod.pin-enabled { Did the flying-pods transition get removed? When showing the lock screen the pods themselves should still animate in. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:782: chrome.send('authenticateUserWithPin', [this.user.username, pin]); Undo this change https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:839: * @type {!HTMLDivElement} I think it makes sense to define the setPinVisible method inside of this type (then all of these new accessors can be removed and conceptually the keyboard belongs to the UserPod). We should keep the PIN resource loading logic inside of lock.js, though. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:13: <div id="pin-container"> The rest of this file uses classes for identification; maybe this should do the same?
Patchset #8 (id:160001) has been deleted
https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:16: function showPinKeyboardAsync(userPod, visible) { On 2016/06/17 18:54:22, jdufault wrote: > Please rename this function now that it can also hide the pin keyboard. Done. https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:21: userPod.signInElement, userPod, userPod.pinContainer]; On 2016/06/17 18:54:22, jdufault wrote: > Make userPod first element in the array? Done. https://codereview.chromium.org/2027683003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:29: else { On 2016/06/17 18:54:22, jdufault wrote: > nit: } else { Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:8: login.createScreen('AccountPickerScreen', 'account-picker', function() { On 2016/06/17 18:54:22, jdufault wrote: > Restore newline Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:197: /** On 2016/06/17 18:54:23, jdufault wrote: > Restore newline Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:206: if(pod) On 2016/06/17 18:54:23, jdufault wrote: > nit: if ( Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:207: showPinKeyboardAsync(pod, true); On 2016/06/17 18:54:22, jdufault wrote: > When is there going to be a username with showPin=true but no pod? Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:221: On 2016/06/17 18:54:23, jdufault wrote: > I'd put loadPinKeyboardIfNeeded at the end of the function because the other > statements are all directly manipulating DOM. Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:43: .pod.pin-enabled { On 2016/06/17 18:54:23, jdufault wrote: > Did the flying-pods transition get removed? When showing the lock screen the > pods themselves should still animate in. These should be covered by the new .pod transitions. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:782: chrome.send('authenticateUserWithPin', [this.user.username, pin]); On 2016/06/17 18:54:23, jdufault wrote: > Undo this change Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:839: * @type {!HTMLDivElement} On 2016/06/17 18:54:23, jdufault wrote: > I think it makes sense to define the setPinVisible method inside of this type > (then all of these new accessors can be removed and conceptually the keyboard > belongs to the UserPod). We should keep the PIN resource loading logic inside of > lock.js, though. Done. https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/120001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:13: <div id="pin-container"> On 2016/06/17 18:54:23, jdufault wrote: > The rest of this file uses classes for identification; maybe this should do the > same? Done.
https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:31: var podrow = $('pod-row'); nit: podRow https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:32: podrow.setFocusedPodPinVisibility(true); What's the plan for this callback? Right now it is always displaying the keyboard, which means showPinkeyboardAsync is a better name. If the plan is to invoke another callback like we discussed offline, then that seems fine to me, but add a TODO please. https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:225: $('pod-row').loadPods(users); Why was this moved? https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard class="pin-keyboard"></pin-keyboard> The pin-keyboard class only exists for the selector, right? Why not just select for a pin-keyboard element? There will only ever be one, so it should be fine. https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... File ui/login/resource_loader.js (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... ui/login/resource_loader.js:221: var element = document.getElementsByClassName(id)[0]; How about this: - Rename id to selector - If selector is a string, assume it is an id and use $(selector). - If selector is a function, call it and store the return value as element. That way the function remains generalized. To get the desired behavior, do something like waitUntilLayoutComplete( () => document.getElementsByClassName('pin-keyboard')[0], (e) => console.log('ready')) https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... ui/login/resource_loader.js:227: callback(); ?
https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:31: var podrow = $('pod-row'); On 2016/06/23 00:02:31, jdufault wrote: > nit: podRow Done. https://codereview.chromium.org/2027683003/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/lock.js:32: podrow.setFocusedPodPinVisibility(true); On 2016/06/23 00:02:31, jdufault wrote: > What's the plan for this callback? Right now it is always displaying the > keyboard, which means showPinkeyboardAsync is a better name. > > If the plan is to invoke another callback like we discussed offline, then that > seems fine to me, but add a TODO please. Done. https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:225: $('pod-row').loadPods(users); On 2016/06/23 00:02:31, jdufault wrote: > Why was this moved? Done. https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard class="pin-keyboard"></pin-keyboard> On 2016/06/23 00:02:31, jdufault wrote: > The pin-keyboard class only exists for the selector, right? Why not just select > for a pin-keyboard element? There will only ever be one, so it should be fine. Done. https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... File ui/login/resource_loader.js (right): https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... ui/login/resource_loader.js:221: var element = document.getElementsByClassName(id)[0]; On 2016/06/23 00:02:31, jdufault wrote: > How about this: > > - Rename id to selector > - If selector is a string, assume it is an id and use $(selector). > - If selector is a function, call it and store the return value as element. > > That way the function remains generalized. To get the desired behavior, do > something like > > waitUntilLayoutComplete( > () => document.getElementsByClassName('pin-keyboard')[0], > (e) => console.log('ready')) Done. https://codereview.chromium.org/2027683003/diff/180001/ui/login/resource_load... ui/login/resource_loader.js:227: callback(); On 2016/06/23 00:02:32, jdufault wrote: > ? Done.
https://codereview.chromium.org/2027683003/diff/200001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard class="pin-keyboard"></pin-keyboard> nit: looks like the selector got adjusted, so this class can be removed https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... File ui/login/resource_loader.js (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:216: * @param {string} id Identifier of the element to wait for. Update docs. https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:221: if (type != 'string' && type != 'function') Crashing is typically better than silently failing, because it makes it easier to find and fix the issue; I'd remove this check. https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:226: if (type == 'string') Alternatively, you could make selector always be a function by doing something like this before the doWait definition: if (typeof selector == 'string) { var id = selector; selector = function() { return $(id) }; } https://codereview.chromium.org/2027683003/diff/200001/ui/login/screen_contai... File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/screen_contai... ui/login/screen_container.css:26: visibility: hidden; What about having the pin-disabled class set by default in the HTML files?
https://codereview.chromium.org/2027683003/diff/200001/ui/login/account_picke... File ui/login/account_picker/user_pod_template.html (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/account_picke... ui/login/account_picker/user_pod_template.html:14: <pin-keyboard class="pin-keyboard"></pin-keyboard> On 2016/06/23 19:51:15, jdufault wrote: > nit: looks like the selector got adjusted, so this class can be removed Done. https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... File ui/login/resource_loader.js (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:216: * @param {string} id Identifier of the element to wait for. On 2016/06/23 19:51:15, jdufault wrote: > Update docs. Done. https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:221: if (type != 'string' && type != 'function') On 2016/06/23 19:51:15, jdufault wrote: > Crashing is typically better than silently failing, because it makes it easier > to find and fix the issue; I'd remove this check. Done. https://codereview.chromium.org/2027683003/diff/200001/ui/login/resource_load... ui/login/resource_loader.js:226: if (type == 'string') On 2016/06/23 19:51:15, jdufault wrote: > Alternatively, you could make selector always be a function by doing something > like this before the doWait definition: > > if (typeof selector == 'string) { > var id = selector; > selector = function() { return $(id) }; > } Done. https://codereview.chromium.org/2027683003/diff/200001/ui/login/screen_contai... File ui/login/screen_container.css (right): https://codereview.chromium.org/2027683003/diff/200001/ui/login/screen_contai... ui/login/screen_container.css:26: visibility: hidden; On 2016/06/23 19:51:15, jdufault wrote: > What about having the pin-disabled class set by default in the HTML files? Done. https://codereview.chromium.org/2027683003/diff/200001/ui/login/screen_contai... ui/login/screen_container.css:26: visibility: hidden; On 2016/06/23 19:51:15, jdufault wrote: > What about having the pin-disabled class set by default in the HTML files? Done.
lgtm https://codereview.chromium.org/2027683003/diff/240001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/240001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:220: nit: remove newline
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@ - Please take a look. https://codereview.chromium.org/2027683003/diff/240001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2027683003/diff/240001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:220: On 2016/06/24 18:11:42, jdufault wrote: > nit: remove newline Done.
https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:42: width 500ms ease-in-out; Why 500ms? Feel kind of slow. Do we have UX stamped on this? https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:61: #account-picker { move this into screen_account_picker.css ? https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; Why we need to use visibility? Do we need to reserve the space? https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1137: else { nit: move it to line 1136 https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1139: currentElement.classList.add('pin-disabled'); nit: we replace the if with: currentElement.classList.toggle('pin-enabled', visible); currentElement.classList.toggle('pin-disabled', !visible);
https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:42: width 500ms ease-in-out; On 2016/06/24 21:19:09, xiyuan wrote: > Why 500ms? Feel kind of slow. Do we have UX stamped on this? we don't have UX stamp. 500ms was a guess. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:42: width 500ms ease-in-out; On 2016/06/24 21:19:09, xiyuan wrote: > Why 500ms? Feel kind of slow. Do we have UX stamped on this? Done. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:61: #account-picker { On 2016/06/24 21:19:09, xiyuan wrote: > move this into screen_account_picker.css ? removed. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:61: #account-picker { On 2016/06/24 21:19:09, xiyuan wrote: > move this into screen_account_picker.css ? Done. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; On 2016/06/24 21:19:09, xiyuan wrote: > Why we need to use visibility? Do we need to reserve the space? i use visibility to prevent people from tabbing to the 0 opacity elements. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1137: else { On 2016/06/24 21:19:09, xiyuan wrote: > nit: move it to line 1136 Done. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1139: currentElement.classList.add('pin-disabled'); On 2016/06/24 21:19:10, xiyuan wrote: > nit: we replace the if with: > > currentElement.classList.toggle('pin-enabled', visible); > currentElement.classList.toggle('pin-disabled', !visible); Done.
https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:42: width 500ms ease-in-out; On 2016/06/24 22:06:39, sammiequon wrote: > On 2016/06/24 21:19:09, xiyuan wrote: > > Why 500ms? Feel kind of slow. Do we have UX stamped on this? > > Done. 180ms is better. But check with UX about all animation times before launch. https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; On 2016/06/24 22:06:39, sammiequon wrote: > On 2016/06/24 21:19:09, xiyuan wrote: > > Why we need to use visibility? Do we need to reserve the space? > > i use visibility to prevent people from tabbing to the 0 opacity elements. Do you need to keep the element's space while it is invisible? If not, how about "display: none"?
https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; On 2016/06/24 22:13:17, xiyuan wrote: > On 2016/06/24 22:06:39, sammiequon wrote: > > On 2016/06/24 21:19:09, xiyuan wrote: > > > Why we need to use visibility? Do we need to reserve the space? > > > > i use visibility to prevent people from tabbing to the 0 opacity elements. > > Do you need to keep the element's space while it is invisible? If not, how about > "display: none"? i think i do because the pin-keyboard is considered done loading when its offsetHeight is not 0 anymore.
lgtm https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; On 2016/06/24 22:39:01, sammiequon wrote: > On 2016/06/24 22:13:17, xiyuan wrote: > > On 2016/06/24 22:06:39, sammiequon wrote: > > > On 2016/06/24 21:19:09, xiyuan wrote: > > > > Why we need to use visibility? Do we need to reserve the space? > > > > > > i use visibility to prevent people from tabbing to the 0 opacity elements. > > > > Do you need to keep the element's space while it is invisible? If not, how > about > > "display: none"? > > i think i do because the pin-keyboard is considered done loading when its > offsetHeight is not 0 anymore. Okay then.
https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2027683003/diff/260001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:190: visibility: hidden; On 2016/06/24 22:39:01, sammiequon wrote: > On 2016/06/24 22:13:17, xiyuan wrote: > > On 2016/06/24 22:06:39, sammiequon wrote: > > > On 2016/06/24 21:19:09, xiyuan wrote: > > > > Why we need to use visibility? Do we need to reserve the space? > > > > > > i use visibility to prevent people from tabbing to the 0 opacity elements. > > > > Do you need to keep the element's space while it is invisible? If not, how > about > > "display: none"? > > i think i do because the pin-keyboard is considered done loading when its > offsetHeight is not 0 anymore. Add a comment noting that?
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2027683003/#ps300001 (title: "Added a comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pin keyboard moved to under the user profile. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard moved to under the user profile. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Pin keyboard moved to under the user profile. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard moved to under the user profile. First iteration of transition keypad on lock screen. BUG=612221 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/d2bacfefb599942fb56c2d0d5c9ce623eeb29150 Cr-Commit-Position: refs/heads/master@{#402017} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/d2bacfefb599942fb56c2d0d5c9ce623eeb29150 Cr-Commit-Position: refs/heads/master@{#402017} |
