1015527 - Back/Forward navigation shouldn't break the Translation UI

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

The Translation feature should behave well with Back/Forward navigation. At a minimum it should be able to re-do language detection. Ideally it can keep the translated/original data and infobar state if the page was bfcache'd. Perfect UX can be left for another bug, this bug is to cover an acceptable behavior.

Comment 1

[Mike de Boer [:mikedeboer]]

I'd also <3 to see if the tree is GC'd on page switch and if so, whether using a WeakMap would make a difference.

Comment 2

[Asaf Romano (gone)]

It's GCed when it goes away from bfcache (some pages are never bf-cached). You can use WeakMap regradless, of course.

Comment 3

[Florian Quèze [:florian]]

Attachment: WIP

This patch fixes the bug, but I'm wondering:
- Should I change the code sending the LanguageDetection:Result message to send Translation:ShowUI instead, so that we can remove the handling of the LanguageDetection:Result message completely? (My guess is yes, but I wanted to ask/discuss it before making the patch larger.)
- Could/should we use the pageshow event instead of nsIWebProgressListener.STATE_STOP's notification? (I really don't know, but I think it would be safer to investigate that as part of bug 997818.)
- What's a reasonable way to test this? (or should I even attempt to test it, given Mano is working on bug 1015933?)

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8434969 [details] [diff] [review]
WIP

(feedback given on irc)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8434969 [details] [diff] [review]
WIP

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

::: browser/components/translation/Translation.jsm
@@ +39,5 @@
>        aBrowser.translationUI.showTranslationUI(aDetectedLanguage);
>      }
> +  },
> +
> +  showUI: function(aBrowser, aUI) {

please note the new structure of the languageDetected function after bug 978158 landed, I think you'll need to make this function more similar to it too.

Details in bug 978158 comment 31.

Comment 6

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes from comment #4)
> Comment on attachment 8434969 [details] [diff] [review]
> WIP
> 
> (feedback given on irc)

For future reference, the discussion we had on IRC:

00:06:05 - felipe: so, why introduce a new message ("Translation:ShowUI"), instead of reusing the LanguageDetected msg?
00:06:46 - flo-retina: because "LanguageDetected" is really not an appropriate name to show some UI saying "the page has been translated from bar to foo"
00:07:47 - flo-retina: like I said in my comment, we can kill the LanguageDetected notification, as what it contains is a subset of what the ShowUI message can contain. But that makes the patch larger (and hopefully the code cleaner).
00:08:21 - felipe: I like that idea, but do you think it's possible to avoid introducing it to browser.js, and keep it in Translation.jsm?
00:08:51 - flo-retina: if I remove "LanguageDetected", that doesn't change the size of the translation code included in browser.js
00:09:15 - flo-retina: the reason why we have that code in browser.js is that Translation.jsm is loaded lazily only if we ever need to show some translation UI.
00:09:37 - felipe: ok right, it would be just a "renaming" them
00:09:39 - flo-retina: we did that to save one compartment for users without language detection enabled
00:09:51 - flo-retina: a renaming + a change of what the parameters contain.
00:09:56 - felipe: yep
00:10:00 - felipe: that direction sounds good to me
00:10:50 - felipe: but I'd still like to avoid calling the message "UI".   Maybe  "LanguageInformation", "LanguageState" or something, that will say (what's the original language, + possibly what's the translated language if it was translated)
00:11:18 - felipe: but anyway, it's just a name.. and I'm in favor of doing what you proposed in the bug!
00:12:13 - flo-retina: it's not just the language, it's also the information about the translation state
00:12:32 - flo-retina: but Translation:TranslationState is a kinda redundant name
00:12:46 - felipe: Translation:DocumentState ?
00:12:56 - flo-retina: hmm, that can work
00:14:46 - felipe: if language was detected, but no translation performed
00:14:55 - felipe: and there's back/fwd navigation
00:15:04 - felipe: your patch is still necessary to re-offer the infobar, right?
00:15:08 - felipe: or does that already work?
00:15:34 - flo-retina: I think that works since we changed from DOMContentLoaded to NETWORK_STOP
00:15:53 - flo-retina: I actually had to make the code in STATE_STOP return early if we had a cached language detection result
00:17:06 - felipe: oh so it just don't show up again because the new language is usually the defaultTargetLanguage
00:17:36 - felipe: in the translated case
00:17:54 - flo-retina: exactly
00:19:03 - felipe: ok
00:19:37 - felipe: and I agree that let's leave the pageshow/hide investigation for another bug
[...]
00:20:31 - flo-retina: I was wondering if that could not be an easy solution to the problem of running language detection before the page is done loading sub resources
[...]
00:22:30 - felipe: the problem is that moving to pageshow/hide might invalidate all the tp5 measurements we made, so I'm not really super excited about doing that change :)
00:24:27 - flo-retina: any fix for "Bug 997818 - Infobar is not displayed as long as the throbber is still present in the tab" would invalidate these measurements
00:27:32 - flo-retina: felipe: do you have an opinion re: tests for 1015527?
00:36:12 - felipe: flo-retina: re: tests, it should be easy to test with a browser-chrome test once we have a stub provider.. let's just file a follow up for it

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8435823 [details] [diff] [review]
Patch v2

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

+ moving the consts from TranslationUI to Translation so they can be exported

::: browser/components/translation/Translation.jsm
@@ +37,5 @@
>      }
>      return this._defaultTargetLanguage;
>    },
>  
> +  showUI: function(aBrowser, aUI) {

if we name this function showUI, we'll have in this file the following functions:

showUI, showTranslationUI, shouldShowInfobar, showURLBarIcon :)

Let's leave those to the TranslationUI object.. A suggestion for a name: documentStateReceived, or documentTranslationInfo

or would it be too disingenuous to continue with `languageDetected`?

and aUI -> aState, although I see there will be a aState.state.. not a big deal I think.. But if you can come up with better names.. :)

@@ +56,3 @@
>  
> +    // Set all values before showing a new translation infobar.
> +    trUI._state = aUI.state;

setting the public setter would cause problems here?

::: browser/components/translation/TranslationContentHandler.jsm
@@ +20,5 @@
>    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>                              .getInterface(Ci.nsIWebProgress);
>    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
>  
> +  global.addEventListener("pageshow", this.onPageShow.bind(this));

create a handleEvent function instead, and just pass `this` here (similar to the message listeners). This avoids one copy of onPageShow per TranslationContentHandler, and the scope of `this` inside the function will be correct without the need for .bind

@@ +49,5 @@
> +      UI.translatedFrom = trDoc.translatedFrom;
> +      UI.translatedTo = trDoc.translatedTo;
> +      UI.originalShown = trDoc.originalShown;
> +    }
> +    else {

nit: else in the same line as braces,  } else {

@@ +127,5 @@
>  
>          this.global.content.translationDocument = translationDocument;
> +        translationDocument.translatedFrom = msg.data.from;
> +        translationDocument.translatedTo = msg.data.to;
> +        translationDocument.translationError = false;

translationError should be set internally by BingTranslator when resolving/rejecting the _onFinishedDeferred promise.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8435823 [details] [diff] [review]
Patch v2

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

Florian and I talked more about this and we agreed on what's left to change on the patch for a r+ with those things addressed (comment 8 minus changing the translationError and _state parts, which we'll leave as is)

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v3 (as checked in)

https://hg.mozilla.org/integration/fx-team/rev/4e05844b3986

(In reply to :Felipe Gomes from comment #8)

> Review of attachment 8435823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> + moving the consts from TranslationUI to Translation so they can be exported

Done, this avoids the hardcoded '0' in a few places in the tests. :-)

> ::: browser/components/translation/Translation.jsm
> @@ +37,5 @@
> >      }
> >      return this._defaultTargetLanguage;
> >    },
> >  
> > +  showUI: function(aBrowser, aUI) {
> 
> if we name this function showUI, we'll have in this file the following
> functions:
> 
> showUI, showTranslationUI, shouldShowInfobar, showURLBarIcon :)
> 
> Let's leave those to the TranslationUI object.. A suggestion for a name:
> documentStateReceived, or documentTranslationInfo

I renamed it to documentStateReceived

> and aUI -> aState, although I see there will be a aState.state.. not a big
> deal I think.. But if you can come up with better names.. :)

I changed aUI to aData.

> @@ +56,3 @@
> >  
> > +    // Set all values before showing a new translation infobar.
> > +    trUI._state = aUI.state;
> 
> setting the public setter would cause problems here?

Using the public setter wouldn't break anything; it would just pointlessly execute some code.

> ::: browser/components/translation/TranslationContentHandler.jsm
> @@ +20,5 @@
> >    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> >                              .getInterface(Ci.nsIWebProgress);
> >    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
> >  
> > +  global.addEventListener("pageshow", this.onPageShow.bind(this));
> 
> create a handleEvent function instead

Done.

> @@ +127,5 @@
> >  
> >          this.global.content.translationDocument = translationDocument;
> > +        translationDocument.translatedFrom = msg.data.from;
> > +        translationDocument.translatedTo = msg.data.to;
> > +        translationDocument.translationError = false;
> 
> translationError should be set internally by BingTranslator when
> resolving/rejecting the _onFinishedDeferred promise.

We discussed this on IRC. The reason for keeping this as it was in the patch is that moving it to BingTranslator would force us to duplicate it once we have multiple providers (which could be as soon as we have a test provider).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e05844b3986

Comment 12

[:Felipe Gomes (needinfo for replies!)]

This build can be used to verify this bug: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e078627d18c6

Translate a page, click on a link in the page, then after navigation finishes go back to the first page. Make sure the infobar in the Translated state comes back, and that Show Original/Translation and re-translation still works.

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Verified as fixed using the build from above. Did some exploratory around this and with no issues found. The testing was done on Windows 8.1 64bit, Windows 7 64bit, Mac OS X 10.9.2, Ubuntu 13.04 64bit and Ubuntu 14.04 32bit.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8436968 [details] [diff] [review]
Patch v3 (as checked in)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic translation feature, which we want to A/B with a subset of Aurora 32 users.
User impact if declined: Navigating back to a translated page (that has been kept on bfcache) won't display the infobar with an option to show the original content
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): small
String or IDL/UUID changes made by this patch: none

Comment 15

[Florian Quèze [:florian]]

I had to tweak this patch a bit to apply on aurora because bug 973292 landed on aurora first and caused some bitrot.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a6785bfed9b

Comment 16

[Andrei Vaida [:avaida]]

This issue is verified fixed on Aurora 32 (2014-06-19) as well, using:
 * Windows 7 64-bit [1], 
 * Mac OS X 10.9.2 [2],
 * Windows 8.1 64-bit [3],
 * Ubuntu 12.04 LTS 32-bit [4].

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
[3] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
[4] Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0