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 all 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
44 changes: 44 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,20 @@ var ReactDOMInput = ReactClass.createClass({

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

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

if (defaultValue == null) {
defaultValue = '';
}

this._lastValue = value != null ? value : 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 +143,22 @@ 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 lastValue = this._lastValue;

this._lastValue = 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

event.stopPropagation();
return undefined;
}
}

if (onChange) {
returnValue = onChange.call(this, event);
}
Expand Down
40 changes: 40 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,17 @@ var ReactDOMTextarea = ReactClass.createClass({

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

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

if (defaultValue == null) {
defaultValue = '';
}

this._lastValue = value != null ? value : defaultValue;
},

getInitialState: function() {
var defaultValue = this.props.defaultValue;
// TODO (yungsters): Remove support for children content in <textarea>.
Expand Down Expand Up @@ -125,6 +149,22 @@ 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 lastValue = this._lastValue;

this._lastValue = event.target.value;

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

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);
});
});