Closed Bug 820344 Opened 13 years ago Closed 11 years ago

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

Categories

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
1.4 S4 (28mar)

People

(Reporter: petercpg, Assigned: flod)

References

Details

(Keywords: l12y, Whiteboard: LocRun1.3, LocRun1.2)

Attachments

(1 file)

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.
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
Blocks: 968853
Whiteboard: LocRun1.3 → LocRun1.3, LocRun1.2
(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
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.
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)
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
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.
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://
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.
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).
Attachment #8388355 - Flags: feedback- → feedback?(kaze)
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.
(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
(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+
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+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S4 (28mar)
Verified on Keon, Git commit 3977de3c 2014-03-24
Status: RESOLVED → VERIFIED
Depends on: 991951
Flags: needinfo?(alizonmeeejeni)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: