Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394953005:
Caret is showing after do a copy operation from editable field. (Closed)
|
Created:
6 years, 5 months ago by AKVT Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, AviD, Cyan, ankit, cjhopman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHide the insertion handle when performing a copy
Make behavior uniform with other apps on Android, where after a copy from
an editable field, the insertion handle is hidden and only the cursor
remains visible.
BUG=394773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283818
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed review comments. #Messages
Total messages: 10 (0 generated)
PTAL
https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2195: hideHandles(); Is there any downside to moving the |hideHandles()| call up so that it's called for either case, i.e., if (mUnselectAllOnActionModeDismiss) { hideHandles(); ... The handles should naturally become hidden from |mImeAdapter.unselect()|, but an explicit |hideHandles()| call would prevent the handles from automatically appearing again if selection is activated outside a user gesture (could be wrong).
PTAL https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2195: hideHandles(); On 2014/07/16 15:08:59, jdduke wrote: > Is there any downside to moving the |hideHandles()| call up so that it's called > for either case, i.e., > > if (mUnselectAllOnActionModeDismiss) { > hideHandles(); > ... > > The handles should naturally become hidden from |mImeAdapter.unselect()|, but an > explicit |hideHandles()| call would prevent the handles from automatically > appearing again if selection is activated outside a user gesture (could be > wrong). Thanks for reviewing. mImeAdapter.unselect() takes care of hiding the handles by itself. For editable case we are setting the selection to end, but not clearing(unselecting) the selection. So caret stays. So hideHandles() is needed only for editable case only. Also if user touches on the editable again later, handles started appearing. Also if I trigger a selection after a copy using JS, that time also selection handle appears.
https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2195: hideHandles(); On 2014/07/16 16:39:09, AKV wrote: > On 2014/07/16 15:08:59, jdduke wrote: > > Is there any downside to moving the |hideHandles()| call up so that it's > called > > for either case, i.e., > > > > if (mUnselectAllOnActionModeDismiss) { > > hideHandles(); > > ... > > > > The handles should naturally become hidden from |mImeAdapter.unselect()|, but > an > > explicit |hideHandles()| call would prevent the handles from automatically > > appearing again if selection is activated outside a user gesture (could be > > wrong). > > Thanks for reviewing. > mImeAdapter.unselect() takes care of hiding the handles by itself. For editable > case we are setting the selection to end, but not clearing(unselecting) the > selection. So caret stays. So hideHandles() is needed only for editable case > only. Also if user touches on the editable again later, handles started > appearing. Also if I trigger a selection after a copy using JS, that time also > selection handle appears. I realize handles in a non-editable region are hidden via |mImeAdapter.unselect()|, but I think we not only want to hide them but also prevent them from automatically showing again, which is only accomplished via |hideHandles()|. I think we can accomplish both by moving the |hideHandles()| call above the |isSelectionEditable()| if branch.
PTAL https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/394953005/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2195: hideHandles(); On 2014/07/16 18:14:23, jdduke wrote: > On 2014/07/16 16:39:09, AKV wrote: > > On 2014/07/16 15:08:59, jdduke wrote: > > > Is there any downside to moving the |hideHandles()| call up so that it's > > called > > > for either case, i.e., > > > > > > if (mUnselectAllOnActionModeDismiss) { > > > hideHandles(); > > > ... > > > > > > The handles should naturally become hidden from |mImeAdapter.unselect()|, > but > > an > > > explicit |hideHandles()| call would prevent the handles from automatically > > > appearing again if selection is activated outside a user gesture (could be > > > wrong). > > > > Thanks for reviewing. > > mImeAdapter.unselect() takes care of hiding the handles by itself. For > editable > > case we are setting the selection to end, but not clearing(unselecting) the > > selection. So caret stays. So hideHandles() is needed only for editable case > > only. Also if user touches on the editable again later, handles started > > appearing. Also if I trigger a selection after a copy using JS, that time also > > selection handle appears. > > I realize handles in a non-editable region are hidden via > |mImeAdapter.unselect()|, but I think we not only want to hide them but also > prevent them from automatically showing again, which is only accomplished via > |hideHandles()|. I think we can accomplish both by moving the |hideHandles()| > call above the |isSelectionEditable()| if branch. Done.
lgtm (If there's an associated bug please add BUG= accordingly), thanks. I went ahead and updated the title/description. In the future, please make the title explain briefly *what* the patch does, using the description to explain the problem and the reason for the change.
On 2014/07/17 14:48:10, jdduke wrote: > lgtm (If there's an associated bug please add BUG= accordingly), thanks. > > I went ahead and updated the title/description. In the future, please make the > title explain briefly *what* the patch does, using the description to explain > the problem and the reason for the change. Thanks for the comments. I updated the BUG id in description.
The CQ bit was checked by ajith.v@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/394953005/20001
Message was sent while issue was closed.
Change committed as 283818 |
