[B2G][l10n][FTE][L12Y] Add suffix string for "Learn more" links

VERIFIED FIXED in 1.4 S4 (28mar)

Status

VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: petercpg, Assigned: flod)

Tracking

({l12y})

unspecified
1.4 S4 (28mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: LocRun1.3, LocRun1.2)

Attachments

(1 attachment)

The file apps/communications/ftu/ftu.properties contains strings:

learn-more-at = Learn more at
external-telemetry.title = Telemetry

learn-about-information = Learn how Mozilla handles your information at
external-privacy.title= Privacy

learnHow = Learn how Mozilla handles your information in our
privacyPolicy = privacy policy

These sentences are split into two parts, but in some languages (Chinese at least), the grammar would need it split in three parts. 
Having the sentence displayed in two parts would still be OK if we move some clauses to the the second string, but it would be too long if the second one is intended to be a hyperlink, IMO.

This is the same case like rights.intro-point1{a|b|c} in about:right in Firefox Browser.

Comment 1

6 years ago
Not sure how we'd fix that. Kaze?
Oops, string about importing contacts (importContacts2, importSim2, importFb2) got the same problem.
Depends on: 851111
Still affects 1.3 in zh-TW.

Device: Geeksphone Keon
Build: 20140205010019
Gaia: 3405205
Platform Version: 28.0
Summary: Add suffix string for "Learn more" links → [B2G][l10n][FTE][L12Y] Add suffix string for "Learn more" links
Whiteboard: LocRun1.3
(Assignee)

Updated

5 years ago
Blocks: 968853

Updated

5 years ago
Whiteboard: LocRun1.3 → LocRun1.3, LocRun1.2
(Assignee)

Comment 4

5 years ago
(In reply to Peter Pin-Guang Chen [:petercpg] (MozTW.org) from comment #2)
> Oops, string about importing contacts (importContacts2, importSim2,
> importFb2) got the same problem.

Are you still seeing this specific problem? Right now I see only individual buttons, so it shouldn't be an issue anymore.

learnHow+privacyPolicy 
This is definitely still an issue.

learn-more-at+external-telemetry.title
"learn-more-at" is actually concatenated with a link (data-l10n-id="external-privacy"), so I believe that the second string should be associated with the "title" tag of this link, and it doesn't seem very useful on a touch device.
Same thing for learn-about-information+privacyPolicy
(Assignee)

Comment 5

5 years ago
Any suggestion to solve this? The only solutions that come to my mind are:
* using spans before and after the link. Suboptimal but it should at least solve the problem.
* using a string like "Learn more at {{link}}" and then replace it with JavaScript and mozL10n.localize. The replacement string would look pretty bad with all that HTML code, and it's probably harder to maintain (i.e. link updates).

Also, any idea why we use external-XXX.title? I thought about screen-readers, but then I noticed not all links have the attribute set.
Flags: needinfo?(kaze)
(In reply to Francesco Lodolo [:flod] from comment #4)
> (In reply to Peter Pin-Guang Chen [:petercpg] (MozTW.org) from comment #2)
> > Oops, string about importing contacts (importContacts2, importSim2,
> > importFb2) got the same problem.
> 
> Are you still seeing this specific problem? Right now I see only individual
> buttons, so it shouldn't be an issue anymore.
> 

No, importContacts2, importSim2, importFb2 are gone, while others are still there.
(Assignee)

Comment 7

5 years ago
Did a test with spans: not great (have to avoid new lines between tags, they create unnecessary white spaces), but it works.
https://github.com/flodolo/gaia/commit/2b3d3254e63121d4ad734b4551b002e4f23cc5ac
(In reply to Francesco Lodolo [:flod] from comment #5)
> * using spans before and after the link. Suboptimal but it should at least
> solve the problem.

That’s what is done in the Settings app for the `crash-reports-description-3' entity — see the (span + a + span in apps/settings/elements/crash_reports.html.

My eyes bleed every time I see that… but at least it’s in three parts, thus getting the job done. If you think it’s OK, we can go with that.

> * using a string like "Learn more at {{link}}" and then replace it with
> JavaScript and mozL10n.localize. The replacement string would look pretty
> bad with all that HTML code, and it's probably harder to maintain (i.e. link
> updates).

In this case the JS code should create a link element (i.e. the URLs should be in the JS code). Such a helper would help:

  var linkRefs = {
    'external-telemetry': 'http://www.mozilla.org/telemetry/',
    'external-privacy': 'www.mozilla.org/privacy/',
    'privacyPolicy': 'https://www.mozilla.org/privacy/firefox-os/'
  };

  function getLocalizedLink(key) {
    var link = document.createElement('a');
    // this has to be synchronous => we can't use mozL10n.localize() here
    link.href = linkRefs[key];
    link.textContent = _(key);
    return link.innerHTML;
  }

  navigator.mozL10n.localize(learnMoreElement, 'learn-more-at', {
    link: getLocalizedLink('external-telemetry')
  });

I agree it’s more complex; this would be nice to have for the Settings app imho (because the link is already built dynamically), but for the FTU it would make us replace a declarative translation with an imperative one (JS-side) — and I’m not sure the trade-off is worth the trouble.

The good thing in this approach (besides the fact that the string makes more sense to translate, imho) is that links can be updated without bothering l10n contributors — as I understood, we prefer to use “international” links and rely on the server to serve the appropriate locale.

Choose the approach you prefer. I’ll trust your judgement on this.

> Also, any idea why we use external-XXX.title? I thought about
> screen-readers, but then I noticed not all links have the attribute set.

Because we can! :-)
Seriously, I have no idea. Eitan, should we keep these title attributes for a11y, or can we remove them?
Flags: needinfo?(kaze) → needinfo?(alizonmeeejeni)
(Assignee)

Comment 9

5 years ago
Thanks Kaze, tentatively assigning this bug to myself so it's not forgotten, especially considering how old it is.

> The good thing in this approach (besides the fact that the string makes more sense to 
> translate, imho) is that links can be updated without bothering l10n contributors — 
> as I understood, we prefer to use “international” links and rely on the server to 
> serve the appropriate locale.

Absolutely. Also, less chances to screw up things for localizers ;-)
Assignee: nobody → francesco.lodolo
You’re making impressive progress in JS. Have fun with this one! ^^

(In reply to Francesco Lodolo [:flod] from comment #9)
> Also, less chances to screw up things for localizers ;-)

You said it. :-D
(Assignee)

Comment 11

5 years ago
Currently working on another branch for the JS version. Everything seems to work fine, still thinking if there's something to test before opening a pull request (also waiting to see about titles).

Some notes on comment 8

(In reply to Fabien Cazenave [:kaze] — on PTO until 2013-03-14 from comment #8)
>   function getLocalizedLink(key) {
>     var link = document.createElement('a');
>     // this has to be synchronous => we can't use mozL10n.localize() here
>     link.href = linkRefs[key];
>     link.textContent = _(key);
>     return link.innerHTML;
>   }

At this point link.innerHTML contains only _(key), not the full HTML code.

>   navigator.mozL10n.localize(learnMoreElement, 'learn-more-at', {
>     link: getLocalizedLink('external-telemetry')
>   });

Since the plan is to inject HTML code (the anchor), I need to use .get and innerHTML, or at least I couldn't find alternative looking into l10n.js

For a first try it doesn't seem to create any problem: start FTE in Italian, go back and switch to English, strings are displayed in the correct language.
(Assignee)

Comment 12

5 years ago
Created attachment 8388355 [details] [review]
Pull request with placeholders for links

Opening pull request, still not asking f/r since I need an answer about titles and a11y.

Note about the PR: I changed 2 links (information, telemetry) from http:// to https://, since the privacy one is already using https://
(Assignee)

Updated

5 years ago
Duplicate of this bug: 818347
(Assignee)

Comment 14

5 years ago
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

Since removing titles would be trivial in the current pull request, I'll start setting review and feedback to get a first check of the code/approach.
Attachment #8388355 - Flags: review?(fernando.campo)
Attachment #8388355 - Flags: feedback?(kaze)
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

I'd like to hear kaze before landing it, but code looks good, and tests works perfectly.

Thanks Francesco
Attachment #8388355 - Flags: review?(fernando.campo) → review+
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

f- mostly because I’d much prefer to rely on mozL10n.localize and explicitely declare that we’re using `.innerHTML` in the *.properties file.

Besides, `external_links.js` breaks the linter — and more generally, I’m not 100% pleased with the approach: using `linkText` to store either the .textContent or an l10n ID is not very clean, and I’d prefer to use a more generic way to handle the link properties.

I can suggest something like this (untested):

  function getLocalizedLink(key) {
    'use strict';

    // external links definition
    var refs = {
      'learn-more-telemetry': {
        href: 'https://www.mozilla.org/telemetry/',
        textContent: 'www.mozilla.org/telemetry/',
        className: 'external'
      },
      'learn-more-information': {
        href: 'https://www.mozilla.org/privacy/',
        textContent: 'www.mozilla.org/privacy/',
        className: 'external'
      },
      'learn-more-privacy': {
        href: 'https://www.mozilla.org/privacy/firefox-os/',
        l10nId: 'learn-more-privacy-link',
        className: 'external'
      }
    };

    // return the HTML code for link "key"
    var link = document.createElement('a');
    var linkRef = refs[key];
    for (prop in linkRef) {
      if (prop == 'l10nId') {
        link.textContent = _(linkRef.l10nId);
      } else {
        link[prop] = linkRef[prop];
      }
    }
    return link.outerHTML;
  }
Attachment #8388355 - Flags: feedback?(kaze) → feedback-
BTW: let’s not use the _() function in this `external_links.js` helper, and please use the full `mozL10n.get()` function instead. This removes an unnecessary constraint on the load order and keeps the file more readable / self-contained.
(Assignee)

Comment 18

5 years ago
Thanks, I had no idea I could mozL10n.localize like that.

Code, and consequently test, updated using your suggestions. Since I'm a known champion at messy-code-indentation, there's probably something to fix there (I wish js-linter could help me).
(Assignee)

Updated

5 years ago
Attachment #8388355 - Flags: feedback- → feedback?(kaze)
(Assignee)

Comment 19

5 years ago
Linter is still failing

> apps/communications/ftu/js/external_links.js: line 1, col 26, 'getLocalizedLink' is defined but never used. (ERROR)

I wonder if the only solution is to move the back in navigation.js where it started.
(Assignee)

Comment 20

5 years ago
(In reply to Francesco Lodolo [:flod] from comment #19)
> Linter is still failing
Link for reference
https://travis-ci.org/mozilla-b2g/gaia/builds/20957257
(Assignee)

Comment 21

5 years ago
(In reply to Francesco Lodolo [:flod] from comment #20)
> (In reply to Francesco Lodolo [:flod] from comment #19)
> > Linter is still failing
> Link for reference
> https://travis-ci.org/mozilla-b2g/gaia/builds/20957257

Linter problem fixed adding a comment in the file (thanks kaze for the tip).
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

f=me with nits addressed. Good work Francesco!
Attachment #8388355 - Flags: feedback?(kaze) → feedback+
(Assignee)

Comment 23

5 years ago
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

Fernando, mind taking a second look at the changes? Code has changed quite a bit from the first review, so I think it would be safer to have a new review after Kaze's feedback.
Attachment #8388355 - Flags: review+ → review?(fernando.campo)
Comment on attachment 8388355 [details] [review]
Pull request with placeholders for links

Everything is still looking good, thanks both for taking the time to fix this.
Attachment #8388355 - Flags: review?(fernando.campo) → review+
(Assignee)

Comment 25

5 years ago
Green run of Travis, setting checkin-needed
https://travis-ci.org/mozilla-b2g/gaia/builds/21022287

Also removing late-l10n keyword, probably survived from previous triage.
Status: NEW → ASSIGNED
Keywords: late-l10n → checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/e0add8a95f4d1ca26f3bf83d32c58cab7a16826b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g-v1.4: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S4 (28mar)
status-b2g-v1.4: fixed → ---
(Assignee)

Comment 27

5 years ago
Verified on Keon, Git commit 3977de3c 2014-03-24
Status: RESOLVED → VERIFIED

Updated

4 years ago
Depends on: 991951
Flags: needinfo?(alizonmeeejeni)
You need to log in before you can comment on or make changes to this bug.