Bug 1534480 - Do not update Fluent DOM attributes if they match current ones. r=smaug
authorZibi Braniecki <zbraniecki@mozilla.com>
Tue, 16 Apr 2019 20:45:52 +0000
changeset 469808 f7f4cfa92ebf
parent 469807 a38b2423065c
child 469809 bcd85ff387fb
push id112825
push usercbrindusan@mozilla.com
push dateWed, 17 Apr 2019 15:58:37 +0000
treeherdermozilla-inbound@7bd43da7830c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1534480
milestone68.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 1534480 - Do not update Fluent DOM attributes if they match current ones. r=smaug Differential Revision: https://phabricator.services.mozilla.com/D23063
dom/base/nsINode.cpp
intl/l10n/DOMLocalization.jsm
intl/l10n/DocumentL10n.cpp
intl/l10n/test/chrome.ini
intl/l10n/test/dom_overlays/test_same_id.html
intl/l10n/test/dom_overlays/test_same_id_args.html
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -2788,22 +2788,26 @@ class LocalizationHandler : public Promi
           return;
         }
       }
 
       Nullable<Sequence<AttributeNameValue>>& attributes =
           l10nData[i].mAttributes;
       if (!attributes.IsNull()) {
         for (size_t j = 0; j < attributes.Value().Length(); ++j) {
-          // Use SetAttribute here to validate the attribute name!
-          elem->SetAttribute(attributes.Value()[j].mName,
-                             attributes.Value()[j].mValue, rv);
-          if (rv.Failed()) {
-            mReturnValuePromise->MaybeRejectWithUndefined();
-            return;
+          nsString& name = attributes.Value()[j].mName;
+          nsString& value = attributes.Value()[j].mValue;
+          RefPtr<nsAtom> nameAtom = NS_Atomize(name);
+          if (!elem->AttrValueIs(kNameSpaceID_None, nameAtom, value,
+                                 eCaseMatters)) {
+            rv = elem->SetAttr(kNameSpaceID_None, nameAtom, value, true);
+            if (rv.Failed()) {
+              mReturnValuePromise->MaybeRejectWithUndefined();
+              return;
+            }
           }
         }
       }
 
       if (content.IsVoid() && attributes.IsNull()) {
         JS::Rooted<JS::Value> wrappedElem(aCx);
         if (!ToJSValue(aCx, elem, &wrappedElem)) {
           mReturnValuePromise->MaybeRejectWithUndefined();
--- a/intl/l10n/DOMLocalization.jsm
+++ b/intl/l10n/DOMLocalization.jsm
@@ -146,49 +146,64 @@ function overlayChildNodes(fromFragment,
     // If all else fails, replace the element with its text content.
     fromFragment.replaceChild(textNode(childNode), childNode);
   }
 
   toElement.textContent = "";
   toElement.appendChild(fromFragment);
 }
 
+function hasAttribute(attributes, name) {
+  if (!attributes) {
+    return false;
+  }
+  for (let attr of attributes) {
+    if (attr.name === name) {
+      return true;
+    }
+  }
+  return false;
+}
+
 /**
  * Transplant localizable attributes of an element to another element.
  *
  * Any localizable attributes already set on the target element will be
  * cleared.
  *
  * @param   {Element|Object} fromElement - The source of child nodes to overlay.
  * @param   {Element} toElement - The target of the overlay.
  * @private
  */
 function overlayAttributes(fromElement, toElement) {
   const explicitlyAllowed = toElement.hasAttribute("data-l10n-attrs")
     ? toElement.getAttribute("data-l10n-attrs")
       .split(",").map(i => i.trim())
     : null;
 
-  // Remove existing localizable attributes.
+  // Remove existing localizable attributes if they
+  // will not be used in the new translation.
   for (const attr of Array.from(toElement.attributes)) {
-    if (isAttrNameLocalizable(attr.name, toElement, explicitlyAllowed)) {
+    if (isAttrNameLocalizable(attr.name, toElement, explicitlyAllowed)
+      && !hasAttribute(fromElement.attributes, attr.name)) {
       toElement.removeAttribute(attr.name);
     }
   }
 
   // fromElement might be a {value, attributes} object as returned by
   // Localization.messageFromBundle. In which case attributes may be null to
   // save GC cycles.
   if (!fromElement.attributes) {
     return;
   }
 
   // Set localizable attributes.
   for (const attr of Array.from(fromElement.attributes)) {
-    if (isAttrNameLocalizable(attr.name, toElement, explicitlyAllowed)) {
+    if (isAttrNameLocalizable(attr.name, toElement, explicitlyAllowed)
+      && toElement.getAttribute(attr.name) !== attr.value) {
       toElement.setAttribute(attr.name, attr.value);
     }
   }
 }
 
 /**
  * Sanitize a child element created by the translation.
  *
@@ -470,20 +485,25 @@ class DOMLocalization extends Localizati
    * </p>
    * ```
    *
    * @param {Element}                element - Element to set attributes on
    * @param {string}                 id      - l10n-id string
    * @param {Object<string, string>} args    - KVP list of l10n arguments
    * @returns {Element}
    */
-  setAttributes(element, id, args) {
-    element.setAttribute(L10NID_ATTR_NAME, id);
+  setAttributes(element, id, args = null) {
+    if (element.getAttribute(L10NID_ATTR_NAME) !== id) {
+      element.setAttribute(L10NID_ATTR_NAME, id);
+    }
     if (args) {
-      element.setAttribute(L10NARGS_ATTR_NAME, JSON.stringify(args));
+      let argsString = JSON.stringify(args);
+      if (argsString !== element.getAttribute(L10NARGS_ATTR_NAME)) {
+        element.setAttribute(L10NARGS_ATTR_NAME, argsString);
+      }
     } else {
       element.removeAttribute(L10NARGS_ATTR_NAME);
     }
     return element;
   }
 
   /**
    * Get the `data-l10n-*` attributes from DOM elements.
--- a/intl/l10n/DocumentL10n.cpp
+++ b/intl/l10n/DocumentL10n.cpp
@@ -258,30 +258,34 @@ already_AddRefed<Promise> DocumentL10n::
   }
   return MaybeWrapPromise(promise);
 }
 
 void DocumentL10n::SetAttributes(JSContext* aCx, Element& aElement,
                                  const nsAString& aId,
                                  const Optional<JS::Handle<JSObject*>>& aArgs,
                                  ErrorResult& aRv) {
-  aElement.SetAttribute(NS_LITERAL_STRING("data-l10n-id"), aId, aRv);
-  if (aRv.Failed()) {
-    return;
+  if (!aElement.AttrValueIs(kNameSpaceID_None, nsGkAtoms::datal10nid, aId,
+                            eCaseMatters)) {
+    aElement.SetAttr(kNameSpaceID_None, nsGkAtoms::datal10nid, aId, true);
   }
+
   if (aArgs.WasPassed()) {
     nsAutoString data;
     JS::Rooted<JS::Value> val(aCx, JS::ObjectValue(*aArgs.Value()));
     if (!nsContentUtils::StringifyJSON(aCx, &val, data)) {
       aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
       return;
     }
-    aElement.SetAttribute(NS_LITERAL_STRING("data-l10n-args"), data, aRv);
+    if (!aElement.AttrValueIs(kNameSpaceID_None, nsGkAtoms::datal10nargs, data,
+                              eCaseMatters)) {
+      aElement.SetAttr(kNameSpaceID_None, nsGkAtoms::datal10nargs, data, true);
+    }
   } else {
-    aElement.RemoveAttribute(NS_LITERAL_STRING("data-l10n-args"), aRv);
+    aElement.UnsetAttr(kNameSpaceID_None, nsGkAtoms::datal10nargs, true);
   }
 }
 
 void DocumentL10n::GetAttributes(JSContext* aCx, Element& aElement,
                                  L10nKey& aResult, ErrorResult& aRv) {
   nsAutoString l10nId;
   nsAutoString l10nArgs;
 
--- a/intl/l10n/test/chrome.ini
+++ b/intl/l10n/test/chrome.ini
@@ -21,8 +21,11 @@
 
 [dom/test_docl10n.xul]
 [dom/test_docl10n_initialize_after_parse.xhtml]
 [dom/test_docl10n_initialize_after_parse.xul]
 [dom/test_docl10n.xhtml]
 [dom/test_docl10n.html]
 [dom/test_docl10n_ready_rejected.html]
 [dom/test_docl10n_removeResourceIds.html]
+
+[dom_overlays/test_same_id.html]
+[dom_overlays/test_same_id_args.html]
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom_overlays/test_same_id.html
@@ -0,0 +1,57 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test Amount of mutations generated from DOM Overlays</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">
+  <link rel="localization" href="toolkit/about/aboutTelemetry.ftl"/>
+  <script type="application/javascript">
+  "use strict";
+  SimpleTest.waitForExplicitFinish();
+
+  let config = {
+    attributes: true,
+    attributeOldValue: true,
+    characterData: true,
+    characterDataOldValue: true,
+    childList: true,
+    subtree: true,
+  };
+  let allMutations = [];
+
+  document.addEventListener("DOMContentLoaded", async function() {
+    await document.l10n.ready;
+
+    let inputElem = document.getElementById("search-input");
+
+    // Test for initial localization applied.
+    is(inputElem.getAttribute("placeholder").length > 0, true);
+
+    let observer = new MutationObserver((mutations) => {
+      for (let mutation of mutations) {
+        allMutations.push(mutation);
+      }
+    });
+    observer.observe(inputElem, config);
+
+    document.l10n.setAttributes(inputElem, "about-telemetry-filter-all-placeholder");
+
+    // Due to the async iteractions between nsINode.localize
+    // and DOMLocalization.jsm, we'll need to wait two frames
+    // to verify that no mutations happened.
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        // Since the l10n-id is the same as the previous one
+        // no mutation should happen in result.
+        is(allMutations.length, 0);
+        SimpleTest.finish();
+      });
+    });
+  }, { once: true});
+  </script>
+</head>
+<body>
+  <input id="search-input" data-l10n-id="about-telemetry-filter-all-placeholder"></input>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/intl/l10n/test/dom_overlays/test_same_id_args.html
@@ -0,0 +1,57 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Test Amount of mutations generated from DOM Overlays</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">
+  <link rel="localization" href="toolkit/about/aboutTelemetry.ftl"/>
+  <script type="application/javascript">
+  "use strict";
+  SimpleTest.waitForExplicitFinish();
+
+  let config = {
+    attributes: true,
+    attributeOldValue: true,
+    characterData: true,
+    characterDataOldValue: true,
+    childList: true,
+    subtree: true,
+  };
+  let allMutations = [];
+
+  document.addEventListener("DOMContentLoaded", async function() {
+    await document.l10n.ready;
+
+    let inputElem = document.getElementById("search-input");
+
+    // Test for initial localization applied.
+    is(inputElem.getAttribute("placeholder").length > 0, true);
+
+    let observer = new MutationObserver((mutations) => {
+      for (let mutation of mutations) {
+        allMutations.push(mutation);
+      }
+    });
+    observer.observe(inputElem, config);
+
+    document.l10n.setAttributes(inputElem, "about-telemetry-filter-placeholder", {selectedTitle: "Test"});
+
+    // Due to the async iteractions between nsINode.localize
+    // and DOMLocalization.jsm, we'll need to wait two frames
+    // to verify that no mutations happened.
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        // Since the l10n-id is the same as the previous one
+        // no mutation should happen in result.
+        is(allMutations.length, 0);
+        SimpleTest.finish();
+      });
+    });
+  }, { once: true});
+  </script>
+</head>
+<body>
+  <input id="search-input" data-l10n-id="about-telemetry-filter-placeholder" data-l10n-args='{"selectedTitle":"Test"}'></input>
+</body>
+</html>