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

Issue 738843 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Regression: Normal mouse pointer is seen instead of Text select pointer while hovering on omnibox

Project Member Reported by keerthan...@techmahindra.com, Jul 3 2017

Issue description

Chrome Version:61.0.3147.0
OS:Ubuntu 14.04, Windows

What steps will reproduce the problem?
(1)Launch chrome and open any page [Eg: Hit F1 ]
(2)Hover on  the URL and observe the mouse pointer

Expected: Text select pointer should be seen 
Actual: Instead, Normal mouse pointer is seen

This is a Regression issue seen from M-61

Manual Bisect Info:
===================
Good Build:61.0.3144.0
Bad Build: 61.0.3145.0



 
Expected_Cursor.ogv
866 KB View Download
Actual_Cursor.ogv
738 KB View Download
Status: Untriaged (was: Unconfirmed)
Able to reproduce the issue in Win-10 using latest canary #61.0.3147.0.
Issue is not seen in OS-Mac.
Cc: jmukthavaram@chromium.org
Labels: -Needs-Bisect hasbisect-per-revision
Owner: k...@chromium.org
Status: Assigned (was: Untriaged)
Able to reproduce the issue on Windows 7,Ubuntu 14.04 using chrome latest Canary-61.0.3147.0.
Manual Bisect info:
------------------
Good Build:  61.0.3144.0-Revision-483234
Bad Build:   61.0.3145.0-Revision-483574

Per revision bisect info:
-------------------------
You are probably looking for a change made after 483361 (known good), but no later than 483362 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspectas some perf builds might get missing due to failure.
https://chromium.googlesource.com/chromium/src/+log/f74368efb998d6401a483783df4140889afa92e8..3dc51db95f327e4f86962c94d6d5455ee75cf433

Possible suspect:
-----------------
https://chromium.googlesource.com/chromium/src/+/3dc51db95f327e4f86962c94d6d5455ee75cf433

krb@Could you please take a look and reassign to the respective owner if it is not related to your change.

Thanks..!!
Labels: ReleaseBlock-Stable
Adding blocker as this is a recent regression.

Thanks!

Comment 4 by k...@chromium.org, Jul 3 2017

Backing out change does make it work again.

Another observation: It works if I click twice in the Omnibox. i.e first to select all, second to create cursor.

Comment 5 by k...@chromium.org, Jul 4 2017

Ok, here are my observations. In all cases, the cursor is an I-beam past the URL. What changes is the cursor over the URL.

59
After navigation, I-beam
Select all, blur: I-beam
Select none (caret), blur: I-beam

60
After navigation: I-beam
Select all, blur: arrow
Select none, blur: I-beam

Pre-change
After navigation: I-beam
Select all, blur: arrow
Select none, blur: I-beam

After-change
After navigation: arrow
Select all, blur: arrow
Select none, blur: I-beam

So it seems something changed in 60 too. It would appear that SelectAll() isn't setting the proper cursor and that, since we're calling it more consistently, the cursor isn't proper more often. Hopefully we can fix both cases.

Comment 6 by k...@chromium.org, Jul 6 2017

Cc: k...@chromium.org
Owner: mgiuca@chromium.org
Matt,

I tracked the change in M60 down to this CL:

https://codereview.chromium.org/2855793003

The only tangible difference there appears to be the removed SelectRange(), but that may not do much either; I don't know. Can you take a look?
Cc: mgiuca@chromium.org
Owner: k...@chromium.org
Hi Ken,

I looked into this. Seems like these are two unrelated bugs although similar. I've filed a separate bug for the regression I caused:  Issue 740008 .

Reverting r473830 fixes my issue but not yours. (And it is due to the removal of SelectRange() in that CL.)

Assigning back to you.

Comment 8 by k...@chromium.org, Jul 7 2017

fwiw, restoring the SelectNone() fixes this issue as well. Again, all my change did was call SelectAll in more cases, thus increasing the cases where, when SelectNone() _wasn't_ called, of the cursor being an arrow.

We can leave me owner, and I'll just mark this fixed when the other is.

Comment 9 by mgiuca@chromium.org, Jul 10 2017

> We can leave me owner, and I'll just mark this fixed when the other is.

My CL (https://chromium-review.googlesource.com/c/563143/) doesn't fix this issue. From my testing, these two problems are unrelated (they just exhibit the same symptoms).
Interesting, while I was looking at my issue I found this change from your CL:

https://chromium.googlesource.com/chromium/src/+/3dc51db95f327e4f86962c94d6d5455ee75cf433%5E%21/#F4

--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
@@ -286,7 +286,7 @@
   // RevertAll after navigation to invalidate the selection range saved on blur.
   omnibox_view->RevertAll();
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(omnibox_view->IsSelectAll());

That explicitly tests that the Omnibox text is not selected on navigation, and you changed it to expect that it *is* selected. The fix for this issue should change that line back and passing that test should mean this issue is fixed.

Comment 11 by k...@chromium.org, Jul 10 2017

I changed the test because Peter and I now want IsSelectAll() after a RevertAll(). We went through all the call locations and decided that it was a better outcome. So ultimately, no, the test will not be changed.

"From my testing, these two problems are unrelated (they just exhibit the same symptoms)."

Again, SelectAll() is our intention, not a problem. The problem here is that, after a Blur(), the Omnibox state is left in whatever the previous state was, be it partially select, completely selected, shifted or whatever. Your change does fix that. Thanks.

btw, in case you see your client change behavior mysteriously, I've reverted the SelectAll() CL until we can narrow down a weird Windows performance issue.

#11 OK so are you saying that the issue reported here is WAI? That is, if I press F1 (opens the help page), then mouse over the URL bar, it is *intended* that the mouse cursor be an arrow, not an I-beam?

That's fine, I'm just trying to figure out what it is since originally you seemed to treat this as a bug. If so, mark this as WontFix.

My bug that I am dealing with (surprisingly with excruciating pain) on  Issue 740008  is completely separate to this so you shouldn't block on that fix landing before WontFixing this.

Comment 13 by k...@chromium.org, Jul 11 2017

I was only saying that RevertAll() followed by IsSelectAll() is expected to be true. I can check F1 tomorrow; I'm on a Chromebook now, which goes 'back' on F1.

Don't worry, I'm not waiting on yours. As I mentioned, we're chasing a performance issue before re-submitting - 739303.
F1 is just a shorthand for opening a new tab with a non-empty URL. You can equivalently Ctrl+Click on a link then switch to it.

Comment 15 by k...@chromium.org, Jul 11 2017

Ok, I see what you're saying: Opening a link into a new tab ("open new background tab" ?) and then switching to it triggers a (unexpected?) SelectAll(). It doesn't actually happen until we switch to the tab, when we call RestoreState().

Peter,

Adding a SelectNone() after the RevertAll() in RestoreState() "fixes" this. Does that sound like the right way to fix it?

More info: RestoreState() is called from OnTabChanged() where 'state' is null, which makes me wonder if a more elegant way to fix would be to correctly set the state (somewhere, not sure.)

(Maybe red herring but this sounds like some other issues we have, like "opening link in new incognito tab has wrong cursor offset".)

I don't think the "open a link in a background tab" part is actually the critical bit here.  The real critical bit is that in general, we (intentionally) made it more the case that text in the omnibox is selected when the omnibox doesn't have focus.  The side effect I did not think about is that the mouse cursor changes to a pointer when over a selected region, so you can drag it.  This confuses user because when the omnibox is unfocused we hide the selection, so it doesn't _look_ like there's a draggable region.

While it's possible to trigger this with the steps described, there are also other cases where text is selected (and thus leads to the pointer cursor when hovered) but you can't see it.  For example, simply selecting all the text and then clicking in the page, versus selecting none of the text and then clicking in the page, lead to the same surprising-feeling cursor difference.

I think this suggests that we either need to:
* Not hide the selection when the omnibox loses focus, which probably will look broken, or
* Actually act like nothing is selected if nothing looks selected; that is, if the omnibox is unfocused, hovering and clicking it should always behave as if there is no selection.  Basically, the only time we care about the "underlying" selection is if the omnibox regains focus by tabbing back into it.

I would do the latter.  There are a couple of ways to actually implement.  The easiest would probably be to replace the code that hides the selection with a change during blur handling that actually deselects the text (but saves the selection into a member that is restored during focus).  The tricky bit there will be ensuring that the restore-during-focus doesn't screw with other selection muckery we do during focus.
A friendly reminder that M61 branch is coming soon on 07/20! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix ASAP to trunk. This way we branch M61 from a high quality trunk. Thank you.

Comment 18 by k...@chromium.org, Jul 11 2017

"For example, simply selecting all the text and then clicking in the page"

This actually doesn't trigger it. See comment 5. OnBlur() has an explicit SelectNone() in it, so these cases are handled.

The problem with the new tab seems to be that the Omnibox doesn't (currently) seem to have much interaction with new tab creation, so the Omnibox doesn't create a default state for it. Explicitly creating it would fix both of these cases (the remaining case on this bug and e.g. new incognito tab).

It triggers it for me on Win Dev.  Is this varies-by-OS?

Comment 20 by k...@chromium.org, Jul 11 2017

More likely the build: ToT doesn't have Matt's revert yet. Not sure about 60...
#20 Note that my revert is being held up by some very unfortunate complications (some ChromeVox logic changed since my CL landed which means I can't simply revert the change). But if you want to imagine what life is like with  Issue 740008  fixed, you can either revert c603f3c7 or just add this line to omnibox_view_views.cc:

https://chromium-review.googlesource.com/c/563143/7/chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Comment 22 by ajha@chromium.org, Jul 14 2017

Just to update, this seems to be working fine on the latest M-61(61.0.3157.0) of Windows-10 and Linux Ubuntu 14.04.

Comment 23 by k...@chromium.org, Jul 17 2017

Status: Fixed (was: Assigned)
I'm marking this fixed, so the release folk know, because the select-all CL (gerrit/548985) was reverted, and it sounds like the behavior change is confirmed.

Sign in to add a comment