Open Bug 1629360 Opened 2 years ago Updated 7 months ago

switch Thunderbird away from sync calls to Localization

Categories

(Thunderbird :: General, task, P1)

Tracking

(Not tracked)

84 Branch

People

(Reporter: mkmelin, Unassigned)

References

Details

(Keywords: leave-open)

Attachments

(5 files, 9 obsolete files)

2.54 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
3.01 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
2.43 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
3.53 KB, patch
pmorris
: review+
Details | Diff | Splinter Review
1.38 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

(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.

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

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

Assignee: nobody → lasana
Status: NEW → ASSIGNED

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?

Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)

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!

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.

Attachment #9153225 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9153225 - Flags: review?(mkmelin+mozilla) → review-
Attached patch bug1629360B.patch (obsolete) — Splinter Review

Recommend changes made.

Attachment #9153225 - Attachment is obsolete: true
Attachment #9153961 - Flags: review?(mkmelin+mozilla)
Attached patch bug1629360C.patch (obsolete) — Splinter Review
Attachment #9154282 - Flags: review?(mkmelin+mozilla)
Attachment #9154287 - Flags: review?(mkmelin+mozilla)
Attachment #9154339 - Flags: review?(mkmelin+mozilla)
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
Attachment #9153961 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #9154282 - Flags: review?(mkmelin+mozilla) → review-
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)
Attachment #9154287 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9154339 [details] [diff] [review]
[checked in] bug1629360E.patch

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

LGTM, r=mkmelin
Attachment #9154339 - Flags: review?(mkmelin+mozilla) → review+

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?

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

Flags: needinfo?(mkmelin+mozilla)

(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...

Flags: needinfo?(mkmelin+mozilla)
Attachment #9153961 - Attachment is obsolete: true
Attachment #9154633 - Flags: review?(mkmelin+mozilla)
Attachment #9154633 - Flags: review?(mkmelin+mozilla) → review+
Attached patch bug1629360F.patch (obsolete) — Splinter Review

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.

Attachment #9155415 - Flags: review?(mkmelin+mozilla)
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/)
Attachment #9155415 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9155415 - Flags: review?(paul)
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."
Attachment #9155415 - Flags: review?(paul) → 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.

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.

(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.

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 :)

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 }

Flags: needinfo?(gandalf)

Could you just set aLabel = null?

Flags: needinfo?(gandalf)
Attached patch bug1629360G.patch (obsolete) — Splinter Review

Made changes requested by Paul.

Attachment #9155415 - Attachment is obsolete: true
Attachment #9157636 - Flags: review?(paul)
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"
Attachment #9157636 - Flags: review?(paul) → review+

nit fixed.

Attachment #9157928 - Flags: review?(paul)
Attachment #9157636 - Attachment is obsolete: true
See Also: → 1646982
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.
Attachment #9157928 - Flags: review?(paul) → review+
Attached patch otr-attempt.patch (obsolete) — Splinter Review

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.

Attachment #9158201 - Flags: feedback?(mkmelin+mozilla)
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?
Attachment #9158201 - Flags: feedback?(mkmelin+mozilla)
Attachment #9154282 - Attachment is obsolete: true
Attachment #9158201 - Attachment is obsolete: true
Attachment #9158201 - Flags: feedback?(mkmelin+mozilla)
Attachment #9158201 - Flags: feedback?(mkmelin+mozilla)
Target Milestone: --- → Thunderbird 79.0

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

Attached patch bug1629360i.patch (obsolete) — Splinter Review

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

Attachment #9158628 - Flags: review?(mkmelin+mozilla)

(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.

Pushed by geoff@darktrojan.net:
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

Attachment #9157928 - Attachment description: bug1629360H.patch → [checked in] bug1629360H.patch
Attachment #9154633 - Attachment description: bug1629360B2.patch → [checked in] bug1629360B2.patch
Attachment #9154339 - Attachment description: bug1629360E.patch → [checked in] bug1629360E.patch
Attachment #9154287 - Attachment description: bug1629360D.patch → [checked in] bug1629360D.patch
Attachment #9158628 - Attachment is obsolete: true
Attachment #9158628 - Flags: review?(mkmelin+mozilla)

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.

Attached patch bug1629360.patch (obsolete) — Splinter Review

Restores the translation for the launch profile button.

Attachment #9185539 - Flags: review?(mkmelin+mozilla)

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).

Thanks Zibi Braniecki! MozXULElement.insertFTLIfNeeded seems super helpful!

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

Attachment #9185539 - Attachment is obsolete: true
Attachment #9185539 - Flags: review?(mkmelin+mozilla)
Attachment #9185557 - Flags: review?(mkmelin+mozilla)
Attachment #9185557 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: Thunderbird 79.0 → 84 Branch

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/74de07c9bd9e
Restore "launch profile" button translation.

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

Works locally for me... investigating.

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

Attachment #9185557 - Attachment is obsolete: true
Attachment #9187761 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9187761 - Flags: review?(mkmelin+mozilla)

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 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
Attachment #9187761 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ecef74e26a44
Restore "launch profile" button translation. r=mkmelin

Status: ASSIGNED → NEW
Assignee: lasana → nobody
You need to log in before you can comment on or make changes to this bug.