1105648 - Refactor SystemBanner's L10n API use

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Gabriele Svelto [:gsvelto]]

L10n strings shouldn't contain formatting characters such as '\n'. One is present in the `low-device-storage' string, see here:

https://github.com/mozilla-b2g/gaia/blob/85739c1733d7437b880084e50153f4c788812cdf/apps/system/locales/system.en-US.properties#L491

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: PR

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8537467 [details] [review]
PR

Thank you Gregor!

I haven't tried the patch but I guess that removing the "\n" from that string will make the notification to look like: "Device storage low. Less than..." all in the same line, while UX defined it to be in two lines. Check the screenshot at [1].

What I would do is to get rid of the low-device-storage string, add the "Device storage low." phrase to free-space [2] and unknown-free-space [3] strings and then do something like:

[...]
free-space.innerHTML[one]=<span>Device storage low.</span><span>Less than {{value}} {{unit}} remaining.</span>
[...]

I a pretty bad at l10n stuff so there might be a better way. What do you think?

[1] https://bug861921.bugzilla.mozilla.org/attachment.cgi?id=746407
[2] https://mxr.mozilla.org/gaia/source/apps/system/locales/system.en-US.properties#495 
[3] https://mxr.mozilla.org/gaia/source/apps/system/locales/system.en-US.properties#502

Comment 3

[Francesco Lodolo [:flod]]

Issue with the current PR: you need a new string ID, otherwise nobody will know that they need to remove the stray \n

I would honestly avoid using innerHTML to display a message on two lines (especially considering that we're trying to move away from it). Can't we just use 2 separate strings and manage the markup in the code?

Comment 4

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

Using .innerHTML is not that bad. The problem is that it will not work in this case, because SystemBanner API doesn't use declarative L10n approach and does not set the L10nId/L10nArgs on the Node. Instead it takes the resolved string and assign it to textContent of the node. This will not work well.

What I'm thinking about is that it seems that would make sense to refactor SystemBanner API to follow [1] and then it should be easy to decide if we want to extend the API to support two-liners or use entity.innerHTML.

(I'd vote for extending the API to support two liners :))

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

Ferjm: If you're ok with this, I'd be happy to take this bug and use it to refactor SystemBanner and its 5 uses.

Comment 5

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

Oh, that's actually a question to gwagner.

Comment 6

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

Attachment: pull request

This is just a proof of concept. I didn't fix tests yet.

Let me know what you think :)

Comment 7

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

Comment on attachment 8538062 [details] [review]
pull request

Fernando, would you be willing to look at this patch?

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8538062 [details] [review]
pull request

LGTM. Thanks!

Comment 10

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

Commit: https://github.com/mozilla-b2g/gaia/commit/b4c3cd25a17b31dea4a0447bdef8e5fe0522b54c
Merge: https://github.com/mozilla-b2g/gaia/commit/b09746dadf8beafd3ee16b830ca04ce4763b2a30

Comment 11

[Stefan Plewako [:stef]]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #3)
> Issue with the current PR: you need a new string ID, otherwise nobody will
> know that they need to remove the stray \n

Wow… :(

Comment 12

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

Stef, sorry, but I believe that your comment is useless as it doesn't explain what you mean. Also, you should not modify the state of the bug if you're not an owner.

Comment 13

[Stefan Plewako [:stef]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> Stef, sorry, but I believe that your comment is useless as it doesn't
> explain what you mean. Also, you should not modify the state of the bug if
> you're not an owner.

OK, I'm sorry, I didn't meant to be pointlessly disruptive and I won't modify the state of the bugs I'm not assigned to.

Comment 14

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

I chatted with Stef on IRC. He referred to comment 3 where Flod pointed out that the patch removed trailing "\n".

We tested it and came to the conclusion that the trailing "\n" does not affect the UI because HTML removes it. So we can safely keep the entity ID.