805325 - On about:home, searchEngineLogo alt-text is the string "undefined"

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Daniel Holbert [:dholbert]]

When I visit about:home, the Google logo to the left of the search-box sometimes doesn't render right away, and we show the alt text.

The alt text is "undefined".  This text is a bit unsettling to see on your home-page.

You can also see this if you visit about:home, right-click the google logo, pick "Inspect Element", and click the "markup" button to show the underlying HTML version of the DOM tree.  It looks like this:
<img xmlns="http://www.w3.org/1999/xhtml" id="searchEngineLogo" src="data:image/png;base64,[snipping out the giant data URI]" alt="undefined" />

Comment 1

[Daniel Holbert [:dholbert]]

The place where we set logoElt.alt is here:

> 141     let logoElt = document.getElementById("searchEngineLogo");
> 142     logoElt.src = searchEngineInfo.image;
> [...]
> 147     logoElt.alt = searchEngineInfo.name;
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js#141

...so searchEngineInfo.name is probably undefined.

Comment 2

[Daniel Holbert [:dholbert]]

Yeah, it's defined at the top of this file:

>  7 const SEARCH_ENGINES = {
>  8   "Google": {
>  9     image: "[data URI]",
[...]
> 71   , params: "source=hp&channel=np"
> 72   }

No "name" field there, hence searchEngineInfo.name is undefined.

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js#7

Comment 3

[Daniel Holbert [:dholbert]]

I think we just want to use "searchEngineName" (the index that we use into SEARCH_ENGINES -- "Google" as defined on line 8, quoted in comment 2).

Comment 4

[Daniel Holbert [:dholbert]]

(Looks like this was a regression from bug 749477, which added this use of ".name" on an entry from SEARCH_ENGINES:
 https://hg.mozilla.org/mozilla-central/rev/08c90c0d3f55#l1.89
whereas previously we were using ".name" on something else)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 6

[Daniel Holbert [:dholbert]]

(Confirmed locally that this fixes the alt text, w/ Inspect Element, and also by clearing image cache, reloading, and watching the alt text flash before the image loads.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix w/ test

Here's the fix again, with a testcase added to browser_aboutHome.js.

Verified locally that the testcase's final check[1] fails without the fix, and passes after I apply the fix.

[1] isnot(altText, "undefined", ...)

Comment 8

[Daniel Holbert [:dholbert]]

Verified that Firefox 17 is unaffected. (using Inspect Element on about:home)

So, this is a new bug in Firefox 18, which is currently in Aurora channel. Once this lands on trunk, I think we should take this on Aurora as well -- it's a low-risk fix, and we might as well avoid shipping this bug to our Beta & Release users.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 674984 [details] [diff] [review]
fix w/ test

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

OOps, thank you!

::: browser/base/content/test/browser_aboutHome.js
@@ +78,5 @@
> +
> +    let searchEngineLogoElt = doc.getElementById("searchEngineLogo");
> +    ok(searchEngineLogoElt, "Found search engine logo");
> +
> +    var altText = searchEngineLogoElt.alt;

nit: let

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix w/ test & review-comment addressed [r=mak]

Fixed that & landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a46d4906eee8

I'll request Aurora approval once this has cycled on tbpl.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 675110 [details] [diff] [review]
fix w/ test & review-comment addressed [r=mak]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 749477

User impact if declined: Incorrect/broken alt text ("undefined") on search-engine logo <img> element, on about:home 

Testing completed (on m-c, etc.): Verified the fix locally, landed on m-c (and patch includes automated test to verify that it's fixed there)

Risk to taking this patch (and alternatives if risky):  Negligible.

String or UUID changes made by this patch: None.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a46d4906eee8

Comment 13

[Scoobidiver (away)]

Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/563d482eada9

Comment 14

[Mihai Morar, (:MihaiMorar)]

Verified fixed for FF 18.b4 Windows 7 x63 and Ubuntu x86, using Inspect Element on about:home.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0(20121212073002)
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0(20121212073002)