Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7553016:
Use text input type to control visibility of virtual keyboard (Closed)
|
Created:
9 years, 4 months ago by Peng Modified:
9 years, 4 months ago Reviewers:
mazda, sadrul, USE_CHROMIUM_ACCT_INSTEAD, commit-bot: I haz the power, bryeung, asargent_no_longer_on_chrome CC:
chromium-reviews, dhollowa Visibility:
Public. |
DescriptionUse text input type to control visibility of virtual keyboard
BUG=None
TEST=On linux desktop
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97998
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix reiview issues #
Total comments: 12
Patch Set 3 : Fix review issues #Patch Set 4 : Rebase on HEAD #Patch Set 5 : Fix a style issue #Patch Set 6 : Rebase on HEAD #
Messages
Total messages: 18 (0 generated)
This CL depends on http://codereview.chromium.org/7302015/ .
Nice! LGTM http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:161: ShowKeyboardForWidget(widget); Please add a TODO for using 'type' http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... File chrome/browser/ui/touch/keyboard/keyboard_manager.h (right): http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... chrome/browser/ui/touch/keyboard/keyboard_manager.h:17: #include "views/ime/text_input_type_tracker.h" sort
http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:161: ShowKeyboardForWidget(widget); On 2011/08/02 16:15:14, sadrul wrote: > Please add a TODO for using 'type' Done. http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... File chrome/browser/ui/touch/keyboard/keyboard_manager.h (right): http://codereview.chromium.org/7553016/diff/1/chrome/browser/ui/touch/keyboar... chrome/browser/ui/touch/keyboard/keyboard_manager.h:17: #include "views/ime/text_input_type_tracker.h" On 2011/08/02 16:15:14, sadrul wrote: > sort Done.
This looks awesome Peng! Just a few comments... http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:82: chrome::NOTIFICATION_EDITABLE_ELEMENT_TOUCHED, Do we still need this? Won't all editable elements generate TextInputType changes? http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:164: ListValue args; Why a ListValue instead of a StringValue? Looks like you'll only ever send one. http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:166: case ui::TEXT_INPUT_TYPE_NONE: Is this the right indentation? I was expecting everything inside the switch to be 2 spaces more indented. http://codereview.chromium.org/7553016/diff/6002/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7553016/diff/6002/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:3271: "description": "This event is sent to virtual keyboard when the text input type is changed.", "to the virtual keyboard"
http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:201: kOnTextInputTypeChanged, json_args, NULL, GURL()); Does this send the event to all extension renderers, instead of just the keyboard extension? Can that be a security/privacy issue?
http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:82: chrome::NOTIFICATION_EDITABLE_ELEMENT_TOUCHED, On 2011/08/03 22:22:04, bryeung wrote: > Do we still need this? Won't all editable elements generate TextInputType > changes? I am not sure if it can be removed safely. Add mazda in this CL, I think he can give me advice. http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:164: ListValue args; On 2011/08/03 22:22:04, bryeung wrote: > Why a ListValue instead of a StringValue? Looks like you'll only ever send one. ListValue is for creating arguments for onTextInputTypeChanged(type). I don't understand how to use StringValue here. Maybe we could just assign json_args to "[\"type\"]" here? http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:166: case ui::TEXT_INPUT_TYPE_NONE: On 2011/08/03 22:22:04, bryeung wrote: > Is this the right indentation? I was expecting everything inside the switch to > be 2 spaces more indented. Done. http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:201: kOnTextInputTypeChanged, json_args, NULL, GURL()); On 2011/08/03 23:01:14, sadrul wrote: > Does this send the event to all extension renderers, instead of just the > keyboard extension? Can that be a security/privacy issue? It will send the event to all extensions which listen on the event by calling chrome.experimental.input.onTextInputTypeChanged.addListener(). And the chrome.experimental.input is only visible to extension declares right permissions in manifest file. So probably it will not be a privacy issue. http://codereview.chromium.org/7553016/diff/6002/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7553016/diff/6002/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:3271: "description": "This event is sent to virtual keyboard when the text input type is changed.", On 2011/08/03 22:22:04, bryeung wrote: > "to the virtual keyboard" Done.
http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:82: chrome::NOTIFICATION_EDITABLE_ELEMENT_TOUCHED, On 2011/08/04 19:47:53, Peng wrote: > On 2011/08/03 22:22:04, bryeung wrote: > > Do we still need this? Won't all editable elements generate TextInputType > > changes? > > I am not sure if it can be removed safely. Add mazda in this CL, I think he can > give me advice. > I believe this is still necessary for this case: (1) user selects a textfield; the keyboard pops up (2) user hides the keyboard using the 'hide' key on the keyboard; the textfield still has the focus (3) user touches the textfield again.
http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... File chrome/browser/ui/touch/keyboard/keyboard_manager.cc (right): http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... chrome/browser/ui/touch/keyboard/keyboard_manager.cc:82: chrome::NOTIFICATION_EDITABLE_ELEMENT_TOUCHED, On 2011/08/04 20:07:56, sadrul wrote: > On 2011/08/04 19:47:53, Peng wrote: > > On 2011/08/03 22:22:04, bryeung wrote: > > > Do we still need this? Won't all editable elements generate TextInputType > > > changes? > > > > I am not sure if it can be removed safely. Add mazda in this CL, I think he > can > > give me advice. > > > > I believe this is still necessary for this case: (1) user selects a textfield; > the keyboard pops up (2) user hides the keyboard using the 'hide' key on the > keyboard; the textfield still has the focus (3) user touches the textfield > again. Yes, it is necessary. TextInputType change notification is not sent without the focus change. The hide key does not cause the focus change by design. So in a case like sadrul mentioned, the keyboard does not show up again unless NOTIFICATION_EDITABLE_ELEMENT_TOUCHED is handled. NOTIFICATION_EDITABLE_ELEMENT_TOUCHED is always sent when a view that has a text input type other than TEXT_INPUT_TYPE_NONE is touched.
On Thu, Aug 4, 2011 at 7:47 PM, <penghuang@chromium.org> wrote: > http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... > chrome/browser/ui/touch/keyboard/keyboard_manager.cc:164: ListValue > args; > On 2011/08/03 22:22:04, bryeung wrote: >> >> Why a ListValue instead of a StringValue? Looks like you'll only ever > > send one. > > ListValue is for creating arguments for onTextInputTypeChanged(type). I > don't understand how to use StringValue here. > > Maybe we could just assign json_args to "[\"type\"]" here? Why does json_args need to be a JS array? Why can't we send just a plain string to the extension? > http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... > chrome/browser/ui/touch/keyboard/keyboard_manager.cc:201: > kOnTextInputTypeChanged, json_args, NULL, GURL()); > On 2011/08/03 23:01:14, sadrul wrote: >> >> Does this send the event to all extension renderers, instead of just > > the >> >> keyboard extension? Can that be a security/privacy issue? > > It will send the event to all extensions which listen on the event by > calling chrome.experimental.input.onTextInputTypeChanged.addListener(). > And the chrome.experimental.input is only visible to extension declares > right permissions in manifest file. So probably it will not be a privacy > issue. Well, perhaps to be slightly more correct, it is no more of a privacy/security issue than we already have by allowing third-party keyboards at all. Any extension that makes use of an api within chrome.input (or whatever the final, stable name becomes) will have to have some additional user messaging to warn them that they need to trust the extension before using it. This is similar to what Android has for third-party keyboards (though I hope we can do better than what Android has). Bryan
On 2011/08/05 17:11:18, bryeung wrote: > On Thu, Aug 4, 2011 at 7:47 PM, <mailto:penghuang@chromium.org> wrote: > > > http://codereview.chromium.org/7553016/diff/6002/chrome/browser/ui/touch/keyb... > > chrome/browser/ui/touch/keyboard/keyboard_manager.cc:164: ListValue > > args; > > On 2011/08/03 22:22:04, bryeung wrote: > >> > >> Why a ListValue instead of a StringValue? Looks like you'll only ever > > > > send one. > > > > ListValue is for creating arguments for onTextInputTypeChanged(type). I > > don't understand how to use StringValue here. > > > > Maybe we could just assign json_args to "[\"type\"]" here? > > Why does json_args need to be a JS array? Why can't we send just a > plain string to the extension? I think it is a rule for all events sent to extensions. For some causes onSomeChanged(arg1, arg2,...) may have two or more arguments, all arguments should be packaged in an array. For this case, onTextInputTypeChanged(type) only has one argument, it still need follow the rule to package the one argument in an array. I tried send the type as plain text, and then I got an error like (the data type is error, expect an array...).
> > chrome/browser/ui/touch/keyboard/keyboard_manager.cc:201: > > kOnTextInputTypeChanged, json_args, NULL, GURL()); > > On 2011/08/03 23:01:14, sadrul wrote: > >> > >> Does this send the event to all extension renderers, instead of just > > > > the > >> > >> keyboard extension? Can that be a security/privacy issue? > > > > It will send the event to all extensions which listen on the event by > > calling chrome.experimental.input.onTextInputTypeChanged.addListener(). > > And the chrome.experimental.input is only visible to extension declares > > right permissions in manifest file. So probably it will not be a privacy > > issue. > > Well, perhaps to be slightly more correct, it is no more of a > privacy/security issue than we already have by allowing third-party > keyboards at all. > > Any extension that makes use of an api within chrome.input (or > whatever the final, stable name becomes) will have to have some > additional user messaging to warn them that they need to trust the > extension before using it. This is similar to what Android has for > third-party keyboards (though I hope we can do better than what > Android has). Ah, that makes sense; I didn't know/realize that extensions wanting to use chrome.input will have to be permitted by the user. Thanks for the detailed explanation.
LGTM
LGTM
Can't apply patch for file chrome/browser/ui/touch/keyboard/keyboard_manager.cc. While running patch -p1 --forward --force; patching file chrome/browser/ui/touch/keyboard/keyboard_manager.cc Hunk #4 FAILED at 93. Hunk #5 succeeded at 154 (offset 13 lines). Hunk #6 succeeded at 169 (offset 13 lines). Hunk #7 succeeded at 257 (offset 13 lines). 1 out of 7 hunks FAILED -- saving rejects to file chrome/browser/ui/touch/keyboard/keyboard_manager.cc.rej
Presubmit check for 7553016-25003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json Presubmit checks took 4.6s to calculate.
Hi Antony, Could you please review the change in extension_api.json? Thanks. On 2011/08/23 22:44:44, I haz the power (commit-bot) wrote: > Presubmit check for 7553016-25003 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json > > Presubmit checks took 4.6s to calculate.
extension_api.json change LGTM
Change committed as 97998 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
