Search bar icon should indicate when a page offers open search

VERIFIED FIXED in Firefox 36

Status

()

Firefox
Search
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: phlsa, Assigned: bwinton)

Tracking

(Depends on: 1 bug)

36 Branch
Firefox 37
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox35 wontfix, firefox36 verified, firefox37 verified)

Details

Attachments

(7 attachments, 7 obsolete attachments)

1000 bytes, image/png
Details
2.21 KB, image/png
Details
425 bytes, image/png
Details
888 bytes, image/png
Details
330.36 KB, image/png
Details
21.98 KB, patch
bwinton
: review+
bwinton
: ui-review+
Details | Diff | Splinter Review
2.82 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
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...

Comment 3

3 years ago
bug 430627 shows what was removed (and what you need to re-add)
Mentor: dao@mozilla.com
Whiteboard: [good first bug][lang=js]

Updated

3 years ago
See Also: → bug 430627
Hardware: x86 → All
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

3 years ago
(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?)

Comment 7

3 years ago
(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.
Mentor: dao@mozilla.com
Whiteboard: [good first bug][lang=js]

Comment 9

3 years ago
(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..:)
(Assignee)

Comment 11

3 years ago
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.  ;)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Created attachment 8537320 [details]
search-indicator-badge-add.png
Created attachment 8537326 [details]
badge-add-search.png
(Assignee)

Comment 17

3 years ago
Created attachment 8537346 [details] [diff] [review]
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!
Attachment #8537346 - Flags: review?(MattN+bmo)
Do you have the answers for comment 8 (since phlsa is away)?
Flags: needinfo?(bwinton)
(Assignee)

Comment 19

3 years ago
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.  :)
Flags: needinfo?(bwinton) → needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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.
Attachment #8537346 - Flags: review?(MattN+bmo) → feedback+
Created attachment 8538107 [details]
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

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
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 ;-).
(Assignee)

Comment 26

3 years ago
Created attachment 8538704 [details] [diff] [review]
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 ;).
Attachment #8537346 - Attachment is obsolete: true
Attachment #8538704 - Flags: ui-review?(shorlander)
Attachment #8538704 - Flags: review?(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.
Attachment #8538704 - Flags: review?(florian) → feedback+
(Assignee)

Comment 28

3 years ago
(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.
Attachment #8538704 - Flags: ui-review?(shorlander) → ui-review+
(Assignee)

Comment 30

3 years ago
(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.
(Assignee)

Comment 32

3 years ago
How do you feel about http://diff.pastebin.mozilla.org/8075689 as a comment?
Flags: needinfo?(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.
Flags: needinfo?(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.
(Assignee)

Comment 35

3 years ago
Created attachment 8539479 [details] [diff] [review]
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!
Attachment #8538704 - Attachment is obsolete: true
Attachment #8539479 - Flags: ui-review+
Attachment #8539479 - Flags: review?(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.
Attachment #8539479 - Flags: review?(florian) → review+
(Assignee)

Comment 37

3 years ago
Created attachment 8539495 [details] [diff] [review]
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.
Attachment #8539479 - Attachment is obsolete: true
Attachment #8539495 - Flags: ui-review+
Attachment #8539495 - Flags: review+
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.
Attachment #8539495 - Flags: approval-mozilla-beta?
Attachment #8539495 - Flags: approval-mozilla-aurora?

Comment 41

3 years ago
Shouldn't the patch also provide tests for the indicator ?
QA Whiteboard: #A9B1B8
(Assignee)

Comment 42

3 years ago
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?
Attachment #8539495 - Attachment is obsolete: true
Attachment #8539495 - Flags: approval-mozilla-beta?
Attachment #8539495 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(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);

Updated

3 years ago
QA Whiteboard: #A9B1B8
Comment on attachment 8539589 [details] [diff] [review]
potential fix for the leak

Makes sense to me.
Attachment #8539589 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Created attachment 8539675 [details] [diff] [review]
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!
Attachment #8539589 - Attachment is obsolete: true
Attachment #8539675 - Flags: review?(gijskruitbosch+bugs)
Attachment #8539675 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Created attachment 8539787 [details] [diff] [review]
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
Attachment #8539675 - Attachment is obsolete: true
Attachment #8539787 - Flags: review?(gijskruitbosch+bugs)
Attachment #8539787 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d0e7ca03cbbe
https://hg.mozilla.org/mozilla-central/rev/886798ce1e0c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8539566 - Attachment is obsolete: true
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].
Attachment #8539495 - Attachment is obsolete: false
Attachment #8539495 - Flags: approval-mozilla-beta?
Attachment #8539495 - Flags: approval-mozilla-aurora?
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8539495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8539495 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

3 years ago
Flags: firefox-backlog+
Depends on: 1116009
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
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
status-firefox37: fixed → verified
QA Contact: petruta.rasa
(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?
Flags: needinfo?(philipp)
Depends on: 1119273
(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 :)
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.