Omnibox - Cursor Position Not Used for Server-Powered Suggestions |
|||||||||
Issue descriptionSupplying the cursor position to the suggest server can yield better suggestions. Desktop takes cursor position into account. Type "new times" and look at the search query suggestions. Then put the cursor in the middle, delete the space, and type the space again (to get the string "new times"). The query suggestions are dramatically different. Android does not take cursor position into account. I did not try iOS; it probably does not either.
,
Jul 19 2017
,
Sep 1 2017
Claiming!
,
Sep 1 2017
Thank you!
,
Sep 1 2017
jdonnelly, mark asks: "Can one of you in LAX with an iOS device check to see if iOS uses cursor position already? See repro steps on crbug.com/738559 "
,
Sep 5 2017
,
Sep 5 2017
Confirmed: iOS behavior matches Android, not desktop. The cursor-aware "new york times" suggestions do not appear when following the repro steps.
,
Sep 5 2017
Thanks for checking on iOS. When looking for the code that needs to be added to make iOS work, I found a relevant bug ( bug 726702 ): // TODO( crbug.com/726702 ): Implement this and have |SetWindowTextAndCaretPos()| // call it. void OmniboxViewIOS::SetCaretPos(size_t caret_pos) {} https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm?l=237-239 I'd also want to make sure that SetWindowTextAndCaretPos is called at the right places. I *think* it is because cursor position works on Mac, and Mac only calls SetWindowTextAndCaretPos in one place (OnTemporaryTextMaybeChanged()), and it's also called in the same place in iOS. I *think* implementing that function should be easy someone with an iOS build setup. The Mac code for SetCaretPos() is only two lines long; I can't imagine iOS being that different... https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm?l=335-338
,
Sep 13 2017
,
Sep 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2985e918abe76fc27e19bee76dbadd3126c0a488 commit 2985e918abe76fc27e19bee76dbadd3126c0a488 Author: Mark Pearson <mpearson@chromium.org> Date: Mon Sep 25 22:17:36 2017 Omnibox - Use Cursor Position When Generating Suggestions on Android Both the local suggestion providers and the server can provide better suggestions with this context. Remove a now redundant / unnecessary function here. Bug: 738559 Change-Id: I4780c99a1347b2aa80bc52cb57a1f772709b3ad8 Reviewed-on: https://chromium-review.googlesource.com/665514 Commit-Queue: Mark Pearson <mpearson@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#504184} [modify] https://crrev.com/2985e918abe76fc27e19bee76dbadd3126c0a488/chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java [modify] https://crrev.com/2985e918abe76fc27e19bee76dbadd3126c0a488/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java [modify] https://crrev.com/2985e918abe76fc27e19bee76dbadd3126c0a488/chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/UrlBarTest.java [modify] https://crrev.com/2985e918abe76fc27e19bee76dbadd3126c0a488/chrome/test/android/javatests/src/org/chromium/chrome/test/util/OmniboxTestUtils.java
,
Oct 19 2017
,
Oct 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b89f46d5db2f3ba8ae3d40200f7b79046385e8eb commit b89f46d5db2f3ba8ae3d40200f7b79046385e8eb Author: Justin Donnelly <jdonnelly@google.com> Date: Wed Oct 25 18:27:02 2017 Implement OmniboxViewIOS::GetSelectionBounds. This will supply the correct cursor position to the suggest backend, allowing us to enable cursor-position-sensitive suggestions. Bug: 738559 Change-Id: I6a9e72f747dc50fb000ddacb0f77503fdd4b6fb3 Reviewed-on: https://chromium-review.googlesource.com/728720 Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org> Commit-Queue: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#511524} [modify] https://crrev.com/b89f46d5db2f3ba8ae3d40200f7b79046385e8eb/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm
,
Aug 7
It seems like the CL in comment 12 fixed the bug?
,
Aug 7
This still needs to be enabled on the server. And right now the experiments look confusing, so it's possible something isn't working right. Leaving open until it's fully launched.
,
Oct 30
This has been launched on Android. iOS still need verification before launch.
,
Nov 5
iOS has been verified and launched. Marking as fixed. :-) (at last!) |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by mpear...@chromium.org
, Jun 30 2017On Android, the core problem is here, where we always pretend the cursor is unset (default to the end of the text). start(profile, url, text, -1, preventInlineAutocomplete, focusedFromFakebox); https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java?l=101