Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410223003:
Hide the PopupTouchHandlerDrawable if it is occluded. (Closed)
|
Created:
4 years, 2 months ago by hush (inactive) Modified:
4 years, 2 months ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org, agrieve+watch_chromium.org, yukawa, aelias_OOO_until_Jul13 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide 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
Committed: https://crrev.com/4a64446000115cca6deba65288de86741a40062e
Cr-Commit-Position: refs/heads/master@{#424896}
Patch Set 1 : Hide the PopupTouchHandlerDrawable if it is occluded. #
Total comments: 9
Patch Set 2 : address comments #Messages
Total messages: 15 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hush@chromium.org changed reviewers: + boliu@chromium.org
PTAL
Description was changed from ========== 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 view, 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 ========== to ========== 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 ==========
https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:276: private boolean isShowingAllowed() { we only check this on invalidate and when visibility changes, is that enough? does Editor check this more frequently? https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:280: return isPositionVisible(mOriginXDip * deviceScale, mOriginYDip * deviceScale) nit, could move this to be the last thing, in case any of the other terms are false, and so this expensive check can be skipped https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:284: private boolean isPositionVisible(final float positionX, final float positionY) { is this copied verbatim? if so, can you document the source? https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:300: if (position[0] + drawableWidth < 0 || position[1] + drawableHeight < 0 I don't think this is always correct.. this assumes the "point" of the handle is at the top left, but the left selection handle, the "point" is at the top right of the drawable, presumably, but I guess there are no soft keyboard coming from the side, so not a big deal.. maybe just ignore the width here..?
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:276: private boolean isShowingAllowed() { On 2016/10/12 04:20:50, boliu wrote: > we only check this on invalidate and when visibility changes, is that enough? > does Editor check this more frequently? Editor in Android does not check more frequently. A rough call path is like below: onPreDraw --> TextViewPositionListener#updatePosition -->Editor$HandleView#isVisible -->Editor#isPositionVisible. That is to say, Editor checks on every invalidation. https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:280: return isPositionVisible(mOriginXDip * deviceScale, mOriginYDip * deviceScale) On 2016/10/12 04:20:50, boliu wrote: > nit, could move this to be the last thing, in case any of the other terms are > false, and so this expensive check can be skipped Done. https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:284: private boolean isPositionVisible(final float positionX, final float positionY) { On 2016/10/12 04:20:50, boliu wrote: > is this copied verbatim? if so, can you document the source? it is not copied verbatim. The change is made to line 288, 291, 297 298, 299, 300 I can still point to the source though. https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:300: if (position[0] + drawableWidth < 0 || position[1] + drawableHeight < 0 On 2016/10/12 04:20:50, boliu wrote: > I don't think this is always correct.. this assumes the "point" of the handle is > at the top left, but the left selection handle, the "point" is at the top right > of the drawable, presumably, but I guess there are no soft keyboard coming from > the side, so not a big deal.. > > maybe just ignore the width here..? The "point" of left selection handle is at its top left. The problem lies exactly in the left selection handle. In the case of phone (Nexus 6P), if you select some texts at the left edge of the WebView, the left selection handle's x-axis location is at -80, and its width is 154. That is, there is a portion of the left handle outside the screen. If I ignore the width, then the left handle won't be shown, which is major problem for me (and other users too).
lgtm https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java (right): https://codereview.chromium.org/2410223003/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PopupTouchHandleDrawable.java:276: private boolean isShowingAllowed() { On 2016/10/12 21:34:21, hush wrote: > On 2016/10/12 04:20:50, boliu wrote: > > we only check this on invalidate and when visibility changes, is that enough? > > does Editor check this more frequently? > > Editor in Android does not check more frequently. A rough call path is like > below: > onPreDraw --> TextViewPositionListener#updatePosition > -->Editor$HandleView#isVisible -->Editor#isPositionVisible. > > That is to say, Editor checks on every invalidation. Draw isn't quite the same as invalidate, but close enough here.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/4a64446000115cca6deba65288de86741a40062e Cr-Commit-Position: refs/heads/master@{#424896} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4a64446000115cca6deba65288de86741a40062e Cr-Commit-Position: refs/heads/master@{#424896} |
