1106432 - Search bar icon should indicate when a page offers open search

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

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

When a web page offers open search, the magnifying glass icon in the search bar should indicate that using an overlay icon (a.k.a. the green plus sign)

Comment 1

[Dão Gottwald [:dao]]

We used to have a graphical indicator but it was decided to remove it in Firefox 4 or so...

Comment 2

[Dão Gottwald [:dao]]

bug 430627

Comment 3

[Tim Nguyen :ntim]

bug 430627 shows what was removed (and what you need to re-add)

Comment 4

[Shubham Jindal [:shubham]]

I am new here and want to contribute to bugs. Can you please explain what the bug actually means? I didn't get the term "a web page offers open search" and how should I start?

Comment 5

[Tim Nguyen :ntim]

(In reply to Shubham Jindal from comment #4)
> I am new here and want to contribute to bugs. Can you please explain what
> the bug actually means? I didn't get the term "a web page offers open
> search" and how should I start?

When you go to [0] for example, when clicking the search dropdown, Firefox will offer you to add the search engine. We also want in this case to add an indicator to the search dropdown. We'll want to use the icon here : [1]. 

We removed the indicator in bug 430627. And we want to reintroduce it. So for this bug, you'll have to reintroduce the indicator code (see bug 430627). In case you don't have the setup done yet. You can check [2] for instructions. 

[0] : http://www.wordreference.com/enfr/welcome
[1] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/search/badge-add-engine.png
[2] : http://codefirefox.com

Comment 6

[Shubham Jindal [:shubham]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Shubham Jindal from comment #4)
> > I am new here and want to contribute to bugs. Can you please explain what
> > the bug actually means? I didn't get the term "a web page offers open
> > search" and how should I start?
> 
> When you go to [0] for example, when clicking the search dropdown, Firefox
> will offer you to add the search engine. We also want in this case to add an
> indicator to the search dropdown. We'll want to use the icon here : [1]. 
> 
> We removed the indicator in bug 430627. And we want to reintroduce it. So
> for this bug, you'll have to reintroduce the indicator code (see bug
> 430627). In case you don't have the setup done yet. You can check [2] for
> instructions. 
> 
> [0] : http://www.wordreference.com/enfr/welcome
> [1] :
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/search/
> badge-add-engine.png
> [2] : http://codefirefox.com

I opened up [0] in Firefox in MacOSX, but when I click on the search dropdown, Firefox doesn't ask me to add the search engine. (Search dropdown is the "maginifier" button in [0] website right?)

Comment 7

[Tim Nguyen :ntim]

(In reply to Shubham Jindal from comment #6)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > (In reply to Shubham Jindal from comment #4)
> > > I am new here and want to contribute to bugs. Can you please explain what
> > > the bug actually means? I didn't get the term "a web page offers open
> > > search" and how should I start?
> > 
> > When you go to [0] for example, when clicking the search dropdown, Firefox
> > will offer you to add the search engine. We also want in this case to add an
> > indicator to the search dropdown. We'll want to use the icon here : [1]. 
> > 
> > We removed the indicator in bug 430627. And we want to reintroduce it. So
> > for this bug, you'll have to reintroduce the indicator code (see bug
> > 430627). In case you don't have the setup done yet. You can check [2] for
> > instructions. 
> > 
> > [0] : http://www.wordreference.com/enfr/welcome
> > [1] :
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/search/
> > badge-add-engine.png
> > [2] : http://codefirefox.com
> 
> I opened up [0] in Firefox in MacOSX, but when I click on the search
> dropdown, Firefox doesn't ask me to add the search engine. (Search dropdown
> is the "maginifier" button in [0] website right?)

No, it should appear in the browser UI (the searchbox next to the urlbar), not the website.

Comment 8

[Dão Gottwald [:dao]]

Why did you make me the mentor for this? This bug is not ready to be worked on. We need to re-evaluated if we really want to add back an indicator, given that the reasons for why the old indicator was removed seem still valid today.

Comment 9

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [:dao] from comment #8)
> Why did you make me the mentor for this? This bug is not ready to be worked
> on. We need to re-evaluated if we really want to add back an indicator,
> given that the reasons for why the old indicator was removed seem still
> valid today.

Ah sorry :(

Shubham, if you want an easy bug that is not being re-evaluated, you can take bug 1043346

Comment 10

[Shubham Jindal [:shubham]]

I will look at bug 1043346 then..:)

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Since I have patches up and waiting for review on my other two bugs, I'm going to try tackling this one, too…  :)

(But this is the last of them, I promise.  ;)

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: search-indicator-badge-add.png

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 14

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 15

[Stephen Horlander [:shorlander]]

Attachment: badge-add-search.png

Comment 16

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1106432.diff

Okay, here's the first cut at the patch.  It's mostly a reversion of the patch for bug 430627, but I've put the "addengines" attribute on the searchBar itself, because we may not be showing the button.

Lemme know what you think!

Comment 18

[Matthew N. [:MattN]]

Do you have the answers for comment 8 (since phlsa is away)?

Comment 19

[Blake Winton (:bwinton) (:☕️)]

The "Do we really want to add back an indicator?" question?  Since Philipp asked me to take this bug on, I can only assume that the answer is "Yes", but will needinfo him in the hopes that he wants to elaborate further.  :)

Comment 20

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton) from comment #19)
> The "Do we really want to add back an indicator?" question?  Since Philipp
> asked me to take this bug on, I can only assume that the answer is "Yes",
> but will needinfo him in the hopes that he wants to elaborate further.  :)

Our assumption is that with the one-click buttons, the non-default providers will become much more useful, and so it makes sense to put some emphasis on open search discovery again.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8537346 [details] [diff] [review]
bug1106432.diff

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

This implements only half of what's expected. I'll attach a mockup soon showing where the other icons should be used.

::: browser/themes/osx/searchbar.css
@@ +62,5 @@
>    -moz-image-region: rect(0, 20px, 20px, 0);
>    margin: 0 -3px;
>  }
>  
> +searchbar[addengines="true"] .searchbar-search-button {

If possible we should use xbl:inherits for the addengines attribute to avoid this expensive CSS selector.

Comment 22

[Florian Quèze [:florian]]

Attachment: mockup from shorlander

This is not the final mockup (I can't find it right now), but it shows the intended usage of the badge-add-search.png icon.

Comment 23

[Tim Nguyen :ntim]

(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to Blake Winton (:bwinton) from comment #19)
> > The "Do we really want to add back an indicator?" question?  Since Philipp
> > asked me to take this bug on, I can only assume that the answer is "Yes",
> > but will needinfo him in the hopes that he wants to elaborate further.  :)
> 
> Our assumption is that with the one-click buttons, the non-default providers
> will become much more useful, and so it makes sense to put some emphasis on
> open search discovery again.

The problems that were pointed out with the old indicator were :
- It's unnecessarily distracting, I agree quite much with this
- It's simply not necessary, since if the user wants to add the engine, he'll just open the dropdown and look for "Add [...]".
- The previous indicator didn't have much meaning (it was a blue glow), though, this reason is no longer relevant with the new indicator.

Comment 24

[Blake Winton (:bwinton) (:☕️)]

A patch is on its way, but in the meantime, here's what it looks like on OS X Retina:
https://dl.dropboxusercontent.com/u/2301433/Screenshots/SearchEngineAddition.png

Florian, did you want to take the upcoming review request, since you're digging in this code anyways?  :)

Comment 25

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton) from comment #24)
> A patch is on its way, but in the meantime, here's what it looks like on OS
> X Retina:
> https://dl.dropboxusercontent.com/u/2301433/Screenshots/SearchEngineAddition.
> png

:-)
 
> Florian, did you want to take the upcoming review request, since you're
> digging in this code anyways?  :)

I can take it, yes. I assume/hope MattN won't mind ;-).

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first version of the patch.

Okay, let's see how this does…
It seems to work fine on OS X Retina, but I admit to not having tested it on Windows or Linux (yet.  I'll be working on that next, but figured we could get the reviews while I wait ;).

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8538704 [details] [diff] [review]
The first version of the patch.

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

The addengine-icon binding seems more complicated than it needs to be; the rest looks good.

::: browser/components/search/content/search.xml
@@ +558,5 @@
>      </handlers>
>    </binding>
>  
> +  <binding id="addengine-icon" extends="chrome://global/content/bindings/general.xml#image">
> +    <implementation implements="nsIObserver">

I don't think this binding needs any code (ie. "<implementation>" section).

I expected something looking like this:

    <content>
      <xul:stack>
        <xul:image class=... xbl:inherits="src=image"/>
        <xul:image class="addengine-badge"/>
      </xul:stack>
    </content>

I think the <xul:stack> element can even be avoided if you add "display: -moz-stack;" next to the -moz-binding CSS rule.

@@ +567,5 @@
> +        replacement.setAttribute("class", "addengine-icon");
> +        var badge = document.createElementNS(kXULNS, "image");
> +        badge.setAttribute("class", "addengine-badge");
> +        badge.setAttribute("top", "-7");
> +        badge.setAttribute("right", "-9");

These two lines look like something that wants to be in the CSS file.

Comment 28

[Blake Winton (:bwinton) (:☕️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> ::: browser/components/search/content/search.xml
> @@ +558,5 @@
> > +  <binding id="addengine-icon" extends="chrome://global/content/bindings/general.xml#image">
> > +    <implementation implements="nsIObserver">
> I don't think this binding needs any code (ie. "<implementation>" section).
> I expected something looking like this:
>     <content>
>       <xul:stack>
>         <xul:image class=... xbl:inherits="src=image"/>
>         <xul:image class="addengine-badge"/>
>       </xul:stack>
>     </content>

I tried that, but it didn't work, but I'll give it another go, in case it was something else that was failing…

> @@ +567,5 @@
> > +        replacement.setAttribute("class", "addengine-icon");
> > +        var badge = document.createElementNS(kXULNS, "image");
> > +        badge.setAttribute("class", "addengine-badge");
> > +        badge.setAttribute("top", "-7");
> > +        badge.setAttribute("right", "-9");
> These two lines look like something that wants to be in the CSS file.

I agree they do look that way, but they're setting the top and right attributes for use by the stack element (because it's not absolutely positioned), so I think they actually belong here (or in the XUL if that works).  ;)

Comment 29

[Stephen Horlander [:shorlander]]

Comment on attachment 8538704 [details] [diff] [review]
The first version of the patch.

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

Looks good based on the screenshot.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(Update: Still didn't work with a <content> element.  I sent a patch to Florian, to see if he could figure out why it's failing.  If he can't, I'm hoping he'll change his f+ to an r+.  ;)

Comment 31

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton) from comment #30)
> (Update: Still didn't work with a <content> element.  I sent a patch to
> Florian, to see if he could figure out why it's failing.  If he can't, I'm
> hoping he'll change his f+ to an r+.  ;)

If we have to go with the approach in the exiting patch, I'll want it to include comments explaining why it's done that way, as I'm sure that would surprise anybody reading that code.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

How do you feel about http://diff.pastebin.mozilla.org/8075689 as a comment?

Comment 33

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton) from comment #30)
> Update: Still didn't work with a <content> element.  I sent a patch to
> Florian, to see if he could figure out why it's failing.

Replace extends="chrome://global/content/bindings/general.xml#image" with extends="xul:box" and your patch works.

Comment 34

[Florian Quèze [:florian]]

Comment on attachment 8538704 [details] [diff] [review]
The first version of the patch.

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

::: browser/themes/linux/searchbar.css
@@ +197,5 @@
>    -moz-box-pack: start;
>  }
>  
> +.addengine-item > .button-box > .button-icon {
> +  -moz-binding: url("chrome://browser/content/search/search.xml#addengine-icon");

-moz-binding: and display: rules usually shouldn't be in the skin/ css.

I think the right place for this would be http://hg.mozilla.org/mozilla-central/annotate/021b09e92d30/browser/base/content/browser.css#l442

And by the way, the new binding you created is used by urlbarBindings.xml#browser-search-autocomplete-result-popup (that's the binding for the panel of the new UI), not by something from search.xml (that's where we have the binding of the searchbar toolbar item). So I think the new binding should live in urlbarBindings.xml, I would put it right after browser-search-autocomplete-result-popup

@@ +209,5 @@
>    background-color: Highlight;
>    color: HighlightText;
>  }
>  
> +.addengine-icon > .button-icon {

This can be simplified by putting the addengine-icon class directly on the image tag.

Comment 35

[Blake Winton (:bwinton) (:☕️)]

Attachment: The second version of the patch.

(Carrying forward ui-r=shorlander.)

(In reply to Florian Quèze [:florian] [:flo] from comment #33)
> Replace extends="chrome://global/content/bindings/general.xml#image" with
> extends="xul:box" and your patch works.

Fixed!

(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> ::: browser/themes/linux/searchbar.css
> @@ +197,5 @@
> > +.addengine-item > .button-box > .button-icon {
> > +  -moz-binding: url("chrome://browser/content/search/search.xml#addengine-icon");
> -moz-binding: and display: rules usually shouldn't be in the skin/ css.
> I think the right place for this would be
> http://hg.mozilla.org/mozilla-central/annotate/021b09e92d30/browser/base/
> content/browser.css#l442

Moved!

> the new binding should live in urlbarBindings.xml, I would put it right
> after browser-search-autocomplete-result-popup

Moved!

> @@ +209,5 @@
> > +.addengine-icon > .button-icon {
> This can be simplified by putting the addengine-icon class directly on the
> image tag.

Simplified!  :)

Thanks for the comments!

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8539479 [details] [diff] [review]
The second version of the patch.

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

Thanks for the updated patch, looks good now :-).

::: browser/base/content/browser.css
@@ +447,5 @@
>  searchbar[oneoffui] {
>    -moz-binding: url("chrome://browser/content/search/search.xml#searchbar-flare") !important;
>  }
>  
>  #PopupAutoCompleteRichResult {

This isn't related to the new search UI (and possibly not related to search at all). Move the new rule to before this one? I think I would actually prefer that you move it to just after #PopupSearchAutoComplete.

@@ +451,5 @@
>  #PopupAutoCompleteRichResult {
>    -moz-binding: url("chrome://browser/content/urlbarBindings.xml#urlbar-rich-result-popup");
>  }
>  
> +.addengine-item > .button-box > .button-icon {

I think it would be nice to add a comment above this selector saying something like:
/* Badge overlay above the icon of open search add buttons in the search panel. */
maybe phrased in a more readable way :).

::: browser/base/content/urlbarBindings.xml
@@ +1233,5 @@
>        ]]></handler>
>      </handlers>
>    </binding>
>  
> +  <binding id="addengine-icon" extends="xul:box">

I think a comment above this line explaining the intended usage of this binding would be appreciated too.

Comment 37

[Blake Winton (:bwinton) (:☕️)]

Attachment: The hopefully final version of the patch.

(Carrying forward ui-r=shorlander and r=florian.)

(In reply to Florian Quèze [:florian] [:flo] from comment #36)
> ::: browser/base/content/browser.css
> @@ +447,5 @@
> >  searchbar[oneoffui] {
> >    -moz-binding: url("chrome://browser/content/search/search.xml#searchbar-flare") !important;
> >  }
> >  #PopupAutoCompleteRichResult {
> 
> This isn't related to the new search UI (and possibly not related to search
> at all). Move the new rule to before this one? I think I would actually
> prefer that you move it to just after #PopupSearchAutoComplete.

Moved.

> @@ +451,5 @@
> > +.addengine-item > .button-box > .button-icon {
> I think it would be nice to add a comment above this selector saying
> something like:
> /* Badge overlay above the icon of open search add buttons in the search
> panel. */
> maybe phrased in a more readable way :).

Added

> ::: browser/base/content/urlbarBindings.xml
> @@ +1233,5 @@
> > +  <binding id="addengine-icon" extends="xul:box">
> I think a comment above this line explaining the intended usage of this
> binding would be appreciated too.

Added.

Comment 38

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/ded76f782c5e

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8539495 [details] [diff] [review]
The hopefully final version of the patch.

Approval Request Comment
[Feature/regressing bug #]: bug 1088660 made open search providers much more useful, but they are barely discoverable. This patch re-adds an indicator that UX really wants, and barely missed 34 (I was implementing it before running out of time when we got the go to land the new search UI).
[User impact if declined]: Open search providers are hardly discoverable.
[Describe test coverage new/current, TBPL]: QA will verify.
[Risks and why]: I can't say there's no risk here, but the patch is actually less scary than it looks; all the JS code added here is actually code that has been part of the browser for a while and removed for Firefox 4 in bug 430627. The rest of the patch is just theming and/or self contained.
[String/UUID change made/needed]: none.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for leaks.
https://hg.mozilla.org/integration/fx-team/rev/ae788545a13c

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1501345&repo=fx-team

Comment 41

[Tim Nguyen :ntim]

Shouldn't the patch also provide tests for the indicator ?

Comment 42

[Blake Winton (:bwinton) (:☕️)]

Attachment: A less leaky version of the patch.

Try run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8ec0ffb4cb9e

Gijs, this seemed to leak when there was no toolbar and we called document.getElementById("searchbar").  Is it reasonable to assume that if there's no "nav-bar", there won't be any toolbars, and so we can skip this method?

Comment 43

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #42)
> Created attachment 8539566 [details] [diff] [review]
> A less leaky version of the patch.
> 
> Try run at
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8ec0ffb4cb9e
> 
> Gijs, this seemed to leak when there was no toolbar and we called
> document.getElementById("searchbar").  Is it reasonable to assume that if
> there's no "nav-bar", there won't be any toolbars, and so we can skip this
> method?

We're making the menu button visible in those windows, and the search bar could be in the menu panel.

It's a bit of an edgecase, but in any case, ideally the solution here should be ensuring that just doing document.getElementById("searchbar") doesn't leak... I'm a little surprised that it does, tbh... did you figure out why?

Comment 44

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #43)

> ideally the solution here should
> be ensuring that just doing document.getElementById("searchbar") doesn't
> leak... I'm a little surprised that it does, tbh... did you figure out why?

I'm still debugging it, and the more I look into it the more surprising it gets.

Just doing document.getElementById("searchbar") triggers a call of the searchbar constructor... but without calling the constructors of the searchbar's anonymous nodes (I would guess that's because these nodes are only constructed if the searchbar is actually displayed).

More specifically, the textbox that's part of the searchbox doesn't get constructed at that time. But when the searchbar then gets destructed when the window is closed, the searchbar's destructor is called, and that code attempts to break an hypothetical reference cycle with the textbox at:
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#122
122         // Make sure to break the cycle from _textbox to us. Otherwise we leak
123         // the world. But make sure it's actually pointing to us.
124         if (this._textbox.mController.input == this)
125           this._textbox.mController.input = null;

Commenting out these lines is enough to make the leak disappear when running the specific tests that leaked on fx-team. Just replacing these lines with "this._textbox;" (or with document.getAnonymousElementByAttribute(this, "anonid", "searchbar-textbox"); directly) is enough to make the test leak again.

Comment 45

[Florian Quèze [:florian]]

When the searchbar-textbox constructor is called from the searchbar's destructor, we leak because the searchbar-textbox's constructor adds an observer on a pref.

The leak also disappears if I comment out these lines:
http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#781
781         // Add observer for suggest preference
782         var prefs = Components.classes["@mozilla.org/preferences-service;1"]
783                             .getService(Components.interfaces.nsIPrefBranch);
784         prefs.addObserver("browser.search.suggest.enabled", this, false);

Comment 46

[Florian Quèze [:florian]]

Attachment: potential fix for the leak

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8cffdcc84b8d
https://tbpl.mozilla.org/?tree=Try&rev=8cffdcc84b8d

Comment 47

[:Gijs (he/him)]

Comment on attachment 8539589 [details] [diff] [review]
potential fix for the leak

Makes sense to me.

Comment 48

[Florian Quèze [:florian]]

Attachment: Leak fix

Tests were green on try with the previous attachment. I'm attaching the same fix with a comment added so that we remember why we added this check.

(In reply to :Gijs Kruitbosch from comment #47)
> Comment on attachment 8539589 [details] [diff] [review]
> potential fix for the leak
> 
> Makes sense to me.

Thanks!

Comment 49

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/4dbb2ddd0093
https://hg.mozilla.org/integration/fx-team/rev/513130bd59ed

Comment 50

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/6eeda8160b58 for https://treeherder.mozilla.org/logviewer.html#?job_id=1511539&repo=fx-team - looks like a11y was depending on something you changed.

Comment 51

[Florian Quèze [:florian]]

Attachment: Leak fix v2

Replaced "BrowserSearch.searchBar" with "document.getBindingParent(this)" so that it doesn't break accessibility tests.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19add214497a
https://tbpl.mozilla.org/?tree=Try&rev=19add214497a

Comment 52

[Florian Quèze [:florian]]

Third attempt...

https://hg.mozilla.org/integration/fx-team/rev/d0e7ca03cbbe
https://hg.mozilla.org/integration/fx-team/rev/886798ce1e0c

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0e7ca03cbbe
https://hg.mozilla.org/mozilla-central/rev/886798ce1e0c

Comment 54

[Florian Quèze [:florian]]

Comment on attachment 8539495 [details] [diff] [review]
The hopefully final version of the patch.

Adding back the approval requests now that the patch actually reached mozilla-central.

Approval Request Comment
[Feature/regressing bug #]: bug 1088660 made open search providers much more useful, but they are barely discoverable. This patch re-adds an indicator that UX really wants, and barely missed 34 (I was implementing it before running out of time when we got the go to land the new search UI).
[User impact if declined]: Open search providers are hardly discoverable.
[Describe test coverage new/current, TBPL]: QA will verify.
[Risks and why]: I can't say there's no risk here, but the patch is actually less scary than it looks; all the JS code added here is actually code that has been part of the browser for a while and removed for Firefox 4 in bug 430627. The rest of the patch is just theming and/or self contained.
[String/UUID change made/needed]: none.

Note: this approval request is for the 2 changesets that landed on central, ie. this attachment + attachment 8539787 [details] [diff] [review].

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8539495 [details] [diff] [review]
The hopefully final version of the patch.

Given the shaky landings & backouts, breaking ally (even if tests pass now) this looks like too much churn to take this late in Beta.  We weren't tracking this in any case, seems like a nice-to-have but not a hard requirement to ship in 35. Let's wait and let this have a full Beta cycle with 36.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0105aa80d95
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3c348e88817

Comment 57

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Developer Edition 36.0a2 and Nightly 37.0a1 2014-01-06 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

Notes: - the green plus sign icon is displayed on the magnifying glass when the search dropdown is shown - difference from shorlander mockup https://bugzilla.mozilla.org/attachment.cgi?id=8538107 where it is only visible with the "Add search engine" button
- Having nothing entered in search toolbar and pressing the "Add search engine" button will results in opening the search drop-down showing the search suggestions and the newly added search icon between the one-click search engines

Comment 58

[Florian Quèze [:florian]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #57)

> - Having nothing entered in search toolbar and pressing the "Add search
> engine" button will results in opening the search drop-down showing the
> search suggestions and the newly added search icon between the one-click
> search engines

Is this a bug we should file, or something that doesn't matter?

Comment 59

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

(In reply to Florian Quèze [:florian] [:flo] from comment #58)
> (In reply to Petruta Rasa [QA] [:petruta] from comment #57)
> 
> > - Having nothing entered in search toolbar and pressing the "Add search
> > engine" button will results in opening the search drop-down showing the
> > search suggestions and the newly added search icon between the one-click
> > search engines
> 
> Is this a bug we should file, or something that doesn't matter?

I think this will mostly solve itself with bug 1126250 :)