1030280 - Move SIMCard dialog from injecting textContent to setting data-l10n-id attributes.

Status: RESOLVED FIXED

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

Description

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

As part of an ongoing effort in bug 1020138 we should move away from injecting textContent manually in SIMCard dialog.

The code is here: https://github.com/mozilla-b2g/gaia/blob/38cd96b0cd191b21f6e8744e62392843e8415f68/apps/system/js/simcard_dialog.js

All cases like this:

this.desc.textContent = _(lockType + 'Code');

should be switched to:

this.desc.setAttribute('data-l10n-id', lockType + 'Code');

Comment 1

[Jeremiah Lee]

I've taken a stab at it, but have a few questions about how to handle a few of the cases where the 'data-l10n-id' attribute is being used. I've linked to a Gist with comments. If you have a moment to clarify, I'd greatly appreciate your help.

https://gist.github.com/jeremiahlee/26ea1531a7d7d88f9d94

Comment 2

[Manuel Casas Barrado [:mancas]]

Attachment: Setting data-l10n-id dynamically

Comment 3

[Mike Hoye [:mhoye]]

Gandalf, can you take a look at this bug, particularly commnent 1?

Comment 4

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

Comment on attachment 8454361 [details] [review]
Setting data-l10n-id dynamically

The patch looks good, but it would be better to use mozL10n.setAttributes (same signature). We're phasing away mozL10n.localize and replacing it with setAttributes. (See bug 994519)

Comment 5

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

(In reply to Jeremiah Lee from comment #1)
> I've taken a stab at it, but have a few questions about how to handle a few
> of the cases where the 'data-l10n-id' attribute is being used. I've linked
> to a Gist with comments. If you have a moment to clarify, I'd greatly
> appreciate your help.
> 
> https://gist.github.com/jeremiahlee/26ea1531a7d7d88f9d94

Sure, sorry for the delay:

>  // this.triesLeftMsg.textContent = _('inputCodeRetriesLeft', l10nArgs);
> this.triesLeftMsg.setAttribute('data-l10n-id', 'inputCodeRetriesLeft', l10nArgs); // Unsure if I can pass more args here

No, you can't. That's why we introduced navigator.mozL10n.setAttributes(node, l10nId, l10nArgs); Use it here :) If you don't have args, then using node.setAttribute('data-l10n-id', l10nId); is cheaper.

> // this.errorMsgHeader.textContent = _('simCardLockedMsg') || '';
> this.errorMsgHeader.setAttribute('data-l10n-id', 'simCardLockedMsg'); // Does this default to an empty string, as done above?

No, but you should ask yourself what the old code is trying to achieve. It literally assigns empty string in case it receives an empty string...

So, your replacement is good!

>  this.errorMsgHeader.setAttribute('data-l10n-id', 'newPinErrorMsg');
> this.errorMsgBody.textContent = ''; // Ok to leave this one since no localization happening?

Hmm, if you clear textContent, you should also node.removeAttribute('data-l10n-id'), otherwise l10n.js will retranslate it later.

Great work!

Comment 6

[Manuel Casas Barrado [:mancas]]

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> Comment on attachment 8454361 [details] [review]
> Setting data-l10n-id dynamically
> 
> The patch looks good, but it would be better to use mozL10n.setAttributes
> (same signature). We're phasing away mozL10n.localize and replacing it with
> setAttributes. (See bug 994519)

Hi, now the patch uses mozL10n.setAttributes instead of mozL10n.localize

Comment 7

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

Comment on attachment 8454361 [details] [review]
Setting data-l10n-id dynamically

looks good now!

Comment 8

[Jeremiah Lee]

> var _ = navigator.mozL10n.setAttributes;

Is this an encouraged practice?

Comment 9

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

(In reply to Jeremiah Lee from comment #8)
> > var _ = navigator.mozL10n.setAttributes;
> 
> Is this an encouraged practice?

No, but I don't think that there's a strong enough reason to discourage it. I'd leave it to app authors to keep their code clean. If they want, they can also do var __ = console.log; and use __('foo'); everywhere if they like.

Comment 10

[Jeremiah Lee]

Attachment: Setting data-l10n-id dynamically w/o underscore

Submitting if the author prefers direct method calls.

Comment 11

[Manuel Casas Barrado [:mancas]]

Master: https://github.com/mozilla-b2g/gaia/commit/50a6de7244d455b1870ed915ee1e6800c7c3c575