-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix(jqLite): deregister special mouseenter
/ mouseleave
events correctly
#13230
Conversation
} | ||
if (!(isDefined(fn) && listenerFns && listenerFns.length > 0)) { |
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.
Why only when !isDefined(fn)
? The previous implementation would remove the native listener when the last handle listenerFn was unregistered. Why prevent that ?
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.
I think that this construct is correct, since we call removeEventListenerFn
in the following cases:
fn
is not defined - i.e. we want to remove all handlers for this eventfn
is defined AND there are no more items in thelistenerFns
array
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.
Right !
From a quick look, it LGTM. 👍 |
Thinking about this, I realized there is the following issue (not introduced by this PR, but might be worth solving as part of it): If the user registers the same callback for Here are a couple of tests I ran, if you want to add them to the suite: // SUCCESS
it('should deregister a mouseenter/leave listener that is also registered for mouseover/out',
function() {
var aElem = jqLite(a);
var onMouseenter = jasmine.createSpy('onMouseenter');
var onMouseleave = jasmine.createSpy('onMouseleave');
aElem.on('mouseenter', onMouseenter);
aElem.on('mouseleave', onMouseleave);
aElem.off('mouseenter', onMouseenter);
aElem.off('mouseleave', onMouseleave);
aElem.on('mouseover', onMouseenter);
aElem.on('mouseout', onMouseleave);
browserTrigger(a, 'mouseover', {relatedTarget: b});
expect(onMouseenter).toHaveBeenCalledOnce();
browserTrigger(a, 'mouseout', {relatedTarget: b});
expect(onMouseleave).toHaveBeenCalledOnce();
}
);
// SUCCESS
it('should deregister specific listener within the listener and call subsequent listeners', function() {
var aElem = jqLite(a),
clickSpy = jasmine.createSpy('click'),
clickOnceSpy = jasmine.createSpy('clickOnce').andCallFake(function() {
aElem.off('click', clickOnceSpy);
});
aElem.on('click', clickOnceSpy);
aElem.on('click', clickSpy);
browserTrigger(a, 'click');
expect(clickOnceSpy).toHaveBeenCalledOnce();
expect(clickSpy).toHaveBeenCalledOnce();
browserTrigger(a, 'click');
expect(clickOnceSpy).toHaveBeenCalledOnce();
expect(clickSpy.callCount).toBe(2);
});
// FAILURE
it('should call a mouseenter/leave listener that is also registered for mouseover/out once',
function() {
var aElem = jqLite(a);
var onMouseenter = jasmine.createSpy('onMouseenter');
var onMouseleave = jasmine.createSpy('onMouseleave');
aElem.on('mouseenter', onMouseenter);
aElem.on('mouseleave', onMouseleave);
aElem.on('mouseover', onMouseenter);
aElem.on('mouseout', onMouseleave);
browserTrigger(a, 'mouseover', {relatedTarget: b});
expect(onMouseenter).toHaveBeenCalledOnce();
browserTrigger(a, 'mouseout', {relatedTarget: b});
expect(onMouseleave).toHaveBeenCalledOnce();
}
); |
Other than the above comment, it LGTM. |
Isn't that effectively by design? The point is that we can't rely on the mouseenter event so we react to the mouseover event instead. If you add handlers to both the mouseenter and mouseover events then it is reasonable to expect to calls. |
Fair enough :) New concern: This new implementation does add event-listeners for both |
Basically, that's the test I have in mind: it('should call a `mouseenter/leave` listener once when `mouseenter/leave` and `mouseover/out` '
+ 'are triggered simultaneously', function() {
var aElem = jqLite(a);
var onMouseenter = jasmine.createSpy('mouseenter');
var onMouseleave = jasmine.createSpy('mouseleave');
aElem.on('mouseenter', onMouseenter);
aElem.on('mouseleave', onMouseleave);
browserTrigger(a, 'mouseenter', {relatedTarget: b});
browserTrigger(a, 'mouseover', {relatedTarget: b});
expect(onMouseenter).toHaveBeenCalledOnce();
browserTrigger(a, 'mouseleave', {relatedTarget: b});
browserTrigger(a, 'mouseout', {relatedTarget: b});
expect(onMouseleave).toHaveBeenCalledOnce();
}); It does indeed pass with the previous implementation, but fails with the new one. |
f5fe4d0
to
9b6f6e6
Compare
@gkalpak I have updated the PR with this test and the fix. It passes both on jqlite and jquery so I think we are good to go. |
while (i--) { | ||
type = types[i]; | ||
if (MOUSE_EVENT_MAP[type]) { | ||
addHandler(MOUSE_EVENT_MAP[type], specialMouseHandlerWrapper); |
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.
I think we need an additional (events[type] || (events[type] = [])).push(fn);
(or equivalent) to also support "manual" triggering.
But now it fails the following test: it('should call a `mouseenter/leave` listener when manually triggering the event', function() {
var aElem = jqLite(a);
var onMouseenter = jasmine.createSpy('mouseenter');
var onMouseleave = jasmine.createSpy('mouseleave');
aElem.on('mouseenter', onMouseenter);
aElem.on('mouseleave', onMouseleave);
a.triggerHandler('mouseenter');
expect(onMouseenter).toHaveBeenCalledOnce();
a.triggerHandler('mouseleave');
expect(onMouseleave).toHaveBeenCalledOnce();
}); Basically, it doesn't call the handler, when "manually" triggering See, https://github.com/angular/angular.js/pull/13230/files#r44645939 for a possible fix. |
a16b7df
to
f5aa207
Compare
An alternative solution...
Closes #12795
Closes #12799