1563128 - Many strings are not localized if one has an args failure

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Ed Lee :Mardak]

Attachment: no initially loaded strings but later rendered context menu is okay

Bug 1563127 is the new tab bug triggering the JSON parse failure of args, but the localization api should probably allow the good strings to appear.

gandalf: ok! that may be a bug worth filling - it seems that if any element in the batch has a broken data-l10n-args, we'll bail from translating the whole batch - https://searchfox.org/mozilla-central/source/dom/l10n/DOMLocalization.cpp#297-299

Comment 1

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

This should be fairly easy to fix. We just need to be better about reporting when JSON fails in GetAttributes and maybe not fail completely in such scenario - https://searchfox.org/mozilla-central/source/dom/l10n/DOMLocalization.cpp#297-299

The only question is if GetAttributes should fail if args are an incorrect JSON or should it work and return empty args.

Pike - thoughts?

Comment 2

[Axel Hecht [:Pike]]

I'd think that empty args is consistent to how we do error fallback, say, if someone had valid json but a typo in the var name.

Would that make GetAttributes infallible then? If not, I'd also replace the return nullptr for a continue?

Comment 3

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

I think this is fixed now. Can you verify?

Comment 4

[Ed Lee :Mardak]

Attachment: still broken in 78 nightly

Still broken with the command line showing JavaScript error: , line 0: SyntaxError: JSON.parse: unexpected character at line 1 column 10 of the JSON data with this change then refreshing the new tab page (some reason first load is okay?):

diff --git a/browser/components/newtab/data/content/activity-stream.bundle.js b/browser/components/newtab/data/content/activity-stream.bundle.js
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -5871,5 +5871,5 @@ class ContextMenuButton extends react__WEBPACK_IMPORTED_MODULE_0___default.a.Pur
       "aria-haspopup": "true",
       "data-l10n-id": tooltip,
-      "data-l10n-args": tooltipArgs ? JSON.stringify(tooltipArgs) : null,
+      "data-l10n-args": "",
       className: "context-menu-button icon",
       onKeyDown: this.onKeyDown,

Comment 5

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

Thank you! Added to my todo :)

Comment 6

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

Attachment: Bug 1563128 - Handle broken l10n-args gracefully. r?nordzilla

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8e1570377c3
Handle broken l10n-args gracefully. r=nordzilla

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a8e1570377c3