988275 - Choosing the same language in Translation Infobar will attempt to translate

Status: VERIFIED FIXED

(Firefox :: Translation, defect)

Description

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Attachment: Screencast showing the issue

Reproducible on try build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d33b91e475d0/ (BuildID: 20140325093017)

Steps to reproduce:
1. Start Firefox.
2. Load https://www.mozilla.org/fr/contribute/.
3. Click 'Translate' on the Translate infobar.
- If 'There has been an error translating this page' message appears hit 'Try Again'
4. In the first dropdown field/or the second, select again the same language as the one it already is.

Expected results: No translation is attempted.

Actual results: Firefox attempts to translate again the webpage.


Notes:
1. Reproducible on Windows, Linux and Mac OS X

Comment 1

[Florian Quèze [:florian]]

Also, choosing the same source and target languages (eg. English for both) shouldn't attempt to retranslate.

Comment 2

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Attachment: patch

I think that onselect is better for menulists, particularly because onselect isn't called if a menuitem is reselected. However, onselect is called when the menu is built, so the fix to |translate| is necessary either way.

However, given that translate doesn't return a promise (or takes a callback), and assuming that's not desired, I believe that it'd be better to do away with |translate|, replacing it with "active" translateFrom and transaleTo setters.

Comment 3

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Also, without a promise or a callback, it's somewhat tricky to test this - we practically wait for something not to happen.

Comment 4

[Florian Quèze [:florian]]

(In reply to Mano from comment #2)

> However, onselect is called when
> the menu is built, so the fix to |translate| is necessary either way.

I don't see how the change to the |translate| method in the patch is discarding the calls while the menu is built.

Note: the case in comment 1 also needs to be handled.

Comment 5

[Florian Quèze [:florian]]

(In reply to Mano from comment #3)
> Also, without a promise or a callback, it's somewhat tricky to test this -
> we practically wait for something not to happen.

The event handler is synchronous, so you can just check that after setting the menu to the same value as before, the info bar is still in the TRANSLATED state rather than TRANSLATING.

Comment 6

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

As the menu builts, the only items that gets selected is the ones for the source and target language, so it shouldn't be a problem. However, I think I should just switch back to oncommand.

I will handle the case in comment 1, but I think that a better solution would be to hide the source language from the target's menu.

Comment 7

[Florian Quèze [:florian]]

(In reply to Mano from comment #6)

> I will handle the case in comment 1, but I think that a better solution
> would be to hide the source language from the target's menu.

The problem I can see with this solution is that if the source language is changed to what was the target, you would need to dynamically change the target to a new value to hide the previous target value.

Comment 8

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.

Comment 9

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Attachment: patch + test

I'm still not sure I fully understand the case in comment 1. Since the re-translations happens right after the selection change, it doesn't make sense that the selected language in one menulist would appear in the other one. Moreover, even if it did, what should happen if both are equal but the page is already translated?

Anyway, I did fix that in |transalte|, but I haven't tested that case. I think we should cover this case properly in a follow up.

Comment 10

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> (In reply to Mano from comment #6)
> 
> > I will handle the case in comment 1, but I think that a better solution
> > would be to hide the source language from the target's menu.
> 
> The problem I can see with this solution is that if the source language is
> changed to what was the target, you would need to dynamically change the
> target to a new value to hide the previous target value.

Or you'd have to change the target first.

Thinking about this further i think that the same-language option (source or target) should be disabled, not hidden. Again, I'd not consider it part of this bug (but I did fix translate not to allow that).

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8420849 [details] [diff] [review]
patch + test

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

While the test refactoring is nice and useful, I would have preferred if it was in a separate attachment or bug, as it is longer than the actual fix + test for this bug, and isn't directly related.

::: browser/components/translation/Translation.jsm
@@ +66,5 @@
>  
>    get doc() this.browser.contentDocument,
>  
>    translate: function(aFrom, aTo) {
> +    if ( aFrom == aTo  ||

Strange spacing here (2 spaces before '||'), and the rest of the code around here doesn't have a space after "if (".

@@ +78,5 @@
>      this.translatedFrom = aFrom;
>      this.translatedTo = aTo;
> +
> +    this.browser.ownerGlobal.setTimeout(() => {
> +      if (Math.random() > 0.3)

This is not meant to get checked in.

::: browser/components/translation/test/browser_translation_infobar.js
@@ +86,5 @@
>    is(!!PopupNotifications.getNotification("translated"), aExpectTranslated,
>       "translated icon " + (aExpectTranslated ? "" : "not ") + "shown");
>  }
>  
> +function checkOriginalOrTranslationButtons(aNotification,

The "Show Original" and "Show Translation" buttons are part of the "translated" state of the infobar, so I don't think it makes much sense to test them when they are hidden anyway. You could inline this function in test_translated.

@@ +91,5 @@
> +                                           aShowOriginalVisible = false,
> +                                           aShowTranslationVisible = false) {
> +  let showOriginal = aNotification._getAnonElt("showOriginal"),
> +      showTranslation = aNotification._getAnonElt("showTranslation");
> +  is(aNotification._getAnonElt("showOriginal").hidden,

I think you wanted to use the showOriginal and showTranslation variables defined above.

@@ +99,5 @@
> +     !aShowTranslationVisible,
> +     "show-translation-button-visible: " + aShowTranslationVisible);
> +}
> +
> +function test_offer(aNotification, aDetectedLangaue) {

spelling: aDetectedLangaue

@@ +117,5 @@
> +     "the infobar is in translating state");
> +  is(aNotification.translation.translatedFrom, aFrom,
> +     "The source language is set correctly");
> +  is(aNotification.translation.translatedTo, aTo,
> +     "The target language is set correctly");  

Nit: trailing whitespace.

@@ +123,5 @@
> +  // The icon is displayed in the translating state if a previous Translation
> +  // is shown.
> +  checkURLBarIcon(aOptions.previousTranslationShown);
> +  checkOriginalOrTranslationButtons(aNotification,
> +                                    !aOptions.showingOriginal,

I think this will cause a JS strict warning when aOptions is set to { previousTranslationShown: true }

@@ +155,4 @@
>  function run_tests(aFinishCallback) {
>    info("Show an info bar saying the current page is in French");
>    let notif = showTranslationUI("fr");
> +  let translationUI = notif.translation;

Is this variable used anywhere?

@@ +200,2 @@
>  
> +  info("Test that re-selecting the same target language does not cause re-translation");

I think you want to duplicate this test to check that re-selecting the source language does not cause re-translation.

@@ +201,5 @@
> +  info("Test that re-selecting the same target language does not cause re-translation");
> +  to.value = to.value;
> +  to.doCommand();
> +  test_translated(notif, "es", "pl");
> +  

Trailing whitespace.

Comment 12

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

I must say it'd be nice to be notified about bug 101386.

Comment 13

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Bug 1013861, that is.

Comment 14

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

So, I'm not sure what _exactly_ changed, but as of now this is somewhat untestsable, at least not in the test patched here. |transalte| is reimplemented in the test (in "fake translation ui"), meaning that translate from Translation.jsm is simply ignored. I could patch both functions (real and fake), but that's just silly: the test would pretty much test itself. The test has value not because it tests early returns and == operators usage, but rather because it makes sure that future revisions of the "translate" command don't re-introduce this bug. And since nothing enforces the implementations to remain the same, I see no point in patching this test as it is.

I'm saying that I'm not sure what changed, because I see that the fake-translate function was the from the first revision. However, I'm pretty sure that Traslation.translate was indeed called the last time I worked on this bug (and even more sure that the test passed with my changes. Now it doesn't, because the test's translate function isn't "fixed").

Comment 15

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

It's possible however to remove the duplicated function, I think. Functions from TranslationStub are simply set (and thus, override) on the real translationUI object. The only difference in the test's version (that's overriding the real function) is the lack of the following lines at the end:

    this.browser.messageManager.sendAsyncMessage(
      "Translation:TranslateDocument",
      { from: aFrom, to: aTo }
    );

Comment 16

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

I discussed these issues with Florian on IRC. We agreed to figure them in bug 1015933. For now, this will land without tests.

Comment 17

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Attachment: patch

Comment 18

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

https://hg.mozilla.org/integration/fx-team/rev/8e085a50318c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e085a50318c

Comment 20

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Verified as fixed on Mac OS X 10.9.2, Ubuntu 13.04 64bit and Windows 8.1 64bit using Nightly build with revision 8e085a50318c.