976554 - Replace content of webpage's text nodes with translation content (or back to original)

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

When a translation is finished, we will have information about its original content and its translated content in a TranslationDocument object.
We need to support switching the page's content between the two data sets.

This is being done in bug 971043 but it's good to have a separate bug for it.

Comment 1

[Mike Hoye [:mhoye]]

Felipe: this bug remains unassigned. Would you be willing to mentor a contributor through it?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Not a good candidate for mentoring as I'm about to start working on this bug very soon, as a sequence from bug 971054

Comment 3

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: trSwapResultsWorking

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Swap page content between translation and original

qref'ed

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8420816 [details] [diff] [review]
Swap page content between translation and original

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

The one concern I have about this patch is that TranslationItems are not reordered to match the order in the translation (eg if "<em>I</em> miss <strong>you</strong>" is translated by "<strong>Tu</strong> <em>me</em> manques"... we will display "<em>me</em> <strong>Tu</strong> manques").

I've just discussed this with Felipe and he thinks it would be better to split this out to another bug. That's fine with me, as long as we fix it before turning on the feature :-).

All the following comments are trivial details:

::: browser/components/translation/TranslationDocument.jsm
@@ +345,5 @@
> +    let curItem = visitStack.shift();
> +
> +    let domNode = curItem.nodeRef;
> +    if (!domNode) {
> +      // Skipping this item due to a missing node.

Is this something we expect to happen?

@@ +355,5 @@
> +    if (!curItem[target]) {
> +      // Translation not found for this item. This could be due to
> +      // an error in the server response. For example, if a translation
> +      // was broken in various chunks, and one of the chunks failed,
> +      // the items from that chunk will be missing it's "translation"

it's -> its

@@ +360,5 @@
> +      // field.
> +      continue;
> +    }
> +
> +    // Now let's walk through all items in the `target` array of the 

Trailing whitespace.

@@ +366,5 @@
> +    // TranslationItem.translation array.
> +    for (let child of curItem[target]) {
> +      // If the array element is another TranslationItem object, let's
> +      // add it to the stack to be visited
> +      if (typeof(child) == "object") {

would |child instanceof TranslationItem| work here?

@@ +375,5 @@
> +
> +      // If it's a string, say, the Nth string of the `target` array, let's
> +      // replace the Nth child TextNode of this element with this string.
> +      // During our translation process we skipped all empty text nodes, so we
> +      // must also skip them here.

Would be nice to mention in this comment too that if there's not enough text nodes, an additional one will be created.

@@ +401,5 @@
> +        return childNode;
> +    }
> +  }
> +
> +  // if there are not enough DOM nodes, let's create a new one

nit: this looks like a whole sentence, so it should start with an uppercase letter and finish with a . :-)

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Swap page content between translation and original

I'll file the follow-up as discussed. As suggested in the other bug, I changed the API here to not expose the internal "original" and "translation" array names. As I don't like functions with boolean params, I split it into two functions as you suggested, showTranslation() and showOriginal().

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> ::: browser/components/translation/TranslationDocument.jsm
> @@ +345,5 @@
> > +    let curItem = visitStack.shift();
> > +
> > +    let domNode = curItem.nodeRef;
> > +    if (!domNode) {
> > +      // Skipping this item due to a missing node.
> 
> Is this something we expect to happen?

I'm not 100% convinced that it won't :)

> 
> @@ +355,5 @@
> > +    if (!curItem[target]) {
> > +      // Translation not found for this item. This could be due to
> > +      // an error in the server response. For example, if a translation
> > +      // was broken in various chunks, and one of the chunks failed,
> > +      // the items from that chunk will be missing it's "translation"
> 
> it's -> its
> 
> @@ +360,5 @@
> > +      // field.
> > +      continue;
> > +    }
> > +
> > +    // Now let's walk through all items in the `target` array of the 
> 
> Trailing whitespace.
> 
> @@ +366,5 @@
> > +    // TranslationItem.translation array.
> > +    for (let child of curItem[target]) {
> > +      // If the array element is another TranslationItem object, let's
> > +      // add it to the stack to be visited
> > +      if (typeof(child) == "object") {
> 
> would |child instanceof TranslationItem| work here?

yeah

> 
> @@ +375,5 @@
> > +
> > +      // If it's a string, say, the Nth string of the `target` array, let's
> > +      // replace the Nth child TextNode of this element with this string.
> > +      // During our translation process we skipped all empty text nodes, so we
> > +      // must also skip them here.
> 
> Would be nice to mention in this comment too that if there's not enough text
> nodes, an additional one will be created.
> 
> @@ +401,5 @@
> > +        return childNode;
> > +    }
> > +  }
> > +
> > +  // if there are not enough DOM nodes, let's create a new one
> 
> nit: this looks like a whole sentence, so it should start with an uppercase
> letter and finish with a . :-)

nits fixed

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Part 1:   https://hg.mozilla.org/integration/fx-team/rev/389b17394862
Part 2:   https://hg.mozilla.org/integration/fx-team/rev/e57eb112e028
Part 3:   https://hg.mozilla.org/integration/fx-team/rev/92f600e20a3b
Part 4:   https://hg.mozilla.org/integration/fx-team/rev/c7064c59041c
 5 & 6:   https://hg.mozilla.org/integration/fx-team/rev/03aa0984c437

I'll work in the test suite on a separate bug

Comment 9

[:Felipe Gomes (needinfo for replies!)]

The above csets were for bug 971054. For this bug:

remote:   https://hg.mozilla.org/integration/fx-team/rev/92970bb568ca

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92970bb568ca

Comment 11

[Bogdan Maris, Desktop Test Engineering]

Got this verified as fixed during my testing in from bug 971054