From 59130470a85aac5cd78e7275958b98db8c63836f Mon Sep 17 00:00:00 2001
From: Lucian <lucian.buzzo@gmail.com>
Date: Fri, 15 Feb 2019 12:18:43 +0000
Subject: [PATCH] Improve handling of decimal points and trailing zeroes in
 numbers

Connects to #674 #958

Change-type: patch
Signed-off-by: Lucian <lucian.buzzo@gmail.com>
---
 src/components/fields/NumberField.js |  85 ++++++++++++++++++--
 src/components/widgets/BaseInput.js  |  28 ++++++-
 test/ArrayField_test.js              |   8 +-
 test/Form_test.js                    |   4 +-
 test/NumberField_test.js             | 112 +++++++++++++++++++++++++--
 test/uiSchema_test.js                |   4 +-
 6 files changed, 215 insertions(+), 26 deletions(-)

diff --git a/src/components/fields/NumberField.js b/src/components/fields/NumberField.js
index 1f7f7c112d..7b36bcf664 100644
--- a/src/components/fields/NumberField.js
+++ b/src/components/fields/NumberField.js
@@ -3,14 +3,83 @@ import React from "react";
 import * as types from "../../types";
 import { asNumber } from "../../utils";
 
-function NumberField(props) {
-  const { StringField } = props.registry.fields;
-  return (
-    <StringField
-      {...props}
-      onChange={value => props.onChange(asNumber(value))}
-    />
-  );
+// Matches a string that ends in a . character, optionally followed by a sequence of
+// digits followed by any number of 0 characters up until the end of the line.
+// Ensuring that there is at least one prefixed character is important so that
+// you don't incorrectly match against "0".
+const trailingCharMatcherWithPrefix = /\.([0-9]*0)*$/;
+
+// This is used for trimming the trailing 0 and . characters without affecting
+// the rest of the string. Its possible to use one RegEx with groups for this
+// functionality, but it is fairly complex compared to simply defining two
+// different matchers.
+const trailingCharMatcher = /[0.]0*$/;
+
+/**
+ * The NumberField class has some special handling for dealing with trailing
+ * decimal points and/or zeroes. This logic is designed to allow trailing values
+ * to be visible in the input element, but not be represented in the
+ * corresponding form data.
+ *
+ * The algorithm is as follows:
+ *
+ * 1. When the input value changes the value is cached in the component state
+ *
+ * 2. The value is then normalized, removing trailing decimal points and zeros,
+ *    then passed to the "onChange" callback
+ *
+ * 3. When the component is rendered, the formData value is checked against the
+ *    value cached in the state. If it matches the cached value, the cached
+ *    value is passed to the input instead of the formData value
+ */
+class NumberField extends React.Component {
+  constructor(props) {
+    super(props);
+
+    this.state = {
+      lastValue: props.value,
+    };
+  }
+
+  handleChange = value => {
+    // Cache the original value in component state
+    this.setState({ lastValue: value });
+
+    // Check that the value is a string (this can happen if the widget used is a
+    // <select>, due to an enum declaration etc) then, if the value ends in a
+    // trailing decimal point or multiple zeroes, strip the trailing values
+    let normalized =
+      typeof value === "string" && value.match(trailingCharMatcherWithPrefix)
+        ? asNumber(value.replace(trailingCharMatcher, ""))
+        : asNumber(value);
+
+    this.props.onChange(normalized);
+  };
+
+  render() {
+    const { StringField } = this.props.registry.fields;
+    const { formData, ...props } = this.props;
+    const { lastValue } = this.state;
+
+    let value = formData;
+
+    if (typeof lastValue === "string" && value) {
+      // Construct a regular expression that checks for a string that consists
+      // of the formData value suffixed with zero or one '.' characters and zero
+      // or more '0' characters
+      const re = new RegExp(`${value}`.replace(".", "\\.") + "\\.?0*$");
+
+      // If the cached "lastValue" is a match, use that instead of the formData
+      // value to prevent the input value from changing in the UI
+      if (lastValue.match(re)) {
+        value = lastValue;
+      }
+    }
+
+    return (
+      <StringField {...props} formData={value} onChange={this.handleChange} />
+    );
+  }
 }
 
 if (process.env.NODE_ENV !== "production") {
diff --git a/src/components/widgets/BaseInput.js b/src/components/widgets/BaseInput.js
index d28d59c095..30afb03cda 100644
--- a/src/components/widgets/BaseInput.js
+++ b/src/components/widgets/BaseInput.js
@@ -23,7 +23,32 @@ function BaseInput(props) {
     ...inputProps
   } = props;
 
-  inputProps.type = options.inputType || inputProps.type || "text";
+  // If options.inputType is set use that as the input type
+  if (options.inputType) {
+    inputProps.type = options.inputType;
+  } else if (!inputProps.type) {
+    // If the schema is of type number or integer, set the input type to number
+    if (schema.type === "number") {
+      inputProps.type = "number";
+      // Setting step to 'any' fixes a bug in Safari where decimals are not
+      // allowed in number inputs
+      inputProps.step = "any";
+    } else if (schema.type === "integer") {
+      inputProps.type = "number";
+      // Since this is integer, you always want to step up or down in multiples
+      // of 1
+      inputProps.step = "1";
+    } else {
+      inputProps.type = "text";
+    }
+  }
+
+  // If multipleOf is defined, use this as the step value. This mainly improves
+  // the experience for keyboard users (who can use the up/down KB arrows).
+  if (schema.multipleOf) {
+    inputProps.step = schema.multipleOf;
+  }
+
   const _onChange = ({ target: { value } }) => {
     return props.onChange(value === "" ? options.emptyValue : value);
   };
@@ -44,7 +69,6 @@ function BaseInput(props) {
 }
 
 BaseInput.defaultProps = {
-  type: "text",
   required: false,
   disabled: false,
   readonly: false,
diff --git a/test/ArrayField_test.js b/test/ArrayField_test.js
index da17bf630f..6cfaa3f7b1 100644
--- a/test/ArrayField_test.js
+++ b/test/ArrayField_test.js
@@ -1102,7 +1102,7 @@ describe("ArrayField", () => {
         "fieldset .field-string input[type=text]"
       );
       const numInput = node.querySelector(
-        "fieldset .field-number input[type=text]"
+        "fieldset .field-number input[type=number]"
       );
       expect(strInput.id).eql("root_0");
       expect(numInput.id).eql("root_1");
@@ -1114,7 +1114,7 @@ describe("ArrayField", () => {
         "fieldset .field-string input[type=text]"
       );
       const numInput = node.querySelector(
-        "fieldset .field-number input[type=text]"
+        "fieldset .field-number input[type=number]"
       );
       expect(strInput.required).eql(true);
       expect(numInput.required).eql(true);
@@ -1129,7 +1129,7 @@ describe("ArrayField", () => {
         "fieldset .field-string input[type=text]"
       );
       const numInput = node.querySelector(
-        "fieldset .field-number input[type=text]"
+        "fieldset .field-number input[type=number]"
       );
       expect(strInput.value).eql("foo");
       expect(numInput.value).eql("42");
@@ -1141,7 +1141,7 @@ describe("ArrayField", () => {
         "fieldset .field-string input[type=text]"
       );
       const numInput = node.querySelector(
-        "fieldset .field-number input[type=text]"
+        "fieldset .field-number input[type=number]"
       );
 
       Simulate.change(strInput, { target: { value: "bar" } });
diff --git a/test/Form_test.js b/test/Form_test.js
index 3933f570d3..340bb5e600 100644
--- a/test/Form_test.js
+++ b/test/Form_test.js
@@ -1693,7 +1693,7 @@ describe("Form", () => {
           liveValidate: true,
         });
 
-        Simulate.change(node.querySelector("input[type=text]"), {
+        Simulate.change(node.querySelector("input[type=number]"), {
           target: { value: "not a number" },
         });
 
@@ -1711,7 +1711,7 @@ describe("Form", () => {
           formData: { branch: 2 },
         });
 
-        Simulate.change(node.querySelector("input[type=text]"), {
+        Simulate.change(node.querySelector("input[type=number]"), {
           target: { value: "not a number" },
         });
 
diff --git a/test/NumberField_test.js b/test/NumberField_test.js
index 687fa65766..3aa7dbab6a 100644
--- a/test/NumberField_test.js
+++ b/test/NumberField_test.js
@@ -3,7 +3,7 @@ import { expect } from "chai";
 import { Simulate } from "react-addons-test-utils";
 import sinon from "sinon";
 
-import { createFormComponent, createSandbox } from "./test_utils";
+import { createFormComponent, createSandbox, setProps } from "./test_utils";
 
 describe("NumberField", () => {
   let sandbox;
@@ -17,7 +17,7 @@ describe("NumberField", () => {
   });
 
   describe("TextWidget", () => {
-    it("should render a string field", () => {
+    it("should render a number input", () => {
       const { node } = createFormComponent({
         schema: {
           type: "number",
@@ -25,7 +25,7 @@ describe("NumberField", () => {
       });
 
       expect(
-        node.querySelectorAll(".field input[type=text]")
+        node.querySelectorAll(".field input[type=number]")
       ).to.have.length.of(1);
     });
 
@@ -129,18 +129,90 @@ describe("NumberField", () => {
       expect(node.querySelector(".field input").value).eql("2");
     });
 
-    it("should not cast the input as a number if it ends with a dot", () => {
+    describe("when inputting a number that ends with a dot and/or zero it should normalize it, without changing the input value", () => {
       const { comp, node } = createFormComponent({
         schema: {
           type: "number",
         },
       });
 
-      Simulate.change(node.querySelector("input"), {
-        target: { value: "2." },
+      const $input = node.querySelector("input");
+
+      const tests = [
+        {
+          input: "2.",
+          output: 2,
+        },
+        {
+          input: "2.0",
+          output: 2,
+        },
+        {
+          input: "2.3",
+          output: 2.3,
+        },
+        {
+          input: "2.30",
+          output: 2.3,
+        },
+        {
+          input: "2.300",
+          output: 2.3,
+        },
+        {
+          input: "2.3001",
+          output: 2.3001,
+        },
+        {
+          input: "2.03",
+          output: 2.03,
+        },
+        {
+          input: "2.003",
+          output: 2.003,
+        },
+        {
+          input: "2.00300",
+          output: 2.003,
+        },
+        {
+          input: "200300",
+          output: 200300,
+        },
+      ];
+
+      tests.forEach(test => {
+        it(`should work with an input value of ${test.input}`, () => {
+          Simulate.change($input, {
+            target: { value: test.input },
+          });
+
+          expect(comp.state.formData).eql(test.output);
+          expect($input.value).eql(test.input);
+        });
+      });
+    });
+
+    it("should update input values correctly when formData prop changes", () => {
+      const schema = {
+        type: "number",
+      };
+
+      const { comp, node } = createFormComponent({
+        schema,
+        formData: 2.03,
       });
 
-      expect(comp.state.formData).eql("2.");
+      const $input = node.querySelector("input");
+
+      expect($input.value).eql("2.03");
+
+      setProps(comp, {
+        schema,
+        formData: 203,
+      });
+
+      expect($input.value).eql("203");
     });
 
     it("should render the widget with the expected id", () => {
@@ -150,7 +222,7 @@ describe("NumberField", () => {
         },
       });
 
-      expect(node.querySelector("input[type=text]").id).eql("root");
+      expect(node.querySelector("input[type=number]").id).eql("root");
     });
 
     it("should render with trailing zeroes", () => {
@@ -181,6 +253,19 @@ describe("NumberField", () => {
       expect(node.querySelector(".field input").value).eql("2.000");
     });
 
+    it("should allow a zero to be input", () => {
+      const { node } = createFormComponent({
+        schema: {
+          type: "number",
+        },
+      });
+
+      Simulate.change(node.querySelector("input"), {
+        target: { value: "0" },
+      });
+      expect(node.querySelector(".field input").value).eql("0");
+    });
+
     it("should render customized StringField", () => {
       const CustomStringField = () => <div id="custom" />;
 
@@ -195,6 +280,17 @@ describe("NumberField", () => {
 
       expect(node.querySelector("#custom")).to.exist;
     });
+
+    it("should use step to represent the multipleOf keyword", () => {
+      const { node } = createFormComponent({
+        schema: {
+          type: "number",
+          multipleOf: 5,
+        },
+      });
+
+      expect(node.querySelector("input").step).to.eql("5");
+    });
   });
 
   describe("SelectWidget", () => {
diff --git a/test/uiSchema_test.js b/test/uiSchema_test.js
index 5db7431318..a964cdada5 100644
--- a/test/uiSchema_test.js
+++ b/test/uiSchema_test.js
@@ -1933,7 +1933,7 @@ describe("uiSchema", () => {
 
       it("should disable a number text widget", () => {
         shouldBeDisabled(
-          "input[type=text]",
+          "input[type=number]",
           {
             type: "number",
           },
@@ -2226,7 +2226,7 @@ describe("uiSchema", () => {
 
       it("should mark as readonly a number text widget", () => {
         shouldBeReadonly(
-          "input[type=text]",
+          "input[type=number]",
           {
             type: "number",
           },