fix(jqLite): deregister special mouseenter / mouseleave events correctly

[petebacondarwin]

An alternative solution...

Closes #12795
Closes #12799

[gkalpak]

Why only when !isDefined(fn) ? The previous implementation would remove the native listener when the last handle listenerFn was unregistered. Why prevent that ?

[petebacondarwin]

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 event
• fn is defined AND there are no more items in the listenerFns array

[gkalpak]

Right !

[gkalpak]

From a quick look, it LGTM.
Will take a more careful look and try some more tests tomorrow.

👍

[gkalpak]

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 mouseenter and mouseover, then the callback is executed twice upon mouseover. (The same is true for mouseleave/mouseout.)

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();
      }
    );

[gkalpak]

Other than the above comment, it LGTM.

[petebacondarwin]

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.

[gkalpak]

If you add handlers to both the mouseenter and mouseover events then it is reasonable to expect to calls.

Fair enough :)

New concern:
The previous implementation didn't actually add an event-listener for mouseenter/mouseleave events.

This new implementation does add event-listeners for both mouseenter/mouseleave and mouseover/mouseout. This will result in calling the same listener twice (although the user registered it once), when both mouseenter and mouseover are triggered.

[gkalpak]

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.

[petebacondarwin]

@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.

[gkalpak]

I think we need an additional (events[type] || (events[type] = [])).push(fn); (or equivalent) to also support "manual" triggering.

[gkalpak]

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 mouseenter/leave.
At least I think so (I haven't tried it).

See, https://github.com/angular/angular.js/pull/13230/files#r44645939 for a possible fix.