We currently have a number of apps that (mostly unintentionally) use the heuristics of DOM Node localization that we're trying to get rid of (see bug 1053629 for details). In particular it refers to scenarios like this: <div data-l10n-id="foo"> Some translatable text <p data-l10n-id="foo2">Some more translatable text</p> </div> The paradigm of client-side DOM localization that L20n brings is that every localizable node has its content provided by the localization (probably via Shadow DOM or similar "L10n DOM" mechanism), which means that there should be no child nodes. In the future, we will support localization of whole DOM Fragments via mechanism called DOM Overlays (bug 994357), but first we need to clean up the current use cases. The way to clean it up is to switch the above example to: <div> <span data-l10n-id="foo">Some translatable text</span> <p data-l10n-id="foo2">Some more translatable text</p> </div>

This patch fixes all occurrences of nodes in HTML which have data-l10n-id with localizable text and child nodes. There is one case left in communications and it is related to importSim2 l10nId. This code is generated dynamically and it overlaps with a patch that is about to land (bug 1052136). I'll wait for that patch to land, add this case to this patch and start asking for reviews.

Ben - review request for you since you're peer of Contacts which is the only piece that changes more than adding <span/> :) As I indicated in my previous comment, I intend to also update SIM DOM generator [1] once bug XXX gets fixed because with the fix, the code will assign data-l10n-id on the button with child nodes, so I'll just add a <span/>. Asking for review now to give you time to look at this. [1] https://github.com/mozilla-b2g/gaia/blob/6ff857140361989a2bf11f203f49e074aa85aed3/apps/communications/contacts/js/utilities/sim_dom_generator.js

and by bug XXX I meant bug 1052136 :)

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with two comments below and assuming you've tested these changes to rule out CSS regressions. Thanks! ::: apps/sms/index.html @@ +257,4 @@ > </div> > <div class="composer-button-container"> > <span id="messages-counter-label" data-counter="" data-l10n-id="mms-label">MMS</span> > + <button id="messages-send-button" class="icon" aria-label="send" type="submit"> It should be possible to localize the ARIA label here; I don't think removing data-l10n-id is correct? ::: tests/python/gaia-ui-tests/gaiatest/apps/contacts/regions/settings_form.py @@ +14,4 @@ > _order_by_last_name_locator = (By.CSS_SELECTOR, 'p[data-l10n-id="contactsOrderBy"]') > _order_by_last_name_switch_locator = (By.CSS_SELECTOR, 'input[name="order.lastname"]') > _import_from_sim_button_locator = (By.CSS_SELECTOR, "li[id*='import-sim-option'] button") > + _import_from_sdcard_locator = (By.CSS_SELECTOR, 'button.icon-sd') There are two elements in apps/communications/contacts/elements/setting.html that match the 'button.icon-sd' selector. I think we should be more specific here?

Comment on attachment 8486157 [details] [diff] [review] patch Sorry, I'm not going to have time for this review. I'm under the gun for a deadline and I have not looked at contacts app for a while now. I should probably drop my contacts peer status.

The PR bitrotted a bit. I rebased PR on top of the master.

Comment on attachment 8486157 [details] [diff] [review] patch Again, I'm a bit overloaded but this patch should be in master ASAP, Jose would you mind to take a look? Thanks!

Comment on attachment 8486157 [details] [diff] [review] patch thanks Gandalf

disclaimer: I have only reviewed the contacts part. I think it is needed other reviews from the peers of the other affected apps

Jose, since you r+'ed this patch first and not the Communications one, I'd like to land it along with the patch to importSim2. Can you review this small patch?

(In reply to Zibi Braniecki [:gandalf] from comment #10) > Created attachment 8490923 [details] [diff] [review] > follow up patch > > Jose, since you r+'ed this patch first and not the Communications one, I'd > like to land it along with the patch to importSim2. Can you review this > small patch? Hello Gandalf, To be clear, I r+ed only the comms part. You should ask for review to other peers in the rest of apps. Concerning the new patch, please could you provide a PR in order to make the review easier thanks!

Comment on attachment 8490923 [details] [diff] [review] follow up patch cancelling review as per the comment above

(In reply to Jose Manuel Cantera from comment #8) > Comment on attachment 8486157 [details] [diff] [review] > patch > > thanks Gandalf r=me for the comms/contacts part

(In reply to Jose Manuel Cantera from comment #11) > To be clear, I r+ed only the comms part. You should ask for review to other > peers in the rest of apps. Yup! Requesting reviews from others :) > Concerning the new patch, please could you provide a PR in order to make the > review easier The PR linked in the attachments has both patches.

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- Asking for review from owners/peers of all affected apps.

Comment on attachment 8486157 [details] [diff] [review] patch I'll forward the homescreen changes over to Cristian. At this point I think we should consider this legacy code, but we do still ship it on tablet.

Comment on attachment 8486157 [details] [diff] [review] patch Browser app changes are fine, but a bit pointless as it's about to get deleted in bug 1043959.

> We currently have a number of apps that (mostly unintentionally) use the heuristics of DOM > Node localization that we're trying to get rid of (see bug 1053629 for details). Actually, I think it was quite intentional because there was intentional support in l10n.js. That said, I don't really mind :)

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/index.html @@ +257,4 @@ > </div> > <div class="composer-button-container"> > <span id="messages-counter-label" data-counter="" data-l10n-id="mms-label">MMS</span> > + <button id="messages-send-button" class="icon" aria-label="send" type="submit"> What Staś said: we need to localize the aria label. I don't have a good solution for you though. That said, since we don't localize the content, maybe this doesn't go in the way of your changes?

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- Asking for review from owners/peers of all affected apps. ::: apps/sms/index.html @@ +257,4 @@ > </div> > <div class="composer-button-container"> > <span id="messages-counter-label" data-counter="" data-l10n-id="mms-label">MMS</span> > + <button id="messages-send-button" class="icon" aria-label="send" type="submit"> Ah, I know why I removed it! The problem is that there is no send-button entity in our repository, so you don't do this. If you want to readd send-button.ariaLabel, then sure, it will work and it will not go in the way of my changes. :)

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- Per IRC conv, we found a better way to fix it for SMS and julien is going to implement it in another patch. (adding ariaLabel). I'll remove the change to sms in PR.

Comment on attachment 8486157 [details] [diff] [review] patch Review of attachment 8486157 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/ftu/index.html @@ +590,4 @@ > <label class="pack-checkbox"> > <input type="checkbox" id="share-performance" name="debug.performance_data.shared"/> > <span></span> > + <p> You need to rebase to pick up changes here, see: https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/index.html#L596

Comment on attachment 8486111 [details] pull request Moving the remaining review requests to PR since it has the updated code.

Comment on attachment 8486111 [details] pull request No need for ftu review anymore.

Comment on attachment 8486111 [details] pull request LGTM, thanks

Commit: https://github.com/zbraniecki/gaia/commit/4d544a6cc2bdd0e842e168ac145343b53a26f69c Merge: https://github.com/mozilla-b2g/gaia/commit/dbc8904144e679c14dd577eb235912392f396499 Keeping it open for today. Tomorrow I'll try to verify if there's anything else to do in this bug.

seems like we're done here