From 497f4960b5497551b9477d60f359c0b799da04f6 Mon Sep 17 00:00:00 2001 From: jquense Date: Thu, 17 Dec 2015 14:54:28 -0500 Subject: [PATCH 1/3] Only fire input value change events when the value changes fixes #554, fixes #1471, fixes #2185 (still trying to figure out why) --- .../client/eventPlugins/ChangeEventPlugin.js | 135 +++++++++++------- .../__tests__/ChangeEventPlugin-test.js | 88 ++++++++++++ 2 files changed, 170 insertions(+), 53 deletions(-) diff --git a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js index 1a6855d2860..52ee10bc7ca 100644 --- a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js @@ -26,6 +26,8 @@ var keyOf = require('keyOf'); var topLevelTypes = EventConstants.topLevelTypes; +var LAST_VALUE_KEY = '__reactLastInputValue'; + var eventTypes = { change: { phasedRegistrationNames: { @@ -45,6 +47,35 @@ var eventTypes = { }, }; +function getInstIfValueChanged(targetInst, target) { + var value = getInputValue(target); + var lastValue = target[LAST_VALUE_KEY]; + + if (!target.hasOwnProperty(LAST_VALUE_KEY) || value !== lastValue) { + target[LAST_VALUE_KEY] = value; + return targetInst; + } +} + +function getInputValue(elem) { + var value; + + if (elem) { + var type = elem.type; + var nodeName = elem.nodeName; + + if ( + (nodeName && nodeName.toLowerCase() === 'input') && + (type === 'checkbox' || type === 'radio') + ) { + value = elem.checked; + } else { + value = elem.value; + } + } + return value; +} + /** * For IE shims */ @@ -72,10 +103,10 @@ if (ExecutionEnvironment.canUseDOM) { ); } -function manualDispatchChangeEvent(nativeEvent) { +function manualDispatchChangeEvent(nativeEvent, targetInst) { var event = SyntheticEvent.getPooled( eventTypes.change, - activeElementInst, + targetInst, nativeEvent, getEventTarget(nativeEvent) ); @@ -146,15 +177,13 @@ var isInputEventSupported = false; if (ExecutionEnvironment.canUseDOM) { // IE9 claims to support the input event but fails to trigger it when // deleting text, so we ignore its input events. - // IE10+ fire input events to often, such when a placeholder - // changes or when an input with a placeholder is focused. isInputEventSupported = isEventSupported('input') && ( - !('documentMode' in document) || document.documentMode > 11 + !('documentMode' in document) || document.documentMode > 9 ); } /** - * (For IE <=11) Replacement getter/setter for the `value` property that gets + * (For IE <=9) Replacement getter/setter for the `value` property that gets * set on the active element. */ var newValueProp = { @@ -168,12 +197,14 @@ var newValueProp = { }, }; -/** - * (For IE <=11) Starts tracking propertychange events on the passed-in element - * and override the value property so that we can distinguish user events from - * value changes in JS. - */ -function startWatchingForValueChange(target, targetInst) { +function clearActiveElement() { + activeElement = null; + activeElementInst = null; + activeElementValue = null; + activeElementValueProp = null; +} + +function updateActiveElement(target, targetInst) { activeElement = target; activeElementInst = targetInst; activeElementValue = target.value; @@ -181,19 +212,24 @@ function startWatchingForValueChange(target, targetInst) { target.constructor.prototype, 'value' ); +} + +/** + * (For IE <=9) Starts tracking propertychange events on the passed-in element + * and override the value property so that we can distinguish user events from + * value changes in JS. + */ +function startWatchingForValueChange(target, targetInst) { + updateActiveElement(target, targetInst); // Not guarded in a canDefineProperty check: IE8 supports defineProperty only // on DOM elements Object.defineProperty(activeElement, 'value', newValueProp); - if (activeElement.attachEvent) { - activeElement.attachEvent('onpropertychange', handlePropertyChange); - } else { - activeElement.addEventListener('propertychange', handlePropertyChange, false); - } + activeElement.attachEvent('onpropertychange', handlePropertyChange); } /** - * (For IE <=11) Removes the event listeners from the currently-tracked element, + * (For IE <=9) Removes the event listeners from the currently-tracked element, * if any exists. */ function stopWatchingForValueChange() { @@ -203,21 +239,12 @@ function stopWatchingForValueChange() { // delete restores the original property definition delete activeElement.value; - - if (activeElement.detachEvent) { - activeElement.detachEvent('onpropertychange', handlePropertyChange); - } else { - activeElement.removeEventListener('propertychange', handlePropertyChange, false); - } - - activeElement = null; - activeElementInst = null; - activeElementValue = null; - activeElementValueProp = null; + activeElement.detachEvent('onpropertychange', handlePropertyChange); + clearActiveElement(); } /** - * (For IE <=11) Handles a propertychange event, sending a `change` event if + * (For IE <=9) Handles a propertychange event, sending a `change` event if * the value of the active element has changed. */ function handlePropertyChange(nativeEvent) { @@ -233,21 +260,7 @@ function handlePropertyChange(nativeEvent) { manualDispatchChangeEvent(nativeEvent); } -/** - * If a `change` event should be fired, returns the target's ID. - */ -function getTargetInstForInputEvent( - topLevelType, - targetInst -) { - if (topLevelType === topLevelTypes.topInput) { - // In modern browsers (i.e., not IE8 or IE9), the input event is exactly - // what we want so fall through here and trigger an abstract event - return targetInst; - } -} - -function handleEventsForInputEventIE( +function handleEventsForInputEventPolyfill( topLevelType, target, targetInst @@ -256,7 +269,7 @@ function handleEventsForInputEventIE( // In IE8, we can capture almost all .value changes by adding a // propertychange handler and looking for events with propertyName // equal to 'value' - // In IE9-11, propertychange fires for most input events but is buggy and + // In IE9, propertychange fires for most input events but is buggy and // doesn't fire when text is deleted, but conveniently, selectionchange // appears to fire in all of the remaining cases so we catch those and // forward the event if the value has changed @@ -274,7 +287,7 @@ function handleEventsForInputEventIE( } // For IE8 and IE9. -function getTargetInstForInputEventIE( +function getTargetInstForInputEventPolyfill( topLevelType, targetInst ) { @@ -314,10 +327,24 @@ function shouldUseClickEvent(elem) { function getTargetInstForClickEvent( topLevelType, - targetInst + targetInst, + target ) { if (topLevelType === topLevelTypes.topClick) { - return targetInst; + return getInstIfValueChanged(targetInst, target); + } +} + +function getTargetInstForInputOrChangeEvent( + topLevelType, + targetInst, + target +) { + if ( + topLevelType === topLevelTypes.topInput || + topLevelType === topLevelTypes.topChange + ) { + return getInstIfValueChanged(targetInst, target); } } @@ -335,6 +362,8 @@ var ChangeEventPlugin = { eventTypes: eventTypes, + _isInputEventSupported: isInputEventSupported, + extractEvents: function( topLevelType, targetInst, @@ -353,17 +382,17 @@ var ChangeEventPlugin = { } } else if (isTextInputElement(targetNode)) { if (isInputEventSupported) { - getTargetInstFunc = getTargetInstForInputEvent; + getTargetInstFunc = getTargetInstForInputOrChangeEvent; } else { - getTargetInstFunc = getTargetInstForInputEventIE; - handleEventFunc = handleEventsForInputEventIE; + getTargetInstFunc = getTargetInstForInputEventPolyfill; + handleEventFunc = handleEventsForInputEventPolyfill; } } else if (shouldUseClickEvent(targetNode)) { getTargetInstFunc = getTargetInstForClickEvent; } if (getTargetInstFunc) { - var inst = getTargetInstFunc(topLevelType, targetInst); + var inst = getTargetInstFunc(topLevelType, targetInst, targetNode); if (inst) { var event = SyntheticEvent.getPooled( eventTypes.change, diff --git a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js index 0d29c6664a0..4177c386ff7 100644 --- a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js +++ b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js @@ -13,6 +13,7 @@ var React = require('React'); var ReactTestUtils = require('ReactTestUtils'); +var ChangeEventPlugin = require('ChangeEventPlugin'); describe('ChangeEventPlugin', function() { it('should fire change for checkbox input', function() { @@ -27,4 +28,91 @@ describe('ChangeEventPlugin', function() { ReactTestUtils.SimulateNative.click(input); expect(called).toBe(1); }); + + it('should only fire change for checked radio button once', function() { + var called = 0; + + function cb(e) { + called += 1; + } + + var input = ReactTestUtils.renderIntoDocument(); + ReactTestUtils.SimulateNative.click(input); + ReactTestUtils.SimulateNative.click(input); + expect(called).toBe(1); + }); + + it('should deduplicate input value change events', function() { + var input; + var called = 0; + + function cb(e) { + called += 1; + expect(e.type).toBe('change'); + } + + [ + , + , + , + ].forEach(function(element) { + called = 0; + input = ReactTestUtils.renderIntoDocument(element); + + ReactTestUtils.SimulateNative.change(input); + ReactTestUtils.SimulateNative.change(input); + expect(called).toBe(1); + + called = 0; + input = ReactTestUtils.renderIntoDocument(element); + ReactTestUtils.SimulateNative.input(input); + ReactTestUtils.SimulateNative.input(input); + expect(called).toBe(1); + + called = 0; + input = ReactTestUtils.renderIntoDocument(element); + ReactTestUtils.SimulateNative.input(input); + ReactTestUtils.SimulateNative.change(input); + expect(called).toBe(1); + }); + }); + + it('should listen for both change and input events when supported', function() { + var called = 0; + + function cb(e) { + called += 1; + expect(e.type).toBe('change'); + } + + if (!ChangeEventPlugin._isInputEventSupported) { + return; + } + + var input = ReactTestUtils.renderIntoDocument(); + + ReactTestUtils.SimulateNative.input(input); + input.value = 'foo'; + ReactTestUtils.SimulateNative.change(input); + + expect(called).toBe(2); + }); + + it('should only fire events when the value changes for range inputs', function() { + var called = 0; + + function cb(e) { + called += 1; + expect(e.type).toBe('change'); + } + + var input = ReactTestUtils.renderIntoDocument(); + + ReactTestUtils.SimulateNative.input(input); + ReactTestUtils.SimulateNative.change(input); + input.value = 'foo'; + ReactTestUtils.SimulateNative.input(input); + ReactTestUtils.SimulateNative.change(input); + expect(called).toBe(2); + }); }); From 04bfd6e057a2ef70c471449cc6887c6446a9e036 Mon Sep 17 00:00:00 2001 From: jquense Date: Thu, 31 Dec 2015 11:00:20 -0500 Subject: [PATCH 2/3] catch programmatic value changes --- .../client/eventPlugins/ChangeEventPlugin.js | 86 +++++++++++++++---- .../__tests__/ChangeEventPlugin-test.js | 81 ++++++++++++++++- .../wrappers/__tests__/ReactDOMInput-test.js | 4 + 3 files changed, 153 insertions(+), 18 deletions(-) diff --git a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js index 52ee10bc7ca..dfe663604a1 100644 --- a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js @@ -26,7 +26,14 @@ var keyOf = require('keyOf'); var topLevelTypes = EventConstants.topLevelTypes; -var LAST_VALUE_KEY = '__reactLastInputValue'; +function isCheckable(elem) { + var type = elem.type; + var nodeName = elem.nodeName; + return ( + (nodeName && nodeName.toLowerCase() === 'input') && + (type === 'checkbox' || type === 'radio') + ); +} var eventTypes = { change: { @@ -47,12 +54,28 @@ var eventTypes = { }, }; +var LAST_VALUE_KEY = '__reactLastInputValue'; +var lastInputValue = { + get: function(elem) { + if (elem) { + return elem[LAST_VALUE_KEY]; + } + }, + + set: function(elem, value) { + if (elem) { + // cast to a string for comparisons + elem[LAST_VALUE_KEY] = '' + value; + } + }, +}; + function getInstIfValueChanged(targetInst, target) { var value = getInputValue(target); - var lastValue = target[LAST_VALUE_KEY]; + var lastValue = lastInputValue.get(target); if (!target.hasOwnProperty(LAST_VALUE_KEY) || value !== lastValue) { - target[LAST_VALUE_KEY] = value; + lastInputValue.set(target, value); return targetInst; } } @@ -61,14 +84,8 @@ function getInputValue(elem) { var value; if (elem) { - var type = elem.type; - var nodeName = elem.nodeName; - - if ( - (nodeName && nodeName.toLowerCase() === 'input') && - (type === 'checkbox' || type === 'radio') - ) { - value = elem.checked; + if (isCheckable(elem)) { + value = '' + elem.checked; } else { value = elem.value; } @@ -148,12 +165,14 @@ function stopWatchingForChangeEventIE8() { function getTargetInstForChangeEvent( topLevelType, - targetInst + targetInst, + target ) { if (topLevelType === topLevelTypes.topChange) { return targetInst; } } + function handleEventsForChangeEventIE8( topLevelType, target, @@ -319,10 +338,7 @@ function shouldUseClickEvent(elem) { // Use the `click` event to detect changes to checkbox and radio inputs. // This approach works across all browsers, whereas `change` does not fire // until `blur` in IE8. - return ( - (elem.nodeName && elem.nodeName.toLowerCase() === 'input') && - (elem.type === 'checkbox' || elem.type === 'radio') - ); + return isCheckable(elem); } function getTargetInstForClickEvent( @@ -364,6 +380,41 @@ var ChangeEventPlugin = { _isInputEventSupported: isInputEventSupported, + willDeleteListener(inst) { + var targetNode; + + // An uglier internal check to avoid a try/catch + // if the instance is unmounted the node will already be removed + if (inst._nativeNode !== null) { + targetNode = ReactDOMComponentTree.getNodeFromInstance(inst); + } + + if (targetNode) { + delete targetNode.value; + } + }, + + didPutListener: function(inst, registrationName) { + var targetNode = ReactDOMComponentTree.getNodeFromInstance(inst); + var valueField = isCheckable(targetNode) ? 'checked' : 'value'; + var descriptor = Object.getOwnPropertyDescriptor( + targetNode.constructor.prototype, + valueField + ); + + Object.defineProperty(targetNode, valueField, { + enumerable: descriptor.enumerable, + configurable: true, + get: function() { + return descriptor.get.call(this); + }, + set: function(val) { + lastInputValue.set(this, val); + descriptor.set.call(this, val); + }, + }); + }, + extractEvents: function( topLevelType, targetInst, @@ -417,4 +468,7 @@ var ChangeEventPlugin = { }; +// exposed for testing ONLY +ChangeEventPlugin.__lastInputValue = lastInputValue; + module.exports = ChangeEventPlugin; diff --git a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js index 4177c386ff7..12c740ce30d 100644 --- a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js +++ b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js @@ -12,9 +12,19 @@ 'use strict'; var React = require('React'); +var ReactDOM = require('ReactDOM'); var ReactTestUtils = require('ReactTestUtils'); var ChangeEventPlugin = require('ChangeEventPlugin'); +var lastInputValue = ChangeEventPlugin.__lastInputValue; + +function setUntrackedValue(elem, value) { + var current = lastInputValue.get(elem); + + elem.value = value; + lastInputValue.set(elem, current); +} + describe('ChangeEventPlugin', function() { it('should fire change for checkbox input', function() { var called = 0; @@ -29,6 +39,69 @@ describe('ChangeEventPlugin', function() { expect(called).toBe(1); }); + it('should catch setting the value programmatically', function() { + + var input = ReactTestUtils.renderIntoDocument( + + ); + + input.value = 'bar'; + + expect(lastInputValue.get(input)).toBe('bar'); + }); + + it('should not fire change when setting the value programmatically', function() { + var called = 0; + + function cb(e) { + called += 1; + expect(e.type).toBe('change'); + } + + var input = ReactTestUtils.renderIntoDocument( + + ); + + input.value = 'bar'; + ReactTestUtils.SimulateNative.change(input); + expect(called).toBe(0); + + setUntrackedValue(input, 'foo'); + ReactTestUtils.SimulateNative.change(input); + + expect(called).toBe(1); + }); + + it('should not fire change when setting checked programmatically', function() { + var called = 0; + + function cb(e) { + called += 1; + expect(e.type).toBe('change'); + } + + var input = ReactTestUtils.renderIntoDocument( + + ); + + input.checked = true; + ReactTestUtils.SimulateNative.click(input); + expect(called).toBe(0); + + input.checked = false; + lastInputValue.set(input, undefined); + ReactTestUtils.SimulateNative.click(input); + + expect(called).toBe(1); + }); + + it('should unmount', function() { + var container = document.createElement('div'); + var input = ReactDOM.render(, container); + + ReactDOM.unmountComponentAtNode(container); + }); + it('should only fire change for checked radio button once', function() { var called = 0; @@ -92,7 +165,9 @@ describe('ChangeEventPlugin', function() { var input = ReactTestUtils.renderIntoDocument(); ReactTestUtils.SimulateNative.input(input); - input.value = 'foo'; + + setUntrackedValue(input, 'foo'); + ReactTestUtils.SimulateNative.change(input); expect(called).toBe(2); @@ -110,7 +185,9 @@ describe('ChangeEventPlugin', function() { ReactTestUtils.SimulateNative.input(input); ReactTestUtils.SimulateNative.change(input); - input.value = 'foo'; + + setUntrackedValue(input, 'foo'); + ReactTestUtils.SimulateNative.input(input); ReactTestUtils.SimulateNative.change(input); expect(called).toBe(2); diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js index a42227bb971..c9eee3716e8 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js @@ -13,6 +13,9 @@ var emptyFunction = require('emptyFunction'); +var ChangeEventPlugin = require('ChangeEventPlugin'); + +var lastInputValue = ChangeEventPlugin.__lastInputValue; describe('ReactDOMInput', function() { var EventConstants; @@ -150,6 +153,7 @@ describe('ReactDOMInput', function() { var node = ReactDOM.render(stub, container); node.value = 'giraffe'; + lastInputValue.set(node, undefined); var fakeNativeEvent = new function() {}; fakeNativeEvent.target = node; From 669e3dff96234ff4ec5626b93f50877b8889444d Mon Sep 17 00:00:00 2001 From: jquense Date: Tue, 8 Mar 2016 09:09:55 -0500 Subject: [PATCH 3/3] move value tracking to seperate module --- .../__tests__/inputValueTracking-test.js | 165 +++++++++++++++ .../client/eventPlugins/ChangeEventPlugin.js | 196 ++++-------------- .../__tests__/ChangeEventPlugin-test.js | 42 +++- .../dom/client/inputValueTracking.js | 133 ++++++++++++ .../wrappers/__tests__/ReactDOMInput-test.js | 15 +- src/renderers/dom/shared/ReactDOMComponent.js | 15 +- .../__tests__/ReactDOMComponent-test.js | 44 ++++ 7 files changed, 437 insertions(+), 173 deletions(-) create mode 100644 src/renderers/dom/client/__tests__/inputValueTracking-test.js create mode 100644 src/renderers/dom/client/inputValueTracking.js diff --git a/src/renderers/dom/client/__tests__/inputValueTracking-test.js b/src/renderers/dom/client/__tests__/inputValueTracking-test.js new file mode 100644 index 00000000000..b03c04552cb --- /dev/null +++ b/src/renderers/dom/client/__tests__/inputValueTracking-test.js @@ -0,0 +1,165 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @emails react-core + */ +'use strict'; + +var React = require('React'); +var ReactTestUtils = require('ReactTestUtils'); +var inputValueTracking = require('inputValueTracking'); + +describe('inputValueTracking', function() { + var input, checkbox, mockComponent; + + beforeEach(function() { + input = document.createElement('input'); + input.type = 'text'; + checkbox = document.createElement('input'); + checkbox.type = 'checkbox'; + mockComponent = { _nativeNode: input, _wrapperState: {} }; + }); + + it('should attach tracker to wrapper state', function() { + inputValueTracking.track(mockComponent); + + expect( + mockComponent._wrapperState.hasOwnProperty('valueTracker') + ).toBe(true); + }); + + it('should define `value` on the instance node', function() { + inputValueTracking.track(mockComponent); + + expect( + input.hasOwnProperty('value') + ).toBe(true); + }); + + it('should define `checked` on the instance node', function() { + mockComponent._nativeNode = checkbox; + inputValueTracking.track(mockComponent); + + expect(checkbox.hasOwnProperty('checked')).toBe(true); + }); + + it('should initialize with the current value', function() { + input.value ='foo'; + + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + expect(tracker.getValue()).toEqual('foo'); + }); + + it('should initialize with the current `checked`', function() { + mockComponent._nativeNode = checkbox; + checkbox.checked = true; + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + expect(tracker.getValue()).toEqual('true'); + }); + + it('should track value changes', function() { + input.value ='foo'; + + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + input.value ='bar'; + expect(tracker.getValue()).toEqual('bar'); + }); + + it('should tracked`checked` changes', function() { + mockComponent._nativeNode = checkbox; + checkbox.checked = true; + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + checkbox.checked = false; + expect(tracker.getValue()).toEqual('false'); + }); + + it('should update value manually', function() { + input.value ='foo'; + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + tracker.setValue('bar'); + expect(tracker.getValue()).toEqual('bar'); + }); + + it('should coerce value to a string', function() { + input.value ='foo'; + inputValueTracking.track(mockComponent); + + var tracker = mockComponent._wrapperState.valueTracker; + + tracker.setValue(500); + expect(tracker.getValue()).toEqual('500'); + }); + + it('should update value if it changed and return result', function() { + inputValueTracking.track(mockComponent); + input.value ='foo'; + + var tracker = mockComponent._wrapperState.valueTracker; + + expect( + inputValueTracking.updateValueIfChanged(mockComponent) + ).toBe(false); + + tracker.setValue('bar'); + + expect( + inputValueTracking.updateValueIfChanged(mockComponent) + ).toBe(true); + + expect(tracker.getValue()).toEqual('foo'); + }); + + it('should track value and return true when updating untracked instance', function() { + input.value ='foo'; + + expect( + inputValueTracking.updateValueIfChanged(mockComponent) + ) + .toBe(true); + + var tracker = mockComponent._wrapperState.valueTracker; + expect(tracker.getValue()).toEqual('foo'); + }); + + it('should return tracker from node', function() { + var node = ReactTestUtils.renderIntoDocument(); + var tracker = inputValueTracking._getTrackerFromNode(node); + expect(tracker.getValue()).toEqual('foo'); + }); + + it('should stop tracking', function() { + inputValueTracking.track(mockComponent); + + expect( + mockComponent._wrapperState.hasOwnProperty('valueTracker') + ).toBe(true); + + inputValueTracking.stopTracking(mockComponent); + + expect( + mockComponent._wrapperState.hasOwnProperty('valueTracker') + ).toBe(false); + + expect(input.hasOwnProperty('value')).toBe(false); + }); +}); diff --git a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js index dfe663604a1..13602979d5e 100644 --- a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js @@ -19,6 +19,7 @@ var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactUpdates = require('ReactUpdates'); var SyntheticEvent = require('SyntheticEvent'); +var inputValueTracking = require('inputValueTracking'); var getEventTarget = require('getEventTarget'); var isEventSupported = require('isEventSupported'); var isTextInputElement = require('isTextInputElement'); @@ -26,14 +27,6 @@ var keyOf = require('keyOf'); var topLevelTypes = EventConstants.topLevelTypes; -function isCheckable(elem) { - var type = elem.type; - var nodeName = elem.nodeName; - return ( - (nodeName && nodeName.toLowerCase() === 'input') && - (type === 'checkbox' || type === 'radio') - ); -} var eventTypes = { change: { @@ -54,52 +47,24 @@ var eventTypes = { }, }; -var LAST_VALUE_KEY = '__reactLastInputValue'; -var lastInputValue = { - get: function(elem) { - if (elem) { - return elem[LAST_VALUE_KEY]; - } - }, - - set: function(elem, value) { - if (elem) { - // cast to a string for comparisons - elem[LAST_VALUE_KEY] = '' + value; - } - }, -}; - -function getInstIfValueChanged(targetInst, target) { - var value = getInputValue(target); - var lastValue = lastInputValue.get(target); - - if (!target.hasOwnProperty(LAST_VALUE_KEY) || value !== lastValue) { - lastInputValue.set(target, value); - return targetInst; - } -} - -function getInputValue(elem) { - var value; - - if (elem) { - if (isCheckable(elem)) { - value = '' + elem.checked; - } else { - value = elem.value; - } - } - return value; +function createAndAccumulateChangeEvent(inst, nativeEvent, target) { + var event = SyntheticEvent.getPooled( + eventTypes.change, + inst, + nativeEvent, + target + ); + event.type = 'change'; + EventPropagators.accumulateTwoPhaseDispatches(event); + return event; } - /** * For IE shims */ var activeElement = null; var activeElementInst = null; -var activeElementValue = null; -var activeElementValueProp = null; + + /** * SECTION: handle `change` event @@ -120,14 +85,12 @@ if (ExecutionEnvironment.canUseDOM) { ); } -function manualDispatchChangeEvent(nativeEvent, targetInst) { - var event = SyntheticEvent.getPooled( - eventTypes.change, - targetInst, +function manualDispatchChangeEvent(nativeEvent) { + var event = createAndAccumulateChangeEvent( + activeElementInst, nativeEvent, getEventTarget(nativeEvent) ); - EventPropagators.accumulateTwoPhaseDispatches(event); // If change and propertychange bubbled, we'd just bind to it like all the // other events and have it go through ReactBrowserEventEmitter. Since it @@ -163,10 +126,15 @@ function stopWatchingForChangeEventIE8() { activeElementInst = null; } +function getInstIfValueChanged(targetInst) { + if (inputValueTracking.updateValueIfChanged(targetInst)) { + return targetInst; + } +} + function getTargetInstForChangeEvent( topLevelType, - targetInst, - target + targetInst ) { if (topLevelType === topLevelTypes.topChange) { return targetInst; @@ -201,37 +169,6 @@ if (ExecutionEnvironment.canUseDOM) { ); } -/** - * (For IE <=9) Replacement getter/setter for the `value` property that gets - * set on the active element. - */ -var newValueProp = { - get: function() { - return activeElementValueProp.get.call(this); - }, - set: function(val) { - // Cast to a string so we can do equality checks. - activeElementValue = '' + val; - activeElementValueProp.set.call(this, val); - }, -}; - -function clearActiveElement() { - activeElement = null; - activeElementInst = null; - activeElementValue = null; - activeElementValueProp = null; -} - -function updateActiveElement(target, targetInst) { - activeElement = target; - activeElementInst = targetInst; - activeElementValue = target.value; - activeElementValueProp = Object.getOwnPropertyDescriptor( - target.constructor.prototype, - 'value' - ); -} /** * (For IE <=9) Starts tracking propertychange events on the passed-in element @@ -239,11 +176,8 @@ function updateActiveElement(target, targetInst) { * value changes in JS. */ function startWatchingForValueChange(target, targetInst) { - updateActiveElement(target, targetInst); - - // Not guarded in a canDefineProperty check: IE8 supports defineProperty only - // on DOM elements - Object.defineProperty(activeElement, 'value', newValueProp); + activeElement = target; + activeElementInst = targetInst; activeElement.attachEvent('onpropertychange', handlePropertyChange); } @@ -255,11 +189,9 @@ function stopWatchingForValueChange() { if (!activeElement) { return; } - - // delete restores the original property definition - delete activeElement.value; activeElement.detachEvent('onpropertychange', handlePropertyChange); - clearActiveElement(); + activeElement = null; + activeElementInst = null; } /** @@ -270,13 +202,9 @@ function handlePropertyChange(nativeEvent) { if (nativeEvent.propertyName !== 'value') { return; } - var value = nativeEvent.srcElement.value; - if (value === activeElementValue) { - return; + if (getInstIfValueChanged(activeElementInst)) { + manualDispatchChangeEvent(nativeEvent); } - activeElementValue = value; - - manualDispatchChangeEvent(nativeEvent); } function handleEventsForInputEventPolyfill( @@ -323,10 +251,7 @@ function getTargetInstForInputEventPolyfill( // keystroke if user does a key repeat (it'll be a little delayed: right // before the second keystroke). Other input methods (e.g., paste) seem to // fire selectionchange normally. - if (activeElement && activeElement.value !== activeElementValue) { - activeElementValue = activeElement.value; - return activeElementInst; - } + return getInstIfValueChanged(activeElementInst); } } @@ -338,29 +263,31 @@ function shouldUseClickEvent(elem) { // Use the `click` event to detect changes to checkbox and radio inputs. // This approach works across all browsers, whereas `change` does not fire // until `blur` in IE8. - return isCheckable(elem); + var nodeName = elem.nodeName; + return ( + (nodeName && nodeName.toLowerCase() === 'input') && + (elem.type === 'checkbox' || elem.type === 'radio') + ); } function getTargetInstForClickEvent( topLevelType, - targetInst, - target + targetInst ) { if (topLevelType === topLevelTypes.topClick) { - return getInstIfValueChanged(targetInst, target); + return getInstIfValueChanged(targetInst); } } function getTargetInstForInputOrChangeEvent( topLevelType, - targetInst, - target + targetInst ) { if ( topLevelType === topLevelTypes.topInput || topLevelType === topLevelTypes.topChange ) { - return getInstIfValueChanged(targetInst, target); + return getInstIfValueChanged(targetInst); } } @@ -380,41 +307,6 @@ var ChangeEventPlugin = { _isInputEventSupported: isInputEventSupported, - willDeleteListener(inst) { - var targetNode; - - // An uglier internal check to avoid a try/catch - // if the instance is unmounted the node will already be removed - if (inst._nativeNode !== null) { - targetNode = ReactDOMComponentTree.getNodeFromInstance(inst); - } - - if (targetNode) { - delete targetNode.value; - } - }, - - didPutListener: function(inst, registrationName) { - var targetNode = ReactDOMComponentTree.getNodeFromInstance(inst); - var valueField = isCheckable(targetNode) ? 'checked' : 'value'; - var descriptor = Object.getOwnPropertyDescriptor( - targetNode.constructor.prototype, - valueField - ); - - Object.defineProperty(targetNode, valueField, { - enumerable: descriptor.enumerable, - configurable: true, - get: function() { - return descriptor.get.call(this); - }, - set: function(val) { - lastInputValue.set(this, val); - descriptor.set.call(this, val); - }, - }); - }, - extractEvents: function( topLevelType, targetInst, @@ -443,16 +335,13 @@ var ChangeEventPlugin = { } if (getTargetInstFunc) { - var inst = getTargetInstFunc(topLevelType, targetInst, targetNode); + var inst = getTargetInstFunc(topLevelType, targetInst); if (inst) { - var event = SyntheticEvent.getPooled( - eventTypes.change, + var event = createAndAccumulateChangeEvent( inst, nativeEvent, nativeEventTarget ); - event.type = 'change'; - EventPropagators.accumulateTwoPhaseDispatches(event); return event; } } @@ -468,7 +357,4 @@ var ChangeEventPlugin = { }; -// exposed for testing ONLY -ChangeEventPlugin.__lastInputValue = lastInputValue; - module.exports = ChangeEventPlugin; diff --git a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js index 12c740ce30d..a30d525814a 100644 --- a/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js +++ b/src/renderers/dom/client/eventPlugins/__tests__/ChangeEventPlugin-test.js @@ -15,14 +15,28 @@ var React = require('React'); var ReactDOM = require('ReactDOM'); var ReactTestUtils = require('ReactTestUtils'); var ChangeEventPlugin = require('ChangeEventPlugin'); +var inputValueTracking = require('inputValueTracking'); -var lastInputValue = ChangeEventPlugin.__lastInputValue; +function getTrackedValue(elem) { + var tracker = inputValueTracking._getTrackerFromNode(elem); + return tracker.getValue(); +} -function setUntrackedValue(elem, value) { - var current = lastInputValue.get(elem); +function setTrackedValue(elem, value) { + var tracker = inputValueTracking._getTrackerFromNode(elem); + tracker.setValue(value); +} - elem.value = value; - lastInputValue.set(elem, current); +function setUntrackedValue(elem, value) { + var tracker = inputValueTracking._getTrackerFromNode(elem); + var current = tracker.getValue(); + + if (elem.type === 'checkbox' || elem.type === 'radio') { + elem.checked = value; + } else { + elem.value = value; + } + tracker.setValue(current); } describe('ChangeEventPlugin', function() { @@ -35,19 +49,20 @@ describe('ChangeEventPlugin', function() { } var input = ReactTestUtils.renderIntoDocument(); + + setUntrackedValue(input, true); ReactTestUtils.SimulateNative.click(input); + expect(called).toBe(1); }); it('should catch setting the value programmatically', function() { - var input = ReactTestUtils.renderIntoDocument( ); input.value = 'bar'; - - expect(lastInputValue.get(input)).toBe('bar'); + expect(getTrackedValue(input)).toBe('bar'); }); it('should not fire change when setting the value programmatically', function() { @@ -89,7 +104,7 @@ describe('ChangeEventPlugin', function() { expect(called).toBe(0); input.checked = false; - lastInputValue.set(input, undefined); + setTrackedValue(input, undefined); ReactTestUtils.SimulateNative.click(input); expect(called).toBe(1); @@ -98,7 +113,7 @@ describe('ChangeEventPlugin', function() { it('should unmount', function() { var container = document.createElement('div'); var input = ReactDOM.render(, container); - + ReactDOM.unmountComponentAtNode(container); }); @@ -110,6 +125,7 @@ describe('ChangeEventPlugin', function() { } var input = ReactTestUtils.renderIntoDocument(); + setUntrackedValue(input, true); ReactTestUtils.SimulateNative.click(input); ReactTestUtils.SimulateNative.click(input); expect(called).toBe(1); @@ -132,18 +148,21 @@ describe('ChangeEventPlugin', function() { called = 0; input = ReactTestUtils.renderIntoDocument(element); + setUntrackedValue(input, '40'); ReactTestUtils.SimulateNative.change(input); ReactTestUtils.SimulateNative.change(input); expect(called).toBe(1); called = 0; input = ReactTestUtils.renderIntoDocument(element); + setUntrackedValue(input, '40'); ReactTestUtils.SimulateNative.input(input); ReactTestUtils.SimulateNative.input(input); expect(called).toBe(1); called = 0; input = ReactTestUtils.renderIntoDocument(element); + setUntrackedValue(input, '40'); ReactTestUtils.SimulateNative.input(input); ReactTestUtils.SimulateNative.change(input); expect(called).toBe(1); @@ -163,6 +182,7 @@ describe('ChangeEventPlugin', function() { } var input = ReactTestUtils.renderIntoDocument(); + setUntrackedValue(input, 'bar'); ReactTestUtils.SimulateNative.input(input); @@ -182,7 +202,7 @@ describe('ChangeEventPlugin', function() { } var input = ReactTestUtils.renderIntoDocument(); - + setUntrackedValue(input, '40'); ReactTestUtils.SimulateNative.input(input); ReactTestUtils.SimulateNative.change(input); diff --git a/src/renderers/dom/client/inputValueTracking.js b/src/renderers/dom/client/inputValueTracking.js new file mode 100644 index 00000000000..3b877be63ec --- /dev/null +++ b/src/renderers/dom/client/inputValueTracking.js @@ -0,0 +1,133 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule inputValueTracking + */ + +'use strict'; +var ReactDOMComponentTree = require('ReactDOMComponentTree'); + +function isCheckable(elem) { + var type = elem.type; + var nodeName = elem.nodeName; + return ( + (nodeName && nodeName.toLowerCase() === 'input') && + (type === 'checkbox' || type === 'radio') + ); +} + +function getTracker(inst) { + return inst._wrapperState.valueTracker; +} + +function attachTracker(inst, tracker) { + inst._wrapperState.valueTracker = tracker; +} + +function detachTracker(inst) { + delete inst._wrapperState.valueTracker; +} + +function getValueFromNode(node) { + var value; + if (node) { + value = isCheckable(node) + ? '' + node.checked + : node.value; + } + return value; +} + +var inputValueTracking = { + // exposed for testing + _getTrackerFromNode(node) { + return getTracker( + ReactDOMComponentTree.getInstanceFromNode(node) + ); + }, + + track: function(inst) { + if (getTracker(inst)) { + return; + } + + var node = ReactDOMComponentTree.getNodeFromInstance(inst); + var valueField = isCheckable(node) ? 'checked' : 'value'; + var descriptor = Object.getOwnPropertyDescriptor( + node.constructor.prototype, + valueField + ); + + var currentValue = '' + node[valueField]; + + // if someone has already defined a value bail and don't track value + // will cause over reporting of changes, but it's better then a hard failure + // (needed for certain tests that spyOn input values) + if (node.hasOwnProperty(valueField)) { + return; + } + + Object.defineProperty(node, valueField, { + enumerable: descriptor.enumerable, + configurable: true, + get: function() { + return descriptor.get.call(this); + }, + set: function(value) { + currentValue = '' + value; + descriptor.set.call(this, value); + }, + }); + + attachTracker(inst, { + getValue() { + return currentValue; + }, + setValue(value) { + currentValue = '' + value; + }, + stopTracking() { + detachTracker(inst); + delete node[valueField]; + }, + }); + }, + + updateValueIfChanged(inst) { + if (!inst) { + return false; + } + var tracker = getTracker(inst); + + if (!tracker) { + inputValueTracking.track(inst); + return true; + } + + var lastValue = tracker.getValue(); + var nextValue = getValueFromNode( + ReactDOMComponentTree.getNodeFromInstance(inst) + ); + + if (nextValue !== lastValue) { + tracker.setValue(nextValue); + return true; + } + + return false; + }, + + stopTracking(inst) { + var tracker = getTracker(inst); + if (tracker) { + tracker.stopTracking(); + } + }, +}; + +module.exports = inputValueTracking; diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js index c9eee3716e8..34f08814a80 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js @@ -13,9 +13,6 @@ var emptyFunction = require('emptyFunction'); -var ChangeEventPlugin = require('ChangeEventPlugin'); - -var lastInputValue = ChangeEventPlugin.__lastInputValue; describe('ReactDOMInput', function() { var EventConstants; @@ -24,6 +21,14 @@ describe('ReactDOMInput', function() { var ReactDOMFeatureFlags; var ReactLink; var ReactTestUtils; + var inputValueTracking; + + function setUntrackedValue(elem, value) { + var tracker = inputValueTracking._getTrackerFromNode(elem); + var current = tracker.getValue(); + elem.value = value; + tracker.setValue(current); + } beforeEach(function() { jest.resetModuleRegistry(); @@ -33,6 +38,7 @@ describe('ReactDOMInput', function() { ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactLink = require('ReactLink'); ReactTestUtils = require('ReactTestUtils'); + inputValueTracking = require('inputValueTracking'); spyOn(console, 'error'); }); @@ -152,8 +158,7 @@ describe('ReactDOMInput', function() { var container = document.createElement('div'); var node = ReactDOM.render(stub, container); - node.value = 'giraffe'; - lastInputValue.set(node, undefined); + setUntrackedValue(node, 'giraffe'); var fakeNativeEvent = new function() {}; fakeNativeEvent.target = node; diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index ec1a0f4ab5f..d3c1fe64c2a 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -41,6 +41,7 @@ var invariant = require('invariant'); var isEventSupported = require('isEventSupported'); var keyOf = require('keyOf'); var shallowEqual = require('shallowEqual'); +var inputValueTracking = require('inputValueTracking'); var validateDOMNesting = require('validateDOMNesting'); var warning = require('warning'); @@ -256,6 +257,10 @@ var mediaEvents = { topWaiting: 'waiting', }; +function trackInputValue() { + inputValueTracking.track(this); +} + function trapBubbledEventsLocal() { var inst = this; // If a component renders to null or if another component fatals and causes @@ -478,6 +483,7 @@ ReactDOMComponent.Mixin = { case 'input': ReactDOMInput.mountWrapper(this, props, nativeParent); props = ReactDOMInput.getNativeProps(this, props); + transaction.getReactMountReady().enqueue(trackInputValue, this); transaction.getReactMountReady().enqueue(trapBubbledEventsLocal, this); break; case 'option': @@ -492,6 +498,7 @@ ReactDOMComponent.Mixin = { case 'textarea': ReactDOMTextarea.mountWrapper(this, props, nativeParent); props = ReactDOMTextarea.getNativeProps(this, props); + transaction.getReactMountReady().enqueue(trackInputValue, this); transaction.getReactMountReady().enqueue(trapBubbledEventsLocal, this); break; } @@ -580,10 +587,10 @@ ReactDOMComponent.Mixin = { } switch (this._tag) { - case 'button': case 'input': - case 'select': case 'textarea': + case 'select': + case 'button': if (props.autoFocus) { transaction.getReactMountReady().enqueue( AutoFocusUtils.focusDOMComponent, @@ -1029,6 +1036,10 @@ ReactDOMComponent.Mixin = { } } break; + case 'input': + case 'textarea': + inputValueTracking.stopTracking(this); + break; case 'html': case 'head': case 'body': diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 1e07d5a4c20..aa9cfd0f14d 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -19,6 +19,7 @@ describe('ReactDOMComponent', function() { var ReactDOM; var ReactDOMFeatureFlags; var ReactDOMServer; + var inputValueTracking; beforeEach(function() { jest.resetModuleRegistry(); @@ -26,6 +27,7 @@ describe('ReactDOMComponent', function() { ReactDOM = require('ReactDOM'); ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactDOMServer = require('ReactDOMServer'); + inputValueTracking = require('inputValueTracking'); }); describe('updateDOM', function() { @@ -1074,6 +1076,24 @@ describe('ReactDOMComponent', function() { ); }); + it('should track input values', function() { + var container = document.createElement('div'); + var inst = ReactDOM.render(, container); + + var tracker = inputValueTracking._getTrackerFromNode(inst); + + expect(tracker.getValue()).toEqual('foo'); + }); + + it('should track textarea values', function() { + var container = document.createElement('div'); + var inst = ReactDOM.render(