Jesse Ruderman Reporter
	Description • 14 years ago

http://hg.mozilla.org/mozilla-central/file/291cea9d9fca/browser/base/content/aboutHome.js#l43

The middle of a tri-licensed file is not a great place for the Google logo.

	Reed Loden [:reed]
	Comment 1 • 14 years ago

same thing for Yandex's logo, too, no?

	Marco Bonardo [:mak]
	Comment 2 • 14 years ago

yes, yandex is the same, while there I could also fix the alt text on the logos in the meanwhile.

	Luis Villa [Outside Counsel; for non-law use luis@tieguy.org]
	Comment 3 • 14 years ago

Note that chromium apparently deals with the problem of licensing of search engine branding by just not shipping search engine logos/favicons at all and caching them from the web on first run.

Obviously resolving that would require writing code, not just moving a file around, and would have a (slight?) first-run penalty. But it would be more elegant from a licensing perspective.

	Mike Hommey [:glandium]
	Comment 4 • 14 years ago

So it looks like I got it completely wrong in bug 563723 comment 113 and that bug 563723 comment 66 was never addressed. But anyways, bug 563723 comment 113 contains something that could help here, I think. Why not use the searchplugins icons ? Sure, the licensing issue remains, but a) it also needs to be addressed in some way b) the searchplugins code has cache already (though it's not entirely suitable for icons downloading). It would also have an additional nice benefit in that it would allow the user to use in about:home just any search engine in the search box.

	Marco Bonardo [:mak]
	Comment 5 • 14 years ago

(In reply to comment #4)
> Why not use the searchplugins
> icons ?

because this is not an icon, but a full logo

> It would also have an additional nice
> benefit in that it would allow the user to use in about:home just any search
> engine in the search box.

That we don't know if we want to allow, as of now we don't want that.

	Marco Bonardo [:mak]
	Comment 6 • 14 years ago

As I see it, this should block b6 and be pushed before the string freeze if we want to fix alt texts.

	Marco Bonardo [:mak]
	Comment 7 • 14 years ago

string changes are removals.

	Marco Bonardo [:mak]
	Comment 8 • 14 years ago

Comment on attachment 471245 [details] [diff] [review]
patch v1.0

>diff --git a/browser/base/jar.mn b/browser/base/jar.mn
>--- a/browser/base/jar.mn
>+++ b/browser/base/jar.mn
>@@ -57,6 +57,8 @@ browser.jar:
> *       content/browser/web-panels.xul                (content/web-panels.xul)
> *       content/browser/baseMenuOverlay.xul           (content/baseMenuOverlay.xul)
> *       content/browser/nsContextMenu.js              (content/nsContextMenu.js)
>+        content/browser/search/google.png            (../../other-licenses/search/google.png)
>+        content/browser/search/yandex.png            (../../other-licenses/search/yandex.png)


Note to myself: fix spacing

	Marco Bonardo [:mak]
	Comment 9 • 14 years ago

note that we have new high-res logos but those will come later, with a redesign (I think shorlander is working on mockups), so for now I just used binary version of the data: uris.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 14 years ago

Comment on attachment 471245 [details] [diff] [review]
patch v1.0

>diff --git a/browser/base/content/aboutHome.js b/browser/base/content/aboutHome.js

>+  if (gSearchEngine.image) {
>+    let logoElt = document.getElementById("searchEngineLogo");
>+    logoElt.src = gSearchEngine.image;
>+    logoElt.alt = gSearchEngine.description;

Just use gSearchEngine.name (and omit the related change in nsBrowserContentHandler).

This same problem applies to our search engine description files... we should probably use the same approach to fix that. I'd put the logos you're moving here in other-licenses/branding/search, and we can put the other images there too (we don't currently support them pointing to chrome:// URIs, though, so fixing that will be a bit trickier).

	Mike Hommey [:glandium]
	Comment 11 • 14 years ago

(In reply to comment #10)
> This same problem applies to our search engine description files... we should
> probably use the same approach to fix that. I'd put the logos you're moving
> here in other-licenses/branding/search, and we can put the other images there
> too (we don't currently support them pointing to chrome:// URIs, though, so
> fixing that will be a bit trickier).

In that case, the files should be either in a subdirectory, or namespaced, because the search plugins will also have a google.png file, which will clash with the one from here.

That being said, I think this doesn't solve the licensing issue at all, since the distributed binaries are supposedly MPL. These files are not.

Other than that, you may not care, but it doesn't really help me either since I will still have to strip the files (as they are non-free material), and then modify all these chrome urls.

	Mike Hommey [:glandium]
	Comment 12 • 14 years ago

BTW, Kev Needham is working on the search engine icons licensing.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 13 • 14 years ago

The name of the file doesn't matter - we can name them anything.

The search service supports loading images remotely. It would be good to have about:home support that as well.

	Mike Hommey [:glandium]
	Comment 14 • 14 years ago

> The search service supports loading images remotely. It would be good to have
> about:home support that as well.

It most probably does, though caching might be an issue (though it would already cache through the standard browser cache)

	Marco Bonardo [:mak]
	Comment 15 • 14 years ago

This fetches the image on idle from the network, and stores it in our local store.
I had alreay to move the utils to a module for mozilla snippets, so this is just anticipating that.

The only "missing" part of this patch are the urls, I used working placeholders for now. If the patch is approved in this state, I'll ask for proper uris.

Now, while working on this I discovered that our local store is cleared by Clear Recent History, as any other local store, along with cookies. This is depressing, will have to file a blocking bug about that.

	Marco Bonardo [:mak]
	Comment 16 • 14 years ago

minor fix to module uri

	Marco Bonardo [:mak]
	Comment 17 • 14 years ago

Filed Bug 592990 regarding clear recent history issue.

	Dão Gottwald [:dao]
	Comment 18 • 14 years ago

Comment on attachment 471446 [details] [diff] [review]
patch v2.1

>--- a/browser/base/content/aboutHome.xhtml
>+++ b/browser/base/content/aboutHome.xhtml
>@@ -65,13 +65,12 @@
>   <body dir="&locale.dir;" onload="onLoad(event)">
>     <div id="pageContainer">
>       <div id="topSection">
>-        <img id="brandLogo" src="chrome://branding/content/icon128.png"
>-             title="&abouthome.brandLogo.title;"/>
>+        <img id="brandLogo" src="chrome://branding/content/icon128.png"/>

I think you want alt="" here, otherwise screen readers might read out the src.

	Marco Bonardo [:mak]
	Comment 19 • 14 years ago

(In reply to comment #18)
> I think you want alt="" here, otherwise screen readers might read out the src.

I thought screen readers were skipping images without a alt considering them part of the page layout

	Dão Gottwald [:dao]
	Comment 20 • 14 years ago

They probably don't do that, since authors forget setting alt on images with meaningful content.

	Marco Bonardo [:mak]
	Comment 21 • 14 years ago

(In reply to comment #20)
> They probably don't do that, since authors forget setting alt on images with
> meaningful content.

Well, Marco Zehe told me images without alt are usually skipped by the screen reader, but he also told that an alt on the logo would be appreciated to easy communication. So I'll figure out what is better to put there as description.

	Dão Gottwald [:dao]
	Comment 22 • 14 years ago

(In reply to comment #21)
> Well, Marco Zehe told me images without alt are usually skipped by the screen
> reader, but he also told that an alt on the logo would be appreciated to easy
> communication.

To communicate *what*?

	Marco Bonardo [:mak]
	Comment 23 • 14 years ago

(In reply to comment #22)
> To communicate *what*?

ehr, sorry, to easy communication between sighted and blind users... so for example if a blind user has to talk about this page, can say "below the Firefox image" and so on...

	Dão Gottwald [:dao]
	Comment 24 • 14 years ago

It's an implementation detail that this is an <img> in the first place. Since it doesn't contribute to the content, I'd argue that it would be put there with CSS ideally. (So if you'd pick View > Page Style > No Style, it would disappear.)

	Marco Bonardo [:mak]
	Comment 25 • 14 years ago

This will be redesigned shortly and I don't want to lose time fixing details, I'll just add the alt as Marco suggested since it's the less invasive change for the string freeze.

	Dão Gottwald [:dao]
	Comment 26 • 14 years ago

(In reply to comment #25)
> This will be redesigned shortly

Not really relevant, unless you're planning to replace "Minefield" in "Minefield Start" with the logo (in which case the alternative text shouldn't be "Minefield logo" but "Minefield"). As it stands, the logo doesn't add to the content -- the logical hierarchy is "Minefield Start - Google - [input] - Search" etc., not "Minefield Start - Minefield logo - Google" or something like this. The string isn't useful or going to be useful, so considering the string freeze, the correct step seems to be to remove it.

	Marco Bonardo [:mak]
	Comment 27 • 14 years ago

addresses Dao's feedback: use a pseudoelement instead of an image for brand logo

	Marco Bonardo [:mak]
	Comment 28 • 14 years ago

I'm going to split the strings part of this bug.

	Marco Bonardo [:mak]
	Comment 29 • 14 years ago

Comment on attachment 471523 [details] [diff] [review]
patch v2.2

clearing review request till we figure out what's the final shape we want for this bug.
Possibilities:
- put logos in other-licenses
- fetch logos from network.

the patch also need unbitrotting since other patches will land before this one (that is not yet blocking)

	Luis Villa [Outside Counsel; for non-law use luis@tieguy.org]
	Comment 30 • 14 years ago

Please choose whichever implementation makes the most engineering/design sense; ignore my earlier comment.

	Johnathan Nightingale [:johnath]
	Comment 31 • 14 years ago

Notes from the Grand Retriage: doesn't block, would take patch

	Marco Bonardo [:mak]
	Comment 32 • 14 years ago

not actively working on this.