Closed Bug 988275 Opened 7 years ago Closed 7 years ago

Choosing the same language in Translation Infobar will attempt to translate

Categories

(Firefox :: Translation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: bogdan_maris, Assigned: mano)

References

Details

(Whiteboard: [translation] p=1 s=it-32c-31a-30b.3 [qa!])

Attachments

(2 files, 2 obsolete files)

Reproducible on try build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/florian@queze.net-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
Also, choosing the same source and target languages (eg. English for both) shouldn't attempt to retranslate.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: [translation] → [translation] p=1
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: [translation] p=1 → [translation] p=1 s=it-32c-31a-30b.1 [qa?]
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8417900 - Flags: review?(florian)
Also, without a promise or a callback, it's somewhat tricky to test this - we practically wait for something not to happen.
(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.
(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.
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.
(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.
Whiteboard: [translation] p=1 s=it-32c-31a-30b.1 [qa?] → [translation] p=1 s=it-32c-31a-30b.1 [qa+]
Mass move of translation bugs to the new Translation component.
Component: General → Translation
Version: 31 Branch → unspecified
Attached patch patch + test (obsolete) — Splinter Review
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.
Attachment #8417900 - Attachment is obsolete: true
Attachment #8417900 - Flags: review?(florian)
Attachment #8420849 - Flags: review?(florian)
Whiteboard: [translation] p=1 s=it-32c-31a-30b.1 [qa+] → [translation] p=1 s=it-32c-31a-30b.2 [qa+]
(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 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.
Attachment #8420849 - Flags: review?(florian) → review-
I must say it'd be nice to be notified about bug 101386.
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").
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 }
    );
I discussed these issues with Florian on IRC. We agreed to figure them in bug 1015933. For now, this will land without tests.
Attached patch patchSplinter Review
Attachment #8420849 - Attachment is obsolete: true
Attachment #8428666 - Flags: review?(florian)
Attachment #8428666 - Flags: review?(florian) → review+
Whiteboard: [translation] p=1 s=it-32c-31a-30b.2 [qa+] → [translation] p=1 s=it-32c-31a-30b.3 [qa+]
QA Contact: bogdan.maris
https://hg.mozilla.org/mozilla-central/rev/8e085a50318c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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.
Status: RESOLVED → VERIFIED
Whiteboard: [translation] p=1 s=it-32c-31a-30b.3 [qa+] → [translation] p=1 s=it-32c-31a-30b.3 [qa!]
You need to log in before you can comment on or make changes to this bug.