switch Thunderbird away from sync calls to Localization
Categories
(Thunderbird :: General, task, P1)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: lasana)
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.
Reporter | ||
Comment 1•8 months ago
|
||
Reporter | ||
Comment 2•8 months ago
|
||
This patch lists (at the time) all offenders: https://hg.mozilla.org/comm-central/rev/63c6dce22725
Reporter | ||
Comment 3•8 months ago
|
||
Could be good to start off with one patch per case.
Assignee | ||
Updated•8 months ago
|
Assignee | ||
Comment 4•8 months ago
|
||
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?
Assignee | ||
Updated•8 months ago
|
Reporter | ||
Comment 5•8 months ago
|
||
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.
Assignee | ||
Comment 6•8 months ago
|
||
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!
Assignee | ||
Comment 7•8 months ago
|
||
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.
Assignee | ||
Updated•8 months ago
|
Reporter | ||
Comment 8•8 months ago
|
||
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)
Assignee | ||
Comment 9•8 months ago
|
||
Recommend changes made.
Assignee | ||
Comment 10•8 months ago
|
||
Assignee | ||
Comment 11•8 months ago
|
||
Assignee | ||
Comment 12•8 months ago
|
||
Reporter | ||
Comment 13•8 months ago
|
||
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
Reporter | ||
Comment 14•8 months ago
|
||
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.
Reporter | ||
Comment 15•8 months ago
|
||
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)
Reporter | ||
Comment 16•8 months ago
|
||
Comment on attachment 9154339 [details] [diff] [review] [checked in] bug1629360E.patch Review of attachment 9154339 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=mkmelin
Assignee | ||
Comment 17•8 months ago
|
||
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?
Assignee | ||
Comment 18•8 months ago
|
||
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
Reporter | ||
Comment 19•8 months ago
|
||
(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...
Assignee | ||
Comment 20•8 months ago
|
||
Reporter | ||
Updated•8 months ago
|
Assignee | ||
Comment 21•8 months ago
|
||
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.
Reporter | ||
Comment 22•7 months ago
|
||
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/)
Assignee | ||
Updated•7 months ago
|
Comment 23•7 months ago
|
||
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•7 months ago
|
||
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
.
Reporter | ||
Comment 25•7 months ago
|
||
(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•7 months ago
|
||
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 :)
Reporter | ||
Comment 27•7 months ago
|
||
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 }
Assignee | ||
Comment 29•7 months ago
|
||
Made changes requested by Paul.
Comment 30•7 months ago
•
|
||
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"
Assignee | ||
Updated•7 months ago
|
Comment 32•7 months ago
|
||
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.
Assignee | ||
Comment 33•7 months ago
|
||
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.
Reporter | ||
Comment 34•7 months ago
|
||
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?
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Comment 35•7 months ago
|
||
This file also needs to be updated:
https://searchfox.org/comm-central/source/mailnews/base/content/msgAccountCentral.js#23
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Comment 36•7 months ago
|
||
This search reveals more locations where the sync API is being used.
https://searchfox.org/comm-central/search?q=formatValueSync&case=true&path=
Assignee | ||
Comment 37•7 months ago
|
||
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
Reporter | ||
Comment 38•7 months ago
|
||
(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•7 months ago
|
||
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
Updated•7 months ago
|
Updated•7 months ago
|
Updated•7 months ago
|
Updated•7 months ago
|
Assignee | ||
Updated•7 months ago
|
Reporter | ||
Comment 40•3 months ago
|
||
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.
Assignee | ||
Comment 41•3 months ago
|
||
Restores the translation for the launch profile button.
Comment 42•3 months ago
|
||
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)
.
Assignee | ||
Comment 43•3 months ago
|
||
Thanks Zibi Braniecki! MozXULElement.insertFTLIfNeeded
seems super helpful!
Assignee | ||
Comment 44•3 months ago
|
||
Uses the MozXULElement.insertFTLIfNeeded
api to inject the ftl file.
Reporter | ||
Updated•3 months ago
|
Assignee | ||
Updated•3 months ago
|
Comment 45•3 months ago
|
||
Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/74de07c9bd9e
Restore "launch profile" button translation.
Reporter | ||
Comment 46•3 months ago
|
||
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
Reporter | ||
Comment 47•3 months ago
|
||
Assignee | ||
Comment 48•3 months ago
|
||
Works locally for me... investigating.
Assignee | ||
Comment 49•2 months ago
|
||
Looks like, on the try server, the window is gone by the time the setTimeout callback is executed.
Reporter | ||
Comment 50•2 months ago
|
||
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?
Reporter | ||
Updated•2 months ago
|
Assignee | ||
Comment 51•2 months ago
|
||
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.
Reporter | ||
Comment 52•2 months ago
|
||
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
Reporter | ||
Updated•2 months ago
|
Comment 53•2 months ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ecef74e26a44
Restore "launch profile" button translation. r=mkmelin
Description
•