If suggestion has favicon, match it in the omnibox |
||||
Issue descriptionIf a suggestion has favicon, match it in the omnibox.
,
Jun 27 2018
,
Jun 28 2018
emilyschechter@, just to confirm, this is just for favicons, and not other icons? For example, If I bookmark a website that has no favicon (e.g. example.com), the omnibox suggests will display a star next to that result, but the omnibox top should still show the globe it shows today?
,
Jun 28 2018
Interesting... it seems reasonable to me to match whatever the icon is, including bookmark. Are there any other cases like that you can think of? cc'ing Joel in case he has an opinion.
,
Jun 28 2018
emilyschechter@ The search queries? For example, in Tommy's screenshot above, the 4th suggestion is a google search for the term 'facebook. In current stable, both the suggestion and the omnibox show a magnifying glass. In current material design, the suggestion shows the chrome icon (as in the screenshot) and the omnibox shows the search provider icon (e.g. `G` for google) or defaults to the chrome icon if the search provider isn't google. I feel like this is this intentional / correct behavior, but want to make sure.
,
Jun 28 2018
clarification on above comment: current material design will use the generic search icon (which looks like a circle around a dot in the suggestions, but will display the search provider's icon in the omnibox. If the search provider has no icon provided, then the omnibox too will default to the circle & dot icon. Should the suggestions also use the search provider's icon for search suggestions (and thereby match the omnibox icon) or should they remain different?
,
Jun 29 2018
Good question but yes, we deliberately choose to not use the search provider's favicon for search suggestions. We might revisit that later, though. Agreed that it would make sense to mirror the bookmark icon in the omnibox, if that's not especially difficult.
,
Jul 3
I think we should limit this to having Refresh enabled as wells as having favicons enabled. My reasoning being that we only show search engine favicons in Refresh and we should do the same for pages.
,
Jul 3
Double checking, For the website favicon: Both the omnibox and the popup will check both flags (refresh & favicon). (Currently, the omnibox never shows a favicon; the popup checks only the favicon flag) For the search provider icon: The omnibox will check the refresh flag. The popup will always show the magnifying glass icon. (Currently, same behavior) For the bookmark star icon Both the omnibox and popup will show the bookmark icon regardless of either flag. (Currently, the omnibox never shows the bookmark icon regardless of either flag; the popup always shows the bookmark icon regardless of either flag)
,
Jul 6
#9: everything there is correct with one exception. The omnibox should only show the bookmark icon if the refresh flag is enabled. Noted just for the record here, manukh already made this adjustment in his WIP CL: https://crrev.com/c/1124671.
,
Jul 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6291a1f17a1f81dc42efe96e5db0decf04dd4d13 commit 6291a1f17a1f81dc42efe96e5db0decf04dd4d13 Author: manuk <manukh@chromium.org> Date: Mon Jul 16 20:12:15 2018 Show favicon and bookmark star in omnibox. When the user activates the omnibox popup, the popup suggestions have favicons and bookmark stars when appropriate. Selecting one (either because one is selected by default or by using the arrow or tab keys), displays the suggested match in the omnibox textfield. Previously, when the selected suggestion had a favicon or bookmark star icon, the omnibox would display the url or search icons. There is still a discpreency between omnibox and omnibox popup suggestion icons; with material refresh active, search suggestion icons will display the default search icon (magnifying glass) whereas the omnibox will display the default search provider's icon if available (`G` for Google). Bug: 856755 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5874d72d0bde711fb3870a1ab0fd1f6e5c699eb8 Reviewed-on: https://chromium-review.googlesource.com/1124671 Commit-Queue: manuk hovanesian <manukh@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#575400} [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/BUILD.gn [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_edit_model.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_edit_model.h [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_edit_model_unittest.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_view.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_view.h [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/omnibox_view_unittest.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_client.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_client.h [add] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_edit_model.cc [add] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_edit_model.h [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_view.cc [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/omnibox/browser/test_omnibox_view.h [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/components/toolbar/toolbar_model.h [modify] https://crrev.com/6291a1f17a1f81dc42efe96e5db0decf04dd4d13/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm
,
Jul 17
Verified in Canary. Assigning to emilyschechter to confirm that it's working as intended.
,
Sep 6
Suggestion: If omnibox is in edit mode and suggestion box is not activated (i.e. click Omnibox twice), perhaps we shouldn't show the favicon? Also, if you edit the Omnibox to a previously visited site, say google.com, and you un-focus the Omnibox by clicking on the page, then the favicon is still visible (Google favicon), but the lock icon disappears. It feels weird to have these behaviours: - If no changes were made to the Omnibox in edit mode, unfocusing Omnibox hides the favicon and restores the lock icon (assuming current site is secure) - If changes were made to the Omnibox in edit mode, unfocusing Omnibox does *not* hide the favicon, and lock icon is gone I think the lock icon should be shown in place of the favicon if the Omnibox is visible but the suggestion dialog is not. |
||||
►
Sign in to add a comment |
||||
Comment 1 by tommycli@chromium.org
, Jun 26 2018Owner: manukh@chromium.org
52.8 KB
52.8 KB View Download