Skip to content

Commit

Permalink
Move mouse event disabling on interactive elements to SimpleEventPlug…
Browse files Browse the repository at this point in the history
…in. Related perf tweak to click handlers. (#7642)

* Cull disabled mouse events at plugin level. Remove component level filters

* DisabledInputUtils tests are now for SimpleEventPlugin

* Add click bubbling test

* Add isInteractive function. Use in iOS click exception rules

* Invert interactive check in local click listener. Add test coverage

* Reduce number of mouse events disabable. Formatting in isIteractive()

* Switch isInteractive tag order for alignment

* Update formatting of isInteractive method
  • Loading branch information
nhunzaker authored and sophiebits committed Sep 8, 2016
1 parent df03318 commit 73c50e7
Show file tree
Hide file tree
Showing 8 changed files with 91 additions and 104 deletions.
33 changes: 29 additions & 4 deletions src/renderers/dom/client/eventPlugins/SimpleEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,25 @@ function getDictionaryKey(inst: ReactInstance): string {
return '.' + inst._rootNodeID;
}

function isInteractive(tag) {
return (
tag === 'button' || tag === 'input' ||
tag === 'select' || tag === 'textarea'
);
}

function shouldPreventMouseEvent(inst) {
if (inst) {
var disabled = inst._currentElement && inst._currentElement.props.disabled;

if (disabled) {
return isInteractive(inst._tag);
}
}

return false;
}

var SimpleEventPlugin: PluginModule<MouseEvent> = {

eventTypes: eventTypes,
Expand Down Expand Up @@ -217,13 +236,18 @@ var SimpleEventPlugin: PluginModule<MouseEvent> = {
return null;
}
/* falls through */
case 'topContextMenu':
case 'topDoubleClick':
case 'topMouseDown':
case 'topMouseMove':
case 'topMouseUp':
// Disabled elements should not respond to mouse events
if (shouldPreventMouseEvent(targetInst)) {
return null;
}
/* falls through */
case 'topMouseOut':
case 'topMouseOver':
case 'topMouseUp':
case 'topContextMenu':
EventConstructor = SyntheticMouseEvent;
break;
case 'topDrag':
Expand Down Expand Up @@ -286,7 +310,8 @@ var SimpleEventPlugin: PluginModule<MouseEvent> = {
// non-interactive elements, which means delegated click listeners do not
// fire. The workaround for this bug involves attaching an empty click
// listener on the target node.
if (registrationName === 'onClick') {
// http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html
if (registrationName === 'onClick' && !isInteractive(inst._tag)) {
var key = getDictionaryKey(inst);
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
if (!onClickListeners[key]) {
Expand All @@ -303,7 +328,7 @@ var SimpleEventPlugin: PluginModule<MouseEvent> = {
inst: ReactInstance,
registrationName: string,
): void {
if (registrationName === 'onClick') {
if (registrationName === 'onClick' && !isInteractive(inst._tag)) {
var key = getDictionaryKey(inst);
onClickListeners[key].remove();
delete onClickListeners[key];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,22 @@
'use strict';


describe('DisabledInputUtils', () => {
describe('SimpleEventPlugin', function() {
var React;
var ReactDOM;
var ReactTestUtils;

var elements = ['button', 'input', 'select', 'textarea'];
var onClick = jest.fn();

function expectClickThru(element) {
onClick.mockClear();
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(element));
ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(element));
expect(onClick.mock.calls.length).toBe(1);
}

function expectNoClickThru(element) {
onClick.mockClear();
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(element));
ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(element));
expect(onClick.mock.calls.length).toBe(0);
}

Expand All @@ -36,17 +36,31 @@ describe('DisabledInputUtils', () => {
return element;
}

var onClick = jest.fn();
beforeEach(function() {
React = require('React');
ReactDOM = require('ReactDOM');
ReactTestUtils = require('ReactTestUtils');
});

elements.forEach(function(tagName) {
it('A non-interactive tags click when disabled', function() {
var element = (<div onClick={ onClick } />);
expectClickThru(mounted(element));
});

describe(tagName, () => {
it('A non-interactive tags clicks bubble when disabled', function() {
var element = ReactTestUtils.renderIntoDocument(
<div onClick={onClick}><div /></div>
);
var child = ReactDOM.findDOMNode(element).firstChild;

beforeEach(() => {
React = require('React');
ReactDOM = require('ReactDOM');
ReactTestUtils = require('ReactTestUtils');
});
onClick.mockClear();
ReactTestUtils.SimulateNative.click(child);
expect(onClick.mock.calls.length).toBe(1);
});

['button', 'input', 'select', 'textarea'].forEach(function(tagName) {

describe(tagName, function() {

it('should forward clicks when it starts out not disabled', () => {
var element = React.createElement(tagName, {
Expand Down Expand Up @@ -105,4 +119,37 @@ describe('DisabledInputUtils', () => {
});
});
});


describe('iOS bubbling click fix', function() {
// See http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html

beforeEach(function() {
onClick.mockClear();
});

it ('does not add a local click to interactive elements', function() {
var container = document.createElement('div');

ReactDOM.render(<button onClick={ onClick }></button>, container);

var node = container.firstChild;

node.dispatchEvent(new MouseEvent('click'));

expect(onClick.mock.calls.length).toBe(0);
});

it ('adds a local click listener to non-interactive elements', function() {
var container = document.createElement('div');

ReactDOM.render(<div onClick={ onClick }></div>, container);

var node = container.firstChild;

node.dispatchEvent(new MouseEvent('click'));

expect(onClick.mock.calls.length).toBe(0);
});
});
});
50 changes: 0 additions & 50 deletions src/renderers/dom/client/wrappers/DisabledInputUtils.js

This file was deleted.

24 changes: 0 additions & 24 deletions src/renderers/dom/client/wrappers/ReactDOMButton.js

This file was deleted.

3 changes: 1 addition & 2 deletions src/renderers/dom/client/wrappers/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var DisabledInputUtils = require('DisabledInputUtils');
var DOMPropertyOperations = require('DOMPropertyOperations');
var LinkedValueUtils = require('LinkedValueUtils');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
Expand Down Expand Up @@ -71,7 +70,7 @@ var ReactDOMInput = {
// in corner cases such as min or max deriving from value, e.g. Issue #7170)
min: undefined,
max: undefined,
}, DisabledInputUtils.getHostProps(inst, props), {
}, props, {
defaultChecked: undefined,
defaultValue: undefined,
value: value != null ? value : inst._wrapperState.initialValue,
Expand Down
3 changes: 1 addition & 2 deletions src/renderers/dom/client/wrappers/ReactDOMSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var DisabledInputUtils = require('DisabledInputUtils');
var LinkedValueUtils = require('LinkedValueUtils');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactUpdates = require('ReactUpdates');
Expand Down Expand Up @@ -146,7 +145,7 @@ function updateOptions(inst, multiple, propValue) {
*/
var ReactDOMSelect = {
getHostProps: function(inst, props) {
return Object.assign({}, DisabledInputUtils.getHostProps(inst, props), {
return Object.assign({}, props, {
onChange: inst._wrapperState.onChange,
value: undefined,
});
Expand Down
3 changes: 1 addition & 2 deletions src/renderers/dom/client/wrappers/ReactDOMTextarea.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

'use strict';

var DisabledInputUtils = require('DisabledInputUtils');
var LinkedValueUtils = require('LinkedValueUtils');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactUpdates = require('ReactUpdates');
Expand Down Expand Up @@ -56,7 +55,7 @@ var ReactDOMTextarea = {
// to only set the value if/when the value differs from the node value (which would
// completely solve this IE9 bug), but Sebastian+Ben seemed to like this solution.
// The value can be a boolean or object so that's why it's forced to be a string.
var hostProps = Object.assign({}, DisabledInputUtils.getHostProps(inst, props), {
var hostProps = Object.assign({}, props, {
value: undefined,
defaultValue: undefined,
children: '' + inst._wrapperState.initialValue,
Expand Down
8 changes: 0 additions & 8 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ var DOMPropertyOperations = require('DOMPropertyOperations');
var EventPluginHub = require('EventPluginHub');
var EventPluginRegistry = require('EventPluginRegistry');
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactDOMButton = require('ReactDOMButton');
var ReactDOMComponentFlags = require('ReactDOMComponentFlags');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactDOMInput = require('ReactDOMInput');
Expand Down Expand Up @@ -542,9 +541,6 @@ ReactDOMComponent.Mixin = {
};
transaction.getReactMountReady().enqueue(trapBubbledEventsLocal, this);
break;
case 'button':
props = ReactDOMButton.getHostProps(this, props, hostParent);
break;
case 'input':
ReactDOMInput.mountWrapper(this, props, hostParent);
props = ReactDOMInput.getHostProps(this, props);
Expand Down Expand Up @@ -887,10 +883,6 @@ ReactDOMComponent.Mixin = {
var nextProps = this._currentElement.props;

switch (this._tag) {
case 'button':
lastProps = ReactDOMButton.getHostProps(this, lastProps);
nextProps = ReactDOMButton.getHostProps(this, nextProps);
break;
case 'input':
lastProps = ReactDOMInput.getHostProps(this, lastProps);
nextProps = ReactDOMInput.getHostProps(this, nextProps);
Expand Down

0 comments on commit 73c50e7

Please sign in to comment.