From 0df170965fe6a207b09014dec777a57f777b0392 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 2 May 2017 08:21:23 -0400 Subject: [PATCH 1/3] 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: https://github.com/facebook/react/issues/9561#issuecomment-298394312 --- .../fixtures/text-inputs/InputTestCase.js | 62 +++++++++++ .../components/fixtures/text-inputs/index.js | 104 +++++++++--------- scripts/fiber/tests-passing.txt | 4 + .../dom/fiber/wrappers/ReactDOMFiberInput.js | 2 +- .../wrappers/__tests__/ReactDOMInput-test.js | 60 +++++++++- .../stack/client/wrappers/ReactDOMInput.js | 2 +- 6 files changed, 178 insertions(+), 56 deletions(-) create mode 100644 fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js diff --git a/fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js b/fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js new file mode 100644 index 0000000000000..44e5f0eed27a7 --- /dev/null +++ b/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 ( + +
{children}
+ +
+
+ Controlled {type} + +

+ Value: {JSON.stringify(this.state.value)} +

+
+ +
+ Uncontrolled {type} + +
+
+ + ); + } +} + +export default InputTestCase; diff --git a/fixtures/dom/src/components/fixtures/text-inputs/index.js b/fixtures/dom/src/components/fixtures/text-inputs/index.js index a1683672ce66b..bc40fa67d9182 100644 --- a/fixtures/dom/src/components/fixtures/text-inputs/index.js +++ b/fixtures/dom/src/components/fixtures/text-inputs/index.js @@ -1,62 +1,64 @@ const React = window.React; -class TextInputFixtures extends React.Component { - state = { - color: '#ffaaee', - }; +import Fixture from '../../Fixture'; +import FixtureSet from '../../FixtureSet'; +import TestCase from '../../TestCase'; +import InputTestCase from './InputTestCase'; - renderControlled = (type) => { - let id = `controlled_${type}`; +class TextInputFixtures extends React.Component { + render() { + return ( + + + +
  • Move the cursor to after "2" in the text field
    • +
  • Type ".2" into the text input
    • +     - let onChange = e => { - let value = e.target.value; - if (type === 'number') { - value = value === '' ? '' : parseFloat(value, 10) || 0; - } - this.setState({ - [type] : value, - }); - }; + + The text field's value should not update. + - let state = this.state[type] || ''; + +
    +
    + Value as number + {}} /> +
    - return ( -
    - - -   → {JSON.stringify(state)} -
    - ); - } +
    + Value as string + {}} /> +
    +
    +     - renderUncontrolled = (type) => { - let id = `uncontrolled_${type}`; - return ( -
    - - -
    - ); - } +

    + 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). +

    +     - 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 ( -
    event.preventDefault()}> -
    - Controlled - {types.map(this.renderControlled)} -
    -
    - Uncontrolled - {types.map(this.renderUncontrolled)} -
    -
    + + + + + + + + + + + + + + + +     ); } } diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 02bc0a8cb8573..01cba63cf1284 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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` @@ -1671,6 +1674,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * can deprioritize a tree from without dropping work * can resume work in a subtree even when a parent bails out * can resume work in a bailed subtree within one pass +* can resume mounting a class component * can reuse work done after being preempted * can reuse work that began but did not complete, after being preempted * can reuse work if shouldComponentUpdate is false, after being preempted diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index ebc9812077ea2..e93b057154cde 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js @@ -210,7 +210,7 @@ var ReactDOMInput = { node.value = '' + value; } // eslint-disable-next-line - } else if (value != node.value) { + } else { // 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; diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index 16b2fba74814a..87a4d6b000e16 100644 --- a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +++ b/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 = ; + 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 = ; + 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 ; + } + } + + var stub = ReactTestUtils.renderIntoDocument(); + 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 = ; stub = ReactTestUtils.renderIntoDocument(stub); @@ -411,10 +465,10 @@ describe('ReactDOMInput', () => { }); ReactDOM.render(, container); - expect(nodeValueSetter.mock.calls.length).toBe(0); + expect(nodeValueSetter.mock.calls.length).toBe(1); ReactDOM.render(, container); - expect(nodeValueSetter.mock.calls.length).toBe(1); + expect(nodeValueSetter.mock.calls.length).toBe(2); }); it('should properly control a value of number `0`', () => { @@ -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', () => { diff --git a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js index 15c98554b4578..1fcea21865a16 100644 --- a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js @@ -202,7 +202,7 @@ var ReactDOMInput = { node.value = '' + value; } // eslint-disable-next-line - } else if (value != node.value) { + } else { // 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; From 3e1a7681273ef00e2e2209ba0d85e93fd28c5275 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 2 May 2017 19:59:15 -0400 Subject: [PATCH 2/3] 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. --- .../components/fixtures/text-inputs/index.js | 28 +++++++++++++++++++ .../dom/fiber/wrappers/ReactDOMFiberInput.js | 3 +- .../stack/client/wrappers/ReactDOMInput.js | 3 +- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/text-inputs/index.js b/fixtures/dom/src/components/fixtures/text-inputs/index.js index bc40fa67d9182..6d6ee9a687db6 100644 --- a/fixtures/dom/src/components/fixtures/text-inputs/index.js +++ b/fixtures/dom/src/components/fixtures/text-inputs/index.js @@ -43,6 +43,34 @@ class TextInputFixtures extends React.Component {

    + + +
  • Type "user@example.com"
    • +
  • Select "@"
    • +
  • Type ".", to replace "@" with a period
    • +     + + + The text field's cursor should not jump to the end. + + + +     + + + +
  • Type "http://www.example.com"
    • +
  • Select "www."
    • +
  • Press backspace/delete
    • +     + + + The text field's cursor should not jump to the end. + + + +     + diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index e93b057154cde..41de9687e429d 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/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 { + } 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; diff --git a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js index 1fcea21865a16..b05ca82b352db 100644 --- a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js +++ b/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 { + } 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; From d421181b86cab854b27fe182c15c6cad528a67c4 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 2 May 2017 20:09:42 -0400 Subject: [PATCH 3/3] Add copy command after build for interup with surge.sh --- fixtures/dom/package.json | 2 +- .../dom/shared/wrappers/__tests__/ReactDOMInput-test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fixtures/dom/package.json b/fixtures/dom/package.json index 214de66fb73a6..d27936a4da275 100644 --- a/fixtures/dom/package.json +++ b/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" } diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index 87a4d6b000e16..19a38b9e182f5 100644 --- a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js @@ -465,10 +465,10 @@ describe('ReactDOMInput', () => { }); ReactDOM.render(, container); - expect(nodeValueSetter.mock.calls.length).toBe(1); + expect(nodeValueSetter.mock.calls.length).toBe(0); ReactDOM.render(, container); - expect(nodeValueSetter.mock.calls.length).toBe(2); + expect(nodeValueSetter.mock.calls.length).toBe(1); }); it('should properly control a value of number `0`', () => {