Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1913673002:
Close IME on tapping on Blimp web content pane (Closed)
|
Created:
4 years, 8 months ago by shaktisahu Modified:
4 years, 8 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClose IME on tapping on Blimp web content pane
Currently if IME is open for typing URL and user taps anywhere outside
the URL bar, the IME doesn't close. This fix is to close the IME first
when the user taps on BlimpView and then process the touch event.
BUG=603705
Committed: https://crrev.com/11b00bc93e47ec05a0b4f4bf6583a165a6d755ba
Cr-Commit-Position: refs/heads/master@{#389937}
Patch Set 1 #Patch Set 2 : Cleanup unwanted log #
Total comments: 8
Patch Set 3 : merge origin/master #Patch Set 4 : Addressed comments #
Total comments: 1
Patch Set 5 : addressed nits #Messages
Total messages: 16 (6 generated)
Description was changed from ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 ========== to ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 ==========
shaktisahu@chromium.org changed reviewers: + khushalsagar@chromium.org, nyquist@chromium.org
https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:27: private static final String TAG = "BlimpView"; nit: Unused. https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:102: // TODO(shaktisahu): Detect if the IME was open and return immediately. Could we use UiUtils.isKeyboardShowing for this instead? https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:103: if (eventAction == MotionEvent.ACTION_UP) { Why do this only for ACTION_UP? If the user tries to scroll, we should hide the IME in that case as well.
https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:27: private static final String TAG = "BlimpView"; On 2016/04/23 01:29:21, Khushal wrote: > nit: Unused. Just added to help in debugging and logging. I think most of the Android classes use this convention. https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:102: // TODO(shaktisahu): Detect if the IME was open and return immediately. On 2016/04/23 01:29:21, Khushal wrote: > Could we use UiUtils.isKeyboardShowing for this instead? I tried using that. Seems like this method isn't able to detect if the keyboard is open because of some soft input adjustment configuration we are currently using. https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:103: if (eventAction == MotionEvent.ACTION_UP) { On 2016/04/23 01:29:21, Khushal wrote: > Why do this only for ACTION_UP? If the user tries to scroll, we should hide the > IME in that case as well. Good catch. Initially, I didn't want to call this twice for every tap. Now I think it's better to remove the condition.
https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:27: private static final String TAG = "BlimpView"; On 2016/04/23 02:08:05, shaktisahu wrote: > On 2016/04/23 01:29:21, Khushal wrote: > > nit: Unused. > > Just added to help in debugging and logging. I think most of the Android classes > use this convention. Yeah, but we usually remove it if it ends up not being used in production code.
https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1913673002/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:27: private static final String TAG = "BlimpView"; On 2016/04/26 18:28:58, nyquist wrote: > On 2016/04/23 02:08:05, shaktisahu wrote: > > On 2016/04/23 01:29:21, Khushal wrote: > > > nit: Unused. > > > > Just added to help in debugging and logging. I think most of the Android > classes > > use this convention. > > Yeah, but we usually remove it if it ends up not being used in production code. Done.
lgtm. Thanks!
lgtm https://codereview.chromium.org/1913673002/diff/60001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1913673002/diff/60001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpView.java:101: // TODO(shaktisahu): Detect if the IME was open and return immediately. Nit: Maybe file a bug for this, so we don't forget? Bonus points for linking to the bug here.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/1913673002/#ps80001 (title: "addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913673002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913673002/80001
Message was sent while issue was closed.
Description was changed from ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 ========== to ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 ========== to ========== Close IME on tapping on Blimp web content pane Currently if IME is open for typing URL and user taps anywhere outside the URL bar, the IME doesn't close. This fix is to close the IME first when the user taps on BlimpView and then process the touch event. BUG=603705 Committed: https://crrev.com/11b00bc93e47ec05a0b4f4bf6583a165a6d755ba Cr-Commit-Position: refs/heads/master@{#389937} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/11b00bc93e47ec05a0b4f4bf6583a165a6d755ba Cr-Commit-Position: refs/heads/master@{#389937} |
