1629360 - switch Thunderbird away from sync calls to Localization

Status: NEW

(Thunderbird :: General, task, P1)

Description

[Magnus Melin [:mkmelin]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from bug 1629275 comment #9)

p.s. On a separate note, it is concerning to me to see how many sync Localization instances you have in Thunderbird. Localization should almost always be async. Every time you use sync you require sync I/O which we're trying to kill.
I'll maintain the sync approach, but would encourage Tb to move away from it wherever possible toward declarative, async, DOMLocalization API.

Comment 1

[Magnus Melin [:mkmelin]]

https://firefox-source-docs.mozilla.org/l10n/fluent/tutorial.html#runtime-localization

Comment 2

[Magnus Melin [:mkmelin]]

This patch lists (at the time) all offenders: https://hg.mozilla.org/comm-central/rev/63c6dce22725

Comment 3

[Magnus Melin [:mkmelin]]

Could be good to start off with one patch per case.

Comment 4

[Lasana Murray]

Each use of the sync API changed to the async one is going to result in the calling function becoming async (and functions that call it etc.). Promises also imply an error may occur and should be handled. How should I treat with that?

Comment 5

[Magnus Melin [:mkmelin]]

I didn't check too carefully, but I think that's not the case.
See comment 1. What you, likely, should do is e.g. for https://hg.mozilla.org/comm-central/rev/63c6dce22725#l6.39

-> instead do
document.l10n.setAttributes(button, "profiles-launch-profile-plain")

... and the rest will happen automatically.

Comment 6

[Lasana Murray]

I see, I was under the impression that API could not be used for some reason, hence the "formatValueSync" calls. I'll give it a shot. Thanks!

Comment 7

[Lasana Murray]

Attachment: bug1629360-1-use-async-fluent-api-on-remove-address-labels

It took a while to find a case that I can easily see the effects in the UI. This patch only touches the floating "x"s that appear in the compose window next to the Cc and Bcc etc. lists.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9153225 [details] [diff] [review]
bug1629360-1-use-async-fluent-api-on-remove-address-labels

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4060,5 @@
>   */
>  function updateTooltipsOfAddressingFields(row) {
>    let type = row.querySelector(".address-label-container > label").value;
> +  let el = row.querySelector(".aw-firstColBox > label");
> +  el.ownerDocument.l10n.setAttributes(el, "remove-address-row-type", { type });

I'm pretty sure just

document.l10n. ..... would do

::: mail/components/compose/content/messengercompose.xhtml
@@ +2188,5 @@
>                  <label role="button"
>                         onclick="closeLabelOnClick(event);"
>                         onkeypress="closeLabelOnKeyPress(event);"
>                         collapsed="true">
> +                  <image data-l10n-name="image" class="close-icon"/>

you don't need this

@@ +2226,5 @@
>                <hbox class="aw-firstColBox" align="top">
>                  <label role="button"
>                         onclick="closeLabelOnClick(event);"
>                         onkeypress="closeLabelOnKeyPress(event);">
> +                  <image data-l10n-name="image" class="close-icon"/>

I don't think you need these changes

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +4,5 @@
>  
>  # Addressing widget
>  
>  #   $type (String) - the type of the addressing row
> +remove-address-row-type = <image data-l10n-name="image"/>

I don't think you need this, the <image...> part.... The below should be enough.
remove-address-row-type = 
    .tooltiptext = Remove the { $type } field

@@ +5,5 @@
>  # Addressing widget
>  
>  #   $type (String) - the type of the addressing row
> +remove-address-row-type = <image data-l10n-name="image"/>
> +    .tooltiptext = Remove the { $type } field

When we change a l10n value, we need to update the key. (remove-address-row-type would become remove-address-row-type-label, or something)

Comment 9

[Lasana Murray]

Attachment: bug1629360B.patch

Recommend changes made.

Comment 10

[Lasana Murray]

Attachment: bug1629360C.patch

Comment 11

[Lasana Murray]

Attachment: [checked in] bug1629360D.patch

Comment 12

[Lasana Murray]

Attachment: [checked in] bug1629360E.patch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9153961 [details] [diff] [review]
bug1629360B.patch

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

r=mkmelin with the below

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +4,5 @@
>  
>  # Addressing widget
>  
>  #   $type (String) - the type of the addressing row
> +remove-address-row-type-label = 

please remove the trailing space

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9154282 [details] [diff] [review]
bug1629360C.patch

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

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +9,5 @@
>      .tooltiptext = Remove the { $type } field
>  
>  #   $type (String) - the type of the addressing row
>  #   $count (Number) - the number of address pills currently present in the addressing row
> +address-input-type-aria-label =

The "value" changes here but not the key. You need to update the key so that localizers would notice

@@ +19,4 @@
>  
>  #   $email (String) - the email address
>  #   $count (Number) - the number of address pills currently present in the addressing row
> +pill-labels =  

it's just the one label so this is a confusing naming

@@ +19,5 @@
>  
>  #   $email (String) - the email address
>  #   $count (Number) - the number of address pills currently present in the addressing row
> +pill-labels =  
> +    .label      = { $label }

I'd think this is not needed? Just don't set the label attribute and it would keep using whatever label it already has. Nothing to localize here.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9154287 [details] [diff] [review]
[checked in] bug1629360D.patch

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

Patch looks good.
(The way this localization is done is not really correct though, instead of tagging on words to the end localizers should be able to rearrange the words, like https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/browser/locales/en-US/browser/preferences/preferences.ftl#762)

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9154339 [details] [diff] [review]
[checked in] bug1629360E.patch

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

LGTM, r=mkmelin

Comment 17

[Lasana Murray]

Are you referring to the whitespace before $type? If so, that's how the comments were made in the file before the patch. They all line up.
https://searchfox.org/comm-central/source/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl#8

Regarding pills-label change. For some reason the label attribute does not remain when the aria-label is localized. So I manually added it.
I was wondering if it may have something to do with mail-address-pill being a custom element?

Comment 18

[Lasana Murray]

For functions like this one:
https://searchfox.org/comm-central/source/calendar/base/modules/calCalendarDeactivator.jsm#110

Where the localization is used to get a string and not update a DOM node directly, should I leave them as is or should I convert the function to use formatValue() ?

Here is another one:
https://searchfox.org/comm-central/source/chat/modules/OTRUI.jsm#29

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to Lasana Murray from comment #17)

Are you referring to the whitespace before $type?

No, the whitespace after the =, for remove-address-row-type-label

For appending notifications, see https://searchfox.org/mozilla-central/rev/baf1cd492406a9ac31d9ccb7a51c924c7fbb151f/browser/components/aboutlogins/AboutLoginsParent.jsm#789-792,806

The OTR case is more complex...

Comment 20

[Lasana Murray]

Attachment: [checked in] bug1629360B2.patch

Comment 21

[Lasana Murray]

Attachment: bug1629360F.patch

I used window.document instead of document here because it gave an ESLint warning. Added the calendar/calendar-widgets.ftl localization to the messenger.xhtml file so fluent can find the translations.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9155415 [details] [diff] [review]
bug1629360F.patch

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

Looks good, but let's have Paul review (since calendar/)

Comment 23

[Paul Morris [:pmorris]]

Comment on attachment 9155415 [details] [diff] [review]
bug1629360F.patch

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

Looks good, and worked when I tested it. It's too bad we need to do the whole `createDocumentFragment` dance just to get a localized string, but that appears to be the best option here.  Just a couple minor nits.  Great to move towards less sync I/O!

::: calendar/base/modules/calCalendarDeactivator.jsm
@@ +132,2 @@
>  
> +        notificationbox.appendNotification(messageFragment, value, null, priority, null);

Minor nits: please remove the empty line 132 and add an empty line above `let messageFragment ...` so it's clearer which code goes together.  

While we're add it, please add a comment just above `let messageFragment ...` saying something like: "Use Fluent's preferred async, declarative, DOMLocalization API."

Comment 24

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

Looks good, and worked when I tested it. It's too bad we need to do the whole createDocumentFragment dance just to get a localized string, but that appears to be the best option here.

Why do you need that?

     for (let [notificationbox, messageName] of notificationboxes) {
       let existingNotification = notificationbox.getNotificationWithValue(value);
 
       if (calendarIsActivated) {
         notificationbox.removeNotification(existingNotification);
       } else if (!existingNotification) {
-        let message = l10n.formatValueSync(messageName);
         let priority = notificationbox.PRIORITY_WARNING_MEDIUM;
+        let messageFragment = window.document.createDocumentFragment();
+        let message = window.document.createElement("span");
+        window.document.l10n.setAttributes(message, messageName);
+        messageFragment.appendChild(message);
 
-        notificationbox.appendNotification(message, value, null, priority, null);
+        notificationbox.appendNotification(messageFragment, value, null, priority, null);
       }
     }
   },
 
   // calICalendarManagerObserver methods
   onCalendarRegistered(calendar) {
     this.calendars.add(calendar);

it seems to me that the paradigm you should switch to is design the APIs to handle l10n-id/l10n-args arguments and then annotate the DOM with them.

So appendNotification first argument could be sth like string or {id: ..., args: ...} and then you can just pass it and the API can recognize that it was given an object and assign id/args instead of textContent = string.

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #24)

Why do you need that?

We're just following Firefox's lead. It's of course true it would be nice if the the widget had support for something like what you suggest.

Comment 26

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

Ehh, this code already supports Fluent on buttons, all you need to do is extend:

        if (
          aLabel &&
          typeof aLabel == "object" &&
          aLabel.nodeType &&
          aLabel.nodeType == aLabel.DOCUMENT_FRAGMENT_NODE
        ) {
          newitem.messageText.appendChild(aLabel);
        } else {
          newitem.messageText.textContent = aLabel;
        }

to sth like:

         if (
           aLabel &&
           typeof aLabel == "object" &&
           aLabel.nodeType &&
           aLabel.nodeType == aLabel.DOCUMENT_FRAGMENT_NODE
         ) {
           newitem.messageText.appendChild(aLabel);
+        } else if (aLabel && typeof aLabel == "object") {
+         document.l10n.setAttributes(newitem.messageText, aLabel.id, aLabel.args);
         } else {
           newitem.messageText.textContent = aLabel;
         }

Not gonna hold you, just sayin that it's an easy patch if you wanted to make it nicer :)

Comment 27

[Magnus Melin [:mkmelin]]

Thanks gandalf, I filed bug 1646160 with your suggestion.

Do you know if there's a way to avoid having to specify a non-localizable label? See the end of comment 14, like
.label = { $label }

Comment 28

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

Could you just set aLabel = null?

Comment 29

[Lasana Murray]

Attachment: bug1629360G.patch

Made changes requested by Paul.

Comment 30

[Paul Morris [:pmorris]]

Comment on attachment 9157636 [details] [diff] [review]
bug1629360G.patch

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

Looks good, thanks.  I think we should go ahead and land this (with the nit fixed).  Could you open up a follow-up bug that's blocked by bug 1646160 to redo this if/when `appendNotification` is changed?

::: calendar/base/modules/calCalendarDeactivator.jsm
@@ +127,3 @@
>          let priority = notificationbox.PRIORITY_WARNING_MEDIUM;
>  
> +        //Use Fluent's preferred async declarative, DOMLocalization API.

teeny nit: add a space between "//" and "Use"

Comment 31

[Lasana Murray]

Attachment: [checked in] bug1629360H.patch

nit fixed.

Comment 32

[Paul Morris [:pmorris]]

Comment on attachment 9157928 [details] [diff] [review]
[checked in] bug1629360H.patch

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

Thanks, and thanks for opening the follow-up bug.

Comment 33

[Lasana Murray]

Attachment: otr-attempt.patch

I'm having some trouble with the OTRUI.jsm changes. I have attached a patch with what I have done so far. I've changed some of the raw string translations to produce document fragments instead. Problem is, it seems that this file is not loaded in a context that has a window or document global so I get an undefined error.

I see the script loaded here using the ChromeUtils service but I don't understand the XPCOM stuff well enough yet to know what impact that has on the script itself:

https://searchfox.org/comm-central/source/mail/components/im/content/chat-messenger.js#26

Please help.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9158201 [details] [diff] [review]
otr-attempt.patch

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

::: chat/modules/OTRUI.jsm
@@ +19,5 @@
>  
> +function _strFrag(id, args) {
> +  let msgFrag = window.document.createDocumentFragment();
> +  let span = window.document.createElement("span");
> +  window.document.l10n.setAttributes(span, id, args);

I think for using it in the jsm, you could use the l10n defined in the file already?

Comment 35

[Lasana Murray]

This file also needs to be updated:
https://searchfox.org/comm-central/source/mailnews/base/content/msgAccountCentral.js#23

Comment 36

[Lasana Murray]

This search reveals more locations where the sync API is being used.
https://searchfox.org/comm-central/search?q=formatValueSync&case=true&path=

Comment 37

[Lasana Murray]

Attachment: bug1629360i.patch

In hindsight, this patch depends on this one I submitted here before being merged since its the same file:
https://bugzilla.mozilla.org/show_bug.cgi?id=1646154

Comment 38

[Magnus Melin [:mkmelin]]

(In reply to Lasana Murray from comment #36)

This search reveals more locations where the sync API is being used.
https://searchfox.org/comm-central/search?q=formatValueSync&case=true&path=

In the Fluent conversion of OpenPGP strings some will get added. We can convert them over to async later... Doing it all at once would be too risky.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/61b41f6239dc
Use async l10n for donation link and setup title. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e3814792ce88
Async translations in mailWidgets and specialTabs. r=mkmelin
https://hg.mozilla.org/comm-central/rev/42e64a1bc448
Use async fluent api for remove-address-row-type buttons. r=mkmelin
https://hg.mozilla.org/comm-central/rev/9cfb71b9e9f9
Use async translation for deactivated calendar messages. r=pmorris

Comment 40

[Magnus Melin [:mkmelin]]

I just noticed, this one is wrong: https://hg.mozilla.org/comm-central/rev/e3814792ce88e4086ef3e1d93a35539a84f439b4

At https://hg.mozilla.org/comm-central/rev/e3814792ce88e4086ef3e1d93a35539a84f439b4#l2.12 we're now no longer referring to aboutProfilesExtra.ftl anywhere, so that string won't be found/used.

Comment 41

[Lasana Murray]

Attachment: bug1629360.patch

Restores the translation for the launch profile button.

Comment 42

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

This looks like a weird workaround. Let's try to clean it up!

If you have a document, why not add the FTL file to it? You can use MozXULElement.insertFTLIfNeeded to add an FTL file to the document's context.

Then you can just do document.l10n.setAttributes().

That's how Firefox handles a lot of web components that are inserted to many documents.

If you cannot do that, you can still create your own let myDOMLoc = new DOMLocalization and do myDOMLoc.setAttributes(elem, id, args). and even myDOMLoc.translateFragment(root).

Comment 43

[Lasana Murray]

Thanks Zibi Braniecki! MozXULElement.insertFTLIfNeeded seems super helpful!

Comment 44

[Lasana Murray]

Attachment: bug1629360-restore-launch-profile-button-translation.patch

Uses the MozXULElement.insertFTLIfNeeded api to inject the ftl file.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/74de07c9bd9e
Restore "launch profile" button translation.

Comment 46

[Magnus Melin [:mkmelin]]

This seems to be causing:
TEST-UNEXPECTED-FAIL | comm/mail/components/enterprisepolicies/tests/browser/browser_policy_downloads.js | uncaught exception - TypeError: can't access property "MozXULElement", aDocument.defaultView is null at contentTabBaseType.inContentOverlays</<@chrome://messenger/content/specialTabs.js:400:9

Comment 47

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/14c6049615f373ea69e325ca1bfc959a0132dad4

Comment 48

[Lasana Murray]

Works locally for me... investigating.

Comment 49

[Lasana Murray]

Attachment: bug1629360-restore-launch-profile-button-translationv2.patch

Looks like, on the try server, the window is gone by the time the setTimeout callback is executed.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 9187761 [details] [diff] [review]
bug1629360-restore-launch-profile-button-translationv2.patch

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

::: mail/base/content/specialTabs.js
@@ +402,4 @@
>          for (let button of aDocument.querySelectorAll(
>            `[data-l10n-id="profiles-launch-profile"]`
>          )) {
> +          win.document.l10n.setAttributes(

I suppose it should have been aDocument, instead of document?

Comment 51

[Lasana Murray]

I changed it to use the current document on the defaultView, mostly for consistency sake but also in case aDocument is no longer valid for some reason after the timeout.

Comment 52

[Magnus Melin [:mkmelin]]

Comment on attachment 9187761 [details] [diff] [review]
bug1629360-restore-launch-profile-button-translationv2.patch

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

But lets go with this

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ecef74e26a44
Restore "launch profile" button translation. r=mkmelin