Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize onChange behavior in IE for inputs with placeholders #3826

Closed
wants to merge 3 commits into from
Closed

Normalize onChange behavior in IE for inputs with placeholders #3826

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31794f5
Normalize onChange behavior in IE for placeholders
jquense May 6, 2015
81a8c3b
cache IE support test value and fix style issues
jquense May 7, 2015
640d64b
simplify value check, stop propagation
jquense May 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/browser/ui/dom/components/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ var ReactClass = require('ReactClass');
var ReactElement = require('ReactElement');
var ReactMount = require('ReactMount');
var ReactUpdates = require('ReactUpdates');
var ExecutionEnvironment = require('ExecutionEnvironment');
var isTextInputElement = require('isTextInputElement');

var assign = require('Object.assign');
var findDOMNode = require('findDOMNode');
Expand All @@ -27,6 +29,11 @@ var invariant = require('invariant');
var input = ReactElement.createFactory('input');

var instancesByReactID = {};
var hasNoisyInputEvent = false;

if (ExecutionEnvironment.canUseDOM) {
hasNoisyInputEvent = 'documentMode' in document && document.documentMode > 9;
}

function forceUpdateIfMounted() {
/*jshint validthis:true */
Expand All @@ -35,6 +42,15 @@ function forceUpdateIfMounted() {
}
}


function isIEInputEvent(event) {
return (
hasNoisyInputEvent &&
event.nativeEvent.type === 'input' &&
isTextInputElement(event.target)
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 things here.

  1. isIEInputEvent
  2. Put the && at the end of the previous lines and then let's wrap the whole return in parens
return (
  'documentMode' in document &&
  document.documentMode > 9 &&
  etc
)

And now actually a 3rd since I'm looking. We can cache the value of 'documentMode' in document && document.documentMode > 9 so that we don't need to look it up each time (any access of DOM properties has a cost).


/**
* Implements an <input> native component that allows setting these optional
* props: `checked`, `value`, `defaultChecked`, and `defaultValue`.
Expand All @@ -57,8 +73,15 @@ var ReactDOMInput = ReactClass.createClass({

mixins: [AutoFocusMixin, LinkedValueUtils.Mixin, ReactBrowserComponentMixin],

componentWillMount: function() {
var defaultValue = this.props.defaultValue;

this._uncontrolledValue = defaultValue != null ? '' + defaultValue : '';
},

getInitialState: function() {
var defaultValue = this.props.defaultValue;

return {
initialChecked: this.props.defaultChecked || false,
initialValue: defaultValue != null ? defaultValue : null
Expand Down Expand Up @@ -115,6 +138,24 @@ var ReactDOMInput = ReactClass.createClass({
_handleChange: function(event) {
var returnValue;
var onChange = LinkedValueUtils.getOnChange(this.props);

// IE 10+ fire input events when setting/unsetting a placeholder
// we guard against it by checking if the next and
// last values are both empty and bailing out of the change
// https://github.com/facebook/react/issues/3484
if (isIEInputEvent(event)) {
var controlledValue = LinkedValueUtils.getValue(this.props);
var lastValue = controlledValue != null ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively it seems like a bad idea to apply different logic depending on whether or not the component is controlled, tracking the last value and always comparing that seems like the simplest and safest approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

different logic meaning using props.value vs _uncontrolledValue? I thought about just always tracking a 'lastValue' but I think (still early so I may be off base) you end up with additional complexity in componentWillReceiveProps, in cases where you switch between controlled & uncontrolled. It seemed simpler to localize all the logic in one place as much as possible vs spread over more lifecycle hooks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh, there's also a question of what constitutes the "last value" from which to determine if the change event should fire I guess, the last one provided to the component or the last one in the DOM input. There's a problem in that the value of DOM inputs can and will change outside of our control/knowledge and unless I'm mistaken there is no 100% robust mechanism that allows us to know the actual last value of the DOM input.

Perhaps I'm overthinking it, but it seems that either we need to know the last value of the DOM input (which isn't robust AFAIK) or we need to know the last value of the input component but that is only a partial solution (and only for controlled inputs at that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof I hadn't thought of that, you mean if the user provides a value props but then manually changes the DOM node value like findDOMNode(input).value = "can't touch this!"? I want to say in those cases the "right" thing to do is use the value prop as the last value, because the props existence indicates an intent to always handle the value declaratively.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to say in those cases the "right" thing to do is use the value prop as the last value

That can only be applied to controlled components, not uncontrolled. Uncontrolled components are frequently updated through directly setting the value property.

As for controlled components, don't quote me on this, but it's probably a no-go to look at the value-prop for controlled components. Batching/deferring means that the value-prop might not actually be the last value, only a previous value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can only be applied to controlled components, not uncontrolled.

right, sorry I meant correct for just controlled values.

On that point my initial worry seems to be partly true (did a quick fiddle), starting an input uncontrolled, and switching to controlled will confuse "this._lastValue" unless you are also watching for the switch in componentWillRecieveProps that being said, I am not sure that situation can be used to bypass the issue b/c we are specifically checking for empty to empty values

'' + controlledValue :
this._uncontrolledValue;

this._uncontrolledValue = event.target.value;

if ( event.target.value === '' && lastValue === '') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove space after opening paren

return returnValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fragile considering there's lots of code beneath this that may or may not need to be executed, when all this really wants to do is discard the call to onChange right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, honestly i returned returnValue just to appease eslint's consistent-return rule, originally it was just a lone return, which would be less prone to accidental refactoring breakage. Alternatively I could just wrap the whole thing in a big if/else or return undefined perhaps? Not sure how to best handle the lint rule :P

}
}

if (onChange) {
returnValue = onChange.call(this, event);
}
Expand Down
37 changes: 37 additions & 0 deletions src/browser/ui/dom/components/ReactDOMTextarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin');
var ReactClass = require('ReactClass');
var ReactElement = require('ReactElement');
var ReactUpdates = require('ReactUpdates');
var ExecutionEnvironment = require('ExecutionEnvironment');

var assign = require('Object.assign');
var findDOMNode = require('findDOMNode');
Expand All @@ -26,6 +27,11 @@ var invariant = require('invariant');
var warning = require('warning');

var textarea = ReactElement.createFactory('textarea');
var hasNoisyInputEvent = false;

if (ExecutionEnvironment.canUseDOM) {
hasNoisyInputEvent = 'documentMode' in document && document.documentMode > 9;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: couldn't you just use: hasNoisyInputEvent = document.documentMode > 9; since undefined > 9 will be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, though the current way is consistent with similar checks elsewhere in the code base

}

function forceUpdateIfMounted() {
/*jshint validthis:true */
Expand All @@ -34,6 +40,13 @@ function forceUpdateIfMounted() {
}
}

function isIEInputEvent(event) {
return (
hasNoisyInputEvent &&
event.nativeEvent.type === 'input'
);
}

/**
* Implements a <textarea> native component that allows setting `value`, and
* `defaultValue`. This differs from the traditional DOM API because value is
Expand All @@ -55,6 +68,12 @@ var ReactDOMTextarea = ReactClass.createClass({

mixins: [AutoFocusMixin, LinkedValueUtils.Mixin, ReactBrowserComponentMixin],

componentWillMount: function() {
var defaultValue = this.props.defaultValue;

this._uncontrolledValue = defaultValue != null ? '' + defaultValue : '';
},

getInitialState: function() {
var defaultValue = this.props.defaultValue;
// TODO (yungsters): Remove support for children content in <textarea>.
Expand Down Expand Up @@ -125,6 +144,24 @@ var ReactDOMTextarea = ReactClass.createClass({
_handleChange: function(event) {
var returnValue;
var onChange = LinkedValueUtils.getOnChange(this.props);

// IE 10+ fire input events when setting/unsetting a placeholder
// we guard against it by checking if the next and
// last values are both empty and bailing out of the change
// https://github.com/facebook/react/issues/3484
if (isIEInputEvent(event)) {
var controlledValue = LinkedValueUtils.getValue(this.props);
var lastValue = controlledValue != null ?
'' + controlledValue :
this._uncontrolledValue;

this._uncontrolledValue = event.target.value;

if ( event.target.value === '' && lastValue === '') {
return returnValue;
}
}

if (onChange) {
returnValue = onChange.call(this, event);
}
Expand Down
16 changes: 16 additions & 0 deletions src/browser/ui/dom/components/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,4 +348,20 @@ describe('ReactDOMInput', function() {
<input type="checkbox" checkedLink={link} valueLink={emptyFunction} />;
expect(React.render.bind(React, instance, node)).toThrow();
});

it('should not fire change handlers when setting placeholders', function() {
var change = mocks.getMockFunction();
var instance = <input type="text" onChange={change} placeholder={'test'}/>;

instance = ReactTestUtils.renderIntoDocument(instance);

React.findDOMNode(instance).focus()

instance.replaceProps({ type: "text", onChange: change });

expect(change.mock.calls.length).toBe(0);

ReactTestUtils.Simulate.change(React.findDOMNode(instance));
expect(change.mock.calls.length).toBe(1);
});
});
16 changes: 16 additions & 0 deletions src/browser/ui/dom/components/__tests__/ReactDOMTextarea-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,20 @@ describe('ReactDOMTextarea', function() {
expect(link.requestChange.mock.calls.length).toBe(1);
expect(link.requestChange.mock.calls[0][0]).toEqual('test');
});

it('should not fire change handlers when setting placeholders', function() {
var change = mocks.getMockFunction();
var instance = <textarea onChange={change} placeholder={'test'}/>;

instance = ReactTestUtils.renderIntoDocument(instance);

React.findDOMNode(instance).focus()

instance.replaceProps({ onChange: change });

expect(change.mock.calls.length).toBe(0);

ReactTestUtils.Simulate.change(React.findDOMNode(instance));
expect(change.mock.calls.length).toBe(1);
});
});