Bug 1536237 - Add support for letter spacing in Font Editor. r=gl

- Reads the value for the letter-spacing CSS property and shows it in the Font Editor. When it is default, show the "normal" identifier. As soon as the user tries to edit it using the value slider, switch to an em-based value.

- Tweaks the unit conversion method to support letter-spacing: using correct reference node for em units, returning high-precision results even for pixels (allow sub-pixel precision)

Differential Revision: https://phabricator.services.mozilla.com/D25087

--HG--
extra : moz-landing-system : lando

devtools/client/inspector/fonts/components/FontEditor.js

@@ -13,6 +13,7 @@ const FontName = createFactory(require("./FontName"));
 const FontSize = createFactory(require("./FontSize"));
 const FontStyle = createFactory(require("./FontStyle"));
 const FontWeight = createFactory(require("./FontWeight"));
+const LetterSpacing = createFactory(require("./LetterSpacing"));
 const LineHeight = createFactory(require("./LineHeight"));
 
 const { getStr } = require("../utils/l10n");
@@ -161,6 +162,15 @@ class FontEditor extends PureComponent {
     });
   }
 
+  renderLetterSpacing(value) {
+    return value !== null && LetterSpacing({
+      key: `${this.props.fontEditor.id}:letter-spacing`,
+      disabled: this.props.fontEditor.disabled,
+      onChange: this.props.onPropertyChange,
+      value,
+    });
+  }
+
   renderFontStyle(value) {
     return value && FontStyle({
       onChange: this.props.onPropertyChange,
@@ -283,6 +293,8 @@ class FontEditor extends PureComponent {
       this.renderFontSize(properties["font-size"]),
       // Always render UI for line height.
       this.renderLineHeight(properties["line-height"]),
+      // Always render UI for letter spacing.
+      this.renderLetterSpacing(properties["letter-spacing"]),
       // Render UI for font weight if no "wght" registered axis is defined.
       !hasWeightAxis && this.renderFontWeight(properties["font-weight"]),
       // Render UI for font style if no "slnt" or "ital" registered axis is defined.

devtools/client/inspector/fonts/components/FontPropertyValue.js

@@ -19,6 +19,8 @@ class FontPropertyValue extends PureComponent {
     return {
       // Whether to allow input values above the value defined by the `max` prop.
       allowOverflow: PropTypes.bool,
+      // Whether to allow input values below the value defined by the `min` prop.
+      allowUnderflow: PropTypes.bool,
       className: PropTypes.string,
       defaultValue: PropTypes.number,
       disabled: PropTypes.bool.isRequired,
@@ -34,20 +36,28 @@ class FontPropertyValue extends PureComponent {
       nameLabel: PropTypes.bool,
       onChange: PropTypes.func.isRequired,
       step: PropTypes.number,
+      // Whether to show the value input field.
+      showInput: PropTypes.bool,
+      // Whether to show the unit select dropdown.
+      showUnit: PropTypes.bool,
       unit: PropTypes.string,
       unitOptions: PropTypes.array,
       value: PropTypes.number,
+      valueLabel: PropTypes.string,
     };
   }
 
   static get defaultProps() {
     return {
       allowOverflow: false,
+      allowUnderflow: false,
       className: "",
       minLabel: false,
       maxLabel: false,
       nameLabel: false,
       step: 1,
+      showInput: true,
+      showUnit: true,
       unit: null,
       unitOptions: [],
     };
@@ -93,7 +103,7 @@ class FontPropertyValue extends PureComponent {
   /**
    * Check if the given value is valid according to the constraints of this component.
    * Ensure it is a number and that it does not go outside the min/max limits, unless
-   * allowed by the `allowOverflow` props flag.
+   * allowed by the `allowOverflow` and `allowUnderflow` props.
    *
    * @param  {Number} value
    *         Numeric value
@@ -101,18 +111,19 @@ class FontPropertyValue extends PureComponent {
    *         Whether the value conforms to the components contraints.
    */
   isValueValid(value) {
-    const { allowOverflow, min, max } = this.props;
+    const { allowOverflow, allowUnderflow, min, max } = this.props;
 
     if (typeof value !== "number" || isNaN(value)) {
       return false;
     }
 
-    if (min !== undefined && value < min) {
+    // Ensure it does not go below minimum value, unless underflow is allowed.
+    if (min !== undefined && value < min && !allowUnderflow) {
       return false;
     }
 
-    // Ensure it does not exceed maximum value, unless overflow is allowed.
-    if (max !== undefined && value > this.props.max && !allowOverflow) {
+    // Ensure it does not go over maximum value, unless overflow is allowed.
+    if (max !== undefined && value > max && !allowOverflow) {
       return false;
     }
 
@@ -328,6 +339,14 @@ class FontPropertyValue extends PureComponent {
     return createElement(Fragment, null, labelEl, detailEl);
   }
 
+  renderValueLabel() {
+    if (!this.props.valueLabel) {
+      return null;
+    }
+
+    return dom.div({ className: "font-value-label" }, this.props.valueLabel);
+  }
+
   render() {
     // Guard against bad axis data.
     if (this.props.min === this.props.max) {
@@ -366,6 +385,8 @@ class FontPropertyValue extends PureComponent {
     const input = dom.input(
       {
         ...defaults,
+        // Remove lower limit from number input if it is allowed to underflow.
+        min: this.props.allowUnderflow ? null : this.props.min,
         // Remove upper limit from number input if it is allowed to overflow.
         max: this.props.allowOverflow ? null : this.props.max,
         name: this.props.name,
@@ -399,8 +420,9 @@ class FontPropertyValue extends PureComponent {
           },
           range
         ),
-        input,
-        this.renderUnitSelect()
+        this.renderValueLabel(),
+        this.props.showInput && input,
+        this.props.showUnit && this.renderUnitSelect()
       )
     );
   }

devtools/client/inspector/fonts/components/LetterSpacing.js

@@ -0,0 +1,96 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
+const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
+
+const FontPropertyValue = createFactory(require("./FontPropertyValue"));
+
+const { getStr } = require("../utils/l10n");
+const { getUnitFromValue, getStepForUnit } = require("../utils/font-utils");
+
+class LineHeight extends PureComponent {
+  static get propTypes() {
+    return {
+      disabled: PropTypes.bool.isRequired,
+      onChange: PropTypes.func.isRequired,
+      value: PropTypes.string.isRequired,
+    };
+  }
+
+  constructor(props) {
+    super(props);
+    // Local state for min/max bounds indexed by unit to allow user input that
+    // goes out-of-bounds while still providing a meaningful default range. The indexing
+    // by unit is needed to account for unit conversion (ex: em to px) where the operation
+    // may result in out-of-bounds values. Avoiding React's state and setState() because
+    // `value` is a prop coming from the Redux store while min/max are local. Reconciling
+    // value/unit changes is needlessly complicated and adds unnecessary re-renders.
+    this.historicMin = {};
+    this.historicMax = {};
+  }
+
+  getDefaultMinMax(unit) {
+    let min;
+    let max;
+    switch (unit) {
+      case "px":
+        min = -10;
+        max = 10;
+        break;
+      default:
+        min = -0.2;
+        max = 0.6;
+        break;
+    }
+
+    return { min, max };
+  }
+
+  render() {
+    // For a unitless or a NaN value, default unit to "em".
+    const unit = getUnitFromValue(this.props.value) || "em";
+    // When the initial value of "letter-spacing" is "normal", the parsed value
+    // is not a number (NaN). Guard by setting the default value to 0.
+    let value = parseFloat(this.props.value);
+    const hasKeywordValue = isNaN(value);
+    value = isNaN(value) ? 0 : value;
+
+    let { min, max } = this.getDefaultMinMax(unit);
+    min = Math.min(min, value);
+    max = Math.max(max, value);
+    // Allow lower and upper bounds to move to accomodate the incoming value.
+    this.historicMin[unit] = this.historicMin[unit]
+      ? Math.min(this.historicMin[unit], min)
+      : min;
+    this.historicMax[unit] = this.historicMax[unit]
+      ? Math.max(this.historicMax[unit], max)
+      : max;
+
+    return FontPropertyValue({
+      allowOverflow: true,
+      allowUnderflow: true,
+      disabled: this.props.disabled,
+      label: getStr("fontinspector.letterSpacingLabel"),
+      min: this.historicMin[unit],
+      max: this.historicMax[unit],
+      name: "letter-spacing",
+      onChange: this.props.onChange,
+      // Increase the increment granularity because letter spacing is very sensitive.
+      step: getStepForUnit(unit) / 100,
+      // Show the value input and unit only when the value is not a keyword.
+      showInput: !hasKeywordValue,
+      showUnit: !hasKeywordValue,
+      unit,
+      unitOptions: ["em", "rem", "px"],
+      value,
+      // Show the value as a read-only label if it's a keyword.
+      valueLabel: hasKeywordValue ? this.props.value : null,
+    });
+  }
+}
+
+module.exports = LineHeight;

devtools/client/inspector/fonts/components/moz.build

@@ -19,5 +19,6 @@ DevToolsModules(
     'FontSize.js',
     'FontStyle.js',
     'FontWeight.js',
+    'LetterSpacing.js',
     'LineHeight.js',
 )

devtools/client/inspector/fonts/fonts.js

@@ -42,6 +42,7 @@ const FONT_PROPERTIES = [
   "font-style",
   "font-variation-settings",
   "font-weight",
+  "letter-spacing",
   "line-height",
 ];
 const REGISTERED_AXES_TO_FONT_PROPERTIES = {
@@ -168,7 +169,9 @@ class FontInspector {
     // NodeFront instance of selected/target element.
     const node = this.node;
     // Reference node based on which to convert relative sizes like "em" and "%".
-    const referenceNode = (property === "line-height") ? node : node.parentNode();
+    const referenceNode = (property === "line-height" || property === "letter-spacing")
+      ? node
+      : node.parentNode();
     // Default output value to input value for a 1-to-1 conversion as a guard against
     // unrecognized CSS units. It will not be correct, but it will also not break.
     let out = value;
@@ -278,12 +281,15 @@ class FontInspector {
       out = 0;
     }
 
-    // Return rounded pixel values. Limit other values to 3 decimals.
-    if (fromPx) {
+    // Return values limited to 3 decimals when:
+    // - the unit is converted from pixels to something else
+    // - the value is for letter spacing, regardless of unit (allow sub-pixel precision)
+    if (fromPx || property === "letter-spacing") {
       // Round values like 1.000 to 1
       return out === Math.round(out) ? Math.round(out) : out.toFixed(3);
     }
 
+    // Round pixel values.
     return Math.round(out);
   }
 
@@ -360,6 +366,7 @@ class FontInspector {
    * - font-size
    * - font-weight
    * - font-stretch
+   * - letter-spacing
    * - line-height
    *
    * This list is used to filter out values when reading CSS font properties from rules.
@@ -372,6 +379,7 @@ class FontInspector {
       "font-size",
       "font-weight",
       "font-stretch",
+      "letter-spacing",
       "line-height",
     ].reduce((acc, property) => {
       return acc.concat(this.cssProperties.getValues(property));

devtools/client/inspector/fonts/utils/font-utils.js

@@ -43,7 +43,7 @@ module.exports = {
    *         CSS unit type, like "px", "em", "rem", etc or null.
    */
   getUnitFromValue(value) {
-    if (typeof value !== "string") {
+    if (typeof value !== "string" || isNaN(parseFloat(value))) {
       return null;
     }
 

devtools/client/locales/en-US/font-inspector.properties

@@ -49,6 +49,10 @@ fontinspector.showMore=Show more
 # LOCALIZATION NOTE (fontinspector.showLess): Label for an expanded list of fonts.
 fontinspector.showLess=Show less
 
+# LOCALIZATION NOTE (fontinspector.letterSpacingLabel): Label for the UI to change the
+# letter spacing in the font editor.
+fontinspector.letterSpacingLabel=Spacing
+
 # LOCALIZATION NOTE (fontinspector.lineHeightLabelCapitalized): Label for the UI to change the line height in the font editor.
 fontinspector.lineHeightLabelCapitalized=Line Height
 

devtools/client/themes/fonts.css

@@ -285,6 +285,14 @@
   border-right: none;
 }
 
+.font-value-label {
+  /* Combined width of .font-value-input and .font-value-select */
+  width: calc(60px + 3.8em);
+  margin-left: 10px;
+  padding-top: 2px;
+  padding-bottom: 4px;
+}
+
 /* Mock separator because inputs don't have distinguishable borders in dark theme */
 .theme-dark .font-value-input + .font-value-select {
   margin-left: 2px;