Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Strange behavior of triggerHandler in jqLite #8469

Closed
meeroslav opened this issue Aug 4, 2014 · 4 comments
Closed

Strange behavior of triggerHandler in jqLite #8469

meeroslav opened this issue Aug 4, 2014 · 4 comments

Comments

@meeroslav
Copy link

When using input file onchange functionality we can do it directly by binding function to change event:
(angular.element(input)).on("change", function (evt) {
//...use here evt.target.files
});
input.click();

Fun starts when I try to unit test it. Normally triggerHandler passes some dummy data (preventDefault and stopPropagation properties to be exact). In order to test it I need to pass some argument like this:
(angular.element(input)).triggerHandler("change", [{target: {files: fileList}]);

However triggerHandler passes in that case following data to my listenerFunction:
[{preventDefault: noop, stopPropagation: noop },{target: {files: fileList}]. Since actual image data is in second argument, listener will not pick it up.

I think error is in following line in angular.js in triggerHandler definition:
triggerHandler: function (element, eventName, eventData) {
...
eventData = eventData || [];
var event = [{
preventDefault: noop,
stopPropagation: noop
}]
forEach(eventFns, function (fn) {
fn.apply(element, event.concat(eventData));
});
}

which should be:
triggerHandler: function (element, eventName, eventData) {
...
var event = {
preventDefault: noop,
stopPropagation: noop
}
forEach(eventFns, function (fn) {
fn.apply(element, [eventData || event]);
});
}

and the call would then be:
(angular.element(input)).triggerHandler("change", {target: {files: fileList});

@petebacondarwin
Copy link
Contributor

I don't believe this is invalid behaviour. The event handler is (and should be) called with the event being the first parameter and the data being subsequent parameters. This is also the way that jQuery does it. See https://github.com/jquery/jquery/blob/master/src/event.js#L272

I have not found any examples on the web that rely on the eventData being attached to the event as properties. Most of them don't use any parameters and just go to the triggering element to get the new file information.

Can you provide information about why you expect it to behave as you suggest?

@meeroslav
Copy link
Author

Exactly my point. jQuery behavior and angular jqLite are not equal.

When you call (angular.element(input)).triggerHandler("change", someDataObject) and jQuery(input).triggerHandler("change", someDataObject) jQuery returns this object as onChange argument while angular completely ignores it.

Reason is as described in my first post concatenation between array and object.

@petebacondarwin
Copy link
Contributor

{preventDefault: noop, stopPropagation: noop } is not dummy data it is the dummy event. Dummy data should always be attached as subsequent parameters to the handler not to the event itself.

This Plunk shows that the first and only parameter sent to the handler when it is called for real is an Event object. http://plnkr.co/edit/7Fh92todilbTpeJfOlmv?p=preview

This Plunk shows that jQuery's triggerHandler will attach the "extra" data as a second parameter and not to the first parameter, which is the event object. http://plnkr.co/edit/Q6SzHTWe1Qw8ufR8OFlx?p=preview. Moreover you'll notice that the event.target.files object is not updated with the dummy files data that you are passing in.

This Plunk shows how it should be done in jQuery. Note that instead of providing two parameters triggerHandler(eventType, data) we are only providing one triggerHandler(mockEvent). http://plnkr.co/edit/hs4EUMhP0NBWM6RmvuQq?p=preview

The real problem regarding Angular is that we are not supporting this ability to pass in our own Event object in our triggerHandler. I'll look into this.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Aug 6, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Aug 8, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Aug 8, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Aug 8, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
petebacondarwin added a commit that referenced this issue Aug 11, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

In addition the commit provides some performance and memory usage
improvements by only creating objects and doing work that is necessary.

This commit also renames the parameters inline with jQuery.

Closes #8469
Closes #8505
@tkrotoff
Copy link

FYI you can write:

el.triggerHandler({
  type: 'change',
  target: {
    files: [
      new Blob(['data0']),
      new Blob(['data1'])
    ]
  }
});

el.triggerHandler(jQuery.Event('change', {
  target: {
    files: [
      new Blob(['data0']),
      new Blob(['data1'])
    ]
  }
}));

el.triggerHandler($.Event('change', {
  target: {
    files: [
      new Blob(['data0']),
      new Blob(['data1'])
    ]
  }
}));

They are all similar.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants