Inconsistent behavior with long press selection in the omnibox |
||||
Issue descriptionChrome Version: 64.0.3279.0 (Official build) canary (32-bit) OS: Android 6.0.1 What steps will reproduce the problem? 1. Launch Chrome 2. Go to a web page with multiple slashes in the URL (e.g. http://www.espn.com/college-football/story/_/page/RoadtoAtlanta112817/acc-sec-play-games-other-cfp-scenarios) 3. Long press on the domain in the omnibox What is the expected result? Consistent behavior, e.g. the domain should be selected every time. What happens instead? Inconsistent behavior. Sometimes the domain is selected without the protocol (https://), sometimes :// from the protocol are also included in the selection with the domain, sometimes the entire URL up to the last slash is selected, sometimes the entire URL is selected, etc. Videos: https://drive.google.com/open?id=1SUSPJtw5cdNYsStt-ALuHL025c2NcYZB https://drive.google.com/open?id=1q67Zc0UAgxvMyRrM7SeryPWBfyrBgChN
,
Nov 29 2017
Naively, I think Android "should" be handling this correctly. Editor#selectCurrentWord ( https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/widget/Editor.java#844) should be being triggered and that should check needsToSelectAllToSelectWordOrParagraph() which should return true for URI. Long press selection in the omnibox is super unpredictable right now, which is quite unfortunate. I'll send it over to rlanday@ to investigate (UrlBar.java is the starting point). Quite the opposite end of text input, but he's done enough investigation in the various IME quirks that he might be able to spend a bit on this to make this not so rough. If you don't have cycles, punt it back to me and I can take a peek. Ideally, long pressing on the omnibox should select the full text always. Longer term, we might want it to select single words if you are inputting something with spaces in it.
,
Dec 12 2017
It appears that the whole URL *is* getting selected, but Editor immediately starts a drag selection (long press calls Editor#selectCurrentWordAndStartDrag()). Might be a bug in EditText that we need to work around.
,
Jan 3 2018
I think there are two problems here: 1. UrlBar#onTouchEvent() is not properly calling super.onTouchEvent() in the case that it's not focused: https://chromium.googlesource.com/chromium/src/+/b457d926a263ff8c41f7902897c38e113afe0970/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java#398 This is necessary so EditText can call Editor#onTouchEvent() to record the initial tap location and detect dragging properly. See comment here: https://android.googlesource.com/platform/frameworks/base/+/b7941c50a0953b380c65fa152c9c7da954d3d238/core/java/android/widget/Editor.java#5335 "This is done even when the View does not have focus, so that long presses can start selection and tap can move cursor from this tap position." 2. The animation (ToolbarPhone#triggerUrlFocusAnimation()) that sort of zooms in on the address being edited moves the text over to the side, so even if you keep your finger in one place, it's still being moved relative to the UrlBar, which triggers a drag event. I think we need some sort of logic to suppress either touch events or selection changes while the animation is running.
,
Jan 3 2018
I believe the onTouchEvent not calling super was an intentional decision on my part to prevent the UrlBar from bringing up the soft keyboard. The goal was to defer bringing up the keyboard till the url focus animation had completed (as they could not run in parallel with any consistent frame rate). Granted, this was years and years ago, so many things might have changed, or I might be incorrectly remembering this altogether.
,
Jan 3 2018
I guess there's no need to call super here when the UrlBar is not focused since I think we need to suppress the touch move events anyway.
,
Jan 3 2018
I think we actually do need to call super for the touch down event since otherwise when we call it for the touch up event (necessary for the selection popup menu and handles to show), it would still think we're trying to do a drag selection. Doing so doesn't seem to perceptibly change the timing of the keyboard coming up on my Nexus 6P.
,
Jan 3 2018
> Doing so doesn't seem to perceptibly change the timing of the > keyboard coming up on my Nexus 6P. The Nexus 6P is a pretty good quality device. You may want to try an older device. On my lower-end device (from 2013), I often notice lag getting the keyboard to appear. (Guess: I think the keyboard process has often been evicted from memory, so displaying the keyboard actually causes a new process to start.) I wonder what would happen if another animation ran at the same time...
,
Jan 3 2018
It also works fine on my Nexus 4. On both devices, with or without my CL applied, it appears that the keyboard does not slide up until after the URL focus animation is finished (which is the desired behavior according to tedchoc@).
,
Jan 3 2018
The easiest way to see if it effects the keyboard timing is to just increase the focus animation duration (ToolbarPhone#URL_FOCUS_CHANGE_ANIMATION_DURATION_MS) to some obscene number like 5 seconds and see if the keyboard comes up before it finishes.
,
Jan 3 2018
If I do that, the keyboard comes up before the animation is finished both before and after my change (https://chromium-review.googlesource.com/c/chromium/src/+/849260). Since there doesn't seem to actually be a change in behavior with regard to the keyboard, I think it's safe to proceed.
,
Jan 3 2018
Are you testing in Chrome Home or not Chrome Home? In non-Chrome Home, I overwrote the value and the keyboard doesn't show up until the animation is complete.
,
Jan 3 2018
I do not have Chrome Home enabled. Both before and after my change, it appears that the keyboard shows on touch up. So if you release your finger early, the keyboard will appear before the animation ends; otherwise, it will not. Does this match what you're seeing? The current code only skips calling super.onTouchEvent() if the UrlBar is not currently focused. While the animation is running, the UrlBar is considered to be focused already, so even the current code is propagating the touch up event. It's just skipping the touch down.
,
Jan 4 2018
tedchoc@ and I compared what we're seeing and it looks like he was describing the behavior for a regular tap and I was describing the behavior for a long press. My current CL does not change the behavior for long press, but it makes the keyboard show early for a single tap. Will try to find a workaround.
,
Jan 4 2018
I thought that maybe hanging onto the touch down event and only passing it to super.onTouchEvent() if we're about to handle a long press would avoid changing the keyboard behavior, but this screws up the behavior for long presses again (it still gets interpreted as a drag selection; I haven't been able to figure out why). Given this, do we think it's acceptable to add in the call to super.onTouchEvent() and change the keyboard behavior for single taps?
,
Jan 4 2018
To change the keyboard showing logic, I think we'd need to gather some frame rate metrics (naively, you can just add a log call to the draw method in toolbarphone or locationbarphone) around focusing the omnibox w/ and w/o the keyboard showing. We'd need to verify on a regular page and on the NTP. The NTP has more things moving around so that might be a bigger case. We'd also need to verify on the current batch of low end android one devices as they're likely the lowest target point now. With your test to propagate touch events, are we calling super.onTouchEvent at all? Once we trigger the long press, do we start sending super.onTouchEvents with different positions? I wonder if we mutated all the touch events during the focus transition to maintain the same position could we trick it into thinking the finger didn't move?
,
Jan 4 2018
I'm blocking super.onTouchEvent() for the touch move events. I'm calling it for: - The touch down (delayed until immediately before we call super.performLongClick()) - (super.performLongClick() happens here) - The touch up I'm thinking that maybe changing the timing is causing the performLongClick() to trigger some sort of double click behavior? Not sure exactly.
,
Jan 4 2018
Issue 605640 has been merged into this issue.
,
Jan 5 2018
It turns out that the approach mentioned in comment 15 (holding onto the touch down event and only releasing it if we're about to do a long press) actually does work, if I correctly copy the MotionEvent using MotionEvent.obtain() (apparently they are recycled, which I was not aware of). This avoids changing the keyboard timing behavior, so I think we can move forward with this.
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7431258886724a34681028ab108612fe9fbb45a1 commit 7431258886724a34681028ab108612fe9fbb45a1 Author: Ryan Landay <rlanday@chromium.org> Date: Tue Jan 09 00:30:13 2018 Improve behavior on Android when long-pressing Omnibox Currently, when long-pressing the Omnibox on Android (when it's not already focused), it selects the full URL (as intended) for a split second, but then usually immediately changes the selection to something else. This happens because 1. We don't call super.onTouchEvent() on the touch down event so EditText knows where the first touch was (to know if subsequent touch move events should do a drag selection or not). 2. When the URL focus animation runs, it moves the UrlBar to the side slightly, which makes a finger held still look like a moving finger from the UrlBar's perspective (interpreted as a drag selection). The fix is as follows: 1. We need to call super.onTouchEvent() on touch down events that result in a long press, but we don't want to always call it, because this would mean that for single taps, the keyboard might start showing before the URL focus animation finishes, which could result in performance issues. Therefore, we hold onto touch down events received while the UrlBar is not focused, and only call super.onTouchEvent() on them if we're about to call super.performLongClick() 2. If we receive a touch down event while the UrlBar is not focused, ignore all subsequent touch move events until the next touch down event to avoid triggering a drag selection. Bug: 789297 Change-Id: I1802f1b7d058e9ef5d38239e37aae004d651da2a Reviewed-on: https://chromium-review.googlesource.com/849260 Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org> Commit-Queue: Ryan Landay <rlanday@chromium.org> Cr-Commit-Position: refs/heads/master@{#527828} [modify] https://crrev.com/7431258886724a34681028ab108612fe9fbb45a1/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java [modify] https://crrev.com/7431258886724a34681028ab108612fe9fbb45a1/chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/UrlBarTest.java
,
Jan 9 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by twelling...@chromium.org
, Nov 28 2017