Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM. r?stas draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Thu, 01 Mar 2018 16:49:37 -0800
changeset 763480 2eef3764eafbc312d7f2de720adc41f8bd1446b6
parent 763479 35cfdb0cdbfce3fac2cb1f14f39064734e55037d
child 763610 31a45925885876191795a8daaa3f1333d027d642
push id101466
push userbmo:gandalf@aviary.pl
push dateTue, 06 Mar 2018 01:43:38 +0000
reviewersstas
bugs1363862
milestone60.0a1
Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM. r?stas MozReview-Commit-ID: 1zAbSMapi86
browser/components/preferences/in-content/tests/browser_fluent.js
intl/l10n/DOMLocalization.jsm
intl/l10n/Localization.jsm
intl/l10n/test/chrome.ini
intl/l10n/test/dom/test_domloc_attr_sanitized.html
intl/l10n/test/dom/test_domloc_overlay_missing_all.html
intl/l10n/test/dom/test_domloc_overlay_sanitized.html
intl/l10n/test/dom/test_domloc_repeated_l10nid.html
--- a/browser/components/preferences/in-content/tests/browser_fluent.js
+++ b/browser/components/preferences/in-content/tests/browser_fluent.js
@@ -36,14 +36,14 @@ add_task(async function() {
   ]);
 
   let elem = doc.querySelector(
     `#contentProcessCount > menupopup > menuitem[value="${defaultProcessCount}"]`);
 
   Assert.deepEqual(msg, {
     value: null,
     attrs: [
-      ["label", elem.getAttribute("label")]
+      {name: "label", value: elem.getAttribute("label")}
     ]
   });
 
   await BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
--- a/intl/l10n/DOMLocalization.jsm
+++ b/intl/l10n/DOMLocalization.jsm
@@ -102,19 +102,19 @@ function overlayElement(targetElement, t
   // translation.
   for (const attr of Array.from(targetElement.attributes)) {
     if (isAttrNameLocalizable(attr.name, targetElement, explicitlyAllowed)) {
       targetElement.removeAttribute(attr.name);
     }
   }
 
   if (translation.attrs) {
-    for (const [name, val] of translation.attrs) {
+    for (const {name, value} of translation.attrs) {
       if (isAttrNameLocalizable(name, targetElement, explicitlyAllowed)) {
-        targetElement.setAttribute(name, val);
+        targetElement.setAttribute(name, value);
       }
     }
   }
 }
 
 /**
  * Sanitize `translationFragment` using `sourceElement` to add functional
  * HTML attributes to children.  `sourceElement` will have all its child nodes
@@ -299,16 +299,57 @@ function shiftNamedElement(element, loca
     if (child.localName === localName) {
       element.removeChild(child);
       return child;
     }
   }
   return null;
 }
 
+/**
+ * Sanitizes a translation before passing them to Node.localize API.
+ *
+ * It returns `false` if the translation contains DOM Overlays and should
+ * not go into Node.localize.
+ *
+ * Note: There's a third item of work that JS DOM Overlays do - removal
+ * of attributes from the previous translation.
+ * This is not trivial to implement for Node.localize scenario, so
+ * at the moment it is not supported.
+ *
+ * @param {{
+ *          localName: string,
+ *          namespaceURI: string,
+ *          type: string || null
+ *          l10nId: string,
+ *          l10nArgs: Array<Object> || null,
+ *          l10nAttrs: string ||null,
+ *        }}                                     l10nItems
+ * @param {{value: string, attrs: Object}} translations
+ * @returns boolean
+ * @private
+ */
+function sanitizeTranslationForNodeLocalize(l10nItem, translation) {
+  if (reOverlay.test(translation.value)) {
+    return false;
+  }
+
+  if (translation.attrs) {
+    const explicitlyAllowed = l10nItem.l10nAttrs === null ? null :
+      l10nItem.l10nAttrs.split(",").map(i => i.trim());
+    for (const [j, {name}] of translation.attrs.entries()) {
+      if (!isAttrNameLocalizable(name, l10nItem, explicitlyAllowed)) {
+        translation.attrs.splice(j, 1);
+      }
+    }
+  }
+  return true;
+}
+
+
 const L10NID_ATTR_NAME = "data-l10n-id";
 const L10NARGS_ATTR_NAME = "data-l10n-args";
 
 const L10N_ELEMENT_QUERY = `[${L10NID_ATTR_NAME}]`;
 
 /**
  * The `DOMLocalization` class is responsible for fetching resources and
  * formatting translations.
@@ -463,17 +504,17 @@ class DOMLocalization extends Localizati
   /**
    * Translate all roots associated with this `DOMLocalization`.
    *
    * @returns {Promise}
    */
   translateRoots() {
     const roots = Array.from(this.roots);
     return Promise.all(
-      roots.map(root => this.translateElements(this.getTranslatables(root)))
+      roots.map(root => this.translateFragment(root))
     );
   }
 
   /**
    * Pauses the `MutationObserver`.
    *
    * @private
    */
@@ -528,31 +569,83 @@ class DOMLocalization extends Localizati
           this.translateElements(Array.from(this.pendingElements));
           this.pendingElements.clear();
           this.pendingrAF = null;
         });
       }
     }
   }
 
-
   /**
    * Translate a DOM element or fragment asynchronously using this
    * `DOMLocalization` object.
    *
    * Manually trigger the translation (or re-translation) of a DOM fragment.
    * Use the `data-l10n-id` and `data-l10n-args` attributes to mark up the DOM
    * with information about which translations to use.
    *
    * Returns a `Promise` that gets resolved once the translation is complete.
    *
    * @param   {DOMFragment} frag - Element or DocumentFragment to be translated
    * @returns {Promise}
    */
   translateFragment(frag) {
+    if (frag.localize) {
+      // This is a temporary fast-path offered by Gecko to workaround performance
+      // issues coming from Fluent and XBL+Stylo performing unnecesary
+      // operations during startup.
+      // For details see bug 1441037, bug 1442262, and bug 1363862.
+
+      // A sparse array which will store translations separated out from
+      // all translations that is needed for DOM Overlay.
+      const overlayTranslations = [];
+
+      const getTranslationsForItems = async l10nItems => {
+        const keys = l10nItems.map(l10nItem => [l10nItem.l10nId, l10nItem.l10nArgs]);
+        const translations = await this.formatMessages(keys);
+
+        // Here we want to separate out elements that require DOM Overlays.
+        // Those elements will have to be translated using our JS
+        // implementation, while everything else is going to use the fast-path.
+        for (const [i, translation] of translations.entries()) {
+          if (translation === undefined) {
+            continue;
+          }
+
+          const hasOnlyText =
+            sanitizeTranslationForNodeLocalize(l10nItems[i], translation);
+          if (!hasOnlyText) {
+            // Removing from translations to make Node.localize skip it.
+            // We will translate it below using JS DOM Overlays.
+            overlayTranslations[i] = translations[i];
+            translations[i] = undefined;
+          }
+        }
+
+        // We pause translation observing here because Node.localize
+        // will translate the whole DOM next, using the `translations`.
+        //
+        // The observer will be resumed after DOM Overlays are localized
+        // in the next microtask.
+        this.pauseObserving();
+        return translations;
+      };
+
+      return frag.localize(getTranslationsForItems.bind(this))
+        .then(untranslatedElements => {
+          for (let i = 0; i < overlayTranslations.length; i++) {
+            if (overlayTranslations[i] !== undefined &&
+                untranslatedElements[i] !== undefined) {
+              overlayElement(untranslatedElements[i], overlayTranslations[i]);
+            }
+          }
+          this.resumeObserving();
+        })
+        .catch(() => this.resumeObserving());
+    }
     return this.translateElements(this.getTranslatables(frag));
   }
 
   /**
    * Translate a list of DOM elements asynchronously using this
    * `DOMLocalization` object.
    *
    * Manually trigger the translation (or re-translation) of a list of elements.
@@ -580,17 +673,19 @@ class DOMLocalization extends Localizati
    * @param {Array<Element>} elements
    * @param {Array<Object>}  translations
    * @private
    */
   applyTranslations(elements, translations) {
     this.pauseObserving();
 
     for (let i = 0; i < elements.length; i++) {
-      overlayElement(elements[i], translations[i]);
+      if (translations[i] !== undefined) {
+        overlayElement(elements[i], translations[i]);
+      }
     }
 
     this.resumeObserving();
   }
 
   /**
    * Collects all translatable child elements of the element.
    *
--- a/intl/l10n/Localization.jsm
+++ b/intl/l10n/Localization.jsm
@@ -352,23 +352,20 @@ function messageFromContext(ctx, errors,
 
   const formatted = {
     value: ctx.format(msg, args, errors),
     attrs: null,
   };
 
   if (msg.attrs) {
     formatted.attrs = [];
-    for (const attrName in msg.attrs) {
-      const formattedAttr = ctx.format(msg.attrs[attrName], args, errors);
-      if (formattedAttr !== null) {
-        formatted.attrs.push([
-          attrName,
-          formattedAttr
-        ]);
+    for (const name in msg.attrs) {
+      const value = ctx.format(msg.attrs[name], args, errors);
+      if (value !== null) {
+        formatted.attrs.push({ name, value });
       }
     }
   }
 
   return formatted;
 }
 
 /**
--- a/intl/l10n/test/chrome.ini
+++ b/intl/l10n/test/chrome.ini
@@ -1,12 +1,16 @@
+[dom/test_domloc_attr_sanitized.html]
 [dom/test_domloc_getAttributes.html]
 [dom/test_domloc_setAttributes.html]
 [dom/test_domloc_translateElements.html]
 [dom/test_domloc_translateFragment.html]
 [dom/test_domloc_connectRoot.html]
 [dom/test_domloc_disconnectRoot.html]
+[dom/test_domloc_repeated_l10nid.html]
 [dom/test_domloc_translateRoots.html]
 [dom/test_domloc_mutations.html]
 [dom/test_domloc_overlay.html]
 [dom/test_domloc_overlay_repeated.html]
+[dom/test_domloc_overlay_missing_all.html]
 [dom/test_domloc_overlay_missing_children.html]
+[dom/test_domloc_overlay_sanitized.html]
 [dom/test_domloc.xul]
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom/test_domloc_attr_sanitized.html
@@ -0,0 +1,54 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test DOMLocalization's attr sanitization functionality</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript">
+  "use strict";
+  const { DOMLocalization } =
+    ChromeUtils.import("resource://gre/modules/DOMLocalization.jsm", {});
+  const { MessageContext } =
+    ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+
+  async function* mockGenerateMessages(locales, resourceIds) {
+    const mc = new MessageContext(locales);
+    mc.addMessages(`
+key1 = Value for Key 1
+
+key2 = Value for <a>Key 2<a/>.
+    `);
+    yield mc;
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(async () => {
+    const domLoc = new DOMLocalization(
+      window,
+      [],
+      mockGenerateMessages
+    );
+
+    await domLoc.translateFragment(document.body);
+
+    const elem1 = document.querySelector("#elem1");
+    const elem2 = document.querySelector("#elem2");
+
+    ok(elem1.textContent.includes("Value for"));
+    // This is a limitation of us using Node.localize API
+    // Documenting it here to make sure we notice when we fix it
+    is(elem1.getAttribute("title"), "Old Translation");
+
+    ok(elem2.textContent.includes("Value for"));
+    ok(!elem2.hasAttribute("title"));
+
+    SimpleTest.finish();
+  });
+  </script>
+</head>
+<body>
+  <p id="elem1" title="Old Translation" data-l10n-id="key1"></p>
+  <p id="elem2" title="Old Translation" data-l10n-id="key2"></p>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom/test_domloc_overlay_missing_all.html
@@ -0,0 +1,46 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test DOMLocalization's DOMOverlay functionality</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript">
+  "use strict";
+  const { DOMLocalization } =
+    ChromeUtils.import("resource://gre/modules/DOMLocalization.jsm", {});
+  const { MessageContext } =
+    ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+
+  async function* mockGenerateMessages(locales, resourceIds) {
+    const mc = new MessageContext(locales);
+    // No translations!
+    yield mc;
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(async () => {
+
+    const domLoc = new DOMLocalization(
+      window,
+      [],
+      mockGenerateMessages
+    );
+
+    await domLoc.translateFragment(document.body);
+
+    // we just care that it doesn't throw here.
+    ok(1);
+
+    SimpleTest.finish();
+  });
+  </script>
+</head>
+<body>
+  <p data-l10n-id="title">
+    <a href="http://www.mozilla.org"></a>
+    <a href="http://www.firefox.com"></a>
+    <a href="http://www.w3.org"></a>
+  </p>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom/test_domloc_overlay_sanitized.html
@@ -0,0 +1,55 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test DOMLocalization's DOMOverlay functionality</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript">
+  "use strict";
+  const { DOMLocalization } =
+    ChromeUtils.import("resource://gre/modules/DOMLocalization.jsm", {});
+  const { MessageContext } =
+    ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+
+  async function* mockGenerateMessages(locales, resourceIds) {
+    const mc = new MessageContext(locales);
+    mc.addMessages(`
+key1 =
+    .href = https://www.hacked.com
+
+key2 =
+    .href = https://pl.wikipedia.org
+`);
+    yield mc;
+  }
+
+  async function test() {
+    const domLoc = new DOMLocalization(
+      window,
+      [],
+      mockGenerateMessages
+    );
+
+    await domLoc.translateFragment(document.body);
+
+    const key1Elem = document.querySelector("[data-l10n-id=key1]");
+    const key2Elem = document.querySelector("[data-l10n-id=key2]");
+
+
+    is(key1Elem.hasAttribute("href"), false, "href translation should not be allowed");
+    is(key2Elem.getAttribute("href"), "https://pl.wikipedia.org",
+      "href translation should be allowed");
+
+    SimpleTest.finish();
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(test);
+  </script>
+</head>
+<body>
+  <a data-l10n-id="key1"></a>
+  <a data-l10n-id="key2" data-l10n-attrs="href"></a>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom/test_domloc_repeated_l10nid.html
@@ -0,0 +1,63 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test DOMLocalization's matching l10nIds functionality</title>
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript">
+  "use strict";
+  const { DOMLocalization } =
+    ChromeUtils.import("resource://gre/modules/DOMLocalization.jsm", {});
+  const { MessageContext } =
+    ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+
+  async function* mockGenerateMessages(locales, resourceIds) {
+    const mc = new MessageContext(locales);
+    mc.addMessages(`
+key1 = Translation For Key 1
+
+key2 = Visit <a>this link<a/>.
+    `);
+    yield mc;
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  addLoadEvent(async () => {
+    const domLoc = new DOMLocalization(
+      window,
+      [],
+      mockGenerateMessages
+    );
+
+    await domLoc.translateFragment(document.body);
+
+    ok(document.querySelector("#elem1").textContent.includes("Key 1"));
+    ok(document.querySelector("#elem2").textContent.includes("Key 1"));
+
+    const elem3 = document.querySelector("#elem3");
+    const elem4 = document.querySelector("#elem4");
+
+    ok(elem3.textContent.includes("Visit"));
+    is(elem3.querySelector("a").getAttribute("href"), "http://www.mozilla.org");
+
+    ok(elem4.textContent.includes("Visit"));
+    is(elem4.querySelector("a").getAttribute("href"), "http://www.firefox.com");
+
+    SimpleTest.finish();
+  });
+  </script>
+</head>
+<body>
+  <h1 id="elem1" data-l10n-id="key1"></h1>
+  <h2 id="elem2" data-l10n-id="key1"></h2>
+
+  <p id="elem3" data-l10n-id="key2">
+    <a href="http://www.mozilla.org"></a>
+  </p>
+
+  <p id="elem4" data-l10n-id="key2">
+    <a href="http://www.firefox.com"></a>
+  </p>
+</body>
+</html>