Bug 1531343 - Remove auto-increment from range inputs in Font Editor; r=gl
authorRazvan Caliman <rcaliman@mozilla.com>
Thu, 28 Feb 2019 18:15:38 +0000
changeset 519820 6a9be28daf6e47f812310a8b4f78153f8cf5b6a8
parent 519819 ec7068a12a7b90ad6ceb368a4ee6097986b21559
child 519821 6d5c965e0c8e63c2a2ca0cbbc33a08c463b1a5e3
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgl
bugs1531343
milestone67.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 1531343 - Remove auto-increment from range inputs in Font Editor; r=gl With @mbalfanz's permission, I am removing the auto-increment behavior from the range inputs in the Font Editor. The feature isn't easily discoverable and when it does work, its user experience isn't good. We never invested time to refine it and we're unlikely to do so. At this point, this is dead code so we should remove it. What is getting removed is the logic to support the esoteric auto-increment behavior which happens when dragging the range input thumb to the end of the slider and waiting for a while (1000ms). There's needless complexity in handling the concert of events for a little perceived user value. Users can still type out-of-range values in the text input field for the properties which allow it: `font-size` and `line-height`. For these properties' components, there's a new prop, `allowOverflow`, which permits values above the `max` defined. Other properties are bounded by `min` and `max` just like before. Differential Revision: https://phabricator.services.mozilla.com/D21534
devtools/client/inspector/fonts/components/FontPropertyValue.js
devtools/client/inspector/fonts/components/FontSize.js
devtools/client/inspector/fonts/components/LineHeight.js
--- a/devtools/client/inspector/fonts/components/FontPropertyValue.js
+++ b/devtools/client/inspector/fonts/components/FontPropertyValue.js
@@ -6,27 +6,24 @@
 
 const {
   createElement,
   Fragment,
   PureComponent,
 } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
-const { KeyCodes } = require("devtools/client/shared/keycodes");
 
 const { toFixed } = require("../utils/font-utils");
 
-// Milliseconds between auto-increment interval iterations.
-const AUTOINCREMENT_DELAY = 1000;
-
 class FontPropertyValue extends PureComponent {
   static get propTypes() {
     return {
-      autoIncrement: PropTypes.bool,
+      // Whether to allow input values above the value defined by the `max` prop.
+      allowOverflow: PropTypes.bool,
       className: PropTypes.string,
       defaultValue: PropTypes.number,
       disabled: PropTypes.bool.isRequired,
       label: PropTypes.string.isRequired,
       min: PropTypes.number.isRequired,
       // Whether to show the `min` prop value as a label.
       minLabel: PropTypes.bool,
       max: PropTypes.number.isRequired,
@@ -40,72 +37,46 @@ class FontPropertyValue extends PureComp
       unit: PropTypes.string,
       unitOptions: PropTypes.array,
       value: PropTypes.number,
     };
   }
 
   static get defaultProps() {
     return {
-      autoIncrement: false,
+      allowOverflow: false,
       className: "",
       minLabel: false,
       maxLabel: false,
       nameLabel: false,
       step: 1,
       unit: null,
       unitOptions: [],
     };
   }
 
   constructor(props) {
     super(props);
-    // Interval ID for the auto-increment operation.
-    this.interval = null;
     this.state = {
       // Whether the user is dragging the slider thumb or pressing on the numeric stepper.
       interactive: false,
       // Snapshot of the value from props before the user starts editing the number input.
       // Used to restore the value when the input is left invalid.
       initialValue: this.props.value,
       // Snapshot of the value from props. Reconciled with props on blur.
       // Used while the user is interacting with the inputs.
       value: this.props.value,
     };
 
-    this.autoIncrement = this.autoIncrement.bind(this);
     this.onBlur = this.onBlur.bind(this);
     this.onChange = this.onChange.bind(this);
     this.onFocus = this.onFocus.bind(this);
-    this.onKeyDown = this.onKeyDown.bind(this);
-    this.onKeyUp = this.onKeyUp.bind(this);
     this.onMouseDown = this.onMouseDown.bind(this);
-    this.onMouseMove = this.onMouseMove.bind(this);
     this.onMouseUp = this.onMouseUp.bind(this);
     this.onUnitChange = this.onUnitChange.bind(this);
-    this.stopAutoIncrement = this.stopAutoIncrement.bind(this);
-  }
-
-  componentDidUpdate(prevProps, prevState) {
-    // Clear the auto-increment interval if interactive state changed from true to false.
-    if (prevState.interactive && !this.state.interactive) {
-      this.stopAutoIncrement();
-    }
-  }
-
-  componentWillUnmount() {
-    this.stopAutoIncrement();
-  }
-
-  /**
-   * Increment the current value with a step of the next order of magnitude.
-   */
-  autoIncrement() {
-    const value = this.props.value + this.props.step * 10;
-    this.updateValue(value);
   }
 
   /**
    * Given a `prop` key found on the component's props, check the matching `propLabel`.
    * If `propLabel` is true, return the `prop` value; Otherwise, return null.
    *
    * @param {String} prop
    *        Key found on the component's props.
@@ -115,52 +86,38 @@ class FontPropertyValue extends PureComp
     const label = this.props[`${prop}Label`];
     // Decimal count used to limit numbers in labels.
     const decimals = Math.abs(Math.log10(this.props.step));
 
     return label ? toFixed(this.props[prop], decimals) : null;
   }
 
   /**
-   * Check if the given value is at or exceeds the maximum value for the slider and number
-   * inputs. Using Math.floor() on maximum value because unit conversion can yield numbers
-   * with decimals that can't be reached with the step increment for the converted unit.
-   * For example: max = 1000.75% and step = 1
-   *
-   * @param  {Number} value
-   *         Numeric value.
-   * @return {Boolean}
-   */
-  isAtUpperBound(value) {
-    return value >= Math.floor(this.props.max);
-  }
-
-  /**
    * 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 `autoIncrement` props flag.
+   * allowed by the `allowOverflow` props flag.
    *
    * @param  {Number} value
    *         Numeric value
    * @return {Boolean}
    *         Whether the value conforms to the components contraints.
    */
   isValueValid(value) {
-    const { autoIncrement, min, max } = this.props;
+    const { allowOverflow, min, max } = this.props;
 
     if (typeof value !== "number" || isNaN(value)) {
       return false;
     }
 
     if (min !== undefined && value < min) {
       return false;
     }
 
-    // Ensure it does not exceed maximum value, unless auto-incrementing is permitted.
-    if (max !== undefined && value > this.props.max && !autoIncrement) {
+    // Ensure it does not exceed maximum value, unless overflow is allowed.
+    if (max !== undefined && value > this.props.max && !allowOverflow) {
       return false;
     }
 
     return true;
   }
 
   /**
    * Handler for "blur" events from the range and number input fields.
@@ -243,87 +200,36 @@ class FontPropertyValue extends PureComp
       return {
         ...prevState,
         interactive: true,
         initialValue: this.props.value,
       };
     });
   }
 
-  /**
-   * Handler for "keydown" events from the range input field.
-   * Begin auto-incrementing if the slider value is already at the upper boun and the
-   * keyboard gesture requests a higher value.
-   *
-   * @param {Event} e
-   *        KeyDown event.
-   */
-  onKeyDown(e) {
-    if (this.isAtUpperBound(this.props.value) &&
-        e.keyCode === KeyCodes.DOM_VK_UP || e.keyCode === KeyCodes.DOM_VK_RIGHT) {
-      this.startAutoIncrement();
-    }
-  }
-
-  onKeyUp(e) {
-    if (e.keyCode === KeyCodes.DOM_VK_UP || e.keyCode === KeyCodes.DOM_VK_RIGHT) {
-      this.stopAutoIncrement();
-    }
-  }
-
   onUnitChange(e) {
     this.props.onChange(this.props.name, this.props.value, this.props.unit,
        e.target.value);
     // Reset internal state value and wait for converted value from props.
     this.setState((prevState) => {
       return {
         ...prevState,
         value: null,
       };
     });
   }
 
   onMouseDown() {
     this.toggleInteractiveState(true);
   }
 
-  /**
-   * Handler for "mousemove" event from range input. If the user is actively interacting
-   * by dragging the slider thumb, start or stop the auto-incrementing behavior depending
-   * on whether the input value is at the upper bound or not.
-   *
-   * @param {MouseEvent} e
-   */
-  onMouseMove(e) {
-    if (this.state.interactive && e.buttons) {
-      this.isAtUpperBound(this.props.value)
-        ? this.startAutoIncrement()
-        : this.stopAutoIncrement();
-    }
-  }
-
   onMouseUp() {
-    this.stopAutoIncrement();
     this.toggleInteractiveState(false);
   }
 
-  startAutoIncrement() {
-    // Do not set auto-increment interval if not allowed to or if one is already set.
-    if (!this.props.autoIncrement || this.interval) {
-      return;
-    }
-
-    this.interval = setInterval(this.autoIncrement, AUTOINCREMENT_DELAY);
-  }
-
-  stopAutoIncrement() {
-    clearInterval(this.interval);
-    this.interval = null;
-  }
-
   /**
    * Toggle the "interactive" state which causes render() to use `value` fom internal
    * state instead of from props to prevent jittering during continous dragging of the
    * range input thumb or incrementing from the number input.
    *
    * @param {Boolean} isInteractive
    *        Whether to mark the interactive state on or off.
    */
@@ -441,34 +347,31 @@ class FontPropertyValue extends PureComp
       // While interacting with the range and number inputs, prevent updating value from
       // outside props which is debounced and causes jitter on successive renders.
       value: this.state.interactive ? this.state.value : propsValue,
     };
 
     const range = dom.input(
       {
         ...defaults,
-        onKeyDown: this.onKeyDown,
-        onKeyUp: this.onKeyUp,
         onMouseDown: this.onMouseDown,
-        onMouseMove: this.onMouseMove,
         onMouseUp: this.onMouseUp,
         className: "font-value-slider",
         disabled: this.props.disabled,
         name: this.props.name,
         title: this.props.label,
         type: "range",
       }
     );
 
     const input = dom.input(
       {
         ...defaults,
-        // Remove upper limit from number input if it is allowed to auto-increment.
-        max: this.props.autoIncrement ? null : this.props.max,
+        // Remove upper limit from number input if it is allowed to overflow.
+        max: this.props.allowOverflow ? null : this.props.max,
         name: this.props.name,
         className: "font-value-input",
         disabled: this.props.disabled,
         type: "number",
       }
     );
 
     return dom.label(
--- a/devtools/client/inspector/fonts/components/FontSize.js
+++ b/devtools/client/inspector/fonts/components/FontSize.js
@@ -54,17 +54,17 @@ class FontSize extends PureComponent {
     // Ensure we store the max value ever reached for this unit type. This will be the
     // max value of the input and slider. Without this memoization, the value and slider
     // thumb get clamped at the upper bound while decrementing an out-of-bounds value.
     this.historicMax[unit] = this.historicMax[unit]
       ? Math.max(this.historicMax[unit], max)
       : max;
 
     return FontPropertyValue({
-      autoIncrement: true,
+      allowOverflow: true,
       disabled: this.props.disabled,
       label: getStr("fontinspector.fontSizeLabel"),
       min: 0,
       max: this.historicMax[unit],
       name: "font-size",
       onChange: this.props.onChange,
       step: getStepForUnit(unit),
       unit,
--- a/devtools/client/inspector/fonts/components/LineHeight.js
+++ b/devtools/client/inspector/fonts/components/LineHeight.js
@@ -57,17 +57,17 @@ class LineHeight extends PureComponent {
     // Ensure we store the max value ever reached for this unit type. This will be the
     // max value of the input and slider. Without this memoization, the value and slider
     // thumb get clamped at the upper bound while decrementing an out-of-bounds value.
     this.historicMax[unit] = this.historicMax[unit]
       ? Math.max(this.historicMax[unit], max)
       : max;
 
     return FontPropertyValue({
-      autoIncrement: true,
+      allowOverflow: true,
       disabled: this.props.disabled,
       label: getStr("fontinspector.lineHeightLabelCapitalized"),
       min: 0,
       max: this.historicMax[unit],
       name: "line-height",
       onChange: this.props.onChange,
       step: getStepForUnit(unit),
       unit,