979424 - Implement structure and state switching for translation infobar

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Bug 971044 became a sort-of metabug, so filing a new one to implement the basic infobar structure for the Translation project. This bug should unblock all work related to the infobar styling and features. Note: this infobar is more complex than the typical ones that disappear after interaction, so it's probably important to write it in a way that brings the concepts of "states" or "phases" to the bar ("translation offer", "in progress", "translated + show original", "error"), to facilitate other bugs.  

The translation infobar should be an infobar with the following features:

- It displays the webpage's language in a language dropdown, pre-selected to the language detected
- Allows the user to correct this detection by choosing another language from the dropdown
- Has a Translate and Not Now buttons, and an Options menupopup button
- Doesn't go away when Translate is clicked. Instead, it switches to "Translating webpage" and triggers the translation service
- Displays success or error information on completion

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP

No l10n yet.

Comment 2

[Florian Quèze [:florian]]

Screenshots of the current WIP: http://imgur.com/a/KtsTK

Comment 3

[:Felipe Gomes (needinfo for replies!)]

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

(patch was discussed on irc :)

Comment 4

[Florian Quèze [:florian]]

Attachment: WIP2

I've changed the info bar structure implementation to an XBL binding. XBL-required cruft aside, I think the new code is much more readable.

I'm seeking feedback about these files:
 browser/base/content/browser.css
 browser/components/translation/jar.mn
 browser/components/translation/moz.build
 browser/components/translation/translation-infobar.xml
 browser/locales/en-US/chrome/browser/translationInfoBar.dtd
 browser/locales/jar.mn
These are the file changes that are relevant for this bug. I think the work here is almost done, and the only reason why the flag is f? rather than r? is that I feel this needs a test (although I'm not completely sure yet what needs to be tested).

These file changes are required to see the infobar in action but are a WIP for bug 971048:
 browser/components/moz.build
 browser/base/content/browser.js
 browser/components/translation/Translation.jsm
 content/base/src/nsDocumentEncoder.cpp

Comment 5

[Sevaan Franks [:sevaan]]

Attachment: Various Translation Infobar States

Attached is an image showing the various states of the translation infobar

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v3

Now with tests (pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=b32c113ece88).

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8393563 [details] [diff] [review]
Patch v3

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

Great patch! I really liked how it turned out.

The only thing I'm torn about is the control between the infobar and Translation.jsm. While I liked the simplicity here, I think it's a bit strange for the UI to organize itself and call the translate methods, instead of the opposite. E.g., I think that the button that does:

|document.getBindingParent(this).showOriginal();|

should really do:

|document.getBindingParent(this).translation.showOriginal();|

and then Translation.jsm would call a UI function originalShown() to update the UI (in addition to perform its task). (same for translate(), showTranslatedContent())


Although I like that, the way it is now, Translation.jsm does not need to keep a reference to the infobar. But I think changing that is not a problem.

What do you think about this?


And again you'll need a rubberstamp from a build peer on the moz.build/jar.mn changes

::: browser/base/content/browser.css
@@ +792,5 @@
>    width: auto;
>    height: auto;
>  }
>  
> +notification[value="translation"] {

Add a /* Translation */ comment above, and maybe let's sneak a /* Social */ below to improve this file's organization? :)

::: browser/components/translation/translation-infobar.xml
@@ +23,5 @@
> +        <xul:hbox anonid="details" align="center" flex="1">
> +          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image,type,value"/>
> +          <xul:deck anonid="translationStates" selectedIndex="0">
> +
> +            <!-- offer to translate -->

can you add an id to each of the boxes on the deck to make it possible to apply unique styles to them, in case that becomes necessary later?

@@ +37,5 @@
> +                          oncommand="document.getBindingParent(this).close();"/>
> +            </xul:hbox>
> +
> +            <!-- translating -->
> +            <xul:vbox pack="center">

why is translating a vbox and all others are hbox? if possible let's keep them all equal

@@ +82,5 @@
> +                           oncommand="document.getBindingParent(this).close();"/>
> +      </xul:hbox>
> +    </content>
> +    <implementation>
> +      <method name="init">

some documentation on the public methods would be good. Specially here explaining what object is aTranslation

@@ +93,5 @@
> +                           .createBundle("chrome://global/locale/languageNames.properties");
> +
> +            let detectedLanguage = this.getElt("detectedLanguage");
> +            let fromLanguage = this.getElt("fromLanguage");
> +            for (let code of this.translation.kDetectedLanguages) {

can you change this from kDetectedLanguages to supportedSourceLanguages?

and kTranslatedLanguages to supportedTargetLanguages.

I wanted to use those names for the properties which will be fed to Translation.jsm from the translation provider (in bug 971054).

@@ +106,5 @@
> +          ]]>
> +        </body>
> +      </method>
> +
> +      <method name="getElt">

_getAnonElt for this private method

@@ +116,5 @@
> +
> +      <property name="detectedLanguage" onget="return this.getAttribute('detectedLanguage');">
> +        <setter><![CDATA[
> +          this.getElt("detectedLanguage").value = val;
> +          this.setAttribute("detectedLanguage", val);

instead of using an attribute to store detectedLanguage, we can just use a field, _detectedLanguage.  Since there's already this public property getter, no point in using an attribute too.

@@ +124,5 @@
> +
> +      <field name="STATE_OFFER" readonly="true">0</field>
> +      <field name="STATE_TRANSLATING" readonly="true">1</field>
> +      <field name="STATE_TRANSLATED" readonly="true">2</field>
> +      <field name="STATE_ERROR" readonly="true">3</field>

move these as close to the top of the file as possible? (to the beginning of <implementation>)

@@ +135,5 @@
> +        <body>
> +          <![CDATA[
> +            if (this.state == this.STATE_OFFER) {
> +              this.getElt("fromLanguage").value = this.getElt("detectedLanguage").value;;
> +              this.getElt("toLanguage").value = "en"; //FIXME

reminder to use the temporary thing we discussed on IRC and annotate this with a bug number to follow up

::: browser/locales/en-US/chrome/browser/translationInfoBar.dtd
@@ +1,1 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

I think we should call this file just translation.dtd. It might be useful if we are looking for a place to add more strings later that are not part of the infobar.

And we should prepend all entities with "translation." to reduce chance of an entity collision.


Let's leave investigating accesskeys/accessibility in general to a new bug

Comment 8

[Florian Quèze [:florian]]

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

> Great patch! I really liked how it turned out.

Thanks!

> The only thing I'm torn about is the control between the infobar and
> Translation.jsm. While I liked the simplicity here, I think it's a bit
> strange for the UI to organize itself and call the translate methods,
> instead of the opposite. E.g., I think that the button that does:
> 
> |document.getBindingParent(this).showOriginal();|
> 
> should really do:
> 
> |document.getBindingParent(this).translation.showOriginal();|

I think it makes sense for the infobar binding to have methods for the things its UI does, even if we move the real logic to Translation.jsm.

> and then Translation.jsm would call a UI function originalShown() to update
> the UI (in addition to perform its task). (same for translate(),
> showTranslatedContent())

So something I'm really unsure about is whether switching between original and translated content is a synchronous or asynchronous operation.

For the current patch, I pretended it was synchronous because that's easier. If it's asynchronous, we may still want to keep the methods in the binding to disable the button after it's been clicked; to avoid having multiple clicks on "show original" while we are already in process of switching the displayed content.

I think it would make sense to keep it simple for now, and finish it once we actually have the code switching the content back and forth between translation and original.


For the translate method, I think in the future (maybe as soon as bug 974460) we will need to remember the state we are in independently of whether the infobar is opened or has been closed. So I think the code waiting for the result of the Translation.translate promise will need to move out of the xbl binding.

But for now having the code controlling state changes in the binding makes testing easier, so I would prefer keeping things as is until we actually add the Translation.jsm file (I may do it soon in bug 971048).

> ::: browser/components/translation/translation-infobar.xml
> @@ +23,5 @@
> > +        <xul:hbox anonid="details" align="center" flex="1">
> > +          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image,type,value"/>
> > +          <xul:deck anonid="translationStates" selectedIndex="0">
> > +
> > +            <!-- offer to translate -->
> 
> can you add an id to each of the boxes on the deck to make it possible to
> apply unique styles to them, in case that becomes necessary later?

Ids are generally not a great fit for XBL bindings. For the next patch I'm assuming adding class names addresses your concern about applying styles to each state.

> @@ +37,5 @@
> > +                          oncommand="document.getBindingParent(this).close();"/>
> > +            </xul:hbox>
> > +
> > +            <!-- translating -->
> > +            <xul:vbox pack="center">
> 
> why is translating a vbox and all others are hbox? if possible let's keep
> them all equal

Initially I didn't have a box there at all and the label was a direct child of the deck.
A vbox with pack="center" is the only working way I've found to get the label to position itself correctly in the middle of the available vertical space (ie. at the same vertical position as the labels of the other states).

> > +      <property name="detectedLanguage" onget="return this.getAttribute('detectedLanguage');">
> > +        <setter><![CDATA[
> > +          this.getElt("detectedLanguage").value = val;
> > +          this.setAttribute("detectedLanguage", val);
> 
> instead of using an attribute to store detectedLanguage, we can just use a
> field, _detectedLanguage.  Since there's already this public property
> getter, no point in using an attribute too.

Done. This was initially a DOM attribute because I was trying to use xbl:inherit to set automatically the initial value of the detected language menu list. That didn't work, because the menulist constructor ran before the attribute got set.

> @@ +135,5 @@
> > +        <body>
> > +          <![CDATA[
> > +            if (this.state == this.STATE_OFFER) {
> > +              this.getElt("fromLanguage").value = this.getElt("detectedLanguage").value;;
> > +              this.getElt("toLanguage").value = "en"; //FIXME
> 
> reminder to use the temporary thing we discussed on IRC and annotate this
> with a bug number to follow up

This is actually needed in more than one place, so I've changed it to a "defaultTargetLanguage" getter in Translation.jsm. No bug number, because that file is just a WIP at this point.

> Let's leave investigating accesskeys/accessibility in general to a new bug

Ok.

Comment 9

[Florian Quèze [:florian]]

Attachment: Patch v4

Comment 10

[Florian Quèze [:florian]]

Attachment: Updated Translation.jsm WIP

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8394872 [details] [diff] [review]
Patch v4

Requesting review from :gps for the moz.build and jar.mn changes.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8394872 [details] [diff] [review]
Patch v4

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

You don't need build peer review for these jar.mn and moz.build changes. But they do look sane.

Comment 13

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/1d0181b678e8

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d0181b678e8

Comment 15

[Francesco Lodolo [:flod]]

I wonder if this structure will create problems in some languages, but I don't see alternatives with dropdown lists involved.

Where does the list of language names come from?

Comment 16

[Florian Quèze [:florian]]

(In reply to Francesco Lodolo [:flod] from comment #15)
> I wonder if this structure will create problems in some languages, but I
> don't see alternatives with dropdown lists involved.

We briefly considered that some locales may need to have some text on both sides of the dropdowns, so we may need to add a third (empty for the en-US locale) label on the right side of the second drop down.

We weren't sure this was actually needed, so we skipped this for now, but if it's needed, feel free to file a bug.

Also FYI, we are only going to ship this translation experiment to the Polish, Turkish and Vietnamese locales initially.

> Where does the list of language names come from?

We are reusing this existing list: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/languageNames.properties

Comment 17

[Francesco Lodolo [:flod]]

Not sure this works, especially for a complex language like Polish (language may need to be adapted to case, unless using "language X" and decline "language" instead of "X").

Stefan (or Sara), can you take a look at this bug and let me know your thoughts?

Comment 18

[Stefan Plewako [:stef]]

(In reply to Francesco Lodolo [:flod] from comment #17)

I don't see all the strings from attachment 8393507 [details] in the patch (comment #14) - hardcoded strings?

languageNames.properties has names in nominative which makes it impossible to use in polish natural sentences (like in en-US) - I would probably use sth like "Original language: " for translation.translatedFrom.label and "Current language: " for translation.translatedTo.label.

I would probably also try as much as possible to not to use "translation" term.

Comment 19

[Florian Quèze [:florian]]

(In reply to Stefan Plewako [:stef] from comment #18)
> (In reply to Francesco Lodolo [:flod] from comment #17)
> 
> I don't see all the strings from attachment 8393507 [details] in the patch
> (comment #14) - hardcoded strings?

These strings are all in https://hg.mozilla.org/mozilla-central/diff/1d0181b678e8/browser/locales/en-US/chrome/browser/translation.dtd except for the language names that are coming from languageNames.properties.

Comment 20

[Stefan Plewako [:stef]]

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> (In reply to Stefan Plewako [:stef] from comment #18)
> > (In reply to Francesco Lodolo [:flod] from comment #17)
> > 
> > I don't see all the strings from attachment 8393507 [details] in the patch
> > (comment #14) - hardcoded strings?
> 
> These strings are all in
> https://hg.mozilla.org/mozilla-central/diff/1d0181b678e8/browser/locales/en-
> US/chrome/browser/translation.dtd except for the language names that are
> coming from languageNames.properties.

Just checked that at it looks like in the options the file http://hg.mozilla.org/mozilla-central/file/tip/toolkit/locales/en-US/chrome/global/notification.dtd is used and it contains wording that doesn't fit well with bars/panels and also shouldn't be reused here.

Comment 21

[Florian Quèze [:florian]]

(In reply to Stefan Plewako [:stef] from comment #20)

> Just checked that at it looks like in the options the file
> http://hg.mozilla.org/mozilla-central/file/tip/toolkit/locales/en-US/chrome/
> global/notification.dtd is used and it contains wording that doesn't fit
> well with bars/panels and also shouldn't be reused here.

The only string used from notification.dtd is <!ENTITY closeNotification.tooltip "Close this message">

This string is used as the tooltip of the [X] button at the end of all notification bars.

Comment 22

[Stefan Plewako [:stef]]

(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> (In reply to Stefan Plewako [:stef] from comment #20)
> 
> > Just checked that at it looks like in the options the file
> > http://hg.mozilla.org/mozilla-central/file/tip/toolkit/locales/en-US/chrome/
> > global/notification.dtd is used and it contains wording that doesn't fit
> > well with bars/panels and also shouldn't be reused here.
> 
> The only string used from notification.dtd is <!ENTITY
> closeNotification.tooltip "Close this message">
> 
> This string is used as the tooltip of the [X] button at the end of all
> notification bars.

So en-US have the same problem… and what goes in the options menu?!?

The patch for so simple UI shouldn't have so many issues and it should be backed out IMO.

Comment 23

[Florian Quèze [:florian]]

(In reply to Stefan Plewako [:stef] from comment #22)

> So en-US have the same problem…

I still don't understand what this 'problem' is. Can you explain with more details?

> and what goes in the options menu?!?

It's not implemented yet. We will handle it in bug 976573.

> The patch for so simple UI shouldn't have so many issues and it should be
> backed out IMO.

What are the specific issues you are concerned about?

Comment 24

[Stefan Plewako [:stef]]

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> I still don't understand what this 'problem' is. Can you explain with more
> details?

The problem is that "message" is not "bar" and "notfication" is not "translation bar".

> What are the specific issues you are concerned about?

1. Not implemented features should not land.
2. Reusing strings rarely makes close/good translations possible and much often creates problems - in general it should be avoided.

Comment 25

[Francesco Lodolo [:flod]]

(In reply to Stefan Plewako [:stef] from comment #24)
> (In reply to Florian Quèze [:florian] [:flo] from comment #23)
> The problem is that "message" is not "bar" and "notfication" is not
> "translation bar".

Sorry but I'm not following either. Where do you see these terms? I don't see them in the .dtd file or image mock-ups

> 2. Reusing strings rarely makes close/good translations possible and much
> often creates problems - in general it should be avoided.
It depends on the context: a tooltip on a button in a toolbar (common structure for most notifications), must be shared (IMO).

Comment 26

[Florian Quèze [:florian]]

(In reply to Stefan Plewako [:stef] from comment #24)
> (In reply to Florian Quèze [:florian] [:flo] from comment #23)
> > I still don't understand what this 'problem' is. Can you explain with more
> > details?
> 
> The problem is that "message" is not "bar" and "notfication" is not
> "translation bar".

Are you saying that the "Close this message" wording is poor and should be improved? If so I kinda agree, but it's not specifically related to the translation infobar (it applies to all notification bars) so it would be better discussed in a separate bug.

> > What are the specific issues you are concerned about?
> 
> 1. Not implemented features should not land.

Large features need to land in parts and are implemented when they are ready. The translation infobar bar landed here is not included in current nightlies (the 'translation' folder is actually not even compiled right now).

> 2. Reusing strings rarely makes close/good translations possible and much
> often creates problems - in general it should be avoided.

I agree that strings should not be reused in different contexts; what I'm saying is that this translation infobar is a slightly modified notification bar, so it makes sense here to reuse the string used for the tooltip of other notification bars.

Comment 27

[Stefan Plewako [:stef]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> It depends on the context: a tooltip on a button in a toolbar (common
> structure for most notifications), must be shared (IMO).

Yes, it depends on the context and in general it may be good idea to share generic strings but translation bar looks like toolbar/action bar and not notification and I don't think that majority of notifications are in a form of toolbar now.

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Large features need to land in parts and are implemented when they are
> ready. The translation infobar bar landed here is not included in current
> nightlies (the 'translation' folder is actually not even compiled right now).

There is nothing about large features (translation bar doesn't look like one) that forces them to be landed in parts - it just may be good idea that saves time. The problem is that options drop down should land as a whole with it's content in one part - we have long enough history of bad prelanded strings.

> I agree that strings should not be reused in different contexts; what I'm
> saying is that this translation infobar is a slightly modified notification
> bar, so it makes sense here to reuse the string used for the tooltip of
> other notification bars.

Notification bar(s)? notification.dtd may fit well in doorhanger notification, web notification (https://developer.mozilla.org/en-US/docs/WebAPI/Using_Web_Notifications), out of the window/system notification (like update ready) but not in a toolbar.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to Stefan Plewako [:stef] from comment #27)
> There is nothing about large features (translation bar doesn't look like
> one) that forces them to be landed in parts - it just may be good idea that
> saves time. The problem is that options drop down should land as a whole
> with it's content in one part - we have long enough history of bad prelanded
> strings.

Translation is a large feature (see the dependency tree of bug 973271). There are no "pre-landed" strings here, as far as I can tell - they're all used, it's just that the feature in general isn't enabled yet.

Comment 29

[Francesco Lodolo [:flod]]

(In reply to Stefan Plewako [:stef] from comment #27)
> Notification bar(s)? notification.dtd may fit well in doorhanger
> notification, web notification
> (https://developer.mozilla.org/en-US/docs/WebAPI/Using_Web_Notifications),
> out of the window/system notification (like update ready) but not in a
> toolbar.

I disagree, I think that's the less intrusive way of notifying users. That's what Google does in Chrome, that's what we're mimicking on mozilla.org (https://www.mozilla.org/it/).

Having said that, such discussion would make more sense in a newsgroup, not in a bug marked as resolved.

Comment 30

[Bogdan Maris, Desktop Test Engineering]

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> The translation infobar bar landed here is not included in current
> nightlies (the 'translation' folder is actually not even compiled right now).

Then, is there a way I can verify this fix?

Comment 31

[Florian Quèze [:florian]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #30)
> (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > The translation infobar bar landed here is not included in current
> > nightlies (the 'translation' folder is actually not even compiled right now).
> 
> Then, is there a way I can verify this fix?

Nothing easy. You would need to make your own build with a patch added to show the infobar.

Comment 32

[Jenn Chaulk (:jchaulk)]

Hi Bogdan, just looking for an update on when verification will be complete for this item, as our iteration ends on Monday, March 31st.

Comment 33

[Bogdan Maris, Desktop Test Engineering]

(In reply to Jenn Chaulk from comment #32)
> Hi Bogdan, just looking for an update on when verification will be complete
> for this item, as our iteration ends on Monday, March 31st.

First thing tomorrow I will start building and then I will verify this fix.

Comment 34

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #30)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > The translation infobar bar landed here is not included in current
> > > nightlies (the 'translation' folder is actually not even compiled right now).
> > 
> > Then, is there a way I can verify this fix?
> 
> Nothing easy. You would need to make your own build with a patch added to
> show the infobar.

I pushed to the try server the changes you need to test the infobar: https://tbpl.mozilla.org/?tree=Try&rev=d33b91e475d0

Comment 35

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> (In reply to Florian Quèze [:florian] [:flo] from comment #31)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #30)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > > The translation infobar bar landed here is not included in current
> > > > nightlies (the 'translation' folder is actually not even compiled right now).
> > > 
> > > Then, is there a way I can verify this fix?
> > 
> > Nothing easy. You would need to make your own build with a patch added to
> > show the infobar.
> 
> I pushed to the try server the changes you need to test the infobar:
> https://tbpl.mozilla.org/?tree=Try&rev=d33b91e475d0

Builds are at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d33b91e475d0/

Comment 36

[Bogdan Maris, Desktop Test Engineering]

Thanks Florian for builds.

(In reply to :Felipe Gomes from comment #0)
> The translation infobar should be an infobar with the following features:
> 
> - It displays the webpage's language in a language dropdown, pre-selected to
> the language detected
Verified.
> - Allows the user to correct this detection by choosing another language
> from the dropdown
Verified.
> - Has a Translate and Not Now buttons, and an Options menupopup button
Verified.
> - Doesn't go away when Translate is clicked. Instead, it switches to
> "Translating webpage" and triggers the translation service
Verified.
> - Displays success or error information on completion
Verified.

Testing was done on Windows 7 64bit, Windows XP 32bit, Mac OS X 10.8.5 and Ubuntu 13.10 32bit

Comment 37

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.