Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444443002:
cros: Allow pin keyboard to keep focus without popping up the pin keyboard. (Closed)
|
Created:
4 years, 2 months ago by sammiequon Modified:
4 years, 1 month ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Allow pin keyboard to keep focus without popping up the pin keyboard.
We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered.
BUG=653297, 653642, 661334
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/344b6d2380dbbf312a4725c308eb0cd4d0627177
Cr-Commit-Position: refs/heads/master@{#431769}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 16
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 6
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 12
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 8
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 6
Patch Set 7 : Fixed patch set 6 errors. #Patch Set 8 : Rebased. #
Total comments: 13
Patch Set 9 : Fixed patch set 8 errors. #
Total comments: 6
Patch Set 10 : Fixed patch set 9 errors. #
Total comments: 4
Patch Set 11 : Nits. #Patch Set 12 : Closure compiler. #
Total comments: 4
Patch Set 13 : Moved blocking variable to scoped class. #Messages
Total messages: 74 (41 generated)
Description was changed from ========== WIP: cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another hack to prevent the virtual keyboard from popping up on button clicks. BUG= TEST=none ========== to ========== WIP: cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another hack to prevent the virtual keyboard from popping up on button clicks. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP: cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another hack to prevent the virtual keyboard from popping up on button clicks. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org, rsadam@chromium.org
Patchset #1 (id:20001) has been deleted
Description was changed from ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/10/26 17:11:37, sammiequon wrote: > Description was changed from > > ========== > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > We want each button click to transfer focus back to the pin keyboard, but this > not work well with virtual keyboard. Previously kept focus on the buttons, but > added key events to each button so they users can still use the keyboard after > pressing a keyboard. This CL tries another method by preventing the virtual > keyboard from popping up on button clicks. > > BUG= > TEST=none > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > ========== > > to > > ========== > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > We want each button click to transfer focus back to the pin keyboard, but this > not work well with virtual keyboard. Previously kept focus on the buttons, but > added key events to each button so they users can still use the keyboard after > pressing a keyboard. This CL tries another method by preventing the virtual > keyboard from popping up on button clicks. Since I was working on the PIN, I > changed the input back to type text which fixes the bug with the caret jumping > around when non-numerical input is entered. > > BUG= > TEST=none > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > ========== jdufault@, rsadam@ - Please take a look. Do you predict any issues with this CL? Thanks!
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2016/10/26 17:15:02, sammiequon wrote: > On 2016/10/26 17:11:37, sammiequon wrote: > > Description was changed from > > > > ========== > > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > > > We want each button click to transfer focus back to the pin keyboard, but this > > not work well with virtual keyboard. Previously kept focus on the buttons, but > > added key events to each button so they users can still use the keyboard after > > pressing a keyboard. This CL tries another method by preventing the virtual > > keyboard from popping up on button clicks. > > > > BUG= > > TEST=none > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > > ========== > > > > to > > > > ========== > > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > > > We want each button click to transfer focus back to the pin keyboard, but this > > not work well with virtual keyboard. Previously kept focus on the buttons, but > > added key events to each button so they users can still use the keyboard after > > pressing a keyboard. This CL tries another method by preventing the virtual > > keyboard from popping up on button clicks. Since I was working on the PIN, I > > changed the input back to type text which fixes the bug with the caret jumping > > around when non-numerical input is entered. > > > > BUG= > > TEST=none > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > > ========== > > jdufault@, rsadam@ - Please take a look. Do you predict any issues with this CL? > Thanks! As discussed offline the two test cases to verify is a11y and devices with the no keyboards (they shouldn't have to have to click the keyboard). Looking at your CL it seems you target the former. I'm no longer an OWNER of ui/keyboard. Please add sadrul@ or bshe@ instead.
On 2016/10/26 17:25:41, rsadam wrote: > On 2016/10/26 17:15:02, sammiequon wrote: > > On 2016/10/26 17:11:37, sammiequon wrote: > > > Description was changed from > > > > > > ========== > > > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > > > > > We want each button click to transfer focus back to the pin keyboard, but > this > > > not work well with virtual keyboard. Previously kept focus on the buttons, > but > > > added key events to each button so they users can still use the keyboard > after > > > pressing a keyboard. This CL tries another method by preventing the virtual > > > keyboard from popping up on button clicks. > > > > > > BUG= > > > TEST=none > > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > > > ========== > > > > > > to > > > > > > ========== > > > cros: Allow pin keyboard to keep focus without popping up the pin keyboard. > > > > > > We want each button click to transfer focus back to the pin keyboard, but > this > > > not work well with virtual keyboard. Previously kept focus on the buttons, > but > > > added key events to each button so they users can still use the keyboard > after > > > pressing a keyboard. This CL tries another method by preventing the virtual > > > keyboard from popping up on button clicks. Since I was working on the PIN, I > > > changed the input back to type text which fixes the bug with the caret > jumping > > > around when non-numerical input is entered. > > > > > > BUG= > > > TEST=none > > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > > > ========== > > > > jdufault@, rsadam@ - Please take a look. Do you predict any issues with this > CL? > > Thanks! > > As discussed offline the two test cases to verify is a11y and devices with the > no keyboards (they shouldn't have to have to click the keyboard). Looking at > your CL it seems you target the former. > > I'm no longer an OWNER of ui/keyboard. Please add sadrul@ or bshe@ instead. I tested without a11y keyboard and virtual keyboard enabled (though I don't have a keyboardless device). It seems like without the PIN, you would still have to click to open the keyboard. In any case, this change should only affect when the PIN is active, so they should need the virtual keyboard unless they forgot their PIN.
https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:522: keyboard::SetWebUiOverrideEnabled(false); I would move this to ~CoreOobeHandler and ~PeopleHandler. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:55: var PIN_INPUT_ALLOWED_NON_NUMBER_KEY_CODES = [8, 9, 37, 39]; What do these key codes map to? https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:166: // area to delete the correct character(s). Can we fire a backspace key event on the input element instead? https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:240: if (code == 38 || code == 40) { If you're going to use a temporary, name it keyCode please, so that it is immediately clear what it is referring to. In this case I don't think a temporary is needed though, since you're just using it to shorten a value fetch. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:145: AddCallback("enableVirtualKeyboardOverride", The way this is named implies that calling it with true will enable the virtual keyboard. The name should be inverted. https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:127: Remove newline (follow existing style). https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetWebUiOverrideEnabled(bool enabled); It's not clear from the calling site what this does - it reads as if it is enabling the virtual keyboard. What about naming this SetForceDisableVirtualKeyboardForWebUi? It is pretty long, so feel free to come up with something shorter. https://codereview.chromium.org/2444443002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1907: selectionStart--; Like above, can we just send a backspace key event to the input element?
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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 sammiequon@chromium.org
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:522: keyboard::SetWebUiOverrideEnabled(false); On 2016/10/26 17:56:16, jdufault wrote: > I would move this to ~CoreOobeHandler and ~PeopleHandler. Moved to ~CoreOobeHandler. I think ~PeopleHandler is only called when md-settings gets closed, by then the force_disable should be turned off when the dialog with the pin keyboard is closed. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:55: var PIN_INPUT_ALLOWED_NON_NUMBER_KEY_CODES = [8, 9, 37, 39]; On 2016/10/26 17:56:16, jdufault wrote: > What do these key codes map to? Done. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:166: // area to delete the correct character(s). On 2016/10/26 17:56:16, jdufault wrote: > Can we fire a backspace key event on the input element instead? As per offline, this cannot be achieved. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:240: if (code == 38 || code == 40) { On 2016/10/26 17:56:16, jdufault wrote: > If you're going to use a temporary, name it keyCode please, so that it is > immediately clear what it is referring to. > > In this case I don't think a temporary is needed though, since you're just using > it to shorten a value fetch. Done. https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2444443002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:145: AddCallback("enableVirtualKeyboardOverride", On 2016/10/26 17:56:16, jdufault wrote: > The way this is named implies that calling it with true will enable the virtual > keyboard. The name should be inverted. Done. https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.cc:127: On 2016/10/26 17:56:16, jdufault wrote: > Remove newline (follow existing style). Done. https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/40001/ui/keyboard/keyboard_ut... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetWebUiOverrideEnabled(bool enabled); On 2016/10/26 17:56:16, jdufault wrote: > It's not clear from the calling site what this does - it reads as if it is > enabling the virtual keyboard. > > What about naming this SetForceDisableVirtualKeyboardForWebUi? It is pretty > long, so feel free to come up with something shorter. Done. https://codereview.chromium.org/2444443002/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1907: selectionStart--; On 2016/10/26 17:56:16, jdufault wrote: > Like above, can we just send a backspace key event to the input element? As per offline, this cannot be achieved.
https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.value.substring(selectionEnd, this.value.length); I believe you can change this.value.substring(selectionEnd, this.value.length) to this.value.substring(selectionEnd) https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.value.substring(selectionEnd, this.value.length); Needs indent https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:85: chrome.send('ForceDisableVirtualKeyboard', [true]); SetForceDisableVirtualKeyboard? https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:149: AddCallback("forceDisableVirtualKeyboard", setForceDisableVirtualKeyboard https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1882: if (this.passwordElement.value != e.detail.pin) Why is this check needed? Please either add a comment or remove it. https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1905: // If it is just a caret, remove the character infront of the caret. infront => in front https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1910: this.passwordElement.value.substring(selectionEnd, drop length after substring(selectionEnd) https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1914: this.passwordElement.setSelectionRange(selectionStart, selectionStart); The other function that does this doesn't have the setSelectionRange call. Does it need it?
https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.value.substring(selectionEnd, this.value.length); On 2016/10/27 19:54:13, jdufault wrote: > Needs indent Done. https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.value.substring(selectionEnd, this.value.length); On 2016/10/27 19:54:13, jdufault wrote: > I believe you can change this.value.substring(selectionEnd, this.value.length) > to this.value.substring(selectionEnd) Done. https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.js (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.js:85: chrome.send('ForceDisableVirtualKeyboard', [true]); On 2016/10/27 19:54:14, jdufault wrote: > SetForceDisableVirtualKeyboard? Done. https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2444443002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:149: AddCallback("forceDisableVirtualKeyboard", On 2016/10/27 19:54:14, jdufault wrote: > setForceDisableVirtualKeyboard Done. https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1882: if (this.passwordElement.value != e.detail.pin) On 2016/10/27 19:54:14, jdufault wrote: > Why is this check needed? Please either add a comment or remove it. Done. https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1905: // If it is just a caret, remove the character infront of the caret. On 2016/10/27 19:54:14, jdufault wrote: > infront => in front Done. https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1910: this.passwordElement.value.substring(selectionEnd, On 2016/10/27 19:54:14, jdufault wrote: > drop length after substring(selectionEnd) Done. https://codereview.chromium.org/2444443002/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:1914: this.passwordElement.setSelectionRange(selectionStart, selectionStart); On 2016/10/27 19:54:14, jdufault wrote: > The other function that does this doesn't have the setSelectionRange call. Does > it need it? Yes it does. Done.
https://codereview.chromium.org/2444443002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:167: var selectionStart = this.$$('#pin-input').selectionStart; Maybe you want to introduce a temporary for this.$$('#pin-input')? https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1898: handlePinCleared_: function(e) { Could this be handled by the pin-change event? What about handling the pin-keyboard an override input element, ie, <pin-keyboard override-input="{{passwordElement}}"> Then the pin keyboard would just use that instead of the default input element it contains. Then we could also get rid of the pin-change/etc events. https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1899: // Clear the text based on the caret location or selected region of the Add these comments in the other implementation.
sammiequon@chromium.org changed reviewers: - rsadam@chromium.org
https://codereview.chromium.org/2444443002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:167: var selectionStart = this.$$('#pin-input').selectionStart; On 2016/10/31 22:28:49, jdufault wrote: > Maybe you want to introduce a temporary for this.$$('#pin-input')? Done. https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1899: // Clear the text based on the caret location or selected region of the On 2016/10/31 22:28:49, jdufault wrote: > Add these comments in the other implementation. Done.
https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/2444443002/diff/100001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1898: handlePinCleared_: function(e) { On 2016/10/31 22:28:49, jdufault wrote: > Could this be handled by the pin-change event? > > What about handling the pin-keyboard an override input element, ie, > > <pin-keyboard override-input="{{passwordElement}}"> > > Then the pin keyboard would just use that instead of the default input element > it contains. Then we could also get rid of the pin-change/etc events. Done.
https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: placeholder="[[getInputPlaceholder_(enablePassword)]]" Did you test with settings? It seems like |value| is no longer getting updated when this input is used. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:21: * <pin-keyboard on-pin-change="onPinChange" on-submit="onPinSubmit" Update example / docs. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:81: * The password element the pin keyboard is associated with. I would extend this comment to also say that a default input element is used if none is given. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.selectionRange[0] == this.selectionRange[1] ? Can this be simplified to this.selectionRange = [this.selectionRange[0] + 1, this.selectionRange[0] + 1] Since when this.selectionRange[0] == this.selectionRange[1], the true branch can just as well be this.selectionRange[0] + 1 (since they are equal). This means that the two branches are the same so we can drop the if entirely. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:174: this.selectionRange[0] + 1]; I think having separate selectionStart/selectionEnd variables may make this easier to read. // The code in the comment above becomes this.selectionStart += 1; this.selectionEnd = this.selectionStart; https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:313: var isUsableKey = (event.keyCode >= 48 && event.keyCode <= 57) && I think this will be cleaner as a helper function. function isKeyUsable_(keyCode) { if (keyCode >= 48 && keyCode <= 57) return true; if (...) return true; return false; }
Description was changed from ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG= TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG=653297, 653642, 661334 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: placeholder="[[getInputPlaceholder_(enablePassword)]]" On 2016/11/02 19:06:45, jdufault wrote: > Did you test with settings? It seems like |value| is no longer getting updated > when this input is used. Yes, tested with settings. This value is updated at line 202: this.inputElement.value = this.value;. I did this so that the value updated is done the exact same way as if there was a external password element. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:21: * <pin-keyboard on-pin-change="onPinChange" on-submit="onPinSubmit" On 2016/11/02 19:06:45, jdufault wrote: > Update example / docs. Done. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:81: * The password element the pin keyboard is associated with. On 2016/11/02 19:06:45, jdufault wrote: > I would extend this comment to also say that a default input element is used if > none is given. Done. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:172: this.selectionRange[0] == this.selectionRange[1] ? On 2016/11/02 19:06:45, jdufault wrote: > Can this be simplified to > > this.selectionRange = [this.selectionRange[0] + 1, > this.selectionRange[0] + 1] > > Since when this.selectionRange[0] == this.selectionRange[1], the true branch can > just as well be this.selectionRange[0] + 1 (since they are equal). This means > that the two branches are the same so we can drop the if entirely. > Done. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:174: this.selectionRange[0] + 1]; On 2016/11/02 19:06:45, jdufault wrote: > I think having separate selectionStart/selectionEnd variables may make this > easier to read. > > // The code in the comment above becomes > this.selectionStart += 1; > this.selectionEnd = this.selectionStart; Done. https://codereview.chromium.org/2444443002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:313: var isUsableKey = (event.keyCode >= 48 && event.keyCode <= 57) && On 2016/11/02 19:06:45, jdufault wrote: > I think this will be cleaner as a helper function. > > function isKeyUsable_(keyCode) { > if (keyCode >= 48 && keyCode <= 57) > return true; > > if (...) > return true; > > return false; > } Done. Though I pass the event through to avoid too many parameters.
https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:132: get inputElement() { What about using passwordElement directly? In attached/ctor, if the embedder has not set passwordElement initialize it to this.$$('#pin-input'). https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:140: get selectionStart() { Should these four methods be private? https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:313: isValidKeyForInput_: function(event) { Since this is taking an event instead of a keyCode, it should be named as such. isValidEventForInput_ or something along those lines. https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:356: if (!isValidKeyForInput_(event)) { This seems like it should be throwing errors? this.isValidKeyForInput_
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:132: get inputElement() { On 2016/11/02 22:23:16, jdufault wrote: > What about using passwordElement directly? > > In attached/ctor, if the embedder has not set passwordElement initialize it to > this.$$('#pin-input'). Done. https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:140: get selectionStart() { On 2016/11/02 22:23:17, jdufault wrote: > Should these four methods be private? Done. https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:313: isValidKeyForInput_: function(event) { On 2016/11/02 22:23:16, jdufault wrote: > Since this is taking an event instead of a keyCode, it should be named as such. > > isValidEventForInput_ > > or something along those lines. Done. https://codereview.chromium.org/2444443002/diff/160001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:356: if (!isValidKeyForInput_(event)) { On 2016/11/02 22:23:16, jdufault wrote: > This seems like it should be throwing errors? > > this.isValidKeyForInput_ Thanks. It just lets everything through so I didn't notice.
https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:176: is-invisible$=[[!!passwordElement]]> Is this working as expected? There should always be a password element now. Maybe just hide the input element from inside of onPasswordElementAttached? https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:134: get selectionStart() { Please append _ to the name of these four methods. https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:181: var selectionEnd = this.selectionEnd; Are selectionStart/selectionEnd used for caching the values, or does something like the below work? If the variables are used just for caching you should explicitly call that out in a comment. this.value = this.value.substring(0, this.selectionStart) + numberValue + this.value.substring(this.selectionEnd) this.selectionStart += 1 this.selectionEnd = this.selectionStart;
Forgot to add a rebase patch, the red and green sections in pin_keyboard.html is from rebase. https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:176: is-invisible$=[[!!passwordElement]]> On 2016/11/03 17:42:41, jdufault wrote: > Is this working as expected? There should always be a password element now. > > Maybe just hide the input element from inside of onPasswordElementAttached? It was showing but right under the password element so i forgot about this. Done. https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:134: get selectionStart() { On 2016/11/03 17:42:41, jdufault wrote: > Please append _ to the name of these four methods. Done. https://codereview.chromium.org/2444443002/diff/200001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:181: var selectionEnd = this.selectionEnd; On 2016/11/03 17:42:41, jdufault wrote: > Are selectionStart/selectionEnd used for caching the values, or does something > like the below work? If the variables are used just for caching you should > explicitly call that out in a comment. > > this.value = this.value.substring(0, this.selectionStart) + numberValue + > this.value.substring(this.selectionEnd) > this.selectionStart += 1 > this.selectionEnd = this.selectionStart; Done.
lgtm
sammiequon@chromium.org changed reviewers: + bshe@chromium.org, xiyuan@chromium.org
On 2016/11/08 23:33:46, jdufault wrote: > lgtm bshe@ - Please take a look at ui/keyboard/*. Thanks! xiyuan@ - Please take a look at chrome/browser/* and ui/login/*. Thanks!
https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:212: // them after the the new value is set. Why do we need to reapply the selection range? What if the new value is totally unrelated to the old value. Wouldn't the default behavior make more sense in such case? I guess the logic is intended to go with the 'input' event, maybe move the logic to handleInputChanged_ ? https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:318: // Valid if a is pressed while the control key is pressed to allow users to nit: "a is pressed while the control key is pressed" -> "the key is CTRL+A" https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/people_handler.cc (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/people_handler.cc:528: keyboard::SetForceDisableVirtualKeyboardForWebUi(disable_keyboard); Do we need to make sure this is reverted in PeopleHandler's dtor similar to CoreOobeHandler? Doing it in js might be not be enough, e.g. user closes the webui while the pin dialog is visible. https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetForceDisableVirtualKeyboardForWebUi(bool disable); Why do we need to call out "webui" in the API? Can it be used for other purposes? Also, do we want to make a ScopedVirtualKeyboardDisabler helper class to ensure the VK is re-enabled after caller is done with it?
https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:212: // them after the the new value is set. On 2016/11/09 18:06:32, xiyuan wrote: > Why do we need to reapply the selection range? What if the new value is totally > unrelated to the old value. Wouldn't the default behavior make more sense in > such case? > > I guess the logic is intended to go with the 'input' event, maybe move the logic > to handleInputChanged_ ? The default behavior works fine if we are just working with the input element and keyboard. It is the number and clear buttons which we have to manually alter the input elements value that changes selection range to be at the end. https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:318: // Valid if a is pressed while the control key is pressed to allow users to On 2016/11/09 18:06:32, xiyuan wrote: > nit: "a is pressed while the control key is pressed" > -> "the key is CTRL+A" Done. https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/people_handler.cc (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/people_handler.cc:528: keyboard::SetForceDisableVirtualKeyboardForWebUi(disable_keyboard); On 2016/11/09 18:06:32, xiyuan wrote: > Do we need to make sure this is reverted in PeopleHandler's dtor similar to > CoreOobeHandler? Doing it in js might be not be enough, e.g. user closes the > webui while the pin dialog is visible. Decided to remove blocking keyboard in settings, since user may switch tabs with the pin dialog on and not be able to auto bring up the keyboard, and the smaller pin dialog looks ok with the keyboard and will not be used too often. https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetForceDisableVirtualKeyboardForWebUi(bool disable); On 2016/11/09 18:06:32, xiyuan wrote: > Why do we need to call out "webui" in the API? Can it be used for other > purposes? > > Also, do we want to make a ScopedVirtualKeyboardDisabler helper class to ensure > the VK is re-enabled after caller is done with it? I added the webui becuase this function is used exclusively for the lock screen webui at the moment and hopefully will be replaced by a better solution in the future. If something else uses this we can drop the webui; what do you think? Added scoped helper class.
https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:212: // them after the the new value is set. On 2016/11/10 21:06:01, sammiequon wrote: > On 2016/11/09 18:06:32, xiyuan wrote: > > Why do we need to reapply the selection range? What if the new value is > totally > > unrelated to the old value. Wouldn't the default behavior make more sense in > > such case? > > > > I guess the logic is intended to go with the 'input' event, maybe move the > logic > > to handleInputChanged_ ? > > The default behavior works fine if we are just working with the input element > and keyboard. It is the number and clear buttons which we have to manually alter > the input elements value that changes selection range to be at the end. Thanks for the clarification. I am still not convinced the selection range caching logic belongs here. Suppose the current value is "123" with caret at the end. What happens when we set the value to "abcd"? Would the logic give us "abc|d" (where "|" is the caret)? If the problem is with onNumberTap_ and onPinClear_, we should move the logic there. https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetForceDisableVirtualKeyboardForWebUi(bool disable); On 2016/11/10 21:06:01, sammiequon wrote: > On 2016/11/09 18:06:32, xiyuan wrote: > > Why do we need to call out "webui" in the API? Can it be used for other > > purposes? > > > > Also, do we want to make a ScopedVirtualKeyboardDisabler helper class to > ensure > > the VK is re-enabled after caller is done with it? > > I added the webui becuase this function is used exclusively for the lock screen > webui at the moment and hopefully will be replaced by a better solution in the > future. If something else uses this we can drop the webui; what do you think? > Added scoped helper class. As an API, it should be generic. I would say drop "webui" unless this should only be called for an webui. And I don't think we have this case here. Also, it seems that we can use keyboard::SetRequestedKeyboardState(keyboard::KEYBOARD_STATE_DISABLED) to achieve similar goal and maybe not even need a new API. What do you think? https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.cc (right): https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.cc:14: SetForceDisableVirtualKeyboardForWebUi(false); Think a scoped object would cache original value in ctor and restore it here. https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.h (right): https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:13: class KEYBOARD_EXPORT ScopedKeyboardDisabler { Document the class. https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:21: nit: nuke the empty line.
https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js (right): https://codereview.chromium.org/2444443002/diff/240001/chrome/browser/resourc... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.js:212: // them after the the new value is set. On 2016/11/10 21:48:35, xiyuan wrote: > On 2016/11/10 21:06:01, sammiequon wrote: > > On 2016/11/09 18:06:32, xiyuan wrote: > > > Why do we need to reapply the selection range? What if the new value is > > totally > > > unrelated to the old value. Wouldn't the default behavior make more sense in > > > such case? > > > > > > I guess the logic is intended to go with the 'input' event, maybe move the > > logic > > > to handleInputChanged_ ? > > > > The default behavior works fine if we are just working with the input element > > and keyboard. It is the number and clear buttons which we have to manually > alter > > the input elements value that changes selection range to be at the end. > > Thanks for the clarification. I am still not convinced the selection range > caching logic belongs here. > > Suppose the current value is "123" with caret at the end. > What happens when we set the value to "abcd"? Would the logic give us "abc|d" > (where "|" is the caret)? > > If the problem is with onNumberTap_ and onPinClear_, we should move the logic > there. I didn't test setting the value(forgot about that case) but I think it would probably be abc|d. Anyhow, I moved the caching logic into onNumberTap_ and onPinClear_. https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetForceDisableVirtualKeyboardForWebUi(bool disable); On 2016/11/10 21:48:35, xiyuan wrote: > On 2016/11/10 21:06:01, sammiequon wrote: > > On 2016/11/09 18:06:32, xiyuan wrote: > > > Why do we need to call out "webui" in the API? Can it be used for other > > > purposes? > > > > > > Also, do we want to make a ScopedVirtualKeyboardDisabler helper class to > > ensure > > > the VK is re-enabled after caller is done with it? > > > > I added the webui becuase this function is used exclusively for the lock > screen > > webui at the moment and hopefully will be replaced by a better solution in the > > future. If something else uses this we can drop the webui; what do you think? > > Added scoped helper class. > > As an API, it should be generic. I would say drop "webui" unless this should > only be called for an webui. And I don't think we have this case here. > > Also, it seems that we can use > > keyboard::SetRequestedKeyboardState(keyboard::KEYBOARD_STATE_DISABLED) > > to achieve similar goal and maybe not even need a new API. > > What do you think? Dropped. Using SetRequestedKeyboardState would only work for the normal keyboard and not the accessibility keyboard since accessibility keyboard takes charge over all other things, except this newly added one. https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.cc (right): https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.cc:14: SetForceDisableVirtualKeyboardForWebUi(false); On 2016/11/10 21:48:35, xiyuan wrote: > Think a scoped object would cache original value in ctor and restore it here. Done. https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.h (right): https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:13: class KEYBOARD_EXPORT ScopedKeyboardDisabler { On 2016/11/10 21:48:35, xiyuan wrote: > Document the class. Done. https://codereview.chromium.org/2444443002/diff/260001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:21: On 2016/11/10 21:48:35, xiyuan wrote: > nit: nuke the empty line. Done.
lgtm Thank you for bearing with me. https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.h (right): https://codereview.chromium.org/2444443002/diff/240001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.h:71: KEYBOARD_EXPORT void SetForceDisableVirtualKeyboardForWebUi(bool disable); On 2016/11/10 22:36:56, sammiequon wrote: > On 2016/11/10 21:48:35, xiyuan wrote: > > On 2016/11/10 21:06:01, sammiequon wrote: > > > On 2016/11/09 18:06:32, xiyuan wrote: > > > > Why do we need to call out "webui" in the API? Can it be used for other > > > > purposes? > > > > > > > > Also, do we want to make a ScopedVirtualKeyboardDisabler helper class to > > > ensure > > > > the VK is re-enabled after caller is done with it? > > > > > > I added the webui becuase this function is used exclusively for the lock > > screen > > > webui at the moment and hopefully will be replaced by a better solution in > the > > > future. If something else uses this we can drop the webui; what do you > think? > > > Added scoped helper class. > > > > As an API, it should be generic. I would say drop "webui" unless this should > > only be called for an webui. And I don't think we have this case here. > > > > Also, it seems that we can use > > > > keyboard::SetRequestedKeyboardState(keyboard::KEYBOARD_STATE_DISABLED) > > > > to achieve similar goal and maybe not even need a new API. > > > > What do you think? > > Dropped. Using SetRequestedKeyboardState would only work for the normal keyboard > and not the accessibility keyboard since accessibility keyboard takes charge > over all other things, except this newly added one. Acknowledged. https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.h (right): https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:20: void SetForceDisableVirtualKeyboardScoped(bool disable); nit: "Scoped" at the end feels redundant since the method is of a Scopedxxx class. https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:23: bool force_disable_keyboard_state_; nit: const bool since we don't change it.
Patchset #11 (id:300001) has been deleted
Patchset #11 (id:320001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... File ui/keyboard/scoped_keyboard_disabler.h (right): https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:20: void SetForceDisableVirtualKeyboardScoped(bool disable); On 2016/11/10 22:40:59, xiyuan wrote: > nit: "Scoped" at the end feels redundant since the method is of a Scopedxxx > class. Done. https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... ui/keyboard/scoped_keyboard_disabler.h:23: bool force_disable_keyboard_state_; On 2016/11/10 22:40:58, xiyuan wrote: > nit: const bool since we don't change it. Done.
On 2016/11/11 16:58:48, sammiequon wrote: > https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... > File ui/keyboard/scoped_keyboard_disabler.h (right): > > https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... > ui/keyboard/scoped_keyboard_disabler.h:20: void > SetForceDisableVirtualKeyboardScoped(bool disable); > On 2016/11/10 22:40:59, xiyuan wrote: > > nit: "Scoped" at the end feels redundant since the method is of a Scopedxxx > > class. > > Done. > > https://codereview.chromium.org/2444443002/diff/280001/ui/keyboard/scoped_key... > ui/keyboard/scoped_keyboard_disabler.h:23: bool force_disable_keyboard_state_; > On 2016/11/10 22:40:58, xiyuan wrote: > > nit: const bool since we don't change it. > > Done. bshe@ - Please take a look at ui/keyboard/*. Thanks!
lgtm with a nit https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.cc:47: bool g_force_disable_keyboard = false; Is this only suppose to be changed by ScopedKeyboardDisabler? Is so, it is probably safer to put it into scoped_keyboard_disabler so you don't need to make SetForceDisableVirtualKeyboard a public method.
https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.cc:47: bool g_force_disable_keyboard = false; On 2016/11/11 17:33:36, bshe wrote: > Is this only suppose to be changed by ScopedKeyboardDisabler? Is so, it is > probably safer to put it into scoped_keyboard_disabler so you don't need to make > SetForceDisableVirtualKeyboard a public method. I'm not sure how to go about implementing this. If g_force_disable_keyboard is removed, how do u get IsKeyboardEnabled() to check whether or not to force disable. Thanks for any clarification.
https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.cc:47: bool g_force_disable_keyboard = false; On 2016/11/11 21:20:58, sammiequon wrote: > On 2016/11/11 17:33:36, bshe wrote: > > Is this only suppose to be changed by ScopedKeyboardDisabler? Is so, it is > > probably safer to put it into scoped_keyboard_disabler so you don't need to > make > > SetForceDisableVirtualKeyboard a public method. > > I'm not sure how to go about implementing this. If g_force_disable_keyboard is > removed, how do u get IsKeyboardEnabled() to check whether or not to force > disable. Thanks for any clarification. I mean move g_force_disable_keyboard into scoped_keyboard_disabler.cc and create a getter in the file so IsKeyboardEnabled can access. The difference is that we don't need to have a public setter. This feels slightly safer if you don't intent to let other file set the value of g_force_disable_keyboard. It is not blocking though, so if you want to submit this CL as it is. I am fine with it.
bshe@ - Please take another look. Thanks! https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... File ui/keyboard/keyboard_util.cc (right): https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... ui/keyboard/keyboard_util.cc:47: bool g_force_disable_keyboard = false; On 2016/11/11 21:44:16, bshe wrote: > On 2016/11/11 21:20:58, sammiequon wrote: > > On 2016/11/11 17:33:36, bshe wrote: > > > Is this only suppose to be changed by ScopedKeyboardDisabler? Is so, it is > > > probably safer to put it into scoped_keyboard_disabler so you don't need to > > make > > > SetForceDisableVirtualKeyboard a public method. > > > > I'm not sure how to go about implementing this. If g_force_disable_keyboard is > > removed, how do u get IsKeyboardEnabled() to check whether or not to force > > disable. Thanks for any clarification. > > I mean move g_force_disable_keyboard into scoped_keyboard_disabler.cc and create > a getter in the file so IsKeyboardEnabled can access. The difference is that we > don't > need to have a public setter. This feels slightly safer if you don't intent to > let other file set the > value of g_force_disable_keyboard. > It is not blocking though, so if you want to submit this CL as it is. I am fine > with it. Thanks. Done.
On 2016/11/11 23:14:27, sammiequon wrote: > bshe@ - Please take another look. Thanks! > > https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... > File ui/keyboard/keyboard_util.cc (right): > > https://codereview.chromium.org/2444443002/diff/360001/ui/keyboard/keyboard_u... > ui/keyboard/keyboard_util.cc:47: bool g_force_disable_keyboard = false; > On 2016/11/11 21:44:16, bshe wrote: > > On 2016/11/11 21:20:58, sammiequon wrote: > > > On 2016/11/11 17:33:36, bshe wrote: > > > > Is this only suppose to be changed by ScopedKeyboardDisabler? Is so, it is > > > > probably safer to put it into scoped_keyboard_disabler so you don't need > to > > > make > > > > SetForceDisableVirtualKeyboard a public method. > > > > > > I'm not sure how to go about implementing this. If g_force_disable_keyboard > is > > > removed, how do u get IsKeyboardEnabled() to check whether or not to force > > > disable. Thanks for any clarification. > > > > I mean move g_force_disable_keyboard into scoped_keyboard_disabler.cc and > create > > a getter in the file so IsKeyboardEnabled can access. The difference is that > we > > don't > > need to have a public setter. This feels slightly safer if you don't intent to > > let other file set the > > value of g_force_disable_keyboard. > > It is not blocking though, so if you want to submit this CL as it is. I am > fine > > with it. > > Thanks. Done. lgtm
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
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/2444443002/#ps380001 (title: "Moved blocking variable to scoped class.")
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 ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG=653297, 653642, 661334 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG=653297, 653642, 661334 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG=653297, 653642, 661334 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Allow pin keyboard to keep focus without popping up the pin keyboard. We want each button click to transfer focus back to the pin keyboard, but this not work well with virtual keyboard. Previously kept focus on the buttons, but added key events to each button so they users can still use the keyboard after pressing a keyboard. This CL tries another method by preventing the virtual keyboard from popping up on button clicks. Since I was working on the PIN, I changed the input back to type text which fixes the bug with the caret jumping around when non-numerical input is entered. BUG=653297, 653642, 661334 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/344b6d2380dbbf312a4725c308eb0cd4d0627177 Cr-Commit-Position: refs/heads/master@{#431769} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/344b6d2380dbbf312a4725c308eb0cd4d0627177 Cr-Commit-Position: refs/heads/master@{#431769} |
