Omnibox UI Experiments: Incorrect Display of Navsuggestions |
||||||||
Issue descriptionIt appears the omnibox UI experiments have trouble with URL suggestions that come from the server. This experience is particularly important for new users. Example for elide path: Without elide path flag set, 1. Type www.google.com/ma in the omnibox 2. See multiple suggestions such as http://www.google.com/maps and http://www.google.com/mail. These are shown properly emphasized to show the match in the path. With elide path flag set, 2. See multiple suggestions shown as google.com/..., i.e., truncated despite the match in the path. Example for remove trivial subdomains: Same as above. Note that I typed "www", so the trivial subdomain shouldn't be omitted, right? Example for hide https: Without flag: 1. Type https://face in the omnibox 2. Observe the following URL suggestions in the dropdown: https://www.facebook.com/login.php https://facebook.com https://www.facebook.com/recover.php https://facebook.com/pokes With flag, the second suggestion is rendered as facebook.com/... i.e., not showing the https, despite there being a match there Further down, some of the URL suggestions are shown with https:// and some are not, yet they're all actually https://. Strange. [Note: the bolding is wrong on some of these suggestions. That's a separate bug.]
,
Sep 1 2017
mpearson: Thanks for the examples. I can confirm that we don't do the match_in_foo calculations in SearchProvider, and that's a hole that I need to fix.
,
Sep 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbcd2eaa0e238689ae2dfdc7dad403248244a58b commit fbcd2eaa0e238689ae2dfdc7dad403248244a58b Author: Tommy C. Li <tommycli@chromium.org> Date: Tue Sep 12 19:30:46 2017 Omnibox UI Experiments: Add match_in_foo to Bookmarks and Navsuggest This CL adds a unified match_in_foo implementation for use between HistoryURLProvider, Bookmarks, and Navsuggest. It deliberately leaves off HQP, as that has does tricky operations using adjusted match offsets rather than unadjusted (like the other providers). It should be very posible to refactor HQP to use this unified code as well, but that should be saved for a separate CL. Bug: 761505 Change-Id: I8107915cae0ef6477e18de32fd924c2a9506e2ad Reviewed-on: https://chromium-review.googlesource.com/661884 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#501358} [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/autocomplete_match.cc [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/autocomplete_match.h [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/autocomplete_match_unittest.cc [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/history_url_provider.cc [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/search_suggestion_parser.cc [modify] https://crrev.com/fbcd2eaa0e238689ae2dfdc7dad403248244a58b/components/omnibox/browser/titled_url_match_utils.cc
,
Sep 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/639b4f1123910ef7b3dcdbd82ae1759585eaf7e5 commit 639b4f1123910ef7b3dcdbd82ae1759585eaf7e5 Author: Tommy C. Li <tommycli@chromium.org> Date: Wed Sep 13 00:11:27 2017 Omnibox UI Experiments: Fix match_in_foo in SearchSuggestionParser. Addresses some nits, but primarily fixes the SearchSuggestionParser match classification logic. Previously the match_start was incorrectly calculated using the cleaned-up formatted URL instead of the original URL, which we need to correctly flag which components the matches are in. Bug: 761505 Change-Id: Ifae972c79e16387c8a9b8c49811a035c1fb017f1 Reviewed-on: https://chromium-review.googlesource.com/663848 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Commit-Position: refs/heads/master@{#501467} [modify] https://crrev.com/639b4f1123910ef7b3dcdbd82ae1759585eaf7e5/components/omnibox/browser/autocomplete_match.cc [modify] https://crrev.com/639b4f1123910ef7b3dcdbd82ae1759585eaf7e5/components/omnibox/browser/autocomplete_match.h [modify] https://crrev.com/639b4f1123910ef7b3dcdbd82ae1759585eaf7e5/components/omnibox/browser/autocomplete_match_unittest.cc [modify] https://crrev.com/639b4f1123910ef7b3dcdbd82ae1759585eaf7e5/components/omnibox/browser/search_suggestion_parser.cc
,
Sep 13 2017
Should be fixed in above two patches.
,
Sep 13 2017
> Should be fixed in above two patches. Should be or definitively is? Did you test the examples in the original report, including the weird https://face one?
,
Sep 13 2017
This is fixed. But I will point out there is one case you may still find strange... though is technically working as intended: When we type "https://face" as the user input, we get the first four nav suggests results looking like: facebook.com/… - https://www.facebook.com/login.php https://facebook.com - https://facebook.com facebook.com/… - https://www.facebook.com/recover.php https://facebook.com/… - https://facebook.com/pokes This is because in the 1st and 3rd navsuggest, the input string "https://face" doesn't actually appear in the URL (there's a www), so it's correctly marking match_in_scheme = false. We used to have a call that looks like: trim_http = !AutocompleteInput::HasHTTPScheme(...), but that wouldn't help this HTTPS case either. So... if the above results bother you, probably the thing to do is to add an extra bool preserve_scheme = match_in_scheme || AutocompleteInput::HasAnyScheme(...) clause... Does that seem like something we want to do?
,
Sep 13 2017
That's a minor polish issue. I'll leave it up to you (and Emily?) whether to bother fixing it.
,
Sep 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d794261726f669ffea05e63e8049c822c274dccc commit d794261726f669ffea05e63e8049c822c274dccc Author: Tommy C. Li <tommycli@chromium.org> Date: Wed Sep 13 20:00:54 2017 Omnibox UI Experiments: Refactor HQP to use shared match_in_foo code. This makes the HQP use the same code for determining match_in_scheme, match_in_subdomain, and match_after_host as the other providers. This is a followup to: https://chromium-review.googlesource.com/c/chromium/src/+/661884 Bug: 761505 Change-Id: I018a6e98df55d8459918c8776044dead2bf50b4a Reviewed-on: https://chromium-review.googlesource.com/663670 Commit-Queue: Tommy Li <tommycli@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Commit-Position: refs/heads/master@{#501722} [modify] https://crrev.com/d794261726f669ffea05e63e8049c822c274dccc/components/omnibox/browser/scored_history_match.cc
,
Sep 13 2017
Emily / Justin, see a potential followup issue in c#7. I'd be in favor of putting forth the effort to polish it up, but I'd like to hear at least one other person also think it's worth the effort...
,
Sep 13 2017
,
Sep 14 2017
What is the suggested solution in this case? Would they all include the "https" or all elide it? I think this would be worth fixing.
,
Sep 14 2017
I'll take over for the polish fix that tommycli suggests. I believe the effect of the proposed fix would be to make all 4 responses show the "https".
,
Sep 19 2017
Untriaged -> Assigned per "I'll take over for the polish fix..."
,
Nov 17 2017
krb: let's sit down together at some point and I can show you what I learned so far on where the proposed change needs to be made.
,
Nov 21 2017
krb: the bug only manifests if you have the elision flag enabled: chrome://flags/#omnibox-ui-hide-suggestion-url-scheme
,
Nov 30 2017
Thanks for the screenshot with your proposed fix (https://crrev.com/c/794432). Can you add another with chrome://flags/#omnibox-ui-hide-suggestion-url-trivial-subdomains also enabled?
,
Nov 30 2017
Cool, thanks. Looks like my concern was misplaced as we're matching (bolding) consistently in all cases.
,
Dec 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7999f246fe254591ca2dd981e2a228a3923a1ad0 commit 7999f246fe254591ca2dd981e2a228a3923a1ad0 Author: Kevin Bailey <krb@chromium.org> Date: Sat Dec 09 18:00:29 2017 [omnibox] Handle URL ellision a little more consistently When the option hide-suggestion-url-scheme is enabled, Omnibox suggestions exclude the scheme in some cases where they would be inconsistent with other suggestions. This change preserves schemes when the user specified one. This change asserts 'preserve_scheme' to GetFormatTypes() now if alternatively the input, interpretted as a URL, has a scheme. This changes the treatment of http (and https when certain flags are set.) Previously it was only asserted if there was a match in the scheme. Added many tests as well. Bug: 761505 Change-Id: I189ececbfad3baf8214e3f289e0c994d3cee9d55 Reviewed-on: https://chromium-review.googlesource.com/794432 Commit-Queue: Kevin Bailey <krb@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#523003} [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/chrome/browser/autocomplete/search_provider_unittest.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_quick_provider.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_quick_provider.h [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_quick_provider_unittest.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_url_provider.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_url_provider.h [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/history_url_provider_unittest.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/search_provider.h [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/search_suggestion_parser.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/titled_url_match_utils.cc [modify] https://crrev.com/7999f246fe254591ca2dd981e2a228a3923a1ad0/components/omnibox/browser/titled_url_match_utils_unittest.cc
,
Dec 11 2017
Some of the tests added in #22 appear to be broken on Win7 Debug (see https://ci.chromium.org/buildbot/chromium.win/Win7%20Tests%20(dbg)(1)/65112).
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47735b427fb7248d51c95d522291aa5a1ea99461 commit 47735b427fb7248d51c95d522291aa5a1ea99461 Author: Maxim Kolosovskiy <kolos@chromium.org> Date: Mon Dec 11 10:17:50 2017 Revert "[omnibox] Handle URL ellision a little more consistently" This reverts commit 7999f246fe254591ca2dd981e2a228a3923a1ad0. Reason for revert: components_unittests failures HistoryQuickProviderTest.DontTrimHttpSchemeIfInputHasScheme HistoryQuickProviderTest.DoTrimHttpsSchemeIfFlag HistoryQuickProviderTest.DontTrimHttpsScheme HistoryQuickProviderTest.DontTrimHttpsSchemeDespiteFlag HistoryQuickProviderTest.DontTrimHttpSchemeIfInputMatches HistoryQuickProviderTest.DoTrimHttpScheme first failed build https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/65092 Original change's description: > [omnibox] Handle URL ellision a little more consistently > > When the option hide-suggestion-url-scheme is enabled, Omnibox > suggestions exclude the scheme in some cases where they would > be inconsistent with other suggestions. This change preserves > schemes when the user specified one. > > This change asserts 'preserve_scheme' to GetFormatTypes() now > if alternatively the input, interpretted as a URL, has a scheme. > This changes the treatment of http (and https when certain flags > are set.) Previously it was only asserted if there was a match > in the scheme. > > Added many tests as well. > > Bug: 761505 > Change-Id: I189ececbfad3baf8214e3f289e0c994d3cee9d55 > Reviewed-on: https://chromium-review.googlesource.com/794432 > Commit-Queue: Kevin Bailey <krb@chromium.org> > Reviewed-by: Mark Pearson <mpearson@chromium.org> > Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> > Cr-Commit-Position: refs/heads/master@{#523003} TBR=mpearson@chromium.org,krb@chromium.org,jdonnelly@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 761505 Change-Id: I2f7f3f747c45204bd4d7507f348cbd8141da41aa Reviewed-on: https://chromium-review.googlesource.com/817298 Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org> Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org> Cr-Commit-Position: refs/heads/master@{#523070} [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/chrome/browser/autocomplete/search_provider_unittest.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_quick_provider.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_quick_provider.h [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_quick_provider_unittest.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_url_provider.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_url_provider.h [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/history_url_provider_unittest.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/search_provider.h [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/search_suggestion_parser.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/titled_url_match_utils.cc [modify] https://crrev.com/47735b427fb7248d51c95d522291aa5a1ea99461/components/omnibox/browser/titled_url_match_utils_unittest.cc
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320 commit ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320 Author: Kevin Bailey <krb@chromium.org> Date: Tue Dec 12 01:07:32 2017 Reland "[omnibox] Handle URL ellision a little more consistently" This makes a tiny fix to the previous change, which was failing on Win 7 debug. There is a line in ScoredHistoryMatch::ScoredHistoryMatch(...) which dereferences a string's rbegin() iterator. (Searching for 'rbegin' will find the only occurrence.) Thus the code expects a non-empty string. I'm guessing it passed the other platforms because they store a '\0' there, while a debug build might be more diligent. This change simply changes history_quick_provider_unittest.cc to pass a non-empty string to that parameter. Revert: 817298 Original CL: 794432 When the option hide-suggestion-url-scheme is enabled, Omnibox suggestions exclude the scheme in some cases where they would be inconsistent with other suggestions. This change preserves schemes when the user specified one. This change asserts 'preserve_scheme' to GetFormatTypes() now if alternatively the input, interpretted as a URL, has a scheme. This changes the treatment of http (and https when certain flags are set.) Previously it was only asserted if there was a match in the scheme. Added many tests as well. Bug: 761505 Change-Id: I417448ffbb370d86c794309520083cff10ea5957 Reviewed-on: https://chromium-review.googlesource.com/820354 Commit-Queue: Kevin Bailey <krb@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Commit-Position: refs/heads/master@{#523283} [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/chrome/browser/autocomplete/search_provider_unittest.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_quick_provider.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_quick_provider.h [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_quick_provider_unittest.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_url_provider.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_url_provider.h [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/history_url_provider_unittest.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/search_provider.h [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/search_suggestion_parser.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/titled_url_match_utils.cc [modify] https://crrev.com/ad4f3d69a83ab8c325f37e3ca86cb05f1ce7d320/components/omnibox/browser/titled_url_match_utils_unittest.cc
,
Dec 13 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mpear...@chromium.org
, Sep 1 2017