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

Revert "Revert #1536" #3246

Merged
merged 2 commits into from
Mar 10, 2015
Merged

Revert "Revert #1536" #3246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
090e4bb
Revert "Revert #1536"
sophiebits Feb 23, 2015
1943a9a
Only call put/deleteListener with truthy listeners
sophiebits Feb 23, 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
3 changes: 1 addition & 2 deletions src/browser/eventPlugins/DefaultEventPluginOrder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ var DefaultEventPluginOrder = [
keyOf({ChangeEventPlugin: null}),
keyOf({SelectEventPlugin: null}),
keyOf({BeforeInputEventPlugin: null}),
keyOf({AnalyticsEventPlugin: null}),
keyOf({MobileSafariClickEventPlugin: null})
keyOf({AnalyticsEventPlugin: null})
];

module.exports = DefaultEventPluginOrder;
56 changes: 0 additions & 56 deletions src/browser/eventPlugins/MobileSafariClickEventPlugin.js
View file
Open in desktop

This file was deleted.

31 changes: 30 additions & 1 deletion src/browser/eventPlugins/SimpleEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
'use strict';

var EventConstants = require('EventConstants');
var EventListener = require('EventListener');
var EventPluginUtils = require('EventPluginUtils');
var EventPropagators = require('EventPropagators');
var ReactMount = require('ReactMount');
var SyntheticClipboardEvent = require('SyntheticClipboardEvent');
var SyntheticEvent = require('SyntheticEvent');
var SyntheticFocusEvent = require('SyntheticFocusEvent');
Expand All @@ -24,8 +26,8 @@ var SyntheticTouchEvent = require('SyntheticTouchEvent');
var SyntheticUIEvent = require('SyntheticUIEvent');
var SyntheticWheelEvent = require('SyntheticWheelEvent');

var emptyFunction = require('emptyFunction');
var getEventCharCode = require('getEventCharCode');

var invariant = require('invariant');
var keyOf = require('keyOf');
var warning = require('warning');
Expand Down Expand Up @@ -289,6 +291,9 @@ for (var type in topLevelEventsToDispatchConfig) {
topLevelEventsToDispatchConfig[type].dependencies = [type];
}

var ON_CLICK_KEY = keyOf({onClick: null});
var onClickListeners = {};

var SimpleEventPlugin = {

eventTypes: eventTypes,
Expand Down Expand Up @@ -417,6 +422,30 @@ var SimpleEventPlugin = {
);
EventPropagators.accumulateTwoPhaseDispatches(event);
return event;
},

didPutListener: function(id, registrationName, listener) {
// Mobile Safari does not fire properly bubble click events on
// 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 === ON_CLICK_KEY) {
var node = ReactMount.getNode(id);
if (!onClickListeners[id]) {
onClickListeners[id] = EventListener.listen(
node,
'click',
emptyFunction
);
}
}
},

willDeleteListener: function(id, registrationName) {
if (registrationName === ON_CLICK_KEY) {
onClickListeners[id].remove();
delete onClickListeners[id];
}
}

};
Expand Down
13 changes: 11 additions & 2 deletions src/browser/ui/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,12 @@ ReactDOMComponent.Mixin = {
}
}
} else if (registrationNameModules.hasOwnProperty(propKey)) {
deleteListener(this._rootNodeID, propKey);
if (lastProps[propKey]) {
// Only call deleteListener if there was a listener previously or
// else willDeleteListener gets called when there wasn't actually a
// listener (e.g., onClick={null})
deleteListener(this._rootNodeID, propKey);
}
} else if (
DOMProperty.isStandardName[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
Expand Down Expand Up @@ -395,7 +400,11 @@ ReactDOMComponent.Mixin = {
styleUpdates = nextProp;
}
} else if (registrationNameModules.hasOwnProperty(propKey)) {
putListener(this._rootNodeID, propKey, nextProp, transaction);
if (nextProp) {
putListener(this._rootNodeID, propKey, nextProp, transaction);
} else if (lastProp) {
deleteListener(this._rootNodeID, propKey);
}
} else if (
DOMProperty.isStandardName[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
Expand Down
2 changes: 0 additions & 2 deletions src/browser/ui/ReactDefaultInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ var DefaultEventPluginOrder = require('DefaultEventPluginOrder');
var EnterLeaveEventPlugin = require('EnterLeaveEventPlugin');
var ExecutionEnvironment = require('ExecutionEnvironment');
var HTMLDOMPropertyConfig = require('HTMLDOMPropertyConfig');
var MobileSafariClickEventPlugin = require('MobileSafariClickEventPlugin');
var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin');
var ReactClass = require('ReactClass');
var ReactComponentBrowserEnvironment =
Expand Down Expand Up @@ -84,7 +83,6 @@ function inject() {
SimpleEventPlugin: SimpleEventPlugin,
EnterLeaveEventPlugin: EnterLeaveEventPlugin,
ChangeEventPlugin: ChangeEventPlugin,
MobileSafariClickEventPlugin: MobileSafariClickEventPlugin,
SelectEventPlugin: SelectEventPlugin,
BeforeInputEventPlugin: BeforeInputEventPlugin
});
Expand Down
53 changes: 53 additions & 0 deletions src/browser/ui/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,59 @@ describe('ReactDOMComponent', function() {
'style={{marginRight: spacing + \'em\'}} when using JSX.'
);
});

it("should execute custom event plugin listening behavior", function() {
var React = require('React');
var SimpleEventPlugin = require('SimpleEventPlugin');

SimpleEventPlugin.didPutListener = mocks.getMockFunction();
SimpleEventPlugin.willDeleteListener = mocks.getMockFunction();

var container = document.createElement('div');
React.render(
<div onClick={() => true} />,
container
);

expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(1);

React.unmountComponentAtNode(container);

expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
});

it("should handle null and missing properly with event hooks", function() {
var React = require('React');
var SimpleEventPlugin = require('SimpleEventPlugin');

SimpleEventPlugin.didPutListener = mocks.getMockFunction();
SimpleEventPlugin.willDeleteListener = mocks.getMockFunction();
var container = document.createElement('div');

React.render(<div onClick={null} />, container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(0);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);

React.render(<div onClick={() => 'apple'} />, container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(1);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);

React.render(<div onClick={() => 'banana'} />, container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);

React.render(<div onClick={null} />, container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);

React.render(<div />, container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);

React.unmountComponentAtNode(container);
expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
});
});

describe('updateComponent', function() {
Expand Down
25 changes: 24 additions & 1 deletion src/event/EventPluginHub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,14 +147,20 @@ var EventPluginHub = {
*/
putListener: function(id, registrationName, listener) {
invariant(
!listener || typeof listener === 'function',
typeof listener === 'function',
'Expected %s listener to be a function, instead got type %s',
registrationName, typeof listener
);

var bankForRegistrationName =
listenerBank[registrationName] || (listenerBank[registrationName] = {});
bankForRegistrationName[id] = listener;

var PluginModule =
EventPluginRegistry.registrationNameModules[registrationName];
if (PluginModule && PluginModule.didPutListener) {
PluginModule.didPutListener(id, registrationName, listener);
}
},

/**
Expand All @@ -174,7 +180,14 @@ var EventPluginHub = {
* @param {string} registrationName Name of listener (e.g. `onClick`).
*/
deleteListener: function(id, registrationName) {
var PluginModule =
EventPluginRegistry.registrationNameModules[registrationName];
if (PluginModule && PluginModule.willDeleteListener) {
PluginModule.willDeleteListener(id, registrationName);
}

var bankForRegistrationName = listenerBank[registrationName];
// TODO: This should never be null -- when is it?
Copy link
Contributor

Choose a reason for hiding this comment

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

This should never be null -- when is it?

Hmm... when onClick={null} like you're trying to guard against in ReactDOMComponent.js?

What if instead of checking lastProps[propKey] in ReactDOMComponent.js, you only invoked PluginModule.willDeleteListener if !!bankForRegistrationName?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean, if bankForRegistrationName[id]? I had that originally but it seemed like willDeleteListener should also be called if you switch from a function to null, which means that EventPluginHub.putListener probably needs to delegate to EventPluginHub.deleteListener if the listener is null… but the putListener function in ReactDOMComponent also does some enqueueing so now we're having listener deletion happen at a different time depending on whether it's gone completely from props or if it's just falsy…

It seemed easier to make ReactDOMComponent call putListener/deleteListener only when an actual listener is present and have EventPluginHub not worry about falsy values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see. Agreed. Do you think we should add a warning to deleteListener, then?

Either way, this looks good to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. I didn't really change this though, just added the TODO. Will probably merge as-is and then maybe add the warning later.

if (bankForRegistrationName) {
delete bankForRegistrationName[id];
}
Expand All @@ -187,6 +200,16 @@ var EventPluginHub = {
*/
deleteAllListeners: function(id) {
for (var registrationName in listenerBank) {
if (!listenerBank[registrationName][id]) {
continue;
}

var PluginModule =
EventPluginRegistry.registrationNameModules[registrationName];
if (PluginModule && PluginModule.willDeleteListener) {
PluginModule.willDeleteListener(id, registrationName);
}

delete listenerBank[registrationName][id];
}
},
Expand Down