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

Issue 789297 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Inconsistent behavior with long press selection in the omnibox

Project Member Reported by mahmoudi@chromium.org, Nov 28 2017

Issue description

Chrome 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
 
Cc: tedc...@chromium.org
+tedchoc@ - any idea how to route this?
Owner: rlanday@chromium.org
Status: Assigned (was: Unconfirmed)
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.
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.
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.
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.
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.
Status: Started (was: Assigned)
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.
> 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...
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@).
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.
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.
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.
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.
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.
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?
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?
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.
Issue 605640 has been merged into this issue.
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.
Project Member

Comment 20 by bugdroid1@chromium.org, 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

Labels: M-65
Status: Fixed (was: Started)

Sign in to add a comment