From 6875fa867f55a730d662c8ac2e6f9e9a3e754901 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Wed, 3 May 2017 14:37:56 -0400 Subject: [PATCH] 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: https://github.com/facebook/react/issues/9561#issuecomment-298394312 * 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 --- fixtures/dom/package.json | 2 +- .../fixtures/text-inputs/InputTestCase.js | 62 +++++++++ .../components/fixtures/text-inputs/index.js | 130 +++++++++++------- scripts/fiber/tests-passing.txt | 3 + .../dom/fiber/wrappers/ReactDOMFiberInput.js | 3 +- .../wrappers/__tests__/ReactDOMInput-test.js | 56 +++++++- .../stack/client/wrappers/ReactDOMInput.js | 3 +- 7 files changed, 203 insertions(+), 56 deletions(-) create mode 100644 fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js diff --git a/fixtures/dom/package.json b/fixtures/dom/package.json index 214de66fb73a..d27936a4da27 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/fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js b/fixtures/dom/src/components/fixtures/text-inputs/InputTestCase.js new file mode 100644 index 000000000000..44e5f0eed27a --- /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 a1683672ce66..6d6ee9a687db 100644 --- a/fixtures/dom/src/components/fixtures/text-inputs/index.js +++ b/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 ( + + + +
  • Move the cursor to after "2" in the text field
    • +
  • Type ".2" into the text input
    • +     - renderControlled = (type) => { - let id = `controlled_${type}`; + + The text field's value should not update. + - let onChange = e => { - let value = e.target.value; - if (type === 'number') { - value = value === '' ? '' : parseFloat(value, 10) || 0; - } - this.setState({ - [type] : value, - }); - }; + +
    +
    + Value as number + {}} /> +
    - let state = this.state[type] || ''; +
    + Value as string + {}} /> +
    +
    +     - return ( -
    - - -   → {JSON.stringify(state)} -
    - ); - } +

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

    +     - renderUncontrolled = (type) => { - let id = `uncontrolled_${type}`; - return ( -
    - - -
    - ); - } + + +
  • Type "user@example.com"
    • +
  • Select "@"
    • +
  • Type ".", to replace "@" with a period
    • +     - 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)} -
    -
    + + 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/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index ea66c5aa3d90..01cba63cf128 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` diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index ebc9812077ea..41de9687e429 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 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; diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index 16b2fba74814..19a38b9e182f 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); @@ -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 15c98554b457..b05ca82b352d 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 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;