Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2302483003:
Pin keyboard improvements. (Closed)
|
Created:
4 years, 3 months ago by sammiequon Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, alemate+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, achuith+watch_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPin keyboard improvements.
- Pin keyboard backspace can be held down to keep deleting.
- Pin keyboard key events used to doubled on settings/options.
- Pin keyboard placeholder displays correct message.
- Removed "clear" string, since the backspace is icon only now.
- Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered.
BUG=642587
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/85b9a7721fd3bee402c31e0efbf465b5e8c97fb8
Cr-Commit-Position: refs/heads/master@{#417514}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebased. #Patch Set 3 : Fixed patch set 1 errors. #
Total comments: 6
Patch Set 4 : Backspace works with touch. #Patch Set 5 : Bubble disappears when user presses a pin button. #
Total comments: 10
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 4
Patch Set 7 : Hold backspace down resembles android pin keyboard now. #
Total comments: 4
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 4
Patch Set 9 : Renamed some variables. #
Total comments: 2
Patch Set 10 : Remove async. #
Messages
Total messages: 37 (12 generated)
Description was changed from ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. BUG=642587 ========== to ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look. Thanks!
https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: intervalId: { Can we use this.async? https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: intervalId = setInterval(function() { this.intervalId_ https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:141: onBackspaceMouseUpLeave_: function() { Does this work for tapping? https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:142: clearInterval(intervalId); this.intervalId_
On 2016/08/31 19:28:56, jdufault wrote: > https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): > > https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: intervalId: { > Can we use this.async? > > https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: intervalId = > setInterval(function() { > this.intervalId_ > > https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:141: > onBackspaceMouseUpLeave_: function() { > Does this work for tapping? > > https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:142: > clearInterval(intervalId); > this.intervalId_ Something else - I think the behavior we want is if you hold down the button, it clears the entire PIN value.
https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: intervalId: { On 2016/08/31 19:28:55, jdufault wrote: > Can we use this.async? Done. https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:131: intervalId = setInterval(function() { On 2016/08/31 19:28:55, jdufault wrote: > this.intervalId_ Done. https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:141: onBackspaceMouseUpLeave_: function() { On 2016/08/31 19:28:55, jdufault wrote: > Does this work for tapping? Yes. https://codereview.chromium.org/2302483003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:142: clearInterval(intervalId); On 2016/08/31 19:28:55, jdufault wrote: > this.intervalId_ Done.
https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: intervalId_: { repeatClearIntervalId_? https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:70: intervalMs_: { Make this a constant value defined at the top of the file. https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:132: this.async(function() { this.onPinClear_(); }, 1); Remove the this.async call if you're still using setInterval. Also, what about removing the helper function? this.intervalId_ = setInterval(this.onPinClear_.bind(this), this.intervalMs_)
Description was changed from ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. - Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:61: intervalId_: { On 2016/09/01 16:40:11, jdufault wrote: > repeatClearIntervalId_? Done. https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:70: intervalMs_: { On 2016/09/01 16:40:11, jdufault wrote: > Make this a constant value defined at the top of the file. Done. https://codereview.chromium.org/2302483003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:132: this.async(function() { this.onPinClear_(); }, 1); On 2016/09/01 16:40:11, jdufault wrote: > Remove the this.async call if you're still using setInterval. > > Also, what about removing the helper function? > > this.intervalId_ = setInterval(this.onPinClear_.bind(this), this.intervalMs_) Done.
https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:27: * Time in milliseconds for which the backspace button clears a character when What about: How long the backspace button must be held down before clearing characters. I don't think you need to say milliseconds in the comment since it is in the variable name. https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:33: Make sure to wrap this all in a function to avoid introducing a global. (function() { // ... })(); https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:68: * The intervalID used for the backspace button set/clear interval. Please add @private https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: onBackspaceGetFocus_: function(event) { Is there an actual "on-focus" event this could listen for? https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:144: clearInterval(this.repeatClearIntervalId_); reset repeatClearIntervalId_ value to 0?
https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:27: * Time in milliseconds for which the backspace button clears a character when On 2016/09/02 20:03:43, jdufault wrote: > What about: > > How long the backspace button must be held down before clearing characters. > > I don't think you need to say milliseconds in the comment since it is in the > variable name. Done. https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:33: On 2016/09/02 20:03:44, jdufault wrote: > Make sure to wrap this all in a function to avoid introducing a global. > > (function() { > > // ... > > })(); Done. https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:68: * The intervalID used for the backspace button set/clear interval. On 2016/09/02 20:03:43, jdufault wrote: > Please add @private Done. https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:130: onBackspaceGetFocus_: function(event) { On 2016/09/02 20:03:43, jdufault wrote: > Is there an actual "on-focus" event this could listen for? on-blur and on-foucsout don't seem to trigger on-mouseup. https://codereview.chromium.org/2302483003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:144: clearInterval(this.repeatClearIntervalId_); On 2016/09/02 20:03:43, jdufault wrote: > reset repeatClearIntervalId_ value to 0? Done.
https://codereview.chromium.org/2302483003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:133: this.onPinClear_(); Buttons normally activate on up events, right? That way you can click the button but cancel the click by moving the mouse away. I think the same thing should work here too. https://codereview.chromium.org/2302483003/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2302483003/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1855: updateSubmitButton_: function() { What about naming this updateInput_ or something like that? updateSubmitButton_ seems a little disingenuous because it does more than that (hides the error, etc).
https://codereview.chromium.org/2302483003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:133: this.onPinClear_(); On 2016/09/02 22:58:53, jdufault wrote: > Buttons normally activate on up events, right? That way you can click the button > but cancel the click by moving the mouse away. I think the same thing should > work here too. Done. https://codereview.chromium.org/2302483003/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2302483003/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1855: updateSubmitButton_: function() { On 2016/09/02 22:58:53, jdufault wrote: > What about naming this updateInput_ or something like that? > > updateSubmitButton_ seems a little disingenuous because it does more than that > (hides the error, etc). Done.
https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:151: this.startAutoBackspaceId_ = setTimeout(function() { setTimeout and this.async do the same/similar things, right? Can we only use one of them? ie, we don't need to nest this.async inside of setTimeout. https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:164: clearInterval(this.repeatBackspaceIntervalId_); Do we need separate interval id variables? Can we reuse it? What do you think of backspaceTimeoutId_ as a name?
https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:151: this.startAutoBackspaceId_ = setTimeout(function() { On 2016/09/06 14:46:37, jdufault wrote: > setTimeout and this.async do the same/similar things, right? Can we only use one > of them? > > ie, we don't need to nest this.async inside of setTimeout. Done. https://codereview.chromium.org/2302483003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:164: clearInterval(this.repeatBackspaceIntervalId_); On 2016/09/06 14:46:37, jdufault wrote: > Do we need separate interval id variables? Can we reuse it? What do you think of > backspaceTimeoutId_ as a name? The reason I use two variables is because I changed this hold backspace to work more like smartphone backspaces (specifically Android M) in that you hold it for X interval and then it starts to delete at Y intervals. If the person releases before X then it counts as a click; to check this i check if the timeOutId != 0. If we share Id then it will always be != 0 for the hold duration. Is there a better approach?
https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:33: var repeatBackspaceIntervalMs = 150; These should probably be named in all caps. What do you think about REPEAT_BACKSPACE_DELAY_MS for a name? https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:41: var startAutoBackspaceMs = 500; INITIAL_BACKSPACE_DELAY_MS?
lgtm after rename
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@ - Please take a look. Thanks! https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:33: var repeatBackspaceIntervalMs = 150; On 2016/09/08 19:30:54, jdufault wrote: > These should probably be named in all caps. > > What do you think about REPEAT_BACKSPACE_DELAY_MS for a name? Done. https://codereview.chromium.org/2302483003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:41: var startAutoBackspaceMs = 500; On 2016/09/08 19:30:54, jdufault wrote: > INITIAL_BACKSPACE_DELAY_MS? Done.
https://codereview.chromium.org/2302483003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:152: this.async(function() { merge error? Please remove the async call.
lgtm % jdufault's comment
https://codereview.chromium.org/2302483003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2302483003/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:152: this.async(function() { On 2016/09/08 22:31:09, jdufault wrote: > merge error? Please remove the async call. Done. Thanks, not sure when that came back.
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/2302483003/#ps180001 (title: "Remove async.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sammiequon@chromium.org changed reviewers: + dbeam@chromium.org
On 2016/09/09 02:55:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) dbeam@ - Please take a look, chrome/browser/ui/webui/options/browser_options_handler.cc. Thanks!
lgtm
The CQ bit was checked by sammiequon@chromium.org
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 improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. - Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. - Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. - Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Pin keyboard improvements. - Pin keyboard backspace can be held down to keep deleting. - Pin keyboard key events used to doubled on settings/options. - Pin keyboard placeholder displays correct message. - Removed "clear" string, since the backspace is icon only now. - Bubble now disappears when user presses a pin keyboard button after a wrong password has been entered. BUG=642587 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/85b9a7721fd3bee402c31e0efbf465b5e8c97fb8 Cr-Commit-Position: refs/heads/master@{#417514} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/85b9a7721fd3bee402c31e0efbf465b5e8c97fb8 Cr-Commit-Position: refs/heads/master@{#417514} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
