Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
411553002:
Hide the PopupZoomer when input is received from IME (Closed)
DescriptionHide the PopupZoomer when input is received from IME
If any user tap results in showing PopupZoomer when focus is on input
field and IME is active, characters can by typed from keypad.
Now hiding the PopupZoomer when ImeEvent is triggered.
BUG=396038
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286446
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebasing the previous patch #Messages
Total messages: 22 (0 generated)
PTAL
On 2014/07/22 13:37:17, ams wrote: > PTAL @jdduke, @aurimas, Could you please have a look at the patch. PopupZoomer remains displayed even when we continue typing from keypad. Hinding is done in case of any scroll or content size change or view size change event. Also it is hidden if touch event or backkey is received. But hiding is not handled in this scenario where focus is still in the input and user can type from keypad. Hence hiding the PopupZoomer on receiving ImeEvent.
https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:551: if (mPopupZoomer.isShowing()) { I'll defer to aurimas@ here, as I don't understand the meaning of |isFinish| off the top of my head (and whether that affects where the hide call should go). https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1713: mPopupZoomer.hide(true); Do we still need this call if we always hide the popup on an IME event? Or does the back key code not propagate to/through IME?
On 2014/07/23 14:39:32, jdduke wrote: > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:551: > if (mPopupZoomer.isShowing()) { > I'll defer to aurimas@ here, as I don't understand the meaning of |isFinish| off > the top of my head (and whether that affects where the hide call should go). > > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1713: > mPopupZoomer.hide(true); > Do we still need this call if we always hide the popup on an IME event? Or does > the back key code not propagate to/through IME? @jdduke, KEYCODE_BACK can be triggered without IME being active as well, hence we need to keep the other hide call. @aurimas, PTAL
https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:551: if (mPopupZoomer.isShowing()) { On 2014/07/23 14:39:31, jdduke wrote: > I'll defer to aurimas@ here, as I don't understand the meaning of |isFinish| off > the top of my head (and whether that affects where the hide call should go). I dont think we need to check for isFinish. I think we should remove that boolean all together as it is only ever true when a keyboard tries to commit "" (empty string). It was added forever ago by olilan@ who was the ime guy before me.
On 2014/07/25 21:09:31, aurimas wrote: > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:551: > if (mPopupZoomer.isShowing()) { > On 2014/07/23 14:39:31, jdduke wrote: > > I'll defer to aurimas@ here, as I don't understand the meaning of |isFinish| > off > > the top of my head (and whether that affects where the hide call should go). > > I dont think we need to check for isFinish. I think we should remove that > boolean all together as it is only ever true when a keyboard tries to commit "" > (empty string). It was added forever ago by olilan@ who was the ime guy before > me. @aurimas, Thanks for the clarification. When I checked the history of ImeAdapter, I found that initially (r154687 onwards) on every finishComposingText(), empty string was sent to checkCompositionQueueAndCallNative() for finishing current composition which was also triggering onImeEvent(true). However later the same was removed and ImeConfirmComposition was done by calling nativeFinishComposingText(). Also when I debugged, I could not find any condition where keyboard commits "" (empty string) triggering onImeEvent() with isFinish = true. If there is no valid usecase for |isFinish|, I would like to cleanup and remove the boolean in another patch set. Please let me know your opinion. @jdduke, PTAL. Also as mentioned earlier, BACK_KEY needs to be handled separately as it gets triggered from HW back key event irrespective of IME.
On 2014/07/28 19:27:05, ams wrote: > On 2014/07/25 21:09:31, aurimas wrote: > > > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > > File > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > > (right): > > > > > https://codereview.chromium.org/411553002/diff/1/content/public/android/java/... > > > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:551: > > if (mPopupZoomer.isShowing()) { > > On 2014/07/23 14:39:31, jdduke wrote: > > > I'll defer to aurimas@ here, as I don't understand the meaning of |isFinish| > > off > > > the top of my head (and whether that affects where the hide call should go). > > > > I dont think we need to check for isFinish. I think we should remove that > > boolean all together as it is only ever true when a keyboard tries to commit > "" > > (empty string). It was added forever ago by olilan@ who was the ime guy before > > me. > > @aurimas, > Thanks for the clarification. > > When I checked the history of ImeAdapter, I found that initially (r154687 > onwards) on every finishComposingText(), empty string was sent to > checkCompositionQueueAndCallNative() for finishing current composition which was > also triggering onImeEvent(true). However later the same was removed and > ImeConfirmComposition was done by calling nativeFinishComposingText(). > > Also when I debugged, I could not find any condition where keyboard commits "" > (empty string) triggering onImeEvent() with isFinish = true. > If there is no valid usecase for |isFinish|, I would like to cleanup and remove > the boolean in another patch set. Please let me know your opinion. > > @jdduke, > > PTAL. > > Also as mentioned earlier, BACK_KEY needs to be handled separately as it gets > triggered from HW back key event irrespective of IME. I think we can remove isFinish, but please do it in a separate CL.
lgtm
The CQ bit was checked by amit.srkr@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amit.srkr@samsung.com/411553002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34451) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34465) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
The CQ bit was checked by amit.srkr@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amit.srkr@samsung.com/411553002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34511) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34515) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
On 2014/07/29 05:12:18, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/34515) > ios_dbg_simulator on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_rel_device on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) > ios_rel_device_ninja on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > mac_chromium_compile_dbg on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) @jdduke, There were multiple build bot failure. However I could not figure out the actual reason from the logs. I have just rebased the patch set again. PTAL.
The CQ bit was checked by amit.srkr@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amit.srkr@samsung.com/411553002/20001
Message was sent while issue was closed.
Change committed as 286446 |
