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)

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

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

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?

(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

(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?)

(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.

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.

(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

I will look at bug 1043346 then..:)

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. ;)

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!

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

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. :)

(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 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.

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.

(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.

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? :)

(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 ;-).

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 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.

(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 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.

(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+. ;)

(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.

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

(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 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.

(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 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.

(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 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.

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

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

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?

(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?

(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.

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);

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

Comment on attachment 8539589 [details] [diff] [review] potential fix for the leak Makes sense to me.

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!

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

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.

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

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

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

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 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.

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

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

(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?

(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 :)