Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(545)

Issue 330623002: Copying text closes the keyboard and the text input gets unfocused, forcing virtual keyboard is get… (Closed)

Created:
6 years, 6 months ago by AKVT
Modified:
6 years, 6 months ago
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, bulach, bcwhite
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Copying text closes the keyboard and the text input gets unfocused, forcing virtual keyboard is getting closed. During copy operation, we are clearing the selection, which is clearing the cursor as well. Also if the rootEditable element is NULL, we are not setting the textInputType for ImeAdapter, which force ImeAdapter to dismiss the keyboard. Now converted the Range selection to caret selection to keep cursor and caret visible, so that user can continue editing. BUG=225090 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278954

Patch Set 1 #

Patch Set 2 : Setting editable selection to end from ContentViewCore inputConnection. #

Patch Set 3 : Optimized the changes. #

Total comments: 2

Patch Set 4 : Added test case curresponding this change. #

Patch Set 5 : Refined the test content. #

Patch Set 6 : Test function modified. #

Total comments: 5

Patch Set 7 : Fixed Indentations #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -1 line) Patch
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 1 chunk +8 lines, -1 line 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java View 1 2 3 4 5 6 2 chunks +17 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
AKVT
PTAL. Dependent Blink change is at https://codereview.chromium.org/330613002/
6 years, 6 months ago (2014-06-12 10:58:46 UTC) #1
bulach
thanks! +tedchoc is a better reviewer and listed in the original bug
6 years, 6 months ago (2014-06-12 12:41:42 UTC) #2
Ted C
Overall this seems reasonable to me. Adding aurimas as he's dealt the most with the ...
6 years, 6 months ago (2014-06-12 18:32:50 UTC) #3
AKVT
PTAL this change.
6 years, 6 months ago (2014-06-19 09:22:44 UTC) #4
AKVT
On 2014/06/12 18:32:50, Ted C wrote: > Overall this seems reasonable to me. Adding aurimas ...
6 years, 6 months ago (2014-06-19 09:24:43 UTC) #5
aurimas (slooooooooow)
Can we add a test for this? https://codereview.chromium.org/330623002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/330623002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2093 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2093: mInputConnection.setSelection(selectionEnd, selectionEnd); ...
6 years, 6 months ago (2014-06-19 17:12:06 UTC) #6
AKVT
@aurimas - Can I add the test inside ContentViewCoreInputConnectionTest class or I need to add ...
6 years, 6 months ago (2014-06-20 10:10:48 UTC) #7
AKVT
PTAL. @aurimas - Since this is a change which is getting triggered from SelectActionModeCallback (manual ...
6 years, 6 months ago (2014-06-20 11:34:56 UTC) #8
AKVT
PTAL
6 years, 6 months ago (2014-06-20 11:53:33 UTC) #9
aurimas (slooooooooow)
LGTM given the test passes
6 years, 6 months ago (2014-06-20 16:23:30 UTC) #10
Ted C
lgtm w/ a couple nits https://codereview.chromium.org/330623002/diff/100001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/330623002/diff/100001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2095 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2095: else else should go ...
6 years, 6 months ago (2014-06-20 16:33:57 UTC) #11
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 6 months ago (2014-06-20 17:37:27 UTC) #12
AKVT
The CQ bit was unchecked by ajith.v@samsung.com
6 years, 6 months ago (2014-06-20 17:37:27 UTC) #13
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 6 months ago (2014-06-20 18:19:48 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/330623002/100001
6 years, 6 months ago (2014-06-20 18:23:43 UTC) #15
Ted C
The CQ bit was unchecked by tedchoc@chromium.org
6 years, 6 months ago (2014-06-20 19:03:39 UTC) #16
Ted C
Please fix the comments before checking the commit queue checkbox https://codereview.chromium.org/330623002/diff/100001/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/330623002/diff/100001/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java#newcode151 ...
6 years, 6 months ago (2014-06-20 19:04:33 UTC) #17
AKVT
PTAL https://codereview.chromium.org/330623002/diff/100001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/330623002/diff/100001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2095 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2095: else On 2014/06/20 16:33:57, Ted C wrote: > ...
6 years, 6 months ago (2014-06-21 07:20:13 UTC) #18
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 6 months ago (2014-06-21 07:20:46 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/330623002/120001
6 years, 6 months ago (2014-06-21 07:21:38 UTC) #20
commit-bot: I haz the power
6 years, 6 months ago (2014-06-21 13:14:46 UTC) #21
Message was sent while issue was closed.
Change committed as 278954

Powered by Google App Engine
This is Rietveld 408576698