1561091 - Refactor getFormattedMessage to set fluent attributes or use plain text

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P1)

Description

[Ed Lee :Mardak]

Split from bug 1558550 https://github.com/mozilla/activity-stream/pull/5125#pullrequestreview-253614166

We can use a shared component instead of copy/pasting the helper function.

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/5127

Comment 2

[Axel Hecht [:Pike]]

Whenever we have API wrappers, we make it harder to do static analysis on our code. In this particular case, that might already be broken, as the IDs and the call sites are rather separated?

I'd like to challenge the existence of this wrapper still, though, or at least discuss if the API use-case is common enough to upstream it, or at least centralize it.

Fluent does fall back to the ID, something like <span data-l10n-id="Just show this text" /> should just render the text, right?

Is this to avoid namespace clashes? To get trees green (I think we fail tests on id fallback?)?

Do we have any pattern in tree to at least share and/or centralize API helpers like this?

If we need this API, can we alleviate some of my concerns around static analysis, and settle for property names that align with our API names? Notably, l10nId and l10nArgs match <span data-l10n-id="foo" data-l10n-args="{}" /> when looking at node.dataset.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'd like to challenge the existence of this wrapper still, though, or at least discuss if the API use-case is common enough to upstream it, or at least centralize it.

I think it is. People have been consistently introducing and using this particular concept since Firefox OS days:

• https://github.com/mozilla-b2g/gaia/blob/975a35c0f5010df341e96d6c5ec60217f5347412/apps/settings/js/panels/icc/icc.js#L82
• https://github.com/mozilla-b2g/gaia/pull/27523/files
• https://searchfox.org/mozilla-central/source/toolkit/content/aboutUrlClassifier.js#308
• https://searchfox.org/mozilla-central/source/toolkit/content/aboutUrlClassifier.js#428
• https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.js#1728
• https://searchfox.org/mozilla-central/source/browser/components/preferences/siteDataSettings.js#41

Fluent does fall back to the ID, something like <span data-l10n-id="Just show this text" /> should just render the text, right?

After attempting to load the whole possible fallback chain searching for this ID.

Do we have any pattern in tree to at least share and/or centralize API helpers like this?

I'd consider adding one. I'm not sure where tho. Starting with a one shared by all AS could be a good starting point.

Comment 4

[Ed Lee :Mardak]

(In reply to Axel Hecht [:Pike] from comment #2)

Whenever we have API wrappers, we make it harder to do static analysis on our code. In this particular case, that might already be broken, as the IDs and the call sites are rather separated?

This bug is just fixing up the 3 calls of 2 duplicated helper functions in CollapsibleSection.jsx and Sections.jsx where a Section can be defined as a built-in section using packaged strings or via a webextension providing section definition programatically with plain strings.

Most of the jsx declaration usage of fluent just plain set data-l10n-id on the html elements, so the helper refactored here is specifically for "other" sources including pocket/remote layout definitions.

Fluent does fall back to the ID, something like <span data-l10n-id="Just show this text" /> should just render the text, right?

At least currently it seems like it just prints out a warning and results in no text content:
<h3 data-l10n-id="Hello World"></h3>
[fluent] Missing translations in en-US: Hello World.

Comment 5

[Ed Lee :Mardak]

I do think it would be nice in the common case to be able to just specify a string to get either fluent translation or plain string. This means pocket server API format is unchanged -- "return a string" so it might currently return "How it works" but could just return "newtab-pocket-how-it-works" instead. (Although I guess the server needs to be more aware of what's the string ids available and if they change…)

Comment 6

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/885c1d54bcb9