-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(gesture): Implement proper event dispatching when using JQuery #1367
Conversation
@@ -361,10 +361,11 @@ angular.module('material.core') | |||
bubbles: true, | |||
cancelable: true | |||
}; | |||
|
|||
/* | |||
* NOTE: dispatchEvent is very performance sensitive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here probably needs to be changed as well
OR
I'm not sure that the name needs to specifically contain jqLite or if it would be enough to just add that to the comment here.
LGTM! Thanks! |
@@ -302,7 +302,7 @@ angular.module('material.core') | |||
onCancel: angular.noop, | |||
options: {}, | |||
|
|||
dispatchEvent: dispatchEvent, | |||
dispatchEvent: typeof jQuery !== 'undefined' && angular.element === jQuery ? jQueryDispatchEvent : nativeDispatchEvent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware that this will probably break (in certain cases) once angular/angular.js#10761 lands (and I believe it's pretty close to landing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we would need to check if jQuery object is really jQuery, but it should work even after merge from what I can read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been merged, btw. The problem is that typeof jQuery !== 'undefined' && angular.element === jQuery
might evaluate to false (thus using nativeDispatchEvent
although jQuery is used (leading to the wrong behaviour again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, jQuery may be defined as another name in global namespace, and then properly register using the new ngJq
directive.
Maybe we should only check if angular.element
has trigger
function ? jqLite doesn't have this function.
https://github.com/angular/angular.js/blob/master/src/Angular.js#L1532-L1536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rschmukler - Can you follow-up on this issue?
Merged, with a small addition to be sure jQuery users get metaKey/ctrlKey/ etc: 88813b7#diff-a82fbe888db156ef2e09e7fcfb85f189R370 Thanks. |
Close #1359
For on and off, jqLite use native events, but JQuery doesn't . So to trigger events properly when JQuery is loaded, the JQuery trigger method must be used instead of native dispatchEvent method.