Issue metadata
Sign in to add a comment
|
Regression: Normal mouse pointer is seen instead of Text select pointer while hovering on omnibox |
||||||||||||||||||||||
Issue descriptionChrome 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
,
Jul 3 2017
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..!!
,
Jul 3 2017
Adding blocker as this is a recent regression. Thanks!
,
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.
,
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.
,
Jul 6 2017
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?
,
Jul 7 2017
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.
,
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.
,
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).
,
Jul 10 2017
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.
,
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.
,
Jul 11 2017
#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.
,
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.
,
Jul 11 2017
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.
,
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".)
,
Jul 11 2017
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.
,
Jul 11 2017
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.
,
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).
,
Jul 11 2017
It triggers it for me on Win Dev. Is this varies-by-OS?
,
Jul 11 2017
More likely the build: ToT doesn't have Matt's revert yet. Not sure about 60...
,
Jul 12 2017
#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
,
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.
,
Jul 17 2017
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 |
|||||||||||||||||||||||
Comment 1 by krajshree@chromium.org
, Jul 3 2017