Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2603663002:
Make IME extensions decide when to show the keyboard. (Closed)
|
Created:
3 years, 12 months ago by Azure Wei Modified:
3 years, 11 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, kalyank, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake IME extensions decide when to show the keyboard.
When opening emoji, handwriting or voice input from opt-in IME menu,
InputMethodManager overrides the keyboard url to tell the VK to switch
keyboard and ImeMenuTray call KeyboardController to show the VK.
Since the VK is always back to main keyset when hidden, it will be on
main keyset when shown, and get onhashchanged event to switch to e/h/v
keyset. Thus, has a flash.
To avoid the flash of main keyset, we need to make IME extension decide
when to bring up the VK. So if the KeyboardController is initialized,
and keyboard has been loaded once, it means IME extension could bring
up the VK. Don't show keyboard from ImeMenuTray.
BUG=667609
TEST=Verfied on local build.
Review-Url: https://codereview.chromium.org/2603663002
Cr-Commit-Position: refs/heads/master@{#442161}
Committed: https://chromium.googlesource.com/chromium/src/+/886f64844d90cffa12dbf183eba086becaf819a0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename as IsKeyboardWindowCreated(). #Patch Set 3 #
Total comments: 4
Patch Set 4 : Addressed comments. #
Total comments: 8
Patch Set 5 : Addressed comments. #
Messages
Total messages: 31 (15 generated)
Description was changed from ========== Make IME extensions decide when to show the keyboard. BUG=667609 TEST=Verfied on local build. ========== to ========== Make IME extensions decide when to show the keyboard. When opening emoji, handwriting or voice input from opt-in IME menu, InputMethodManager overrides the keyboard url to tell the VK to switch keyboard and ImeMenuTray call KeyboardController to show the VK. Since the VK is always back to main keyset when hidden, it will be on main keyset when shown, and get onhashchanged event to switch to e/h/v keyset. Thus, has a flash. To avoid the flash of main keyset, we need to make IME extension decide when to bring up the VK. So if the KeyboardController is initialized, and keyboard has been loaded once, it means IME extension could bring up the VK. Don't show keyboard from ImeMenuTray. BUG=667609 TEST=Verfied on local build. ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org
Hi Shu, please review this CL. Thanks!
The CQ bit was checked by azurewei@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.
On 2016/12/25 06:08:48, Azure Wei wrote: > Hi Shu, please review this CL. Thanks!
https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:437: // show keyboard, OS need to trigger it. Otherwise, make IME extensions // If the keyboard window hasn't been created yet, it means the extension cannot receive anything to show the keyboard. Therefore, instead of relying the extension to show the keyboard, forcibly show the keyboard window here (which will cause the keyboard window created). https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:439: if (!keyboard_controller->KeyboardHasBeenLoaded()) { s/KeyboardHasBeenLoaded/IsKeyboardWindowCreated/g
https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:437: // show keyboard, OS need to trigger it. Otherwise, make IME extensions On 2016/12/26 02:16:39, Shu Chen wrote: > // If the keyboard window hasn't been created yet, it means the extension cannot > receive anything to show the keyboard. Therefore, instead of relying the > extension to show the keyboard, forcibly show the keyboard window here (which > will cause the keyboard window created). Done. https://codereview.chromium.org/2603663002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:439: if (!keyboard_controller->KeyboardHasBeenLoaded()) { On 2016/12/26 02:16:39, Shu Chen wrote: > s/KeyboardHasBeenLoaded/IsKeyboardWindowCreated/g Done.
lgtm
azurewei@chromium.org changed reviewers: + bshe@chromium.org - mcx.dinhof@gmail.com
+bshe for ui/keyboard
ui/keyboard lgtm https://codereview.chromium.org/2603663002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:444: // window here (which will cause the keyboard window created). The comment here is not very clear. It looks like you prevent showing keyboard on native side if keyboard window is already created. I am assuming this is because you want extension to call private api to show keyboard so you skip it here. Perhaps reword your comment here. https://codereview.chromium.org/2603663002/diff/40001/ui/keyboard/keyboard_co... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/2603663002/diff/40001/ui/keyboard/keyboard_co... ui/keyboard/keyboard_controller.cc:310: return container_ != nullptr && !container_->children().empty(); you can probably use if you just want to test if keyboard window is created: keyboard_container_initialized() && ui_->HasKeyboardWindow()
https://codereview.chromium.org/2603663002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:444: // window here (which will cause the keyboard window created). On 2016/12/29 19:22:15, bshe wrote: > The comment here is not very clear. It looks like you prevent showing keyboard > on native side if keyboard window is already created. I am assuming this is > because you want extension to call private api to show keyboard so you skip it > here. Perhaps reword your comment here. Updated the comment here. https://codereview.chromium.org/2603663002/diff/40001/ui/keyboard/keyboard_co... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/2603663002/diff/40001/ui/keyboard/keyboard_co... ui/keyboard/keyboard_controller.cc:310: return container_ != nullptr && !container_->children().empty(); On 2016/12/29 19:22:15, bshe wrote: > you can probably use if you just want to test if keyboard window is created: > keyboard_container_initialized() && ui_->HasKeyboardWindow() Done. Thanks.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2603663002/#ps60001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
azurewei@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb for ash/common/system/chromeos/.
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...)
ping~
owner lgtm w/ nits https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:370: } nit: no {} https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:458: // window here (which will cause the keyboard window created). window to be created https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:460: // native side could just skip to show keyboard. skip showing the keyboard https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:463: } nit: no {}
Thanks! https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:370: } On 2017/01/06 16:59:39, stevenjb wrote: > nit: no {} Done. https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:458: // window here (which will cause the keyboard window created). On 2017/01/06 16:59:39, stevenjb wrote: > window to be created Done. https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:460: // native side could just skip to show keyboard. On 2017/01/06 16:59:39, stevenjb wrote: > skip showing the keyboard Done. https://codereview.chromium.org/2603663002/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:463: } On 2017/01/06 16:59:39, stevenjb wrote: > nit: no {} Done.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org, bshe@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2603663002/#ps80001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483772617033730,
"parent_rev": "f11926072706e80dd0c4762c36ea4cede399de56", "commit_rev":
"886f64844d90cffa12dbf183eba086becaf819a0"}
Message was sent while issue was closed.
Description was changed from ========== Make IME extensions decide when to show the keyboard. When opening emoji, handwriting or voice input from opt-in IME menu, InputMethodManager overrides the keyboard url to tell the VK to switch keyboard and ImeMenuTray call KeyboardController to show the VK. Since the VK is always back to main keyset when hidden, it will be on main keyset when shown, and get onhashchanged event to switch to e/h/v keyset. Thus, has a flash. To avoid the flash of main keyset, we need to make IME extension decide when to bring up the VK. So if the KeyboardController is initialized, and keyboard has been loaded once, it means IME extension could bring up the VK. Don't show keyboard from ImeMenuTray. BUG=667609 TEST=Verfied on local build. ========== to ========== Make IME extensions decide when to show the keyboard. When opening emoji, handwriting or voice input from opt-in IME menu, InputMethodManager overrides the keyboard url to tell the VK to switch keyboard and ImeMenuTray call KeyboardController to show the VK. Since the VK is always back to main keyset when hidden, it will be on main keyset when shown, and get onhashchanged event to switch to e/h/v keyset. Thus, has a flash. To avoid the flash of main keyset, we need to make IME extension decide when to bring up the VK. So if the KeyboardController is initialized, and keyboard has been loaded once, it means IME extension could bring up the VK. Don't show keyboard from ImeMenuTray. BUG=667609 TEST=Verfied on local build. Review-Url: https://codereview.chromium.org/2603663002 Cr-Commit-Position: refs/heads/master@{#442161} Committed: https://chromium.googlesource.com/chromium/src/+/886f64844d90cffa12dbf183eba0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/886f64844d90cffa12dbf183eba0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
