Closed
Bug 988275
Opened 9 years ago
Closed 8 years ago
Choosing the same language in Translation Infobar will attempt to translate
Categories
(Firefox :: Translation, defect)
Firefox
Translation
Tracking
()
VERIFIED
FIXED
Firefox 32
People
(Reporter: bogdan_maris, Assigned: asaf)
References
Details
(Whiteboard: [translation] p=1 s=it-32c-31a-30b.3 [qa!])
Attachments
(2 files, 2 obsolete files)
353.30 KB,
image/gif
|
Details | |
860 bytes,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•9 years ago
|
||
Also, choosing the same source and target languages (eg. English for both) shouldn't attempt to retranslate.
Updated•9 years ago
|
Flags: firefox-backlog?
Updated•9 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Updated•8 years ago
|
Whiteboard: [translation] → [translation] p=1
Updated•8 years ago
|
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: [translation] p=1 → [translation] p=1 s=it-32c-31a-30b.1 [qa?]
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Also, without a promise or a callback, it's somewhat tricky to test this - we practically wait for something not to happen.
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
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•8 years ago
|
||
(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.
Updated•8 years ago
|
Whiteboard: [translation] p=1 s=it-32c-31a-30b.1 [qa?] → [translation] p=1 s=it-32c-31a-30b.1 [qa+]
Comment 8•8 years ago
|
||
Mass move of translation bugs to the new Translation component.
Component: General → Translation
Version: 31 Branch → unspecified
Assignee | ||
Comment 9•8 years ago
|
||
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)
Updated•8 years ago
|
Whiteboard: [translation] p=1 s=it-32c-31a-30b.1 [qa+] → [translation] p=1 s=it-32c-31a-30b.2 [qa+]
Assignee | ||
Comment 10•8 years ago
|
||
(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•8 years ago
|
||
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-
Assignee | ||
Comment 12•8 years ago
|
||
I must say it'd be nice to be notified about bug 101386.
Assignee | ||
Comment 13•8 years ago
|
||
Bug 1013861, that is.
Assignee | ||
Comment 14•8 years ago
|
||
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").
Assignee | ||
Comment 15•8 years ago
|
||
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 } );
Assignee | ||
Comment 16•8 years ago
|
||
I discussed these issues with Florian on IRC. We agreed to figure them in bug 1015933. For now, this will land without tests.
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8420849 -
Attachment is obsolete: true
Attachment #8428666 -
Flags: review?(florian)
Updated•8 years ago
|
Attachment #8428666 -
Flags: review?(florian) → review+
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8e085a50318c
Updated•8 years ago
|
Whiteboard: [translation] p=1 s=it-32c-31a-30b.2 [qa+] → [translation] p=1 s=it-32c-31a-30b.3 [qa+]
Reporter | ||
Updated•8 years ago
|
QA Contact: bogdan.maris
Comment 19•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e085a50318c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Reporter | ||
Comment 20•8 years ago
|
||
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.
Description
•