Closed Bug 1174468 Opened 5 years ago Closed 4 years ago

Firefox shows Microsoft logo in preferences with Yandex set as translation engine

Categories

(Firefox :: Translation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: soeren.hentzschel, Assigned: yarik.sheptykin)

References

Details

Attachments

(1 file, 2 obsolete files)

Yandex was implemented as alternative translation engine in bug 1129056. After changing the translation engine to Yandex the preferences still show the Microsoft logo.
Hi Sören! Thanks for reporting this issue! I am one of those who added yandex translation engine. I will be taking care of this bug.
Assignee: nobody → yarik.sheptykin
I think we can use here plain-text (like "Powered by Yandex") instead of looking for a logo, and just keep the logo a special case for Bing.
(In reply to :Felipe Gomes from comment #2)
> I think we can use here plain-text (like "Powered by Yandex") instead of
> looking for a logo, and just keep the logo a special case for Bing.

Hi, Felipe! Right, Yandex requires the following attribution in their terms of use:

"2.7. When using the Service the User shall include reference to the Yandex technology in the description of the software application, in the respective help topic, on the official website of the software application, as well as on all pages/screens where the data of the service “Yandex.Translate” is used, strictly over or under the results of translation, in format of the following text: “Powered by Yandex.Translate” with the clickable hyperlink to the page http://translate.yandex.com/. The font size of this reference shall not be less than the size of the main text font, and its color shall not differ from the color of the main text font." (https://legal.yandex.com/translate_api/)

To add this I am planning to start from translation-infobar.xul and wrap the contents of attribution menu item into xul:deck. I will then switch between appropriate attribution texts depending on the selected engine. According to my search (http://mxr.mozilla.org/mozilla-central/search?string=microsoft-translator-attribution.png) we also need to mention Yandex in the Preference window.
(In reply to Iaroslav Sheptykin from comment #3)
> To add this I am planning to start from translation-infobar.xul and wrap the
> contents of attribution menu item into xul:deck. I will then switch between
> appropriate attribution texts depending on the selected engine.

Sounds good!


> According to my search we also need to mention Yandex in the Preference window.

From that terms of use I think there's no reason to include it in the Preferences window, so I think you can just hide the Microsoft part in that case.
Status: NEW → ASSIGNED
Hi Guys!

With this patch and Yandex.Translate selected as the translation engine Firefox will 
1. hide Microsoft's attribution from the preference page
2. use "Powered by Yandex.Translate" in the Options list of translation infobar.
3. open http://translate.yandex.com/ when clicked on attribution.

The patch also includes a test that ensures the attribution is added. However, does not cover features 1 and 3. Let me know if this should be added.
Attachment #8624435 - Flags: review?(felipc)
Comment on attachment 8624435 [details] [diff] [review]
Showing Yandex.Translate attribution when Yandex engine is selected

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

::: browser/components/preferences/content.js
@@ +19,5 @@
>        let row = document.getElementById("translationBox");
>        row.removeAttribute("hidden");
> +      // Showing attribution only for Bing Translator.
> +      Components.utils.import("resource:///modules/translation/Translation.jsm");
> +      if(Translation.translationEngine == "bing") {

nit: needs a space between if and (

::: browser/components/preferences/in-content/content.js
@@ +24,5 @@
>        let row = document.getElementById("translationBox");
>        row.removeAttribute("hidden");
> +      // Showing attribution only for Bing Translator.
> +      Components.utils.import("resource:///modules/translation/Translation.jsm");
> +      if(Translation.translationEngine == "bing") {

same nit
Attachment #8624435 - Flags: review?(felipc) → review+
Hi guys! I updated the patch according to the feedback from Felipe (comment 6). How does it look now?
Attachment #8624435 - Attachment is obsolete: true
Attachment #8634642 - Flags: review?(florian)
Comment on attachment 8634642 [details] [diff] [review]
Showing Yandex.Translate attribution when Yandex engine is selected

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

Looks good, thanks! (Note: given Felipe's previous r+, my review was more a sanity check than a thorough review.)

::: browser/components/translation/test/browser_translation_yandex.js
@@ +65,5 @@
> +  gBrowser.removeTab(tab);
> +});
> +
> +
> +add_task(function* test_preference_attribution(){

nit: space between () and {
Attachment #8634642 - Flags: review?(florian) → review+
Hi Guys! 
I updated the patch and pushed it to the tryserver a couple of days ago.
Here is what happened: https://treeherder.mozilla.org/#/jobs?repo=try&revision=013f8256d199
I believe those failures are unrelated. What do you think Florian?
Attachment #8634642 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8636013 - Flags: review+
(In reply to Iaroslav Sheptykin from comment #9)

> I updated the patch and pushed it to the tryserver a couple of days ago.
> Here is what happened:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=013f8256d199
> I believe those failures are unrelated. What do you think Florian?

I also think it's unrelated.
Flags: needinfo?(florian)
Are we ready to check this patch in? Should I add [checkin-needed]?
Yes, this is ready to add the checkin-needed keyword (doing it for you now; feel free to do it next time :-)).
Keywords: checkin-needed
(In reply to Florian Quèze [:florian] [:flo] (PTO until August 3rd) from comment #12)
> Yes, this is ready to add the checkin-needed keyword (doing it for you now;
> feel free to do it next time :-)).

Thanks Florian! Next time I'll do it!
https://hg.mozilla.org/mozilla-central/rev/bfa0f07aeccb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1191306
You need to log in before you can comment on or make changes to this bug.