1806301 - Polish translation for browser/chrome/browser/browser.properties:addonPostInstall.message3 gets cut off

Status: ASSIGNED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Przemysław Siekiera]

Attachment: obraz.png

The string:
%S was added.

Is translated as:
Dodano rozszerzenie „%S”.

The Polish message is not displayed in full when adding a new extension – it displays fine when installing a new theme though.

Feedback via Transvision:
https://transvision.mozfr.org/?sourcelocale=en-US&locale=pl&repo=all_projects&search_type=entities&recherche=browser/chrome/browser/browser.properties:addonPostInstall.message3&entire_string=entire_string

Comment 1

[Przemysław Siekiera]

Attachment: English text when adding an extension

Comment 2

[Przemysław Siekiera]

Attachment: Polish message when adding a new theme

Comment 3

[Francesco Lodolo [:flod]]

Thanks for filing the bug.

This doesn't seem like a problem the localization should solve. I'm guessing we're doing some strange replacement with the string beyond resolving parameters, which is triggered by the specific string in Polish.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

I noticed this bug and I decided to take a deeper look into it (given that I was working on some changes to dialogs and my memory about special handling of the post install dialog has been refreshed as a side effect of this, and I couldn't think of any way the logic on the ExtensionsUI.sys.mjs side could trigger this issue).

Turned out that:

• it also happens only the first time this dialog is being open, when shown again in the same Firefox instance the issue isn't hit anymore
• themes are also affected (but the fact that it only happens the first time the dialog is shown in a Firefox instance is likely what made that to seem to work for themes)
• the only reason why the en-US version of that dialog isn't broken is that it doesn't have part to be shown before the addon name (but if we change the message from "<> was added." to "some-prefix <> was added." then the "some-prefix" part would be there when the dialog is been shown for the first time)
• there is no string manipulation on the ExtensionsUI.sys.mjs side, we just ensure that the part of the string where the addon name should be is set to "<>", which is what the internal logic from panelUI.js expects when the message has to be split and "<>" replaces with what is set on options.name)
• the panelUI.js logic that splits the string is part of the PanelUI._formatDescriptionMessage and it was breaking down the message in the expected start, name and end property values, the logic that uses those values and set the popupnotification attributes accordingly is here in PanelUI._refreshNotificationPanel and it was setting the popupnotification label, name and endlabel attributes as expected
• the popupnotification attributes being set by PanelUI.js logic linked above is then kicking in when the popupnotification node is appended to the doc, the attributes are inherited by markup part of the popupnotification as defined here in popupnotication.js and it was definitely setting the text content and nodes part of the markup (see the XUL markup here) (through the logic defined in the inheritAttribute method defined by the MozXULElementMixin here). >> AT THIS POINT ALL THE EXPECTED TEXT IS WHERE IT SHOULD <<
• Right after the previous step, the first span element (the one set to the label popupnotification attribute value) is cleared (apparently as a side effect) of PanelUI._showNotificationPanel de-lazifying the data-lazy-l10n-id attribute here, the attribute set by that fluent string being de-lazyfied should only be buttonlabel and buttonaccesskey

At this point I was able to see what was going on, but It was still unclear to me why that unrelated span element was the only one losing its text content (and why the other one are not) as a side effect of the data-l10n-id attribute being set...

My best guess at that point what the fact that the popupnotification attribute that was apparently lost was one called label (while the other ones called name and endlabel were not affected at all).

To confirm if that may have been the reason, I tried out to rename the popupnotification attribut to startlabel with the 2 one-line changes below:

diff --git a/browser/components/customizableui/content/panelUI.js b/browser/components/customizableui/content/panelUI.js
--- a/browser/components/customizableui/content/panelUI.js
+++ b/browser/components/customizableui/content/panelUI.js
@@ -992,7 +992,7 @@ const PanelUI = {
 
     if (notification.options.message) {
       let desc = this._formatDescriptionMessage(notification);
-      popupnotification.setAttribute("label", desc.start);
+      popupnotification.setAttribute("startlabel", desc.start);
       popupnotification.setAttribute("name", desc.name);
       popupnotification.setAttribute("endlabel", desc.end);
     }
diff --git a/toolkit/content/widgets/popupnotification.js b/toolkit/content/widgets/popupnotification.js
--- a/toolkit/content/widgets/popupnotification.js
+++ b/toolkit/content/widgets/popupnotification.js
@@ -15,7 +15,7 @@
         ".popup-notification-origin": "value=origin,tooltiptext=origin",
         ".popup-notification-description": "popupid,id=descriptionid",
         ".popup-notification-description > span:first-of-type":
-          "text=label,popupid",
+          "text=startlabel,popupid",
         ".popup-notification-description > b:first-of-type":
           "text=name,popupid",
         ".popup-notification-description > span:nth-of-type(2)":

with just these two lines changed I was unable to hit the issue again, and so that seems to confirm that even if data-l10n-attrs was set to "buttonlabel, buttonaccesskey", the "label" property has been reset someone as a side-effect of setting data-l10n-id.

Taking a look to the underlying L10n internals logic, I "think" that may be because:

• popupnotification is a XUL element and for XUL elements "label" is considered localizable (see IsAttrNameLocalizable logic here)
• and this part of L10nOverlays::OverlayAttributes logic here may be the one unsetting the popupnotification label attributes (because it is a localizable XUL elements attribute and it is not set in the fluent string being set to data-l10n-id)

The regression as been introduced into the post install dialog as a side effect of the changes applied by Bug 1697622 (but the underlying issue was already there before, just went unnoticed because we didn't spot any dialog hitting it yet).

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Eemeli,
is the L10n internal logic part that I'm describing in comment 4 correct? (I don't exclude I may have got some of that wrong, given that I have derived that part by only reading the code on searchfox, and so I wanted to confirm with you that part of my investigation described in comment 4).

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Gijs,
what do you think considering to fix this issue with changes similar to the one described in comment 4? (in the same comment I have also included details from what I observed while investigating this issue, along with the diff of a change I tried out locally on the popupnotification.js and PanelUI.js side to confirm if that would prevent the issue to be hit, I'm not sure if there are other dialog hitting this in practice, but I wouldn't be surprised if there would be more dialogs hitting similar issues with non-enUS locales).

As a side note, if we decide to proceed with that kind of approach to fix the issue, then we will need to apply a change like the one applied to PanelUI.js also on the PopupNotifications.sys.mjs side here.

Is there any other internals using that popupnotification behavior related to the popupnotification label attribute as far as you can recall?

At the moment it seems that dealing with it on the privileged js side may be less risky than dealing with it on the L10nOverlays.cpp side (but I have also needinfo-ed Eemeli to confirm my understanding of that part isn't too far from how that is actually working, and we can ask Eemeli about how risky consider to change that logic from L10nOverlays.cpp from his perspective, at a first glance it feels it may be even harder to track down potential regressions that could be triggered by not considering label as a localizable attribute by default for XUL elements).

Comment 7

[Eemeli Aro [:eemeli]]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

Hi Eemeli,
is the L10n internal logic part that I'm describing in comment 4 correct? (I don't exclude I may have got some of that wrong, given that I have derived that part by only reading the code on searchfox, and so I wanted to confirm with you that part of my investigation described in comment 4).

You're exactly right. label is considered as a localizable attribute here, and therefore it's controlled by DOM localization when the element has a data-l10n-id attribute. If there's no .label attribute in the message, it'll get emptied.

Comment 8

[:Gijs (he/him)]

As far as I recall, the _formatDescriptionMessage code predates fluent existing.

Although a bigger fix, wouldn't it make more sense to make that code use fluent "properly" so that the add-on name is included via fluent instead of via manual concatenation-type hacks? That would also fix the issue and allow getting rid of the _formatDescriptionMessage helper.

Am I missing a reason this is impractical?

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to :Gijs (he/him) from comment #8)

As far as I recall, the _formatDescriptionMessage code predates fluent existing.

Although a bigger fix, wouldn't it make more sense to make that code use fluent "properly" so that the add-on name is included via fluent instead of via manual concatenation-type hacks? That would also fix the issue and allow getting rid of the _formatDescriptionMessage helper.

Am I missing a reason this is impractical?

That may actually be an even better path, not sure why I didn't mention that option (I was likely still too much into "debugging mode" and focused on the why the current implementation wasn't working).

Looking for usage of the usage of the "<>" and "{}" may also be potentially slighly easier to be tracked down through searchfox, e.g. looking for "<>":

• https://searchfox.org/mozilla-central/search?q=%22%3C%3E%22&path=&case=false&regexp=false

I'll double-check what aiming for that would mean for our dialogs using the current behaviors provided through the _formatDescriptionMessage method, that may help to get a picture of what would that imply also for other internals using similar approaches for the popupnotification message.

(I'm leaving the needinfo assigned to me to come back to this with some some more details after having looked into that).

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Oof, howdy yes - this is a very unfortunate collision between inherited attributes, lazy Fluent localization, the somewhat magical automatically localizable attributes (label in this case), and a fairly complicated way of formatting strings that seems to be cargo-culted from PopupNotification.

It all seems rather arcane.

I agree with Gijs - if we can find a way of interpolating and overlaying with Fluent "naturally" rather than using this older mechanism, that'd be ideal.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1806301 - popupnotification-related changes to allow dialog to avoid the pre-Fluent message interpolated message using "<>" and "{}" markers. r?Gijs!,mconley!,mstriemer!

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: WIP: Bug 1806301 - Adjust existing tests assertions to account for the changes applied to the popupnotification description child elements.

Depends on D226949

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: WIP: Bug 1806301 - Remove use legacy <> string interpolation separator from Extension.sys.mjs, ExtensionsUI.sys.mjs and browser-addons.js.

Depends on D226950

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1806301 - Set default popupnotification button string to OK by default to match Acorn style guide. r?mstriemer!

We have been recently asked to change the "Ok!" and "Okay" buttons across a number of our add-ons related dialogs,
because per Acorn style guide those buttons should use "OK" as the button string and so given that the default
is currently "OK!" it seems we should change it for all dialogs, so that they will all match the Acorn style
guide when using the default popupnotification button string.

See acorn style guide here: https://acorn.firefox.com/latest/content/word-list/n-r-Z1EQFKD6#section-ok-03

Depends on D226951

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: WIP: Bug 1806301 - Use default OK button string in the addon post install dialog.

Depends on D226952

Comment 16

[Phabricator Automation]

Comment on attachment 9433415 [details]
WIP: Bug 1806301 - Use default OK button string in the addon post install dialog.

Revision D226953 was moved to bug 1935726. Setting attachment 9433415 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9433414 [details]
Bug 1806301 - Set default popupnotification button string to OK by default to match Acorn style guide. r?mstriemer!

Revision D226952 was moved to bug 1935726. Setting attachment 9433414 [details] to obsolete.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1806301 - Add new tests to popupNotifications mochitests. r?Gijs!,mstriemer

Depends on D226949

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1806301 - Trigger explicit failures in automation on popupnotifications still using the legacy interpolated strings. r?Gijs!,mstriemer!

Depends on D226949