Skip to content

Commit

Permalink
Event: Use one native handler per jQuery handler
Browse files Browse the repository at this point in the history 
Some tests are still failing, need to fix propagation issues
  • Loading branch information
@dmethvin
dmethvin committed May 24, 2016
1 parent 63a303f commit 113ce67
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 87 deletions.
138 changes: 58 additions & 80 deletions src/event.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,26 @@ jQuery.event = {

add: function( elem, types, handler, data, selector ) {

var handleObjIn, eventHandle, tmp,
var handleObjIn, tmp,
events, t, handleObj,
special, handlers, type, namespaces, origType,
elemData = dataPriv.get( elem );
elemData = dataPriv.get( elem ),
handleEventCommon = function( e ) {

// `this` for handleEvent is the handleObj
// Discard the second event of a jQuery.event.trigger() and
// when an event is called after a page has unloaded
return typeof jQuery !== "undefined" && jQuery.event.triggered !== e.type ?
jQuery.event.dispatch.apply( this, arguments ) : undefined;
};

// Don't attach events to noData or text/comment nodes (but allow plain objects)
if ( !elemData ) {
return;
}

// Caller can pass in an object of custom data in lieu of the handler
// (internal, only used by manipulation.js/cloneCopyEvent)
if ( handler.handler ) {
handleObjIn = handler;
handler = handleObjIn.handler;
Expand All @@ -136,15 +145,6 @@ jQuery.event = {
if ( !( events = elemData.events ) ) {
events = elemData.events = {};
}
if ( !( eventHandle = elemData.handle ) ) {
eventHandle = elemData.handle = function( e ) {

// Discard the second event of a jQuery.event.trigger() and
// when an event is called after a page has unloaded
return typeof jQuery !== "undefined" && jQuery.event.triggered !== e.type ?
jQuery.event.dispatch.apply( elem, arguments ) : undefined;
};
}

// Handle multiple events separated by a space
types = ( types || "" ).match( rnotwhite ) || [ "" ];
Expand Down Expand Up @@ -177,8 +177,9 @@ jQuery.event = {
guid: handler.guid,
selector: selector,
needsContext: selector && jQuery.expr.match.needsContext.test( selector ),
namespace: namespaces.join( "." )
}, handleObjIn );
namespace: namespaces.join( "." ),
handleEvent: handleEventCommon
}, handleObjIn, { elem: elem } );

// Init the event handler queue if we're the first
if ( !( handlers = events[ type ] ) ) {
Expand All @@ -187,17 +188,18 @@ jQuery.event = {

// Only use addEventListener if the special events handler returns false
if ( !special.setup ||
special.setup.call( elem, data, namespaces, eventHandle ) === false ) {

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016

Author Owner

This is a pretty serious breakage of the jQuery.event.special hooks contract details but I'm not sure what to do about it. We leaked the implementation detail about the shared single handler to them and we can't easily maintain that mirage now. As long as the hook doesn't use eventHandle though it should work.
special.setup.call( elem, data, namespaces, handleObj ) === false ) {

if ( elem.addEventListener ) {
elem.addEventListener( type, eventHandle );
elem.addEventListener( type, handleObj );
}
}
}

if ( special.add ) {
special.add.call( elem, handleObj );

// If special hooks changed the handler, make sure it has the right guid
if ( !handleObj.handler.guid ) {
handleObj.handler.guid = handler.guid;
}
Expand All @@ -219,7 +221,7 @@ jQuery.event = {
// Detach an event or set of events from an element
remove: function( elem, types, handler, selector, mappedTypes ) {

var j, origCount, tmp,
var j, tmp,
events, t, handleObj,
special, handlers, type, namespaces, origType,
elemData = dataPriv.hasData( elem ) && dataPriv.get( elem );
Expand Down Expand Up @@ -251,7 +253,7 @@ jQuery.event = {
new RegExp( "(^|\\.)" + namespaces.join( "\\.(?:.*\\.|)" ) + "(\\.|$)" );

// Remove matching events
origCount = j = handlers.length;
j = handlers.length;
while ( j-- ) {
handleObj = handlers[ j ];

Expand All @@ -268,25 +270,21 @@ jQuery.event = {
if ( special.remove ) {
special.remove.call( elem, handleObj );
}
if ( !special.teardown ||
special.teardown.call( elem, namespaces ) === false ) {
jQuery.removeEvent( elem, type, handleObj );
}
}
}

// Remove generic event handler if we removed something and no more handlers exist
// (avoids potential for endless recursion during removal of special event handlers)
if ( origCount && !handlers.length ) {
if ( !special.teardown ||
special.teardown.call( elem, namespaces, elemData.handle ) === false ) {

jQuery.removeEvent( elem, type, elemData.handle );
}

if ( !handlers.length ) {
delete events[ type ];
}
}

// Remove data and the expando if it's no longer used
if ( jQuery.isEmptyObject( events ) ) {
dataPriv.remove( elem, "handle events" );
dataPriv.remove( elem, "events" );
}
},

Expand All @@ -295,9 +293,10 @@ jQuery.event = {
// Make a writable jQuery.Event from the native event object
var event = jQuery.event.fix( nativeEvent );

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016

Author Owner

When we multiplexed handlers we got the economy of a single jQuery.event.fix() call for all the handlers on an element. That's gone away but I hope the next step is to look at using a native event. Plus the new v3 code is much faster anyway.

var i, j, ret, matched, handleObj, handlerQueue,
var i, ret, match, matches,
args = new Array( arguments.length ),
handlers = ( dataPriv.get( this, "events" ) || {} )[ event.type ] || [],

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016 •
edited

Author Owner

We're using the .addEventListener(type, object) form now so there is no need to do the data lookup! The this is the handleObj.

BUG: We still need to move delegated events to the front of the queue so they can run before any non-delegated handlers and their "virtual" stopPropagation() works as before. Doing this is likely to make the event bookkeeping and delivery more complex but I'm still hoping to avoid the need to do the data lookup for most cases.
handleObj = this,
elem = this.elem,
special = jQuery.event.special[ event.type ] || {};

// Use the fix-ed jQuery.Event rather than the (read-only) native event
Expand All @@ -307,57 +306,50 @@ jQuery.event = {
args[ i ] = arguments[ i ];
}

event.delegateTarget = this;
event.delegateTarget = elem;

// Call the preDispatch hook for the mapped type, and let it bail if desired
if ( special.preDispatch && special.preDispatch.call( this, event ) === false ) {
if ( special.preDispatch && special.preDispatch.call( elem, event ) === false ) {
return;
}

// Determine handlers
handlerQueue = jQuery.event.handlers.call( this, event, handlers );
// Determine elements this event should target
matches = jQuery.event.handlers.call( elem, event, handleObj );

// Run delegates first; they may want to stop propagation beneath us
i = 0;
while ( ( matched = handlerQueue[ i++ ] ) && !event.isPropagationStopped() ) {
event.currentTarget = matched.elem;
while ( ( match = matches[ i++ ] ) && !event.isPropagationStopped() ) {
event.currentTarget = match;

j = 0;
while ( ( handleObj = matched.handlers[ j++ ] ) &&
!event.isImmediatePropagationStopped() ) {
// Triggered event must either 1) have no namespace, or 2) have namespace(s)
// a subset or equal to those in the bound event (both can have no namespace).
if ( !event.rnamespace || event.rnamespace.test( handleObj.namespace ) ) {

// Triggered event must either 1) have no namespace, or 2) have namespace(s)
// a subset or equal to those in the bound event (both can have no namespace).
if ( !event.rnamespace || event.rnamespace.test( handleObj.namespace ) ) {
event.handleObj = handleObj;
event.data = handleObj.data;

event.handleObj = handleObj;
event.data = handleObj.data;
ret = ( ( jQuery.event.special[ handleObj.origType ] || {} ).handle ||
handleObj.handler ).apply( match, args );

ret = ( ( jQuery.event.special[ handleObj.origType ] || {} ).handle ||
handleObj.handler ).apply( matched.elem, args );

if ( ret !== undefined ) {
if ( ( event.result = ret ) === false ) {
event.preventDefault();
event.stopPropagation();
}
if ( ret !== undefined ) {
if ( ( event.result = ret ) === false ) {
event.preventDefault();
event.stopPropagation();
}
}
}
}

// Call the postDispatch hook for the mapped type
if ( special.postDispatch ) {
special.postDispatch.call( this, event );
special.postDispatch.call( elem, event );
}

return event.result;
},

handlers: function( event, handlers ) {
var i, matches, sel, handleObj,
handlerQueue = [],
delegateCount = handlers.delegateCount,
handlers: function( event, handleObj ) {
var matches = [],
cur = event.target;

// Support: IE <=9
Expand All @@ -366,43 +358,29 @@ jQuery.event = {
//
// Support: Firefox <=42
// Avoid non-left-click in FF but don't block IE radio events (#3861, gh-2343)
if ( delegateCount && cur.nodeType &&
if ( handleObj.selector && cur.nodeType &&

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016 •
edited

Author Owner

Each dispatch only runs on one element, but for the delegated event case we may match multiple elements below us.
( event.type !== "click" || isNaN( event.button ) || event.button < 1 ) ) {

for ( ; cur !== this; cur = cur.parentNode || this ) {
for ( ; cur !== handleObj.elem; cur = cur.parentNode || this ) {

// Don't check non-elements (#13208)
// Don't process clicks on disabled elements (#6911, #8165, #11382, #11764)
if ( cur.nodeType === 1 && ( cur.disabled !== true || event.type !== "click" ) ) {
matches = [];
for ( i = 0; i < delegateCount; i++ ) {
handleObj = handlers[ i ];

// Don't conflict with Object.prototype properties (#13203)
sel = handleObj.selector + " ";

if ( matches[ sel ] === undefined ) {
matches[ sel ] = handleObj.needsContext ?
jQuery( sel, this ).index( cur ) > -1 :
jQuery.find( sel, this, null, [ cur ] ).length;
}
if ( matches[ sel ] ) {
matches.push( handleObj );
}
}
if ( matches.length ) {
handlerQueue.push( { elem: cur, handlers: matches } );
if ( handleObj.needsContext ?
jQuery( handleObj.selector, this ).index( cur ) > -1 :
jQuery.find( handleObj.selector, this, null, [ cur ] ).length ) {
matches.push( cur );
}
}
}
}

// Add the remaining (directly-bound) handlers
if ( delegateCount < handlers.length ) {
handlerQueue.push( { elem: this, handlers: handlers.slice( delegateCount ) } );
// Directly bound handlers
else {
matches.push( handleObj.elem );
}

return handlerQueue;
return matches;
},

addProp: function( name, hook ) {
Expand Down
11 changes: 6 additions & 5 deletions src/event/trigger.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jQuery.extend( jQuery.event, {

trigger: function( event, data, elem, onlyHandlers ) {

var i, cur, tmp, bubbleType, ontype, handle, special,
var i, j, cur, tmp, bubbleType, ontype, handle, special,
eventPath = [ elem || document ],
type = hasOwn.call( event, "type" ) ? event.type : event,
namespaces = hasOwn.call( event, "namespace" ) ? event.namespace.split( "." ) : [];
Expand Down Expand Up @@ -98,11 +98,12 @@ jQuery.extend( jQuery.event, {
bubbleType :
special.bindType || type;

// jQuery handler
handle = ( dataPriv.get( cur, "events" ) || {} )[ event.type ] &&
dataPriv.get( cur, "handle" );
// jQuery handlers
handle = ( dataPriv.get( cur, "events" ) || {} )[ event.type ];
if ( handle ) {
handle.apply( cur, data );
for ( j = 0; j < handle.length; j++ ) {

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016 •
edited

Author Owner

Since there isn't a common event handler we have to trigger each event handler individually.

This comment has been minimized.

Sign in to view

Copy link
@mgol

mgol May 25, 2016

Is there any chance we could be using the native dispatchEvent now?

I guess the answer's no because of the delegated events support?

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 25, 2016

Author Owner

It will introduce a bunch more breaking changes but for v4 I'd like to do that.
handle[ j ].handleEvent.apply( handle[ j ], data );
}
}

// Native handler
Expand Down
1 change: 0 additions & 1 deletion src/manipulation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ function cloneCopyEvent( src, dest ) {
events = pdataOld.events;

if ( events ) {
delete pdataCur.handle;

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016

Author Owner

No common event handler anymore
pdataCur.events = {};

for ( type in events ) {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/event.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ QUnit.test( "on(), multiple events at once and namespaces", function( assert ) {
} );

QUnit.test( "on(), namespace with special add", function( assert ) {
assert.expect( 27 );

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin May 24, 2016

Author Owner

Now there is a teardown for every remove since it's 1:1.
assert.expect( 29 );

var i = 0,
div = jQuery( "<div/>" ).appendTo( "#qunit-fixture" ).on( "test", function() {
Expand Down

0 comments on commit 113ce67

Please sign in to comment.