Bug 1363862 - Use Node.localize for fragment translation in Fluent DOM. r=stas
authorZibi Braniecki <zbraniecki@mozilla.com>
Thu, 01 Mar 2018 16:49:37 -0800
changeset 406707 c483f8c3bf07
parent 406706 3ce625a51597
child 406708 f56d6bee4412
push id33576
push userdluca@mozilla.com
push dateTue, 06 Mar 2018 18:34:13 +0000
treeherdermozilla-central@b498494cfac5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstas
bugs1363862
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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>