1106043 - Search icon doesn't show for some open search providers that had an icon on Firefox 33

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

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

translate.google.com has an open search provider that also provides a logo. However, after adding it to the one-off searches, only the default placeholder icon shows up.

I've had the same issue with dict.cc, though that mysteriously resolved itself after a few reloads.

Comment 1

[Florian Quèze [:florian]]

Same (user-visible) issue with fr.wiktionary.org

It's possible the root cause is not the same for all of these websites, but we should investigate, and maybe file more bugs if there are different problems in our implementation.

Comment 2

[Daniel Veditz [:dveditz]]

The ones I've added to a new profile (search providers don't sync) have all gotten the default magnifying glass icon. Ones I know had icons in the old UI and don't now:
  http://stackoverflow.com
  http://www.wolfram-alpha.com

I was starting to think the feature was simply busted, but I do get the expected icon when I add BMO.

Comment 3

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

http://www.wolframalpha.com/searchDescription.xml has no <Image> specified. http://www.wolframalpha.com/'s favicon (used as a fallback) is over our threshold for "too large", so we also reject it (bug 361923).

http://fr.wiktionary.org has an <Image> pointing to http://bits.wikimedia.org/favicon/piece.ico, but that icon is also too large. Similarly, the favicon fallback is also too large.

Both of these behave the same in Firefox 33 and Firefox 34 for me.

I get different behavior for http://stackoverflow.com/ and https://translate.google.com/ in Firefox 33 vs. Firefox 34, though. I think both of them rely on our favicon fallback - http://stackoverflow.com/opensearch.xml and https://translate.google.com/opensearch.xml?hl=en both have relative icon URIs, which I don't think we handle.

The new code and old code both pass "target.getAttribute("src")" as the iconURI to addEngine():

https://hg.mozilla.org/mozilla-central/annotate/44344099d119/browser/components/search/content/search.xml#l544
https://hg.mozilla.org/mozilla-central/annotate/44344099d119/browser/base/content/urlbarBindings.xml#l1228

I think the issue is that for the new code (urlbarBindings.xml), that attribute is not the icon URI.

The old code sets the menuitem's src attribute to the icon URI here, via setIcon:
https://hg.mozilla.org/mozilla-central/annotate/44344099d119/browser/components/search/content/search.xml#l390
and that is inherited to the menuitem icon via xbl inheritance:
https://hg.mozilla.org/mozilla-central/annotate/44344099d119/toolkit/content/widgets/menu.xml#l211

The new code sets the button's "image" attribute directly:
https://hg.mozilla.org/mozilla-central/annotate/44344099d119/browser/base/content/urlbarBindings.xml#l1099

So the fix is probably to just change this line:
https://hg.mozilla.org/mozilla-central/annotate/44344099d119/browser/base/content/urlbarBindings.xml#l1240

to use "getAttribute("image")".

We should add a test!

Comment 4

[Sylvestre Ledru [:Sylvestre]]

I am going to untrack it. It seems it has been scheduled for 37.

Comment 5

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

We should fix this in 36, given that it's a one-line fix.

Comment 6

[Florian Quèze [:florian]]

There are different issues here:
- some providers don't have an icon in 34 and later, but had one before (http://stackoverflow.com, translate.google.com, ...). I'm rephrasing the bug summary here to cover only this, which is a regression we should fix asap and uplift the fix.
- the threshold to dismiss large icons should likely be increased, covered bybug 361923)
- we should handle relative icon paths; filed bug 1125793 to cover this.
- we need tests for open search handling in the new search UI, covered by bug 1120642.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch as described in comment 3

I verified locally that this works as expected.

Comment 8

[Florian Quèze [:florian]]

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

Comment 9

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/9870579b8ea9

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8554500 [details] [diff] [review]
Patch as described in comment 3

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1088660
[User impact if declined]: some search providers won't have icons after they are installed
[Describe test coverage new/current, TreeHerder]: will be verified by QA
[Risks and why]: low risk, trivial patch
[String/UUID change made/needed]: none.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f5ea68b9c6e2
https://hg.mozilla.org/releases/mozilla-beta/rev/f91cc6838063

Comment 12

[Petruta Horea [:phorea], Desktop QA]

Florian, I noticed that adding a search engine without icon in Firefox 35 and then opening Firefox 36 beta 7 (or 37 / 38) with the same profile doesn't update the icon.

Considering this, adding several search engines (that didn't had an icon in 35) in builds where the fix landed, should be enough to verify this bug if the icons are correctly displayed?

Comment 13

[Florian Quèze [:florian]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #12)
> Florian, I noticed that adding a search engine without icon in Firefox 35
> and then opening Firefox 36 beta 7 (or 37 / 38) with the same profile
> doesn't update the icon.

Makes sense, as the icon was lost at install-time, not at display-time.
 
> Considering this, adding several search engines (that didn't had an icon in
> 35) in builds where the fix landed, should be enough to verify this bug if
> the icons are correctly displayed?

Yes.

Comment 14

[Petruta Horea [:phorea], Desktop QA]

Thanks! :-)

I've verified using several search engines that the icons are properly added using Firefox 36 beta 7, latest Developer  Edition 37.0a2 and latest Nightly 38.0a1 2015-02-05 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.