592112 - Search engine logos should be in other-licenses/

Status: NEW

(Firefox :: General, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Reed Loden [:reed]]

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

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Luis Villa [Outside Counsel; for non-law use [e-mail]]]

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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

string changes are removals.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

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

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

Comment 11

[Mike Hommey [:glandium]]

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

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

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

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.

Comment 14

[Mike Hommey [:glandium]]

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

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2.0

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.

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v2.1

minor fix to module uri

Comment 17

[Marco Bonardo [:mak]]

Filed Bug 592990 regarding clear recent history issue.

Comment 18

[Dão Gottwald [:dao]]

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.

Comment 19

[Marco Bonardo [:mak]]

(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

Comment 20

[Dão Gottwald [:dao]]

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

Comment 21

[Marco Bonardo [:mak]]

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

Comment 22

[Dão Gottwald [:dao]]

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

Comment 23

[Marco Bonardo [:mak]]

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

Comment 24

[Dão Gottwald [:dao]]

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

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[Dão Gottwald [:dao]]

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

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v2.2

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

Comment 28

[Marco Bonardo [:mak]]

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

Comment 29

[Marco Bonardo [:mak]]

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)

Comment 30

[Luis Villa [Outside Counsel; for non-law use [e-mail]]]

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

Comment 31

[Johnathan Nightingale [:johnath]]

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

Comment 32

[Marco Bonardo [:mak]]

not actively working on this.