1008224 - Implement translation infobar tweaks to allow for better display in smaller browser windows

Status: RESOLVED INACTIVE

(Firefox :: Translations, defect)

Description

[Sevaan Franks [:sevaan]]

This is an implementation bug for Bug 1008214

Comment 1

[Iaroslav Sheptykin]

Attachment: WIP: Infobar UI tweaks to allow for better display in smaller browser windows

Hi guys!

I have been trying to get the translation infobar to look as close as possible as described in Bug 1008214. It takes way less space now. However I could not get as far as 564px. The furthest I could get before the infobar started overflowing was 675px on my Linux desktop.

I plan to add code for hiding/showing the translation icon when the space gets short. It will win us some more pixels.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8594791 [details] [diff] [review]
WIP: Infobar UI tweaks to allow for better display in smaller browser windows

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

Thanks for the patch, here is some feedback:

::: browser/components/translation/test/browser_translation_infobar.js
@@ +112,5 @@
> +  let oldWidth = window.outerWidth;
> +
> +  // First checking how notification bar handles big screens.
> +  let windowBorder = 2;
> +  window.resizeTo(1000 + windowBorder, window.outerHeight);

Will this work if the screen of the machine running the test is smaller than 1002px wide?

@@ +239,5 @@
>    waitForCondition(hasTranslationInfoBar, () => {
>      ok(hasTranslationInfoBar(), "there's a 'translate' notification");
>      aFinishCallback();
>    }, "timeout waiting for the info bar to reappear");
> +

nit: please avoid whitespace changes that are not directly related to what you are editing in the file.

::: browser/locales/en-US/chrome/browser/translation.dtd
@@ +13,5 @@
>    -  the correct grammar case to keep the same structure of the original
>    -  sentence. -->
>  <!ENTITY translation.thisPageIsIn.label         "This page is in">
>  <!ENTITY translation.translateThisPage.label    "Translate this page?">
> +<!ENTITY translation.translate.button           "Yes">

I'm not sure changing the explicit "Translate" button to "Yes" which requires reading the rest of the notification to understand it is a good idea. But anyway, when changing a string in a way that will require a new translation, the string id needs to be changed too.

@@ +33,5 @@
>    -
>    -  translation.translatedToSuffix.label (empty in en-US) is for locales that
>    -  need to display some text after the second drop down for the sentence to
>    -  be grammatically correct. -->
> +<!ENTITY translation.translatedFrom.label       "Translated from">

The mockup at attachment 8436011 [details] only uses this shorter string when we are space constrained. When there's spare space, we keep the longer more descriptive string.

@@ +40,5 @@
>  
>  <!ENTITY translation.showOriginal.button        "Show Original">
>  <!ENTITY translation.showTranslation.button     "Show Translation">
>  
> +<!ENTITY translation.errorTranslating.label     "Error translating.">

same here.

::: browser/themes/linux/browser.css
@@ +1227,5 @@
>  
>  notification[value="translation"] button > .button-box,
>  notification[value="translation"] button[type="menu"] > .button-box > .button-menu-dropmarker {
>    padding: 0;
> +  -moz-margin-start: 1ch;

Is this change part of the UX spec?

::: browser/themes/shared/translation/infobar.inc.css
@@ +51,5 @@
>    padding: 5px 0;
>  }
>  
> +notification[value="translation"] button[anonid="showTranslation"] {
> +  display: none;

Why is this change needed / what is it doing?

Comment 3

[Iaroslav Sheptykin]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

Thanks for the detailed feedback, Florian!

> Will this work if the screen of the machine running the test is smaller than
> 1002px wide?

I set my screen resolution to 640 x 480 and this test passed. I am using xfse. Not sure how this would work with other desktop software.
 
> I'm not sure changing the explicit "Translate" button to "Yes" which
> requires reading the rest of the notification to understand it is a good
> idea. But anyway, when changing a string in a way that will require a new
> translation, the string id needs to be changed too.

I personally find "Translate" to be better too. I just saw that this is being used in attachment 8436011 [details].

> The mockup at attachment 8436011 [details] only uses this shorter string
> when we are space constrained. When there's spare space, we keep the longer
> more descriptive string.

You are right. This somehow escaped my attention. Thanks for pointing this out.

> ::: browser/themes/linux/browser.css
> @@ +1227,5 @@
> >  
> >  notification[value="translation"] button > .button-box,
> >  notification[value="translation"] button[type="menu"] > .button-box > .button-menu-dropmarker {
> >    padding: 0;
> > +  -moz-margin-start: 1ch;
> 
> Is this change part of the UX spec?

No, but I noticed that the buttons in attachment 8436011 [details] are thinner than those in current infobar. I therefore thought that it implies the buttons should get thinner.

> ::: browser/themes/shared/translation/infobar.inc.css
> @@ +51,5 @@
> >    padding: 5px 0;
> >  }
> >  
> > +notification[value="translation"] button[anonid="showTranslation"] {
> > +  display: none;
> 
> Why is this change needed / what is it doing?

This is for making showTranslation button hidden initially. Its visibility is controlled at runtime by the infobar implementation [1], however it is first hidden after the translation happened [2]. This forces xul:deck to allocate more space than it would actually need, making the infobar overflow earlier. At least this is how things seem to behave to me in my setup.

1. http://mxr.mozilla.org/mozilla-central/source/browser/components/translation/translation-infobar.xml#310
2. http://mxr.mozilla.org/mozilla-central/source/browser/components/translation/translation-infobar.xml#169

Comment 4

[Iaroslav Sheptykin]

Attachment: WIP: Infobar UI tweaks to allow for better display in smaller browser windows

Hello Florian!

I would like to share with you my progress on adapting translation infobar to constrained screens. I addressed your last feedback in comment 3. With this patch I was able to push the overflowing breakpoint to 678px which is ~100px over what is expected. Consequently, the tests are still failing. I will keep trying to get to the desired 564px. Meanwhile, could you look at the code and let me know if I am approaching the problem in a right way?

Comment 5

[Iaroslav Sheptykin]

Hi everyone! Just a short progress update:
Trying to get closer to the desired breakpoint (564px) on linux I realized that translation infobar looks differently on different platforms. On ubunutu its appearance differs from the one used in the mockup for constrained screens. I posted a question [1] in bug 988482 to get more information on this regarding linux, because the implementation logs are somewhat confusing.

1. https://bugzilla.mozilla.org/show_bug.cgi?id=988482#c3

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8600265 [details] [diff] [review]
WIP: Infobar UI tweaks to allow for better display in smaller browser windows

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

Sorry for the delay. The approach seems reasonable to me.

::: browser/components/translation/translation-infobar.xml
@@ +173,5 @@
>            if (val == Translation.STATE_TRANSLATED)
>              this._handleButtonHiding();
>  
> +          let icon = this._getAnonElt('messageImage');
> +          if (val == Translation.STATE_OFFER)

I know this is on the mockup, but this is a bit strange, so it may be worth double checking with UX that they _actually_ meant this.

::: browser/themes/shared/translation/infobar.inc.css
@@ +30,5 @@
>    overflow: hidden;
>  }
>  
> +@media screen and (max-width: 999px) {
> +  notification[value="translation"] * .hide-when-constrained {

I don't think you need the '*' here.

@@ +35,5 @@
> +    display: none;
> +  }
> +}
> +
> +@media screen and (min-width: 1000px) {

Using a hardcoded px value is unfortunate, as I would expect the breakpoint to vary with the font size. Would a relative unit (https://developer.mozilla.org/en-US/docs/Web/CSS/length#Relative_length_units) work here?

@@ +62,5 @@
>    margin: auto;
>    padding: 5px 0;
>  }
>  
> +notification[value="translation"] button[anonid="showTranslation"] {

It would be nice to add a comment here with a short version of what you explained to me in comment 3.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:florian, could you have a look please?
For more information, please visit auto_nag documentation.