883813 - [SMS][MMS] Placeholder string "Message" is not localizable

Status: RESOLVED FIXED

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

Description

[Leo]

Title:Some tokens are not translated in Portugues and Spanish Languages
Description:
(1)Message - New Messsage : To, Message

Version Info
Mozilla build ID: 20130526070207
Gaia Revision : 4d10e1297b859cacc174c0a54af61a7678d7c32d
Gecko Revision : 52341e43539a0e8b9aa77a9128cc3871439b8aa6

Comment 1

[Leo]

Attachment: Screenshot attached

Comment 2

[Mike Pennisi [:jugglinmike]]

This bug is not limited to the Spanish and Portuguese localizations: the strings identified in Comment 0 are hard-coded as English. I'll begin work on this right away

Comment 3

[Mike Pennisi [:jugglinmike]]

Attachment: Fix localization issues

As far as I can tell, the "To:" message can already be localized--that must simply be an omission in the Spanish and Portuguese localization files. Setting it will surface an unreported problem: the "To" field's width is hard-coded. This patch preemptively resolves that issue.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 763865 [details] [diff] [review]
Fix localization issues

Review of attachment 763865 [details] [diff] [review]:
-----------------------------------------------------------------

> Because the Compose placeholder is rendered from the value of the
> `contenteditable` element's "placeholder" attribute, the localized
> string cannot be declaratively specified in the markup.

It actually can, just use "composeMessage.placeholder" inside your locale file.

::: apps/sms/index.html
@@ +144,5 @@
>            <button id="messages-attach-button" class="icon icon-attachment">Attachment</button>
>            <button id="messages-send-button" disabled data-l10n-id="send" type="submit">Send</button>
>            <!-- The <br> is needed because of 414223, which is a WONTFIX. It's just because of how contentEditable works. https://bugzilla.mozilla.org/show_bug.cgi?id=414223 -->
> +          <!-- This element's placeholder will be set to the locale-appropriate value at runtime. -->
> +          <div id="messages-input" contentEditable="true" name="message"><br></div>

so you can revert this

::: apps/sms/js/compose.js
@@ +136,5 @@
>        dom.attachButton = document.getElementById('messages-attach-button');
>        dom.optionsMenu = document.getElementById('attachment-options-menu');
>  
> +      dom.message.setAttribute('placeholder',
> +        navigator.mozL10n.get('message'));

so you can remove this

::: apps/sms/locales/sms.en-US.properties
@@ -33,5 @@
>  to                         = To:
>  send                       = Send
>  new                        = New
>  message                    = Message
> -composeMessage.placeholder = Message

I see it was there already ;) please revert this, this should work !

if it's not, we have another problem

::: apps/sms/style/sms.css
@@ -713,5 @@
> -  width: 2.5rem;
> -  position: absolute;
> -  top: 1.5rem;
> -  text-overflow: ellipsis;
> -  overflow: hidden;

we probably should keep the overflow: hidden

Comment 5

[Mike Pennisi [:jugglinmike]]

> ::: apps/sms/locales/sms.en-US.properties
> @@ -33,5 @@
> >  to                         = To:
> >  send                       = Send
> >  new                        = New
> >  message                    = Message
> > -composeMessage.placeholder = Message
> 
> I see it was there already ;) please revert this, this should work !
> 
> if it's not, we have another problem

I think we have another problem. I can't seem to get this working. I will investigate the shared "localization.js" script's behavior and report back here when I learn what seems to be the problem

Comment 6

[Mike Pennisi [:jugglinmike]]

It looks like "localization.js" only supports localizing DOM properties. Since `placeholder` is not a valid property of a `div` element, setting this value as a property does not persist.

I've filed bug 884489 and submitted a patch to address this behavior. Since the rest of the changes relate to an entirely different issue (the hard-coded width of the "To:" field), I've filed Bug 884512 to track that work.

Comment 7

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: c52a0694112ced6e3f41af65be5645223b9b7ad7
1.1hd: c52a0694112ced6e3f41af65be5645223b9b7ad7

Comment 8

[Mike Pennisi [:jugglinmike]]

Attachment: Fix localization issues v.2

Now that bug 884489 and bug Bug 884512 have landed, all that remains to address this bug is using the HTML5 "data-" attributes to specify the value of the "Compose" field's placeholder.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 769844 [details] [diff] [review]
Fix localization issues v.2

Review of attachment 769844 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks

please wait before merging, I'm asking if we need a review from the l10n team because of the key change.

Comment 10

[Julien Wajsberg [:julienw]]

resetting flags because these uplifts were really bug 884512.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 769844 [details] [diff] [review]
Fix localization issues v.2

Requesting review from stas because we're past June 30th.

Stas: this change is purely technical: we don't change the actual string, we only change the key, thanks to Bug 884489 change. Is it okay to uplift, or is it too late ?

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 769844 [details] [diff] [review]
Fix localization issues v.2

Maybe Pike is more aware ? see comment 11 please

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 769844 [details] [diff] [review]
Fix localization issues v.2

Review of attachment 769844 [details] [diff] [review]:
-----------------------------------------------------------------

I'm surprised this is needed, we're using foo.placeholder in many places in gaia, https://mxr.mozilla.org/gaia/search?string=placeholder&find=apps

What's the difference between those code paths and what we're doing here?

Comment 15

[Francesco Lodolo [:flod]]

Using decent bug subjects would save us a lot of time, seriously.

Bug 885704 was even triaged for leo+, and then we have a fix here which I accidentally discovered by following pike's bugmail.

Comment 16

[Julien Wajsberg [:julienw]]

Francesco: agreed.

Pike: this works when "placeholder" is a valid property of the object. Here, we were setting it as an additional non-standard attribute, and we converted it to a dataset property instead, because we didn't want the l10n library to translate DOM attributes in addition to properties.

So in Bug 884489 we now support translating dataset and style properties, and that's how it's done here.

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 769844 [details] [diff] [review]
Fix localization issues v.2

Review of attachment 769844 [details] [diff] [review]:
-----------------------------------------------------------------

'k. clearing review flag then, as a real review isn't required.

For context, we posted which flags to use for what on https://groups.google.com/forum/#!topic/mozilla.dev.gaia/_T_BTvJ4fJk. I think you're interested in triage/schedule, and I can only say, it's well past late, string changes were supposed to be done last month. See also https://groups.google.com/forum/#!topic/mozilla.dev.gaia/SrFrWrBNl20 by dietrich.

Comment 18

[Julien Wajsberg [:julienw]]

ok, let's land this then.

Comment 19

[Mike Pennisi [:jugglinmike]]

Re-named and landed on `master` at commit 1c3c806721b973b03d083cd36f9ca338cedeb34f

Thanks for the review, Julien!

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  1c3c806721b973b03d083cd36f9ca338cedeb34f
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 21

[Julien Wajsberg [:julienw]]

v1-train: 304d2cdb82a1d7c70c405ea02f88c46b5364860e

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 304d2cdb82a1d7c70c405ea02f88c46b5364860e