New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 639169 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

WebView: Selection handles should hide under the keyboard

Project Member Reported by aelias@chromium.org, Aug 19 2016

Issue description

In WebView, selection handle popups currently show under the keyboard.  It should be possible to detect we're under the keyboard and hide the popup.  Or we can consider moving the handle into a Surface instead to get rid of this whole class of problem.

See http://b/29402575 and http://b/29884190 (the latter bug has a lot of advice on how to address this).
 

Comment 1 by aelias@chromium.org, Aug 19 2016

Cc: ymalik@chromium.org

Comment 2 by aelias@chromium.org, Sep 21 2016

Labels: -M-54 M-55

Comment 3 by samsmlee@google.com, Sep 21 2016

It should also hide under the action bar/toolbar. In AppCompatActivity, it's a ViewGroup:
http://cs/aosp-master/frameworks/support/v7/appcompat/src/android/support/v7/widget/Toolbar.java
Cc: amaralp@chromium.org
Owner: hush@chromium.org

Comment 5 by hush@chromium.org, Oct 11 2016

In fact, I think WebView is already doing what EditText is doing.
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java?rcl=0&l=491

We set the PopupTouchHandleDrawables to be invisible when they are clipped.
You can see this by going to www.textarea.org inside WebView browser test shell apk. I have recorded a video.

The reason why it does not work on Inbox is because the height of WebView does not change when the user bring up the IME window. That is, IME window floats on top of the WebView.
demo.mp4
4.8 MB View Download

Comment 6 by hush@chromium.org, Oct 11 2016

Cc: yukawa@chromium.org

Comment 7 by hush@chromium.org, Oct 12 2016

CL uploaded here. https://codereview.chromium.org/2410223003/
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a64446000115cca6deba65288de86741a40062e

commit 4a64446000115cca6deba65288de86741a40062e
Author: hush <hush@chromium.org>
Date: Wed Oct 12 22:58:46 2016

Hide the PopupTouchHandlerDrawable if it is occluded.

This CL follows Android Editor's logic to determine if the touch handler is
visible by going up the view hierarchy. If the touch handler is fully occluded
by any of its parent views, then it is considered not visible.

In this way, the touch handlers won't appear overlapped with the IME window, if
the app adjusts the window size based on IME window.

BUG= 639169 

Review-Url: https://codereview.chromium.org/2410223003
Cr-Commit-Position: refs/heads/master@{#424896}

[modify] https://crrev.com/4a64446000115cca6deba65288de86741a40062e/android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java

Comment 9 by aelias@chromium.org, Oct 12 2016

Labels: Merge-Request-55

Comment 10 by tkent@chromium.org, Oct 12 2016

Components: -Blink>TextSelection Blink>Editing>Selection

Comment 11 by dimu@chromium.org, Oct 12 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 13 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afe45453fc54d87bbec43ce683358d43fbb44ef0

commit afe45453fc54d87bbec43ce683358d43fbb44ef0
Author: Hui Shu <hush@google.com>
Date: Wed Oct 12 23:59:25 2016

Hide the PopupTouchHandlerDrawable if it is occluded.

This CL follows Android Editor's logic to determine if the touch handler is
visible by going up the view hierarchy. If the touch handler is fully occluded
by any of its parent views, then it is considered not visible.

In this way, the touch handlers won't appear overlapped with the IME window, if
the app adjusts the window size based on IME window.

BUG= 639169 

Review-Url: https://codereview.chromium.org/2410223003
Cr-Commit-Position: refs/heads/master@{#424896}
(cherry picked from commit 4a64446000115cca6deba65288de86741a40062e)

Review URL: https://codereview.chromium.org/2413113002 .

Cr-Commit-Position: refs/branch-heads/2883@{#78}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/afe45453fc54d87bbec43ce683358d43fbb44ef0/android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java

Comment 13 by hush@chromium.org, Oct 13 2016

Status: Fixed (was: Assigned)

Comment 14 by hush@chromium.org, Oct 13 2016

I forgot to add that my comment #5 was wrong. Inbox does change the height of its window when IME window shows up. If anyone is reading this, please don't be confused

Comment 15 by aluo@chromium.org, Oct 18 2016

Status: Assigned (was: Fixed)
Checked on Nexus 6 running NRD91N monochrome 55.0.2883.18.  The fix has some issues, which I've uploaded video to the folder: go/chrome-androidlogs1/6/639169

1) text handle is not hidden when it first gets behind the top bar where the attach and send functions are located

2) text handle does not appear right away if you scroll the handles into the ime area, then long press some other text in the view, it will appear once the page is scrolled.  This second behavior is new.

Comment 16 by hush@chromium.org, Oct 18 2016

Looking..

Comment 17 by hush@chromium.org, Oct 18 2016

1) The text handle will only be hidden if it is *fully* occluded by something else. This happens at around 50s in your video. So this one is working as I intended it. 
Why did I do that? Because sometimes, the left selection handler partially sticks out of WebView, when you select a word from the beginning. Not showing the left selection handler is very weird. So I made the decision to only hide a selection handler if it is fully occluded.

2) I can see sometimes the selection handlers do not show up, even without my fix. So I didn't give it too much thought.

Are you sure you can't reproduce the same problem without my fix?

Comment 18 by hush@chromium.org, Oct 21 2016

Cc: -paulmiller@chromium.org -hush@chromium.org
Owner: paulmiller@chromium.org
Paul,
Do you want to take a look?
I don't think the problem in #15 is caused by this patch though

Comment 19 by aluo@chromium.org, Oct 24 2016

I can confirm that behavior 2 is new.  Before the text handles wouldn't hide under keyboard, but first long-clicked text above keyboard, the text handles would display.  Now the text handles are hidden under keyboard, and first long-clicked text above keyboard, the text handles would not display.
Project Member

Comment 20 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afe45453fc54d87bbec43ce683358d43fbb44ef0

commit afe45453fc54d87bbec43ce683358d43fbb44ef0
Author: Hui Shu <hush@google.com>
Date: Wed Oct 12 23:59:25 2016

Hide the PopupTouchHandlerDrawable if it is occluded.

This CL follows Android Editor's logic to determine if the touch handler is
visible by going up the view hierarchy. If the touch handler is fully occluded
by any of its parent views, then it is considered not visible.

In this way, the touch handlers won't appear overlapped with the IME window, if
the app adjusts the window size based on IME window.

BUG= 639169 

Review-Url: https://codereview.chromium.org/2410223003
Cr-Commit-Position: refs/heads/master@{#424896}
(cherry picked from commit 4a64446000115cca6deba65288de86741a40062e)

Review URL: https://codereview.chromium.org/2413113002 .

Cr-Commit-Position: refs/branch-heads/2883@{#78}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/afe45453fc54d87bbec43ce683358d43fbb44ef0/android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java

Comment 21 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
removing release block on this one. Prio 2 should not be release-bloc.
Labels: -ReleaseBlock-Stable
Status: Fixed (was: Assigned)

Sign in to add a comment