Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2131823004:
Dialog message saying PIN is incorrect positioned correctly. (Closed)
|
Created:
4 years, 5 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. |
DescriptionDialog message saying PIN is incorrect positioned correctly.
BUG=624517
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7dd2784b02179f0a801b5d3d8f504233cb38449e
Cr-Commit-Position: refs/heads/master@{#404846}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 3 : Rebased. #Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 14
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 6
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 2
Patch Set 7 : Addressed comment. #
Messages
Total messages: 24 (9 generated)
Description was changed from ========== Password entered wrong display works with PIN. BUG=624517 ========== to ========== Password entered wrong display works with PIN. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Password entered wrong display works with PIN. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Password entered wrong display works with PIN. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look.
Did you test this when the PIN keyboard times out from say, too many attempts? https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:705: pinKeyboardShown_: false, Add docs. https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1141: this.pinKeyboardShown_ = visible; Can this just be a DOM query so we have less cached state? That way, it won't be possible for the value to go out of sync. The cached variable also needs to be updated when the PIN keyboard is hidden from ie, a timeout or too many attempts. https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1811: this.parentNode.setActivatedPod(this); What's this for?
Description was changed from ========== Password entered wrong display works with PIN. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Dialog message saying PIN is incorrect positioned correctly. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:705: pinKeyboardShown_: false, On 2016/07/08 21:04:09, jdufault wrote: > Add docs. Done. https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1141: this.pinKeyboardShown_ = visible; On 2016/07/08 21:04:09, jdufault wrote: > Can this just be a DOM query so we have less cached state? That way, it won't be > possible for the value to go out of sync. > > The cached variable also needs to be updated when the PIN keyboard is hidden > from ie, a timeout or too many attempts. Done. https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1811: this.parentNode.setActivatedPod(this); On 2016/07/08 21:04:09, jdufault wrote: > What's this for? The error bubble looks for the current activated pod, if it can't find it it puts the message on the edge of the pod-row. So this line simulates when the enter button does for the password input, and sets the pod as activated.
https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/1/ui/login/account_picker/use... ui/login/account_picker/user_pod_row.js:1811: this.parentNode.setActivatedPod(this); On 2016/07/11 17:35:35, sammiequon wrote: > On 2016/07/08 21:04:09, jdufault wrote: > > What's this for? > > The error bubble looks for the current activated pod, if it can't find it it > puts the message on the edge of the pod-row. So this line simulates when the > enter button does for the password input, and sets the pod as activated. Please add a comment describing this. https://codereview.chromium.org/2131823004/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1352: if (this.passwordElement.value) Add {} Can the PIN submit handler be removed so there's only one authenticateUser call?
https://codereview.chromium.org/2131823004/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1352: if (this.passwordElement.value) On 2016/07/11 18:19:54, jdufault wrote: > Add {} > > Can the PIN submit handler be removed so there's only one authenticateUser call? Done.
https://codereview.chromium.org/2131823004/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2131823004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:84: * Fires an update event with the current PIN value. The event will only be The settings UI integration still listens for the submit event; please revert these deletions. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1133: // Set the focus to the input element after showing/hiding pin keyboard. Revert change. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1350: [this.user.username, this.passwordElement.value]); Does this work? var password = this.passwordElement.value || this.pinKeyboard.value chrome.send('authenticateUser', [this.user.username, password]) https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1419: this.pinKeyboard.clear(); this.pinKeyboard.value = '' should work. Remove clear() method. Can this be if (this.pinKeyboard) https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1817: } else if (this.pinKeyboard.submitButton && Can the if be if (this.pinKeyboard && e.target == this.pinKeyboard.submitButton) Can we just check the target against the entire pin keyboard? That way, we don't need to expose the submitButton element. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3418: e.target == this.focusedPod_.pinKeyboard.inputElement || Is there a way to avoid exposing the inputElement property of the pin keyboard?
https://codereview.chromium.org/2131823004/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2131823004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:84: * Fires an update event with the current PIN value. The event will only be On 2016/07/11 22:35:08, jdufault wrote: > The settings UI integration still listens for the submit event; please revert > these deletions. Done. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1133: // Set the focus to the input element after showing/hiding pin keyboard. On 2016/07/11 22:35:08, jdufault wrote: > Revert change. Done. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1350: [this.user.username, this.passwordElement.value]); On 2016/07/11 22:35:08, jdufault wrote: > Does this work? > > var password = this.passwordElement.value || this.pinKeyboard.value > chrome.send('authenticateUser', [this.user.username, password]) Done. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1419: this.pinKeyboard.clear(); On 2016/07/11 22:35:08, jdufault wrote: > this.pinKeyboard.value = '' should work. Remove clear() method. > > Can this be > > if (this.pinKeyboard) Done. PinKeyboard still exists if the user is entering a normal password, it just has nothing in it. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1817: } else if (this.pinKeyboard.submitButton && On 2016/07/11 22:35:08, jdufault wrote: > Can the if be > > if (this.pinKeyboard && e.target == this.pinKeyboard.submitButton) > > Can we just check the target against the entire pin keyboard? That way, we don't > need to expose the submitButton element. For this we need to check when the click is coming from the submit button. I guess we can to some e.target.parentNode.parentNode or we can hook up the pin-submit event. https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:3418: e.target == this.focusedPod_.pinKeyboard.inputElement || On 2016/07/11 22:35:08, jdufault wrote: > Is there a way to avoid exposing the inputElement property of the pin keyboard? We need the input element to be passed to the show error message so it can point at the correct spot, unless we plan to point the at the whole pin.
https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1419: this.pinKeyboard.clear(); On 2016/07/11 23:13:24, sammiequon wrote: > On 2016/07/11 22:35:08, jdufault wrote: > > this.pinKeyboard.value = '' should work. Remove clear() method. > > > > Can this be > > > > if (this.pinKeyboard) > > Done. > > PinKeyboard still exists if the user is entering a normal password, it just has > nothing in it. Remove the if statement then? https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1177: return this.pinKeyboard.inputElement; nit: remove {} https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1346: !this.pinKeyboard.inputElement.value) use this.pinKeyboard.value instead of inputElement.value I think the this.pinKeyboard.inputElement check can be removed, since this.pinKeyboard.value will always be empty/undefined if this.pinKeyboard.inputElement is null. https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1350: this.pinKeyboard.inputElement.value; this.pinKeyboard.value instead of pinKeyboard.inputElement.value
https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/60001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1419: this.pinKeyboard.clear(); On 2016/07/11 23:31:40, jdufault wrote: > On 2016/07/11 23:13:24, sammiequon wrote: > > On 2016/07/11 22:35:08, jdufault wrote: > > > this.pinKeyboard.value = '' should work. Remove clear() method. > > > > > > Can this be > > > > > > if (this.pinKeyboard) > > > > Done. > > > > PinKeyboard still exists if the user is entering a normal password, it just > has > > nothing in it. > > Remove the if statement then? Done. https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1177: return this.pinKeyboard.inputElement; On 2016/07/11 23:31:41, jdufault wrote: > nit: remove {} Done. https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1346: !this.pinKeyboard.inputElement.value) On 2016/07/11 23:31:41, jdufault wrote: > use this.pinKeyboard.value instead of inputElement.value > > I think the this.pinKeyboard.inputElement check can be removed, since > this.pinKeyboard.value will always be empty/undefined if > this.pinKeyboard.inputElement is null. Done. https://codereview.chromium.org/2131823004/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1350: this.pinKeyboard.inputElement.value; On 2016/07/11 23:31:40, jdufault wrote: > this.pinKeyboard.value instead of pinKeyboard.inputElement.value Done.
lgtm after comment https://codereview.chromium.org/2131823004/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1344: if (!this.passwordElement.value && this.pinKeyboard.inputElement && Move the var password declaration above this, then just do if (!password) return false;
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@ - Please take a look. https://codereview.chromium.org/2131823004/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2131823004/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1344: if (!this.passwordElement.value && this.pinKeyboard.inputElement && On 2016/07/12 00:16:25, jdufault wrote: > Move the var password declaration above this, then just do > > if (!password) > return false; Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2131823004/#ps120001 (title: "Addressed 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 ========== Dialog message saying PIN is incorrect positioned correctly. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Dialog message saying PIN is incorrect positioned correctly. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Dialog message saying PIN is incorrect positioned correctly. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Dialog message saying PIN is incorrect positioned correctly. BUG=624517 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7dd2784b02179f0a801b5d3d8f504233cb38449e Cr-Commit-Position: refs/heads/master@{#404846} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7dd2784b02179f0a801b5d3d8f504233cb38449e Cr-Commit-Position: refs/heads/master@{#404846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
