With bug 1171206 we made a huge step forward to get SMS use modern L10n API, but there are a couple small leftovers that I would like to fix.

That's all I can see now. I updated the Attachment.name based on what you wrote here: https://github.com/mozilla-b2g/gaia/pull/30395#discussion_r32092689 I didn't update tests yet, since I'd prefer to get confirmation that this is the right direction.

Hey Zibi, Looks like patch for bug 1171206 broke placeholder in messages input (it has just disappeared). Is this "part 2" patch going to fix this as well, or I should file new bug? Thanks!

Comment on attachment 8638952 [details] pull request Added some comments on github; and some questions and suggestions as well :) thanks for this work !

(In reply to Oleg Zasypkin [:azasypkin] from comment #2) > Looks like patch for bug 1171206 broke placeholder in messages input (it has > just disappeared). Is this "part 2" patch going to fix this as well, or I > should file new bug? Is it just the "Messages" placeholder in new composer view? If so, then I just caught it and will add it to this patch. If there's more regression then let me know! Overall, we need to figure out how to tackle that. Stas, the scenario is like this, SMS wants sth like this: .properties: newMessage.placeholder = Message .html: <div id="newMessage" data-l10n-id="newMessage"></div> .css: #newMessage:after { content: attr(placeholder); } The problem here is that HTML doesn't allow for placeholder in div, so we can replace it with dataset.placeholder (newMessage.dataPlaceholder in .properties), but we don't allow for that in div. Can we whitelist data-* in localization?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4) > Can we whitelist data-* in localization? Certain frontend frameworks use data-* attributes to program behavior which made us err on the safe side when we designed the DOM bindings. This is why localizing data-* attributes is not currently allowed. We planned to make it possible to change that on-demand in bug 922573. Should we revive it or maybe reconsider the initial decision?

(In reply to Staś Małolepszy :stas from comment #5) > Certain frontend frameworks use data-* attributes to program behavior which > made us err on the safe side when we designed the DOM bindings. This is why > localizing data-* attributes is not currently allowed. > We planned to make it possible to change that on-demand in bug 922573. > Should we revive it or maybe reconsider the initial decision? Adding what's suggested in bug 922573 sounds like a medium level change. I need to fix the regression I introduced fast. I see three ways: 1) Allow for div.placeholder in l10n.js/l20n.js 2) Allow for div.dataPlaceholder in l10n.js/l20n.js 3) Add artificial input there, localize its placeholder and take it for the CSS::after 4) Revert my patch I would love to avoid option 4 :( Do you have any recommendation on what should I do here?

Comment on attachment 8638952 [details] pull request Tests are green. I still need to decide how to fix the placeholder regression, but the rest should be reviewable.

The placeholder attribute is specific to the input element: http://www.w3.org/TR/html5/forms.html#the-placeholder-attribute IIUC, it's not valid to add it to divs. Can we use title instead? .properties: newMessage.title = Message .html: <div id="newMessage" data-l10n-id="newMessage"></div> .css: #newMessage:after { content: attr(title); }

Love it! Thanks so much Stas!

Updated the patch and rebased it on top of master. I believe it's ready.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4) > (In reply to Oleg Zasypkin [:azasypkin] from comment #2) > > Looks like patch for bug 1171206 broke placeholder in messages input (it has > > just disappeared). Is this "part 2" patch going to fix this as well, or I > > should file new bug? > > Is it just the "Messages" placeholder in new composer view? If so, then I > just caught it and will add it to this patch. > Yeah, I was only talking about "Message" placeholder :) Thanks!

I squeezed in one more change to accommodate for bug 1027117.

(In reply to Staś Małolepszy :stas from comment #8) > The placeholder attribute is specific to the input element: > > http://www.w3.org/TR/html5/forms.html#the-placeholder-attribute > > IIUC, it's not valid to add it to divs. Can we use title instead? I think we still used the "placeholder" attribute because we assumed this could be supported at one point by "[contenteditable=true]" element. At least this was not completely absurd :) "title" has accessibility consequences, I'd like to ask Yura his thoughts. > > .properties: > > newMessage.title = Message > > .html: > > <div id="newMessage" data-l10n-id="newMessage"></div> > > .css: > > #newMessage:after { > content: attr(title); > } Hey Yura, do you think having this information as a "title" attribute makes sense? I personally think it's a good thing (because the value was not accessible to screen readers before that), but I'd like your stamp first.

Looked at the patch, I just wonder if the changes in activity_handler.js are useful since we're using the result in a Notification. But it's not a blocker for me. Just waiting for Yura's answer and it will be r=me after this, if Yura is fine with the change.

(In reply to Julien Wajsberg [:julienw] from comment #13) > (In reply to Staś Małolepszy :stas from comment #8) > > The placeholder attribute is specific to the input element: > > > > http://www.w3.org/TR/html5/forms.html#the-placeholder-attribute > > > > IIUC, it's not valid to add it to divs. Can we use title instead? > > I think we still used the "placeholder" attribute because we assumed this > could be supported at one point by "[contenteditable=true]" element. At > least this was not completely absurd :) > > "title" has accessibility consequences, I'd like to ask Yura his thoughts. > > > > > .properties: > > > > newMessage.title = Message > > > > .html: > > > > <div id="newMessage" data-l10n-id="newMessage"></div> > > > > .css: > > > > #newMessage:after { > > content: attr(title); > > } > > Hey Yura, do you think having this information as a "title" attribute makes > sense? I personally think it's a good thing (because the value was not > accessible to screen readers before that), but I'd like your stamp first. Hi Julien, absolutely. In fact the only reason we are using aria-label everywhere across Gaia is so we are consistent and do not overload Gaia devs with various patterns of labeling things (I realize you are changing it from placeholder here). Title should work well here. Thanks!

(In reply to Julien Wajsberg [:julienw] from comment #14) > Looked at the patch, I just wonder if the changes in activity_handler.js are > useful since we're using the result in a Notification. But it's not a > blocker for me. The reason why I prefer is because it pushes the resolving to the latest possible moment, right before displaying. I believe we should write our code so that it carries l10n-ids around, not strings. So that complies with that principle. On top of that, we may want to do more with Notifications in the future to make them retranslatable, and having l10n-id passed to NotificationHelper will make it possible.

Commit: https://github.com/mozilla-b2g/gaia/commit/751465806132624b0f496aa3e8c1ff0512c7d0a8

Gandalf, I don't see the placeholder changes in your commit ? For me on master it's still broken. Should I file a separate bug ?

Comment on attachment 8652051 [details] [gaia] zbraniecki:1187627-fix-placeholder > mozilla-b2g:master r=me let's land this can you please rebase on latest master so that we can have a reasonnably green try ?

Commit https://github.com/zbraniecki/gaia/commit/b696b12997144e0de03ef316e134f2ca4b5a3b3d