[api-minor] Move to Fluent for the localization (bug 1858715)

[calixteman]

• For the generic viewer we use @fluent/dom and @fluent/bundle
• For the builtin pdf viewer in Firefox, we set a localization url and then we rely on document.l10n which is a DOMLocalization object.

[flodolo]

The FTL file should have a license header (example). Also, leave an empty line at the end.

In general, I have a few questions and notes before diving into details.

String structure

previous =
    .title = Previous Page
previous-label = Previous

Can this be a single string? Example:

pdfviewer-previous-button = Previous
    .title = Previous Page

String IDs

The other thing is about namespaced and more semantic IDs:

• ID should hint at the role, e.g. previous-button. That prevents reuse in different contexts.
• ID should be namespaced, e.g. pdfviewer-previous-button.

Plurals

ind-match-count =
    { $total ->
        [1] { $current } of { $total } match
       *[other] { $current } of { $total } matches
    }

Use [one], not [1]. The latter is used only in specific cases, when it's a real plural but more singular/many.

Comments

Fluent support multiple type of comments: if a comment applies to more strings, you should use a group-level comments, and reset it after the strings. That also allows for simplification, e.g. at the end of the file.

# .alt: This is used as a tooltip.

No need to call out the attribute, Used a tooltip is enough.

[calixteman]

• ID should be namespaced, e.g. pdfviewer-previous-button.

Why ? If all the strings have the same prefix then the prefix is useless or am I missing something ?

[flodolo]

Why ? If all the strings have the same prefix then the prefix is useless or am I missing something ?

Strings share the same scope in the Fluent bundle
https://firefox-source-docs.mozilla.org/l10n/fluent/review.html#message-identifiers

Not sure if that's not relevant for the PDF viewer because it only loads this file?

In any case, IDs too short and generic are not allowed in mozilla-central (see the number of linter errors in phabricator), namespacing avoid that issue.

[Snuffleupagus]

Does the NullL10n class still work, e.g. if the viewer is loaded without l10n-data being available, given all the changes made in this patch?
Given that you've removed e.g. this code and this code I unfortunately cannot really imagine that this still works as before!?

You also need to update the l10n-import script to account for the new file-extension, see https://github.com/mozilla/pdf.js/blob/master/external/importL10n/locales.mjs

Should we also remove all old viewer.properties files, since they're unused now?

Does this pass all tests, i.e. gulp test locally, since we're using localization during browser tests?

[calixteman]

String structure

previous =
    .title = Previous Page
previous-label = Previous

Can this be a single string? Example:

pdfviewer-previous-button = Previous
    .title = Previous Page

Unfortunately, I think it won't work because the two strings are for two different elements:
https://github.com/mozilla/pdf.js/pull/17115/files#diff-4b300cea2df7f3f32d97ec5bd2e71eee41932f0fb52f31516f91dbff0c683409R319-R321

[calixteman]

Does the NullL10n class still work, e.g. if the viewer is loaded without l10n-data being available, given all the changes made in this patch?

Do we want to keep the NullL10n stuff ?
Won't fluent+en-US/viewer.ftl have the same role as NullL10n ?
That said, I thought removing it in this patch but I guessed that maybe it'd make this patch a bit too large.
I've the feeling that NullL10n is adapted to what we had, but with fluent it doesn't really make sense.

[Snuffleupagus]

Do we want to keep the NullL10n stuff ?

Well, unless you want to "break" the standalone viewer-components and/or annotationLayer then keeping the NullL10n-functionality (in some form) will definitely be necessary.
Please note how that's being used as a fallback in situations where the normal l10n-stuff isn't available, i.e. outside of the default viewer use-case.

That said, I've got an idea for how we could address this (keeping a modified NullL10n). If you address my initial comments above, especially about the hopefully unneeded createFluentBundle-functionality, then I'll hopefully be able to outline a possible solution here.

[flodolo]

Looks great. Only one question and some nits on the comments.

[Snuffleupagus]

Please run all tests, and assuming it passes, I'll try to take a more detailed look ASAP.

[calixteman]

/botio test

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/48c9156a0489f17/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/675c6eb37846965/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.241.84.105:8877/675c6eb37846965/output.txt

Total script time: 24.69 mins

• Font tests: Passed
• Unit tests: Passed
• Integration Tests: FAILED
• Regression tests: FAILED

  different ref/snapshot: 17
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/675c6eb37846965/reftest-analyzer.html#web=eq.log

[moz-tools-bot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/48c9156a0489f17/output.txt

Total script time: 36.39 mins

• Font tests: Passed
• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 26
  different first/second rendering: 2

Image differences available at: http://54.193.163.58:8877/48c9156a0489f17/reftest-analyzer.html#web=eq.log

[Snuffleupagus]

I can create the file if you prefer, tell me.

Yes, can you please attach the file from #17115 (comment) to this PR (in the correct l10n-directory)?
I want to spend a little time testing this locally (hopefully tomorrow) and that'll help to more easily test all the new stuff.

Good question: I think we could call and await for translateRoots:

While that obviously "works" I believe that it side-steps part of the desired testing unfortunately, since that as far as I understand actually triggers translation.
Isn't the thing that we'd ideally want to await here the existing translation, that's automatically triggered by adding elements to the annotationLayer (such that the ref-tests work the same as the viewer in that respect)?

[Snuffleupagus]

There's a couple of other files that we should probably update as well:

• pdf.js/.gitattributes

  Line 7 in 2a30902

  *.properties text eol=lf

  should be changed into

  *.ftl        text eol=lf
  

• pdf.js/.github/CODEOWNERS

  Line 1 in 2a30902

  l10n/en-US/*.properties @mozilla/pdfjs-l10n

  should be changed into

  l10n/en-US/*.ftl @mozilla/pdfjs-l10n
  

• pdf.js/test/webserver.mjs

  Line 36 in 2a30902

  ".properties": "text/plain",

  should be changed into

  ".ftl": "text/plain", 

[calixteman]

While that obviously "works" I believe that it side-steps part of the desired testing unfortunately, since that as far as I understand actually triggers translation.
Isn't the thing that we'd ideally want to await here the existing translation, that's automatically triggered by adding elements to the annotationLayer (such that the ref-tests work the same as the viewer in that respect)?

I don't see anything we can wait for unfortunately... if you've any ideas, I take.

[calixteman]

Yes, can you please attach the file from #17115 (comment) to this PR (in the correct l10n-directory)?

Done.

[Snuffleupagus]

Unfortunately I don't believe that l10n during the ref-tests currently work at all with this patch, since I've just tested the following diff which works just fine in the viewer but the ref-tests no longer translate as they should (since the annotationLayer-element isn't being "registered" for l10n).

diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js
index 243744581..54ddbd569 100644
--- a/src/display/annotation_layer.js
+++ b/src/display/annotation_layer.js
@@ -2021,7 +2021,7 @@ class PopupAnnotationElement extends AnnotationElement {
 }
 
 class PopupElement {
-  #dateTimePromise = null;
+  #dateObj = null;
 
   #boundKeyDown = this.#keyDown.bind(this);
 
@@ -2078,16 +2078,10 @@ class PopupElement {
     this.#parentRect = parentRect;
     this.#elements = elements;
 
-    const dateObject = PDFDateString.toDateObject(modificationDate);
-    if (dateObject) {
-      // The modification date is shown in the popup instead of the creation
-      // date if it is available and can be parsed correctly, which is
-      // consistent with other viewers such as Adobe Acrobat.
-      this.#dateTimePromise = parent.l10n.get("pdfjs-annotation-date-string", {
-        date: dateObject.toLocaleDateString(),
-        time: dateObject.toLocaleTimeString(),
-      });
-    }
+    // The modification date is shown in the popup instead of the creation
+    // date if it is available and can be parsed correctly, which is
+    // consistent with other viewers such as Adobe Acrobat.
+    this.#dateObj = PDFDateString.toDateObject(modificationDate);
 
     this.trigger = elements.flatMap(e => e.getElementsToTriggerPopup());
     // Attach the event listeners to the trigger element.
@@ -2115,9 +2109,6 @@ class PopupElement {
         if (this.#container.hidden) {
           this.#show();
         }
-        if (this.#dateTimePromise) {
-          await this.#dateTimePromise;
-        }
       });
     }
   }
@@ -2166,11 +2157,14 @@ class PopupElement {
     ({ dir: title.dir, str: title.textContent } = this.#titleObj);
     popup.append(header);
 
-    if (this.#dateTimePromise) {
+    if (this.#dateObj) {
       const modificationDate = document.createElement("span");
       modificationDate.classList.add("popupDate");
-      this.#dateTimePromise.then(localized => {
-        modificationDate.textContent = localized;
+
+      modificationDate.dataset.l10nId = "pdfjs-annotation-date-string";
+      modificationDate.dataset.l10nArgs = JSON.stringify({
+        date: this.#dateObj.toLocaleDateString(),
+        time: this.#dateObj.toLocaleTimeString(),
       });
       header.append(modificationDate);
     }

Also, would the diff above make sense to incorporate into this patch to reduce "manual" translation?

---

I don't see anything we can wait for unfortunately... if you've any ideas, I take.

Unfortunately I don't have a good idea for this. However, to fix the ref-test problem described above the following diff seems to work locally.

diff --git a/test/driver.js b/test/driver.js
index 8e9026105..dad883ecc 100644
--- a/test/driver.js
+++ b/test/driver.js
@@ -233,6 +233,9 @@ class Rasterize {
         outputScale
       );
 
+      // Ensure that the annotationLayer gets translated.
+      document.l10n.connectRoot(div);
+
       // Rendering annotation layer as HTML.
       const parameters = {
         annotations,
@@ -251,6 +254,9 @@ class Rasterize {
       await annotationLayer.showPopups();
       await document.l10n.translateRoots();
 
+      // All translation should be complete here.
+      document.l10n.disconnectRoot(div);
+
       // Inline SVG images from text annotations.
       await inlineImages(div);
       foreignObject.append(div);

[Snuffleupagus]

/botio-linux preview

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/3f0273f4a7b7f9e/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/3f0273f4a7b7f9e/output.txt

Total script time: 1.50 mins

Published

• Viewer: http://54.241.84.105:8877/3f0273f4a7b7f9e/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/3f0273f4a7b7f9e/legacy/web/viewer.html

[Snuffleupagus]

Please run all tests one more time, before landing this.
This looks very nice, and seems like an overall simplification of the l10n-handling.

r=me, assuming that all tests still pass; thank you!

[calixteman]

/botio test

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/5ef2720aa297259/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/ff8cfd97ff26fae/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.241.84.105:8877/ff8cfd97ff26fae/output.txt

Total script time: 24.75 mins

• Font tests: Passed
• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 20
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/ff8cfd97ff26fae/reftest-analyzer.html#web=eq.log

[moz-tools-bot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/5ef2720aa297259/output.txt

Total script time: 37.58 mins

• Font tests: Passed
• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 16
  different first/second rendering: 1

Image differences available at: http://54.193.163.58:8877/5ef2720aa297259/reftest-analyzer.html#web=eq.log

[timvandermeij]

Thank you @calixteman, @Snuffleupagus and @flodolo for your efforts on this! This has been quite a chunk of work, but it's great to see that the overall L10n handling is simpler now and we got rid of the old webl10n code.