1040725 - Enhance previous searches in awesomebar by appending a magnifying glass

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[:Paolo Amadini]

This is the second part of bug 1034381, based on the design in bug 1029849.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Do we really want to do this? Showing the search provider favicon seems potentially better.

Comment 2

[Madhava Enros [:madhava]]

There is some value to showing the favicon, it's true. Is there another place to show a maginfier? Between this and Bug 1047433, the intention is to make it clear that this row will kick off the search.

Comment 3

[Marco Bonardo [:mak]]

Attachment: screenshot

In case of keywords we append a magnifier to the title currently (though this will likely change with bug 951624), so looks like we were already showing the magnifier somewhere else.

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Marco, that looks like a good place for the magnifier.
Let's just make sure that we use the icons supplied by Michael in bug 1038239.

Comment 5

[Alex Bardas :alexbardas]

Attachment: autocomplete-magnifier_position.png

I've attached a screenshot with possible results for autocomplete.

The magnifying glass will replace "Google Search" from the first line of the first 2 results and will be also added for the 3rd result?

Comment 6

[Alex Bardas :alexbardas]

Also, can you provide more feedback about when the magnifying glass should not appear?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Alex: we should probably use the exact same styling as we use for search keywords (Marco's screenshot from comment 3). It should appear in all cases where _maybeRestyleSearchMatch ends up changing the styling. Paolo can probably give you more specific advice.

Comment 8

[Alex Bardas :alexbardas]

Attachment: [1]no-ellipsis.png

Comment 9

[Alex Bardas :alexbardas]

Attachment: [2]ellipsis-magn-glass.png

Comment 10

[Alex Bardas :alexbardas]

Attachment: [3]ellipsis-align.png

Comment 11

[Alex Bardas :alexbardas]

Gavin: I did a research about it today and found out that the styling for search keywords is broken. As you can see from the attached picture [1]no-ellipsis.png, if the text from awesomebar is longer than the width of the autocomplete result list, it will not be truncated (no ellipsis there) and the search icon will not appear anymore. I have a WIP for that ([2]ellipsis-magn-glass.png) but it seems it can be better detailed in a new bug.

I've tried to think of a better approach and to make use of `text-overflow: ellipsis` css property, but unfortunately the end dots from the ellipsis will no longer be perfectly aligned to the right ([3]ellipsis-align.png). As you can see, there is a 1-2px difference. This solution would have cleaned the code because there is some logic for showing the ellipsis for that in autocomplete.xml

What do you suggest? An implementation similar to the one from search keywords and a new bug about the broken / no ellipsis behavior?

Comment 12

[Andrei Vaida [:avaida]]

Assigning this to myself since I'm already working on multiple awesomebar bugs.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alex Bardas :alexbardas from comment #11)
> What do you suggest? An implementation similar to the one from search
> keywords and a new bug about the broken / no ellipsis behavior?

Yes, definitely want a new bug for the truncation/ellipsis issue.

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alex Bardas :alexbardas from comment #5)
> Created attachment 8476892 [details]
> autocomplete-magnifier_position.png
> 
> I've attached a screenshot with possible results for autocomplete.
> 
> The magnifying glass will replace "Google Search" from the first line of the
> first 2 results and will be also added for the 3rd result?

Generally, yes :) I'll just spell out everything here to avoid confusion:

First row: The magnifier should replace the »– Google Search« part.
Second row: Actually, does that case still exist? Shouldn't that look exactly like line 3?
Third row: The magnifier should be added at the end of the first line.

Comment 15

[Alex Bardas :alexbardas]

Attachment: linux_autocomplete.png

I'm using the icons from the new icon archive.

1. Should I replace the current icon for keyword with the icons from that archive?

2. When hovering an autocomplete result, the icon doesn't look great on linux anymore. Should it be replaced with another icon (e.g: to have thicker borders)?

I guess the icon position is good.

Comment 16

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass PATCH 1

As Gavin suggested, the results which are modified by _maybeRestyleSearchMatch() should have an extra magnifying glass after them. There is also a new set of icons which should be used. 

I guess that the current icons for keyword searches need to change for consistency reasons (Philipp will clarify this). There are some problems with hover states with them.

I will submit another bug for the edge case when the search result is too long and the icon is not visible anymore (this also affects the current functionality for keyword searches).

Comment 17

[Alex Bardas :alexbardas]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #13)
> (In reply to Alex Bardas :alexbardas from comment #11)
> > What do you suggest? An implementation similar to the one from search
> > keywords and a new bug about the broken / no ellipsis behavior?
> 
> Yes, definitely want a new bug for the truncation/ellipsis issue.

Bug 1040923 covers this case. It's basically a redesign for the search results in the autocomplete popup.

Comment 18

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

To be considered that it was decided (Gavin, Dolske) that Linux & Windows should not use Hidpi images right now.

Comment 19

[:Paolo Amadini]

Comment on attachment 8479682 [details] [diff] [review]
Enhance previous searches in awesomebar by adding a magnifying glass

This unfortunately breaks past search results styling.

To test with both a past search result and a keyword:
 - Search for "firefox download"
 - Enter a new bookmark "http://example.com/?q=%s" with keyword "fi"
 - Type "fi re" in the address bar

Also, according to bug 951624 comment 41, this should use an inverted version of the magnifying glass when the item is selected. Philipp, I don't think we have that asset yet, how should we proceed?

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alex Bardas :alexbardas from comment #15)
> Created attachment 8479362 [details]
> linux_autocomplete.png
> 
> I'm using the icons from the new icon archive.
> 
> 1. Should I replace the current icon for keyword with the icons from that
> archive?
Yes, that makes sense.

> 2. When hovering an autocomplete result, the icon doesn't look great on
> linux anymore. Should it be replaced with another icon (e.g: to have thicker
> borders)?
Good catch. The icon should turn white along with the text (on all platforms actually). If you don't have a white icon I can make one for you. Just point me to the archive from where you have the other icons.

> I guess the icon position is good.
Yes :)


One last thing: Is the colon at the end of the list entries in your screenshot part of the page title or do we add that? In the latter case, we shouldn't do it.

Comment 21

[Alex Bardas :alexbardas]

The icons are provided in bug 1038239 and I'll need the white versions. Thank you, Philipp.

Comment 22

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

Thank you for the first review Paolo. I've retested now and it seems ok, keyword search results are treated separately. 

I think it would be great to also have some mochitests for this. Is toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js the best place to write those tests?

Comment 23

[:Paolo Amadini]

(In reply to Alex Bardas :alexbardas from comment #22)
> I think it would be great to also have some mochitests for this. Is
> toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js the
> best place to write those tests?

This folder only contains xpcshell tests. I think there might be some related mochitest-browser-chrome tests in "browser/base/content/test/general", but I don't think we have a very extensive test coverage for the URL bar styling, so we'd need to add a new one. I suggest filing a separate bug!

Comment 24

[:Paolo Amadini]

Comment on attachment 8480170 [details] [diff] [review]
Enhance previous searches in awesomebar by adding a magnifying glass

Review of attachment 8480170 [details] [diff] [review]:
-----------------------------------------------------------------

This seems OK, but waiting for the white icon before final review.

::: toolkit/content/widgets/autocomplete.xml
@@ +1492,5 @@
>              type = "bookmark";
> +          // keyword, favicon type results for search engines and switchToTab results
> +          // have an extra magnifying glass icon after them
> +          } else if (type == "keyword" || (type == "favicon" && searchEngine) ||
> +              actionIndex >= 0) {

Hm, is the check for actionIndex now needed, or does the type="keyword" check match this case already?

Comment 25

[Alex Bardas :alexbardas]

(In reply to :Paolo Amadini from comment #24)
> 
> ::: toolkit/content/widgets/autocomplete.xml
> @@ +1492,5 @@
> >              type = "bookmark";
> > +          // keyword, favicon type results for search engines and switchToTab results
> > +          // have an extra magnifying glass icon after them
> > +          } else if (type == "keyword" || (type == "favicon" && searchEngine) ||
> > +              actionIndex >= 0) {
> 
> Hm, is the check for actionIndex now needed, or does the type="keyword"
> check match this case already?

We need that check there because we also want to show the magnifying glass for switch to tab case.

Comment 26

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Search_Icon_white.zip

Here's the icon package!

Comment 27

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

Added the inverted icons for all operating systems.

Comment 28

[Alex Bardas :alexbardas]

After discussing with Dao, we agreed that we probably need SVG icons to make sure that we will not have problems with inverted theme colors. For this case, icons with a black outline color may also work, but the next step would be to integrate the SVGs.

Comment 29

[Marco Bonardo [:mak]]

updating bug title to reflect reality, otherwise it's confusing.

Comment 30

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Search-icon.zip

Attached, SVG search icons.

Color: #808080

Comment 31

[Alex Bardas :alexbardas]

Attachment: search-icon-windows.png

Comment 32

[Alex Bardas :alexbardas]

Attachment: search-icon-linux.png

Comment 33

[Alex Bardas :alexbardas]

Attachment: search-icon-osx.png

Comment 34

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

Dao, if you remember our talk from the work week, you recommended to use svg icons because there was a problem when hovering an autocomplete result on windows 8. And probably on many other custom themes.

I've added the svg icons (please let me know what I do wrong because I couldn't target the path from css and use the fill property in css, I had to add 2 svgs for each os).

Michael recommended a color (#808080), but I know that you would prefer GrayText instead of it :).

I had to target windows aero like here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/autocomplete.css#108.

For convenience, I've attached 3 screenshots (search-icon-*.png) to be easier to follow the result of the patch.

How does this look to you?

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 8488507 [details] [diff] [review]
Enhance previous searches in awesomebar by adding a magnifying glass

>--- a/browser/themes/linux/browser.css
>+++ b/browser/themes/linux/browser.css

> .ac-result-type-keyword,
> .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage) {
>-  list-style-image: url(moz-icon://stock/gtk-find?size=menu);
>+  list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg);
>   width: 16px;
>   height: 16px;
> }
> 
>+.autocomplete-richlistitem[selected="true"] .ac-result-type-keyword {
>+  list-style-image: url(chrome://global/skin/icons/autocomplete-search-inverted.svg);
>+}

Can you inherit the 'selected' to the image node to avoid the descendent selector?

Also, it looks like you should similarly use autocomplete-search-inverted.svg for .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage, selected).

>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css

> .ac-result-type-keyword,
> .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage) {
>-  list-style-image: url(chrome://global/skin/icons/search-textbox.png);
>-  margin: 2px;
>-  width: 12px;
>-  height: 12px;
>+  list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg);
>+  width: 16px;
>+  height: 16px;
>+}

I hope changing the image dimensions from 12px to 16px doesn't affect the surrounding layout or if it does, it does so in an expected way?

>+@media (min-resolution: 2dppx) {
>+  .ac-result-type-keyword,
>+  .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage) {
>+    list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg);
>+  }
>+
>+  .autocomplete-richlistitem[selected="true"] .ac-result-type-keyword {
>+    list-style-image: url(chrome://global/skin/icons/autocomplete-search-inverted.svg);
>+  }
> }

This looks entirely redundant. What are you trying to do here?

>+              let paramsIndex = search.indexOf(' ');

nit: change ' to " while you're touching this :)

>--- /dev/null
>+++ b/toolkit/themes/windows/global/icons/autocomplete-search.svg
>@@ -0,0 +1,14 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
>+<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
>+	 viewBox="0 0 16 16" enable-background="new 0 0 16 16" xml:space="preserve">

Please remove some cruft here like the encoding attribute, the doctype, the version and the id attributes. You can use http://hg.mozilla.org/mozilla-central/annotate/59d4326311e0/browser/themes/windows/content-contextmenu.svg as a model.

>+<g>
>+	<g>
>+		<path fill="GrayText" fill-rule="evenodd" clip-rule="evenodd" d="M9.356,1.178c-3.014,0-5.458,2.45-5.458,5.472c0,1.086,0.32,2.096,0.864,2.947
>+			l-3.279,3.287c-0.396,0.397-0.396,1.041,0,1.438l0.202,0.202c0.396,0.397,1.039,0.397,1.435,0l3.275-3.283
>+			c0.854,0.554,1.869,0.88,2.962,0.88c3.014,0,5.458-2.45,5.458-5.471C14.814,3.627,12.371,1.178,9.356,1.178z M9.356,10.001
>+			c-1.847,0-3.344-1.501-3.344-3.352c0-1.851,1.497-3.352,3.344-3.352c1.846,0,3.344,1.501,3.344,3.352
>+			C12.7,8.501,11.203,10.001,9.356,10.001z"/>
>+	</g>
>+</g>
>+</svg>

Please use spaces instead of tabs.

Looks good otherwise.

Comment 36

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

Thank you for that link and review, I've completely refactored all svgs. I've tested it again on windows, linux & osx and it has the same functionality.

Regarding the change from 12px to 16px : I will also let Philipp take a look and see what he has to say. I'll push to try to create some builds pretty soon.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 8488894 [details] [diff] [review]
Enhance previous searches in awesomebar by adding a magnifying glass

>+.ac-result-type-keyword[selected="true"],
>+.autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage, selected) {
>+  list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg#search-icon-inverted);
>+}
>+
>+%ifdef WINDOWS_AERO
>+@media (-moz-windows-default-theme) {
>+  .ac-result-type-keyword[selected="true"],
>+  .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage, selected) {
>+    list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg#search-icon);
>+  }
>+}
>+%endif

This has the same effect as the above rules:

%ifdef WINDOWS_AERO
@media not all and (-moz-windows-default-theme) {
%endif
  .ac-result-type-keyword[selected="true"],
  .autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage, selected) {
    list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg#search-icon-inverted);
  }
%ifdef WINDOWS_AERO
}
%endif

Comment 38

[Tim Nguyen :ntim]

Comment on attachment 8488894 [details] [diff] [review]
Enhance previous searches in awesomebar by adding a magnifying glass

Review of attachment 8488894 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/linux/global/jar.mn
@@ +38,5 @@
>  +  skin/classic/global/console/console.css                     (console/console.css)
>  +  skin/classic/global/console/console.png                     (console/console.png)
>  +  skin/classic/global/console/console-toolbar.png             (console/console-toolbar.png)
>  +  skin/classic/global/dirListing/remote.png                   (dirListing/remote.png)
> ++  skin/classic/global/icons/autocomplete-search.svg           (icons/autocomplete-search.svg)

Not sure if the + hack works.

::: toolkit/themes/osx/global/jar.mn
@@ +91,5 @@
>    skin/classic/global/dirListing/local.png                           (dirListing/folder.png)
>    skin/classic/global/dirListing/remote.png                          (dirListing/remote.png)
>    skin/classic/global/dirListing/up.png                              (dirListing/up.png)
>    skin/classic/global/icons/autocomplete-dropmarker.png              (icons/autocomplete-dropmarker.png)
> +  skin/classic/global/icons/autocomplete-search.svg                  (icons/autocomplete-search.svg)

This file requires to be preprocessed until bug 1051117 is fixed.

::: toolkit/themes/windows/global/jar.mn
@@ +98,5 @@
>          skin/classic/global/dirListing/remote.png                (dirListing/remote.png)
>          skin/classic/global/dirListing/up.png                    (dirListing/up.png)
>          skin/classic/global/Filepicker.png                       (filepicker/Filepicker.png)
>          skin/classic/global/icons/autoscroll.png                 (icons/autoscroll.png)
> +        skin/classic/global/icons/autocomplete-search.svg        (icons/autocomplete-search.svg)

Same here.

@@ +296,5 @@
>          skin/classic/aero/global/dirListing/remote.png                   (dirListing/remote-aero.png)
>          skin/classic/aero/global/dirListing/up.png                       (dirListing/up-aero.png)
>          skin/classic/aero/global/Filepicker.png                          (filepicker/Filepicker.png)
>          skin/classic/aero/global/icons/autoscroll.png                    (icons/autoscroll-aero.png)
> +        skin/classic/aero/global/icons/autocomplete-search.svg           (icons/autocomplete-search.svg)

Same here.

Comment 39

[Alex Bardas :alexbardas]

(In reply to Tim Nguyen [:ntim] from comment #38)
> Comment on attachment 8488894 [details] [diff] [review]
> Enhance previous searches in awesomebar by adding a magnifying glass
> 
> Review of attachment 8488894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/linux/global/jar.mn
> @@ +38,5 @@
> >  +  skin/classic/global/console/console.css                     (console/console.css)
> >  +  skin/classic/global/console/console.png                     (console/console.png)
> >  +  skin/classic/global/console/console-toolbar.png             (console/console-toolbar.png)
> >  +  skin/classic/global/dirListing/remote.png                   (dirListing/remote.png)
> > ++  skin/classic/global/icons/autocomplete-search.svg           (icons/autocomplete-search.svg)
> 
> Not sure if the + hack works.
> 

I'm not sure I follow. I tested and the svgs are added & rendered correctly. Is there something else I should test?

Comment 40

[Alex Bardas :alexbardas]

I'm looking at http://dxr.mozilla.org/mozilla-central/source/build/docs/jar-manifests.rst . Do I need to add an '*' instead of '+'? Don't really know what I'm doing wrong when adding these svgs.

Comment 41

[Alex Bardas :alexbardas]

Try looks good:

https://tbpl.mozilla.org/?tree=Try&rev=8a68763a2cee

Dao mentioned the possibility of adding a regression only on OS X, because the search icon for keyword searches had a 12x12 size and a margin of 2 px. Now, the search icon on OS X is 16x16. The new icon can also be observed in the attached pictures.

Philipp, can you confirm that the current behavior is the desired one? You can also use this firefox image to test on os x: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8a68763a2cee/try-macosx64/firefox-35.0a1.en-US.mac.dmg

Comment 42

[Alex Bardas :alexbardas]

Attachment: Enhance previous searches in awesomebar by adding a magnifying glass

I had to rebase it on top of bug 1065265. The only change was that I had to use type == "search favicon", which should keep the same behavior.

Comment 43

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alex Bardas :alexbardas from comment #41)
> Try looks good:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8a68763a2cee
> 
> Dao mentioned the possibility of adding a regression only on OS X, because
> the search icon for keyword searches had a 12x12 size and a margin of 2 px.
> Now, the search icon on OS X is 16x16. The new icon can also be observed in
> the attached pictures.
> 
> Philipp, can you confirm that the current behavior is the desired one? You
> can also use this firefox image to test on os x: 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/abardas@mozilla.
> com-8a68763a2cee/try-macosx64/firefox-35.0a1.en-US.mac.dmg

Looks good to me on OS X!

Comment 44

[Alex Bardas :alexbardas]

I can address the issue from comment 38 (if there is any) in a follow up bug because Tom is currently blocked by this bug.

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/219a0112bcb4

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/219a0112bcb4

Comment 47

[Tim Nguyen :ntim]

Now that this has landed, I see no actual search icon showing up in the awesomebar in Nightly. But the autocomplete-search.svg file is accessible via the urlbar.

Let me post a screen.

Comment 48

[Tim Nguyen :ntim]

Attachment: Screenshot of latest Nightly

Comment 49

[Andrei Vaida [:avaida]]

(In reply to Tim Nguyen [:ntim] from comment #47)
> Now that this has landed, I see no actual search icon showing up in the
> awesomebar in Nightly. But the autocomplete-search.svg file is accessible
> via the urlbar.
> 
> Let me post a screen.
The same thing on my end, the magnifying glass is not visible on Nightly 35.0a1 (2014-09-21) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 LTS 32-bit.

Alex, any thoughts on this?

Comment 50

[Alex Bardas :alexbardas]

(In reply to Andrei Vaida, QA [:avaida] from comment #49)
> (In reply to Tim Nguyen [:ntim] from comment #47)
> > Now that this has landed, I see no actual search icon showing up in the
> > awesomebar in Nightly. But the autocomplete-search.svg file is accessible
> > via the urlbar.
> > 
> > Let me post a screen.
> The same thing on my end, the magnifying glass is not visible on Nightly
> 35.0a1 (2014-09-21) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04
> LTS 32-bit.
> 
> Alex, any thoughts on this?

It is related to bug 1065303 which broke something here (that bug just landed), but it can easily be fixed. I'll file a follow up bug to solve it.

Comment 51

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 35.0a1 (2014-10-01), based on the testing performed for Bug 1071164.