Remove loose check on non-number controlled inputs. Fix trailing dot issue. (#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh

Author: nhunzaker

fixtures/dom/package.json

@@ -16,7 +16,7 @@
   "scripts": {
     "start": "react-scripts start",
     "prestart": "cp ../../build/dist/{react,react-dom}.development.js public/",
-    "build": "react-scripts build",
+    "build": "react-scripts build && cp build/index.html build/200.html",
     "test": "react-scripts test --env=jsdom",
     "eject": "react-scripts eject"
   }

fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js

@@ -0,0 +1,62 @@
+const React = window.React;
+
+import Fixture from '../../Fixture';
+
+class InputTestCase extends React.Component {
+  static defaultProps = {
+    type: 'text',
+    defaultValue: '',
+    parseAs: 'text'
+  }
+
+  constructor () {
+    super(...arguments);
+
+    this.state = {
+      value: this.props.defaultValue
+    };
+  }
+
+  onChange = (event) => {
+    const raw = event.target.value;
+
+    switch (this.props.type) {
+      case 'number':
+        const parsed = parseFloat(event.target.value, 10);
+
+        this.setState({ value: isNaN(parsed) ? '' : parsed });
+
+        break;
+      default:
+        this.setState({ value: raw });
+    }
+  }
+
+  render() {
+    const { children, type, defaultValue } = this.props;
+    const { value } = this.state;
+
+    return (
+      <Fixture>
+        <div>{children}</div>
+
+        <div className="control-box">
+          <fieldset>
+            <legend>Controlled {type}</legend>
+            <input type={type} value={value} onChange={this.onChange} />
+            <p className="hint">
+              Value: {JSON.stringify(this.state.value)}
+            </p>
+          </fieldset>
+
+          <fieldset>
+            <legend>Uncontrolled {type}</legend>
+            <input type={type} defaultValue={defaultValue} />
+          </fieldset>
+        </div>
+      </Fixture>
+    );
+  }
+}
+
+export default InputTestCase;

fixtures/dom/src/components/fixtures/text-inputs/index.js

@@ -1,62 +1,92 @@
 const React = window.React;
 
+import Fixture from '../../Fixture';
+import FixtureSet from '../../FixtureSet';
+import TestCase from '../../TestCase';
+import InputTestCase from './InputTestCase';
+
 class TextInputFixtures extends React.Component {
-  state = {
-    color: '#ffaaee',
-  };
+  render() {
+    return (
+      <FixtureSet
+        title="Inputs"
+        description="Common behavior across controled and uncontrolled inputs"
+      >
+        <TestCase title="Numbers in a controlled text field with no handler">
+          <TestCase.Steps>
+            <li>Move the cursor to after "2" in the text field</li>
+            <li>Type ".2" into the text input</li>
+          </TestCase.Steps>
 
-  renderControlled = (type) => {
-    let id = `controlled_${type}`;
+          <TestCase.ExpectedResult>
+            The text field's value should not update.
+          </TestCase.ExpectedResult>
 
-    let onChange = e => {
-      let value = e.target.value;
-      if (type === 'number') {
-        value = value === '' ? '' : parseFloat(value, 10) || 0;
-      }
-      this.setState({
-        [type] : value,
-      });
-    };
+          <Fixture>
+            <div className="control-box">
+              <fieldset>
+                <legend>Value as number</legend>
+                <input value={2} onChange={() => {}} />
+              </fieldset>
 
-    let state = this.state[type] || '';
+              <fieldset>
+                <legend>Value as string</legend>
+                <input value={"2"} onChange={() => {}} />
+              </fieldset>
+            </div>
+          </Fixture>
 
-    return (
-      <div key={type} className="field">
-        <label className="control-label" htmlFor={id}>{type}</label>
-        <input id={id} type={type} value={state} onChange={onChange} />
-        &nbsp; &rarr; {JSON.stringify(state)}
-      </div>
-    );
-  }
+          <p className="footnote">
+            This issue was first introduced when we added extra logic
+            to number inputs to prevent unexpected behavior in Chrome
+            and Safari (see the number input test case).
+          </p>
+        </TestCase>
 
-  renderUncontrolled = (type) => {
-    let id = `uncontrolled_${type}`;
-    return (
-      <div key={type} className="field">
-        <label className="control-label" htmlFor={id}>{type}</label>
-        <input id={id} type={type} />
-      </div>
-    );
-  }
+        <TestCase title="Cursor when editing email inputs">
+          <TestCase.Steps>
+            <li>Type "[email protected]"</li>
+            <li>Select "@"</li>
+            <li>Type ".", to replace "@" with a period</li>
+          </TestCase.Steps>
 
-  render() {
-    // https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input
-    let types = [
-      'text', 'email', 'number', 'url', 'tel',
-      'color', 'date', 'datetime-local',
-      'time', 'month', 'week', 'range', 'password',
-    ];
-    return (
-      <form onSubmit={event => event.preventDefault()}>
-        <fieldset>
-          <legend>Controlled</legend>
-          {types.map(this.renderControlled)}
-        </fieldset>
-        <fieldset>
-          <legend>Uncontrolled</legend>
-          {types.map(this.renderUncontrolled)}
-        </fieldset>
-      </form>
+          <TestCase.ExpectedResult>
+            The text field's cursor should not jump to the end.
+          </TestCase.ExpectedResult>
+
+          <InputTestCase type="email" defaultValue="" />
+        </TestCase>
+
+        <TestCase title="Cursor when editing url inputs">
+          <TestCase.Steps>
+            <li>Type "http://www.example.com"</li>
+            <li>Select "www."</li>
+            <li>Press backspace/delete</li>
+          </TestCase.Steps>
+
+          <TestCase.ExpectedResult>
+            The text field's cursor should not jump to the end.
+          </TestCase.ExpectedResult>
+
+          <InputTestCase type="url" defaultValue="" />
+        </TestCase>
+
+        <TestCase title="All inputs" description="General test of all inputs">
+          <InputTestCase type="text" defaultValue="Text" />
+          <InputTestCase type="email" defaultValue="[email protected]"/>
+          <InputTestCase type="number" defaultValue={0} />
+          <InputTestCase type="url" defaultValue="http://example.com"/>
+          <InputTestCase type="tel" defaultValue="555-555-5555"/>
+          <InputTestCase type="color" defaultValue="#ff0000" />
+          <InputTestCase type="date" defaultValue="2017-01-01" />
+          <InputTestCase type="datetime-local" defaultValue="2017-01-01T01:00" />
+          <InputTestCase type="time" defaultValue="01:00" />
+          <InputTestCase type="month" defaultValue="2017-01" />
+          <InputTestCase type="week" defaultValue="2017-W01" />
+          <InputTestCase type="range" defaultValue={0.5} />
+          <InputTestCase type="password" defaultValue="" />
+        </TestCase>
+      </FixtureSet>
     );
   }
 }

scripts/fiber/tests-passing.txt

@@ -1452,6 +1452,9 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
 * should properly control a value even if no event listener exists
 * should control a value in reentrant events
 * should control values in reentrant events with different targets
+* does change the number 2 to "2.0" with no change handler
+* does change the string "2" to "2.0" with no change handler
+* changes the number 2 to "2.0" using a change handler
 * should display `defaultValue` of number 0
 * only assigns defaultValue if it changes
 * should display "true" for `defaultValue` of `true`

src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js

@@ -209,8 +209,7 @@ var ReactDOMInput = {
           // browsers typically do this as necessary, jsdom doesn't.
           node.value = '' + value;
         }
-        // eslint-disable-next-line
-      } else if (value != node.value) {
+      } else if (node.value !== value) {
         // Cast `value` to a string to ensure the value is set correctly. While
         // browsers typically do this as necessary, jsdom doesn't.
         node.value = '' + value;

src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js

@@ -179,6 +179,60 @@ describe('ReactDOMInput', () => {
     document.body.removeChild(container);
   });
 
+  describe('switching text inputs between numeric and string numbers', () => {
+    it('does change the number 2 to "2.0" with no change handler', () => {
+      var stub = <input type="text" value={2} onChange={jest.fn()} />;
+      stub = ReactTestUtils.renderIntoDocument(stub);
+      var node = ReactDOM.findDOMNode(stub);
+
+      node.value = '2.0';
+
+      ReactTestUtils.Simulate.change(stub);
+
+      expect(node.getAttribute('value')).toBe('2');
+      expect(node.value).toBe('2');
+    });
+
+    it('does change the string "2" to "2.0" with no change handler', () => {
+      var stub = <input type="text" value={'2'} onChange={jest.fn()} />;
+      stub = ReactTestUtils.renderIntoDocument(stub);
+      var node = ReactDOM.findDOMNode(stub);
+
+      node.value = '2.0';
+
+      ReactTestUtils.Simulate.change(stub);
+
+      expect(node.getAttribute('value')).toBe('2');
+      expect(node.value).toBe('2');
+    });
+
+    it('changes the number 2 to "2.0" using a change handler', () => {
+      class Stub extends React.Component {
+        state = {
+          value: 2,
+        };
+        onChange = event => {
+          this.setState({value: event.target.value});
+        };
+        render() {
+          const {value} = this.state;
+
+          return <input type="text" value={value} onChange={this.onChange} />;
+        }
+      }
+
+      var stub = ReactTestUtils.renderIntoDocument(<Stub />);
+      var node = ReactDOM.findDOMNode(stub);
+
+      node.value = '2.0';
+
+      ReactTestUtils.Simulate.change(node);
+
+      expect(node.getAttribute('value')).toBe('2.0');
+      expect(node.value).toBe('2.0');
+    });
+  });
+
   it('should display `defaultValue` of number 0', () => {
     var stub = <input type="text" defaultValue={0} />;
     stub = ReactTestUtils.renderIntoDocument(stub);
@@ -434,7 +488,7 @@ describe('ReactDOMInput', () => {
 
     node.value = '0.0';
     ReactTestUtils.Simulate.change(node, {target: {value: '0.0'}});
-    expect(node.value).toBe('0.0');
+    expect(node.value).toBe('0');
   });
 
   it('should properly control 0.0 for a number input', () => {

src/renderers/dom/stack/client/wrappers/ReactDOMInput.js

@@ -201,8 +201,7 @@ var ReactDOMInput = {
           // browsers typically do this as necessary, jsdom doesn't.
           node.value = '' + value;
         }
-        // eslint-disable-next-line
-      } else if (value != node.value) {
+      } else if (node.value !== value) {
         // Cast `value` to a string to ensure the value is set correctly. While
         // browsers typically do this as necessary, jsdom doesn't.
         node.value = '' + value;