Navsuggest should receive the same visual treatment as other URL suggestions |
||
Issue descriptionOn iOS, URLs from server-side navsuggest receive a visual treatment similar to other suggestions from the Search backend: a single line of black text. These suggestions should have a visual treatment that more clearly indicates the fact that selecting them will trigger an immediate navigation to that URL rather than a navigation to a search results page. In the first attached screenshot, the first suggestion is a URL suggestion from my Chrome history, with a visually clear URL treatment (title and blue URL). The second is a query suggestion (what I've typed in the omnibox). Selecting that will issue a search for that string and is appropriately simple black text. The last two are URL suggestions (navsuggest) which will trigger navigations but they have the query-like visual treatment. I propose changing these suggestions to look as they do in the second screenshot. The URLs are blue like the local URL suggestion. The title shown is the URL again, which matches how these suggestions are shown on Android. Alternatively, the navsuggestions could be shown as just blue text, without a title, but I'd argue that it's clearer if all the URL-type suggestions have both title and URL even if it's redundant in some cases. (The code will also be simpler.) A secondary benefit of this change will be to remove the custom method used to determine which types of suggestions are query types: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm?l=672 This method could be eliminated in favor of the one used on other platforms: https://cs.chromium.org/chromium/src/components/omnibox/browser/autocomplete_match.cc?l=391 Using the cross-platform method will ensure that the definition of search types is up-to-date and doesn't have to be maintained separately.
,
Apr 27 2017
Moreover, there are actually some cases where navsuggest *does* send us a title (try 'www.sbu' in any other platform and you should see www.sbu.edu with the title 'St. Bonaventure University'. This title is not being displayed on iOS however since we're treating this type of suggestion as a query.
,
Apr 27 2017
Here's an analysis of which types would be affected by the proposed fix (switching from the custom isSearchMatch: to AutocompleteMatch::IsSearchType()). ---- Types that won't change (are considered search types by both methods): SEARCH_WHAT_YOU_TYPED SEARCH_HISTORY SEARCH_SUGGEST SEARCH_SUGGEST_ENTITY SEARCH_OTHER_ENGINE ---- Types that will no longer be treated as a query: NAVSUGGEST This is the primary motivation for the proposed change. ---- Types that are not being treated as a query, will be with the proposed change: SEARCH_SUGGEST_TAIL SEARCH_SUGGEST_PERSONALIZED These two currently are not sent to iOS. I plan on adding implementations for them soon, however, and would have had to add them anyway. CALCULATOR This is also improved by the proposed change. Currently the answer (e.g. for the text "2/5", the answer is "0.4") is shown as black text and again as blue text on the second line, as if it was a URL. By treating this type as a search type, only a single line of black text is shown, consistent with other platforms. VOICE_SUGGEST This is only used on Android. If we were to add this feature, it would obviously make sense to treat it as a search type. SEARCH_SUGGEST_PROFILE This represents a Google+ profile. I believe it's only in use (if at all) on Android. At least, Android has the only UI code that accounts for it. Android treats it the same way it does entity suggest, suggesting that it acts like an enhanced search query. Which leads me to believe that if one happened to show up on iOS somehow, we'd be better off treating it as a query.
,
Apr 27 2017
Proposed change sent out for review: https://codereview.chromium.org/2852553002/
,
Apr 28 2017
drive-by: thanks for noticing and fixing this. Android had similar issues years ago until we got it to share more code with the C++ side. Duplicating logic is trouble.
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97567bc01417a53c3157e0fe0fb3e8c4ce714db1 commit 97567bc01417a53c3157e0fe0fb3e8c4ce714db1 Author: jdonnelly <jdonnelly@chromium.org> Date: Mon May 01 15:50:55 2017 Remove the use of custom logic to determine search suggestion types. iOS has a custom method for determining which omnibox suggestion types are search/query types (as opposed to URL types). This change removes it in favor of the cross-platform method used on other platforms. This has useful effects on how we display navsuggest and calculator suggestions and eliminates the need to maintain separate logic. See the bug for a detailed description of the motivation for this change and an analysis of its effects. BUG= 716047 Review-Url: https://codereview.chromium.org/2852553002 Cr-Commit-Position: refs/heads/master@{#468317} [modify] https://crrev.com/97567bc01417a53c3157e0fe0fb3e8c4ce714db1/ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm
,
May 1 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by jdonnelly@chromium.org
, Apr 27 201741.1 KB
41.1 KB View Download