Bug 1469109 - Do not rely on stored TextProperty when updating inline styles. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Fri, 15 Jun 2018 19:30:53 -0700
changeset 479680 7d56af790745509ecb3636a8c7fa8a6c0f7eebf6
parent 479679 db3c96ac1e3a8398a7fec2414487526026efc607
child 479681 645e1b5b6470312fccafb126a1678b939f0d2b7f
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1469109
milestone62.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 1469109 - Do not rely on stored TextProperty when updating inline styles. r=pbro MozReview-Commit-ID: BV6JlIol3k5
devtools/client/inspector/fonts/fonts.js
--- a/devtools/client/inspector/fonts/fonts.js
+++ b/devtools/client/inspector/fonts/fonts.js
@@ -6,17 +6,16 @@
 
 "use strict";
 
 const Services = require("Services");
 const { gDevTools } = require("devtools/client/framework/devtools");
 const { getColor } = require("devtools/client/shared/theme");
 const { createFactory, createElement } = require("devtools/client/shared/vendor/react");
 const { Provider } = require("devtools/client/shared/vendor/react-redux");
-const { throttle } = require("devtools/shared/throttle");
 const { debounce } = require("devtools/shared/debounce");
 const { ELEMENT_STYLE } = require("devtools/shared/specs/styles");
 
 const FontsApp = createFactory(require("./components/FontsApp"));
 
 const { LocalizationHelper } = require("devtools/shared/l10n");
 const INSPECTOR_L10N =
   new LocalizationHelper("devtools/client/locales/inspector.properties");
@@ -58,26 +57,24 @@ class FontInspector {
   constructor(inspector, window) {
     this.document = window.document;
     this.inspector = inspector;
     this.nodeComputedStyle = {};
     this.pageStyle = this.inspector.pageStyle;
     this.ruleView = this.inspector.getPanel("ruleview").view;
     this.selectedRule = null;
     this.store = this.inspector.store;
-    // Map CSS property names to corresponding TextProperty instances from the Rule view.
-    this.textProperties = new Map();
     // Map CSS property names and variable font axis names to methods that write their
     // corresponding values to the appropriate TextProperty from the Rule view.
     // Values of variable font registered axes may be written to CSS font properties under
     // certain cascade circumstances and platform support. @see `getWriterForAxis(axis)`
     this.writers = new Map();
 
     this.snapshotChanges = debounce(this.snapshotChanges, 100, this);
-    this.syncChanges = throttle(this.syncChanges, 100, this);
+    this.syncChanges = debounce(this.syncChanges, 100, this);
     this.onInstanceChange = this.onInstanceChange.bind(this);
     this.onNewNode = this.onNewNode.bind(this);
     this.onPreviewFonts = this.onPreviewFonts.bind(this);
     this.onPropertyChange = this.onPropertyChange.bind(this);
     this.onToggleFontHighlight = this.onToggleFontHighlight.bind(this);
     this.onRuleUpdated = this.onRuleUpdated.bind(this);
     this.onThemeChanged = this.onThemeChanged.bind(this);
     this.update = this.update.bind(this);
@@ -146,18 +143,16 @@ class FontInspector {
 
     this.document = null;
     this.inspector = null;
     this.nodeComputedStyle = {};
     this.pageStyle = null;
     this.ruleView = null;
     this.selectedRule = null;
     this.store = null;
-    this.textProperties.clear();
-    this.textProperties = null;
     this.writers.clear();
     this.writers = null;
   }
 
   /**
    * Get all expected CSS font properties and values from the node's matching rules and
    * fallback to computed style.
    *
@@ -224,27 +219,23 @@ class FontInspector {
    *
    * @param {String} name
    *        CSS property name
    * @param {String} value
    *        CSS property value
    * @return {TextProperty}
    */
   getTextProperty(name, value) {
-    if (!this.textProperties.has(name)) {
-      let textProperty =
-        this.selectedRule.textProps.find(prop => prop.name === name);
-      if (!textProperty) {
-        textProperty = this.selectedRule.editor.addProperty(name, value, "", true);
-      }
-
-      this.textProperties.set(name, textProperty);
+    let textProperty =
+      this.selectedRule.textProps.find(prop => prop.name === name);
+    if (!textProperty) {
+      textProperty = this.selectedRule.editor.addProperty(name, value, "", true);
     }
 
-    return this.textProperties.get(name);
+    return textProperty;
   }
 
   /**
    * Given the axis name of a registered axis, return a method which updates the
    * corresponding CSS font property when called with a value.
    *
    * All variable font axes can be written in the value of the "font-variation-settings"
    * CSS font property. In CSS Fonts Level 4, registered axes values can be used as
@@ -366,29 +357,33 @@ class FontInspector {
    * @return {Boolean}
    */
   isSelectedNodeValid() {
     return this.inspector.selection.nodeFront &&
            this.inspector.selection.isConnected() &&
            this.inspector.selection.isElementNode();
   }
 
-    /**
-   * Sync the Rule view with the styles from the page. Called in a throttled way
+  /**
+   * Sync the Rule view with the latest styles from the page. Called in a debounced way
    * (see constructor) after property changes are applied directly to the CSS style rule
-   * on the page circumventing TextProperty.setValue() which triggers expensive DOM
+   * on the page circumventing direct TextProperty.setValue() which triggers expensive DOM
    * operations in TextPropertyEditor.update().
    *
-   * @param  {TextProperty} textProperty
-   *         Model of CSS declaration for a property in used in the rule view.
+   * @param  {String} name
+   *         CSS property name
    * @param  {String} value
-   *         Value of the CSS property that should be reflected in the rule view.
+   *         CSS property value
    */
-  syncChanges(textProperty, value) {
-    textProperty.updateValue(value);
+  syncChanges(name, value) {
+    const textProperty = this.getTextProperty(name, value);
+    if (textProperty) {
+      textProperty.setValue(value);
+    }
+
     this.ruleView.on("property-value-updated", this.onRuleUpdated);
   }
 
   /**
    * Handler for changes of a font axis value coming from the FontEditor.
    *
    * @param  {String} tag
    *         Tag name of the font axis.
@@ -577,20 +572,19 @@ class FontInspector {
       this.store.dispatch(resetFontEditor());
       return;
     }
 
     // Get computed styles for the selected node, but filter by CSS font properties.
     this.nodeComputedStyle = await this.pageStyle.getComputed(node, {
       filterProperties: FONT_PROPERTIES
     });
-    // Clear any references to writer methods and CSS declarations because the node's
+    // Clear any references to writer methods because the node's
     // styles may have changed since the last font editor refresh.
     this.writers.clear();
-    this.textProperties.clear();
     // Select the node's inline style as the rule where to write property value changes.
     this.selectedRule =
       this.ruleView.rules.find(rule => rule.domRule.type === ELEMENT_STYLE);
     const fontEditor = this.store.getState().fontEditor;
     const properties = this.getFontProperties();
     // Names of fonts declared in font-family property without quotes and space trimmed.
     const declaredFontNames =
       properties["font-family"].split(",").map(font => font.replace(/\"+/g, "").trim());
@@ -691,31 +685,39 @@ class FontInspector {
       // Build a string value for the "font-variation-settings" CSS property
       .map(tag => `"${tag}" ${fontEditor.axes[tag]}`)
       .join(", ");
 
     this.updatePropertyValue(name, value);
   }
 
   /**
-   * Preview a property value (live) then sync the changes (throttled) to the Rule view.
+   * Preview a property value (live) then sync the changes (debounced) to the Rule view.
+   *
+   * NOTE: Until Bug 1462591 is addressed, all changes are written to the element's inline
+   * style attribute. In this current scenario, Rule.previewPropertyValue()
+   * causes the whole inline style representation in the Rule view to update instead of
+   * just previewing the change on the element.
+   * We keep the debounced call to syncChanges() because it explicitly calls
+   * TextProperty.setValue() which performs other actions, including marking the property
+   * as "changed" in the Rule view with a green indicator.
    *
    * @param {String} name
    *        CSS property name
    * @param {String}value
    *        CSS property value
    */
   updatePropertyValue(name, value) {
     const textProperty = this.getTextProperty(name, value);
     if (!textProperty || textProperty.value === value) {
       return;
     }
 
     // Prevent reacting to changes we caused.
     this.ruleView.off("property-value-updated", this.onRuleUpdated);
     // Live preview font property changes on the page.
     textProperty.rule.previewPropertyValue(textProperty, value, "");
-    // Sync Rule view with changes reflected on the page (throttled).
-    this.syncChanges(textProperty, value);
+    // Sync Rule view with changes reflected on the page (debounced).
+    this.syncChanges(name, value);
   }
 }
 
 module.exports = FontInspector;