From fe4a5b400477a367be4c1ff4e3546304d350cbd4 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 27 Mar 2017 12:39:18 -0400 Subject: [PATCH] Fix Chrome number input backspace and invalid input issue (#7359) * Only re-assign defaultValue if it is different * Do not set value if it is the same * Properly cover defaultValue * Use coercion to be smart about value assignment * Add explanation of loose type checks in value assignment. * Add test coverage for setAttribute update. * Only apply loose value check to text inputs * Fix case where empty switches to zero * Handle zero case in controlled input * Correct mistake with default value assignment after rebase * Do not assign bad input to number input * Only trigger number input value attribute updates on blur * Remove reference to LinkedValueUtils * Record new fiber tests * Add tests for blurred number input behavior * Replace onBlur wrapper with rule in ChangeEventPlugin * Sift down to only number inputs * Re-record fiber tests * Add test case for updating attribute on uncontrolled inputs. Make related correction * Handle uncontrolled inputs, integrate fiber * Reorder boolean to mitigate DOM checks * Only assign value if it is different * Add number input browser test fixtures During the course of the number input fix, we uncovered many edge cases. This commit adds browser test fixtures for each of those instances. * Address edge case preventing number precision lower than 1 place 0.0 coerces to 0, however they are not the same value when doing string comparision. This prevented controlled number inputs from inputing the characters `0.00`. Also adds test cases. * Accommodate lack of IE9 number input support IE9 does not support number inputs. Number inputs in IE9 fallback to traditional text inputs. This means that accessing `input.value` will report the raw text, rather than parsing a numeric value. This commit makes the ReactDOMInput wrapper check to see if the `type` prop has been configured to `"number"`. In those cases, it will perform a comparison based upon `parseFloat` instead of the raw input value. * Remove footnotes about IE exponent issues With the recent IE9 fix, IE properly inserts `e` when it produces an invalid number. * Address exception in IE9/10 ChangeEventPlugin blur event On blur, inputs have their values assigned. This is so that number inputs do not conduct unexpected behavior in Chrome/Safari. Unfortunately, there are cases where the target instance might be undefined in IE9/10, raising an exception. * Migrate over ReactDOMInput.js number input fixes to Fiber Also re-record tests * Update number fixtures to use latest components * Add number input test case for dashes and negative numbers * Replace trailing dash test case with replace with dash Also run prettier --- fixtures/dom/src/components/Header.js | 70 ++++++++ fixtures/dom/src/components/fixtures/index.js | 34 ++++ .../fixtures/number-inputs/NumberTestCase.js | 37 ++++ .../fixtures/number-inputs/index.js | 167 ++++++++++++++++++ .../client/eventPlugins/ChangeEventPlugin.js | 25 +++ .../dom/client/wrappers/ReactDOMInput.js | 29 +-- .../wrappers/__tests__/ReactDOMInput-test.js | 126 +++++++++++++ .../dom/shared/HTMLDOMPropertyConfig.js | 29 ++- .../__tests__/DOMPropertyOperations-test.js | 29 +++ 9 files changed, 534 insertions(+), 12 deletions(-) create mode 100644 fixtures/dom/src/components/Header.js create mode 100644 fixtures/dom/src/components/fixtures/index.js create mode 100644 fixtures/dom/src/components/fixtures/number-inputs/NumberTestCase.js create mode 100644 fixtures/dom/src/components/fixtures/number-inputs/index.js diff --git a/fixtures/dom/src/components/Header.js b/fixtures/dom/src/components/Header.js new file mode 100644 index 0000000000..5f717d8be1 --- /dev/null +++ b/fixtures/dom/src/components/Header.js @@ -0,0 +1,70 @@ +import { parse, stringify } from 'query-string'; +import getVersionTags from '../tags'; +const React = window.React; + +const Header = React.createClass({ + getInitialState() { + const query = parse(window.location.search); + const version = query.version || 'local'; + const versions = [version]; + return { version, versions }; + }, + componentWillMount() { + getVersionTags() + .then(tags => { + let versions = tags.map(tag => tag.name.slice(1)); + versions = [`local`, ...versions]; + this.setState({ versions }); + }) + }, + handleVersionChange(event) { + const query = parse(window.location.search); + query.version = event.target.value; + if (query.version === 'local') { + delete query.version; + } + window.location.search = stringify(query); + }, + handleFixtureChange(event) { + window.location.pathname = event.target.value; + }, + render() { + return ( +
+
+ + + React Sandbox (v{React.version}) + + +
+ + +
+
+
+ ); + }, +}); + +export default Header; diff --git a/fixtures/dom/src/components/fixtures/index.js b/fixtures/dom/src/components/fixtures/index.js new file mode 100644 index 0000000000..516e8ad3ae --- /dev/null +++ b/fixtures/dom/src/components/fixtures/index.js @@ -0,0 +1,34 @@ +const React = window.React; +import RangeInputFixtures from './range-inputs'; +import TextInputFixtures from './text-inputs'; +import SelectFixtures from './selects'; +import TextAreaFixtures from './textareas'; +import InputChangeEvents from './input-change-events'; +import NumberInputFixtures from './number-inputs/'; + +/** + * A simple routing component that renders the appropriate + * fixture based on the location pathname. + */ +const FixturesPage = React.createClass({ + render() { + switch (window.location.pathname) { + case '/text-inputs': + return ; + case '/range-inputs': + return ; + case '/selects': + return ; + case '/textareas': + return ; + case '/input-change-events': + return ; + case '/number-inputs': + return ; + default: + return

Please select a test fixture.

; + } + }, +}); + +module.exports = FixturesPage; diff --git a/fixtures/dom/src/components/fixtures/number-inputs/NumberTestCase.js b/fixtures/dom/src/components/fixtures/number-inputs/NumberTestCase.js new file mode 100644 index 0000000000..2c072d478e --- /dev/null +++ b/fixtures/dom/src/components/fixtures/number-inputs/NumberTestCase.js @@ -0,0 +1,37 @@ +const React = window.React; + +import Fixture from '../../Fixture'; + +const NumberTestCase = React.createClass({ + getInitialState() { + return { value: '' }; + }, + onChange(event) { + const parsed = parseFloat(event.target.value, 10) + const value = isNaN(parsed) ? '' : parsed + + this.setState({ value }) + }, + render() { + return ( + +
{this.props.children}
+ +
+
+ Controlled + + Value: {JSON.stringify(this.state.value)} +
+ +
+ Uncontrolled + +
+
+ + ); + }, +}); + +export default NumberTestCase; diff --git a/fixtures/dom/src/components/fixtures/number-inputs/index.js b/fixtures/dom/src/components/fixtures/number-inputs/index.js new file mode 100644 index 0000000000..2c88c333ed --- /dev/null +++ b/fixtures/dom/src/components/fixtures/number-inputs/index.js @@ -0,0 +1,167 @@ +const React = window.React; + +import FixtureSet from '../../FixtureSet'; +import TestCase from '../../TestCase'; +import NumberTestCase from './NumberTestCase'; + +const NumberInputs = React.createClass({ + render() { + return ( + + + +
  • Type "3.1"
    • +
  • Press backspace, eliminating the "1"
    • +     + + + The field should read "3.", preserving the decimal place + + + + +

    + Notes: Chrome and Safari clear trailing + decimals on blur. React makes this concession so that the + value attribute remains in sync with the value property. +

    +     + + + +
  • Type "0.01"
    • +     + + + The field should read "0.01" + + + +     + + + +
  • Type "2e"
    • +
  • Type 4, to read "2e4"
    • +     + + + The field should read "2e4". The parsed value should read "20000" + + + +     + + + +
  • Type "3.14"
    • +
  • Press "e", so that the input reads "3.14e"
    • +     + + + The field should read "3.14e", the parsed value should be empty + + + +     + + + +
  • Type "3.14"
    • +
  • Move the text cursor to after the decimal place
    • +
  • Press "e" twice, so that the value reads "3.ee14"
    • +     + + + The field should read "3.ee14" + + + +     + + + +
  • Type "3.0"
    • +     + + + The field should read "3.0" + + + +     + + + +
  • Type "300"
    • +
  • Move the cursor to after the "3"
    • +
  • Type "."
    • +     + + + The field should read "3.00", not "3" + + +     + + + +
  • Type "3"
    • +
  • Select the entire value"
    • +
  • Type '-' to replace '3' with '-'
    • +     + + + The field should read "-", not be blank. + + +     + + + +
  • Type "-"
    • +
  • Type '3'
    • +     + + + The field should read "-3". + + +     +     + ); + }, +}); + +export default NumberInputs; diff --git a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js index 484bb1a40d..e4a1f9b910 100644 --- a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js @@ -317,6 +317,26 @@ function getTargetInstForClickEvent( } } +function handleControlledInputBlur(inst, node) { + // TODO: In IE, inst is occasionally null. Why? + if (inst == null) { + return; + } + + // Fiber and ReactDOM keep wrapper state in separate places + let state = inst._wrapperState || node._wrapperState; + + if (!state || !state.controlled || node.type !== 'number') { + return; + } + + // If controlled, assign the value attribute to the current value on blur + let value = '' + node.value; + if (node.getAttribute('value') !== value) { + node.setAttribute('value', value); + } +} + /** * This plugin creates an `onChange` event that normalizes change events * across form elements. This event fires at a time when it's possible to @@ -380,6 +400,11 @@ var ChangeEventPlugin = { targetInst ); } + + // When blurring, set the value attribute for number inputs + if (topLevelType === 'topBlur') { + handleControlledInputBlur(targetInst, targetNode); + } }, }; diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js index 8d1ad6ec55..07061ef32c 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/client/wrappers/ReactDOMInput.js @@ -149,11 +149,8 @@ var ReactDOMInput = { initialValue: props.value != null ? props.value : defaultValue, listeners: null, onChange: _handleChange.bind(inst), + controlled: isControlled(props), }; - - if (__DEV__) { - inst._wrapperState.controlled = isControlled(props); - } }, updateWrapper: function(inst) { @@ -202,14 +199,24 @@ var ReactDOMInput = { var node = ReactDOMComponentTree.getNodeFromInstance(inst); var value = LinkedValueUtils.getValue(props); if (value != null) { + if (value === 0 && node.value === '') { + node.value = '0'; + // Note: IE9 reports a number inputs as 'text', so check props instead. + } else if (props.type === 'number') { + // Simulate `input.valueAsNumber`. IE9 does not support it + var valueAsNumber = parseFloat(node.value, 10) || 0; - // Cast `value` to a string to ensure the value is set correctly. While - // browsers typically do this as necessary, jsdom doesn't. - var newValue = '' + value; - - // To avoid side effects (such as losing text selection), only set value if changed - if (newValue !== node.value) { - node.value = newValue; + // eslint-disable-next-line + if (value != valueAsNumber) { + // 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; + } + // eslint-disable-next-line + } else if (value != node.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; } } else { if (props.value == null && props.defaultValue != null) { diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js index ea2d6074d0..8ee2478bbd 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js @@ -268,6 +268,48 @@ describe('ReactDOMInput', () => { expect(node.value).toBe('0'); }); + it('should properly control 0.0 for a text input', () => { + var stub = ; + stub = ReactTestUtils.renderIntoDocument(stub); + var node = ReactDOM.findDOMNode(stub); + + node.value = '0.0'; + ReactTestUtils.Simulate.change(node, {target: {value: '0.0'}}); + expect(node.value).toBe('0.0'); + }); + + it('should properly control 0.0 for a number input', () => { + var stub = ; + stub = ReactTestUtils.renderIntoDocument(stub); + var node = ReactDOM.findDOMNode(stub); + + node.value = '0.0'; + ReactTestUtils.Simulate.change(node, {target: {value: '0.0'}}); + expect(node.value).toBe('0.0'); + }); + + it('should properly transition from an empty value to 0', function() { + var container = document.createElement('div'); + + ReactDOM.render(, container); + ReactDOM.render(, container); + + var node = container.firstChild; + + expect(node.value).toBe('0'); + }); + + it('should properly transition from 0 to an empty value', function() { + var container = document.createElement('div'); + + ReactDOM.render(, container); + ReactDOM.render(, container); + + var node = container.firstChild; + + expect(node.value).toBe(''); + }); + it('should have the correct target value', () => { var handled = false; var handler = function(event) { @@ -937,4 +979,88 @@ describe('ReactDOMInput', () => { 'node.setAttribute("checked", "")', ]); }); + + describe('assigning the value attribute on controlled inputs', function() { + function getTestInput() { + return React.createClass({ + getInitialState: function() { + return { + value: this.props.value == null ? '' : this.props.value, + }; + }, + onChange: function(event) { + this.setState({value: event.target.value}); + }, + render: function() { + var type = this.props.type; + var value = this.state.value; + + return ; + }, + }); + } + + it('always sets the attribute when values change on text inputs', function() { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument(); + var node = ReactDOM.findDOMNode(stub); + + ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); + + expect(node.getAttribute('value')).toBe('2'); + }); + + it('does not set the value attribute on number inputs if focused', () => { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + node.focus(); + + ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); + + expect(node.getAttribute('value')).toBe('1'); + }); + + it('sets the value attribute on number inputs on blur', () => { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); + ReactTestUtils.SimulateNative.blur(node); + + expect(node.getAttribute('value')).toBe('2'); + }); + + it('an uncontrolled number input will not update the value attribute on blur', () => { + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + node.value = 4; + + ReactTestUtils.SimulateNative.blur(node); + + expect(node.getAttribute('value')).toBe('1'); + }); + + it('an uncontrolled text input will not update the value attribute on blur', () => { + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + node.value = 4; + + ReactTestUtils.SimulateNative.blur(node); + + expect(node.getAttribute('value')).toBe('1'); + }); + }); }); diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 31b2b9cd4a..ef0eda5e25 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -210,7 +210,34 @@ var HTMLDOMPropertyConfig = { htmlFor: 'for', httpEquiv: 'http-equiv', }, - DOMPropertyNames: { + DOMPropertyNames: {}, + DOMMutationMethods: { + value: function(node, value) { + if (value == null) { + return node.removeAttribute('value'); + } + + // Number inputs get special treatment due to some edge cases in + // Chrome. Let everything else assign the value attribute as normal. + // https://github.com/facebook/react/issues/7253#issuecomment-236074326 + if (node.type !== 'number' || node.hasAttribute('value') === false) { + node.setAttribute('value', '' + value); + } else if ( + node.validity && + !node.validity.badInput && + node.ownerDocument.activeElement !== node + ) { + // Don't assign an attribute if validation reports bad + // input. Chrome will clear the value. Additionally, don't + // operate on inputs that have focus, otherwise Chrome might + // strip off trailing decimal places and cause the user's + // cursor position to jump to the beginning of the input. + // + // In ReactDOMInput, we have an onBlur event that will trigger + // this function again when focus is lost. + node.setAttribute('value', '' + value); + } + }, }, }; diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 972ef0b71b..d5e755fbdb 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -348,6 +348,35 @@ describe('DOMPropertyOperations', () => { }); + describe('value mutation method', function() { + it('should update an empty attribute to zero', function() { + var stubNode = document.createElement('input'); + var stubInstance = {_debugID: 1}; + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); + + stubNode.setAttribute('type', 'radio'); + + DOMPropertyOperations.setValueForProperty(stubNode, 'value', ''); + spyOn(stubNode, 'setAttribute'); + DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0); + + expect(stubNode.setAttribute.calls.count()).toBe(1); + }); + + it('should always assign the value attribute for non-inputs', function() { + var stubNode = document.createElement('progress'); + var stubInstance = {_debugID: 1}; + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); + + spyOn(stubNode, 'setAttribute'); + + DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30); + DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30'); + + expect(stubNode.setAttribute.calls.count()).toBe(2); + }); + }); + describe('deleteValueForProperty', () => { var stubNode; var stubInstance;