Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drag and drop events on views do not have e.dataTransfer #1073

Closed
juliangutman opened this issue Jun 25, 2012 · 12 comments · Fixed by #1136
Closed

Drag and drop events on views do not have e.dataTransfer #1073

juliangutman opened this issue Jun 25, 2012 · 12 comments · Fixed by #1136

Comments

@juliangutman
Copy link

The html5 drag and drop events on views do not have the dataTransfer property on the event object. Likely because jQuery does not in its default state copy that property to the jQuery event from the browser event.

As per the jQuery documentation:

OtherProperties

Certain events may have properties specific to them. Those can be accessed as properties of the event.originalEvent object. To make special properties available in all event objects, they can be added to the jQuery.event.props array. This is not recommended, since it adds overhead to every event delivered by jQuery.

Example:

// add the dataTransfer property for use with the native drop event
// to capture information about files dropped into the browser window
jQuery.event.props.push("dataTransfer");

@raycohen
Copy link
Contributor

@juliangutman Can you be a little more specific about what you are suggesting? It seems that the data you need can be accessed via event.originalEvent.dataTransfer. Is that not acceptable?

@juliangutman
Copy link
Author

Fair point, it could be done that way.

I was simply suggesting that considering that the drag and drop series of events were the only ones for which a crucial event property was not available on the augmented jQuery event object, it might make sense to add:

jQuery.event.props.push("dataTransfer");

somewhere in the ember source. If not, at least have a note in the api docs around the fact that dataTransfer needs to be accessed via the originalEvent. I spent a bit of time and frustration trying to understand why I couldn't get the dataTransfer property and only after looking through the jQuery documentation did I find that section and luckily the example included the dataTransfer property specifically.

@raycohen
Copy link
Contributor

Looks like there is a way to get html5 event attributes copied over by jQuery with less of a performance hit. If you push directly onto jquery.event.props then every event jQuery sees is going to get a slight performance hit for it. (Consider how fast lots of mouse and scroll events can be generated)

Instead, you can add per-event via jQuery.event.fixHooks so only that type of event will trigger the extra code.

Consider this example, where props is a list of extra properties, and filter can do custom logic with the event:
https://github.com/jquery/jquery/blob/9bb3494ce99889e05a7333e38e4da9579ce6af8e/src/event.js#L462

And these discussions:
http://bugs.jquery.com/ticket/8789

http://bugs.jquery.com/ticket/5249

so for extra events and properties we want to support, we'd add something like this:

jQuery.event.fixHooks.drop = {
  props: ['dataTransfer']
}

@juliangutman
Copy link
Author

Very awesome. Appreciate you looking into this and sending over the
references. Apologies for being sloppy and not digging a bit deeper
myself.

Julian

On Jun 26, 2012, at 12:31 AM, raycohen
reply@reply.github.com
wrote:

Looks like there is a way to get html5 event attributes copied over by jQuery with less of a performance hit. If you push directly onto jquery.event.props then every event jQuery sees is going to get a slight performance hit for it. (Consider how fast lots of mouse and scroll events can be generated)

Instead, you can add per-event via jQuery.event.fixHooks so only that type of event will trigger the extra code.

Consider this example, where props is a list of extra properties, and filter can do custom logic with the event:
https://github.com/jquery/jquery/blob/9bb3494ce99889e05a7333e38e4da9579ce6af8e/src/event.js#L462

And these discussions:
http://bugs.jquery.com/ticket/8789

http://bugs.jquery.com/ticket/5249

so for extra events and properties we want to support, we'd add something like this:

jQuery.event.fixHooks.drop = {
 props: ['dataTransfer']
}

Reply to this email directly or view it on GitHub:
#1073 (comment)

@raycohen
Copy link
Contributor

ping: @wagenet

Since the event delegator is handling drag & drop events, this would be handy. Thoughts?

@wagenet
Copy link
Member

wagenet commented Jun 26, 2012

@raycohen Since you seem to be fairly familiar with how this should work is this something you'd be willing to attempt a PR for?

@juliangutman
Copy link
Author

Sure, I'd be glad to give it a shot.

On Tuesday, June 26, 2012 at 1:08 AM, Peter Wagenet wrote:

@raycohen Since you seem to be fairly familiar with how this should work is this something you'd be willing to attempt a PR for?


Reply to this email directly or view it on GitHub:
#1073 (comment)

@raycohen
Copy link
Contributor

jQuery.event.fixHooks only became available in jQuery 1.7. Ember still supports 1.6.

Is it ok for the code (and the tests) to check the existence of fixHooks before proceeding?

I'm also a bit concerned that fixHooks is not documented as part of the public API for jQuery.

@wagenet
Copy link
Member

wagenet commented Jun 26, 2012

I think we can bump to requiring 1.7 for master so this fix should probably target master then. Maybe @wycats can provide some insight about fixHooks since he's on the jQuery team as well.

@scottgonzalez
Copy link

jQuery.event.fixHooks is the correct way to fix this. If you want to support older versions of jQuery, you should be able to just push on to jQuery.event.props. It'll affect all events, but I'm pretty sure it'll solve the problem.

@wycats
Copy link
Member

wycats commented Jul 9, 2012

Can someone submit a pull request that uses fixHooks and bumps us to 1.7?

@wagenet
Copy link
Member

wagenet commented Jul 12, 2012

This has been merged it looks like.

@wagenet wagenet closed this as completed Jul 12, 2012
rwjblue added a commit that referenced this issue Apr 16, 2020
…g cleanup).

Changelog from the release:

* `@glimmer/integration-tests`, `@glimmer/interfaces`, `@glimmer/runtime`
  * [#1073](glimmerjs/glimmer-vm#1073) Ensure errors during component creation do not cause secondary errors during DOM cleanup ([@pzuraq](https://github.com/pzuraq))

- Chris Garrett ([@pzuraq](https://github.com/pzuraq))
rwjblue added a commit that referenced this issue Apr 23, 2020
…g cleanup).

Changelog from the release:

* `@glimmer/integration-tests`, `@glimmer/interfaces`, `@glimmer/runtime`
  * [#1073](glimmerjs/glimmer-vm#1073) Ensure errors during component creation do not cause secondary errors during DOM cleanup ([@pzuraq](https://github.com/pzuraq))

- Chris Garrett ([@pzuraq](https://github.com/pzuraq))

(cherry picked from commit cf4a4bc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants