New issue
Advanced search Search tips

Issue 838830 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Single-click focusing omnibox sometimes reveals full URL

Project Member Reported by maxwalker@chromium.org, May 2 2018

Issue description

Chrome Version: 68.0.3417.0
OS: macOS

What steps will reproduce the problem?
Single-click URL or omnibox background to select URL.

What is the expected result?
The URL should be selected without revealing the scheme/subdomain.

What happens instead?
Sometimes the full URL is shown, see attachment. I'm not quite sure when this happens exactly, but it's easy to reproduce when clicking in and then out of the omnibox a few times.
 
Focus Omnibox.gif
683 KB View Download
I'm pretty sure what's happening there is your mouse action is being interpreted as a drag, even though no selection results.

Tommy, would it be possible (without a big, complicated change) to not unelide on drag if there's no selection?
Note that you can reproduce this (or at least something similar) by clicking between two characters and dragging through part of one of them. Once you drag through the whole character, you'll make a selection. But short of that, which can still be a significant amount of mouse travel, there's no selection and the URL unelides.
Status: Started (was: Untriaged)
I will investigate this.
Project Member

Comment 4 by bugdroid1@chromium.org, May 2 2018

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

commit 90a71482c170466725cb4e217d8932cb8ac508e9
Author: Tommy C. Li <tommycli@chromium.org>
Date: Wed May 02 23:58:27 2018

Omnibox UI Experiments: Steady State Elisions - Fix drag threshold bug.

Previously, the drag threshold calculation had a bug. The last-click
event was stored as a root_location, which was tested against the drag
event's location relative to the View origin.

This CL makes both the stored last-click and the current drag event
use the root_location. Using root_location is most robust, since the
View itself can move in response to edit-in-progress state.

It also updates the API of textfield to make more explicit that the
stored last click location is a root_location rather than relative
to the position of the View.

This CL also adds a test.

Bug:  838830 ,  797354 
Change-Id: Iedff486a3c6e842bc5c87c6fd597e1faf7a2930d
Reviewed-on: https://chromium-review.googlesource.com/1040347
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555613}
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/chrome/browser/ui/views/omnibox/omnibox_view_views.h
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/ui/views/selection_controller.cc
[modify] https://crrev.com/90a71482c170466725cb4e217d8932cb8ac508e9/ui/views/selection_controller.h

Status: Fixed (was: Started)
Should be fixed in Canaries 3419.0 and later.
Cc: gov...@chromium.org
Labels: Merge-Request-67
Project Member

Comment 7 by sheriffbot@chromium.org, Jun 4 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Above CL only touches Views, so it should only affect ChromeOS and Desktop.
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #5, #8 and per offline chat with  tommycli@, this should be safe to merge. Pls merge ASAP. Thank you.

Also why merge wasn't requested to M67 before as Cl was landed on May 2nd?
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 4 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f39ea53c83e5278d93e4bdbe106271737f4be69a

commit f39ea53c83e5278d93e4bdbe106271737f4be69a
Author: Tommy C. Li <tommycli@chromium.org>
Date: Mon Jun 04 21:52:40 2018

Omnibox UI Experiments: Steady State Elisions - Fix drag threshold bug.

Previously, the drag threshold calculation had a bug. The last-click
event was stored as a root_location, which was tested against the drag
event's location relative to the View origin.

This CL makes both the stored last-click and the current drag event
use the root_location. Using root_location is most robust, since the
View itself can move in response to edit-in-progress state.

It also updates the API of textfield to make more explicit that the
stored last click location is a root_location rather than relative
to the position of the View.

This CL also adds a test.

TBR=tommycli@chromium.org

(cherry picked from commit 90a71482c170466725cb4e217d8932cb8ac508e9)

Bug:  838830 ,  797354 
Change-Id: Iedff486a3c6e842bc5c87c6fd597e1faf7a2930d
Reviewed-on: https://chromium-review.googlesource.com/1040347
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#555613}
Reviewed-on: https://chromium-review.googlesource.com/1086158
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#740}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/chrome/browser/ui/views/omnibox/omnibox_view_views.h
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/ui/views/selection_controller.cc
[modify] https://crrev.com/f39ea53c83e5278d93e4bdbe106271737f4be69a/ui/views/selection_controller.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 4 2018

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

commit 8ee0726dfd163a3daf82d05d98755903746b569b
Author: Tommy C. Li <tommycli@chromium.org>
Date: Mon Jun 04 23:48:47 2018

Omnibox: Fix a bad merge to M67.

Fix a bad merge here:
https://chromium-review.googlesource.com/c/chromium/src/+/1086158

Two lines were transposed.

Bug: 849270,  838830 
Change-Id: If942472b8e4c077c5253ec2711d7fe42705045e5
Reviewed-on: https://chromium-review.googlesource.com/1086347
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#744}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/8ee0726dfd163a3daf82d05d98755903746b569b/chrome/browser/ui/views/omnibox/omnibox_view_views.h

Sign in to add a comment