Skip to main content

Bug Tracker

Side navigation

#10729 closed bug (wontfix)

Opened November 09, 2011 02:47AM UTC

Closed June 26, 2012 01:33AM UTC

Last modified October 04, 2013 02:35PM UTC

rmouseEvent & mouseHooks missing drag* events

Reported by: rwaldron Owned by:
Priority: low Milestone: 1.next
Component: event Version: 1.7
Keywords: Cc: scottgonzalez
Blocked by: Blocking:
Description

Previously, drag* events received the same attention that all events did, now they are neglected and will not receive the same copied properties that mouse events do.

Attachments (0)
Change History (19)

Changed November 09, 2011 03:10AM UTC by rwaldron comment:1

Changed November 09, 2011 03:11AM UTC by rwaldron comment:2

component: unfiledevent
milestone: None1.next
priority: undecidedhigh

Changed November 11, 2011 01:13PM UTC by dmethvin comment:3

#10756 is a duplicate of this ticket.

Changed November 15, 2011 02:26PM UTC by dmethvin comment:4

milestone: 1.next1.7.1

The patch won't work.

This is a regression so we should address it asap.

Changed November 16, 2011 03:06AM UTC by dmethvin comment:5

milestone: 1.7.11.next
status: newopen

I don't see a simple/small way to fix this. If all browsers used the W3C Event hierarchy we could just look for MouseEvent, KeyEvent, etc. to assign the fixHook and not even look at the event name in many cases. IE<9 doesn't follow that rule though. It might be possible to find a property in their event object that is a proxy for mouse/key events?

Since these are not standard browser events we could just ask people to get their properties from event.originalEvent instead, or manually assign the mouseEvent fixHook to them. I'll leave this open while we contemplate.

Changed November 16, 2011 04:42AM UTC by rwaldron comment:6

Agreed

Changed November 16, 2011 01:23PM UTC by mikesherov comment:7

using rwaldron's jsfiddle http://jsfiddle.net/rwaldron/6AXjU/ I tested in IE6-8, and looks like they all have e.DataTransfer on "drop"... looking for similar MouseEvent proxies for "drag*"

Changed November 16, 2011 01:35PM UTC by dmethvin comment:8

So something like this at the top of fix() might do the trick:

if ( !fixHook && event.dataTransfer ) {
    fixHook = jQuery.event.fixHooks[ event.type ] = jQuery.event.mouseHooks;
}

If the event has a hook, as all mouse and key events do, all we're adding is the check for !fixHook so it shouldn't be a perf hit. Now I wonder if we need to generalize this, now or at some point, for other property sniffing.

Changed November 16, 2011 01:46PM UTC by mikesherov comment:9

just to confirm, all drag/drop events contain e.dataTransfer

Changed November 17, 2011 05:40PM UTC by dmethvin comment:10

As far as the generalization is concerned, the issue here is that we really could use at least one level of inheritance. A DragDropEvent is derived from MouseEvent and we don't want to duplicate all that. At minimum we'd want to add the dataTransfer property to the event object I'd think?

Changed November 17, 2011 05:56PM UTC by rnice@nicey.com comment:11

I'm coming in from #10816, sorry to be a newbie, but I'd like to understand the problem.

Assuming that jQ wraps browser events in it's own Event object to normalize their behavior across different browsers and versions; is the problem detecting the event type, so that the correct normalizer/fixer function can be called?

If so, may I suggest (as a newbie) that all properties of the original event be copied over, and the fixer be used to tidy stuff up. That way unknown or new events/use-cases would survive?

Also, why is UI/Widget creating this wrapped Event (for drag/drop)? If jQ has picked up the mouse move event during drag, should it not create the normalized wrapped version before sending it to UI?

I'd love to help, if I can get up to speed.

Changed November 17, 2011 06:45PM UTC by dmethvin comment:12

@rnice, if you set a breakpoint on jQuery.event.fix in several browsers you'll see that there are a TON of properties on the event object; copying all of them over would be very expensive, especially on high-frequency events like mousemove or scroll. Plus the event object is a strange non-standard host object on IE 6/7/8 and I don't even recall if its properties are enumerable. The properties in the original object are read-only so we have to make a copy.

For that reason, we decided a long time ago to copy a minimal subset of standard properties to our own event, normalizing important ones like event.target and event.relatedTarget so they work on all browsers. All the properties are still available through event.originalEvent if someone needs them.

As for why UI is creating its own wrapped event, I'm not sure. There is nothing wrong with higher-level methods adding their own abstractions on top of what core provides if they have specific needs. The new fixHooks provide some of those extensions inside core but are only useful if you can guarantee 1.7 is in use, which won't be the case for UI. Scott Gonzalez mentioned that they sometimes have a situation with event.originalEvent.originalEvent and I have been meaning to follow up and ask him why. It's because of nested synthetic events but not sure why they have to be nested and not just fired at the same level.

Changed November 18, 2011 11:01PM UTC by rnice@nicey.com comment:13

First, thanks for taking the time to explain that, it really helped.

When an event arrives jQ picks it up, wraps it correctly (with pageX/Y etc.) based on it's type, and fires it to any registered handlers. So far, so good.

UI seems to make quite a (questionable) practice of picking up these events, morphing them into it's own event types, and completely re-firing them. In the case of draggable, it picks up the wrapped move event, makes a call to re-wrap the original event now calling it a 'drag' event instead of a 'move', and makes an un-documented trip into jQ's internals to try and figure out which properties to clone.

What Scott is saying, is that he'd like to just use jQ's auto-wrap (event.fix) feature on his cloned event, but he can't because it only works on proper fresh browser events. (jquery.ui.widget.js line 386, see his comment).

There is going to be a performance impact in firing 2 for 1 events with the normalization and handler overhead just to turn a move into an identical drag. However, I'm sure UI is filled with expectations of drags, not moves.

In summary:

jQ needs those 'un-documented' functions like jQuery.event and probably even .Event itself to be private. This whole pickle is because there is no interface/encapsulation and somebody is peeking inside something they shouldn't, and when it changed in 1.7 it broke.

UI needs to send the original Move event to the drag handlers, instead of cloning the event, just changing the name, and re-firing it through the handler chain.

Hope I got that right. I'm happy to have a go if you want (heaven help you) :)

Changed November 19, 2011 04:01AM UTC by dmethvin comment:14

cc: → scott.gonzalez

It's not that uncommon to need to fire a new event type when you get a different one. But once jQuery.event.fix normalizes the native event properties into the jQuery.Event object, calling it again isn't going to yield different normalized results. I think $.extend should be able to do the copy without knowing specific properties, but there must be more going on because I tried changing the UI widget's _trigger method and lots of unit tests fail.

My immediate question is whether we need to patch this specific ticket for 1.7.1, which is going out on Monday, and whether the code suggested above seems like the right solution. Scott, what is your take?

Changed November 24, 2011 09:01PM UTC by dmethvin comment:15

#10816 is a duplicate of this ticket.

Changed December 04, 2011 03:56PM UTC by Cokegod comment:16

_comment0: I think the drag and drop events are more important than adding 3 lines to the top of "fix". In my opinion there should be a hooks object for drag and drop, which will copy all the properties from mouseHooks, adding the dataTransfer property. Of course I also think there should be a drag and drop event Regular Expression.1323014278956098
_comment1: I think the drag and drop events are more important than adding 3 lines to the top of "fix". In my opinion there should be a hooks object for drag and drop, which will copy all the properties from mouseHooks, adding the dataTransfer property.1323014346315033
_comment2: I think the drag and drop events are more important than adding 3 lines to the top of "fix". In my opinion there should be a hooks object for drag and drop, which will copy all the properties from mouseHooks, adding the dataTransfer property. Of course there should also be made some obvious changes affecting by this (like adding a drag and drop Regular Expression).1323014358873785
_comment3: I think the drag and drop events are more important than adding 3 lines to the top of "fix". In my opinion there should be a hooks object for drag and drop, which will copy all the properties from mouseHooks, adding the dataTransfer property. Of course there should also be made some obvious changes affected by this (like adding a drag and drop Regular Expression).1323018586497853
_comment4: I think the drag and drop events are more important than adding 3 lines to the top of `fix`. In my opinion there should be a hooks object for drag and drop, which will copy all the properties from `mouseHooks`, adding the `dataTransfer` property. Then I think all the drag events as names each with a value of the drag and drop hooks object should be added to `fixHooks`. \ \ Here is a possible example for what I think that should be done: \ {{{ \ //inside jQuery.event \ dndHooks: { \ props: jQuery.event.props.concat( "dataTransfer" ), \ filter: jQuery.event.filter \ } \ \ \ //outside jQuery.event \ $.each( ("dragstart dragenter dragover dragleave drag drop dragend").split(" "), function ( i, name ) { \ jQuery.event.fixHooks[ name ] = jQuery.event.dndHooks; \ }); \ }}} \ 1323023911679954
_comment5: I think the drag and drop events are more important than adding 3 lines to the top of `fix`. In my opinion there should be a hooks object for drag and drop, which will copy all the properties from `mouseHooks`, adding the `dataTransfer` property. Then I think all the drag events as names each with a value of the drag and drop hooks object should be added to `fixHooks`. \ \ Here is a possible example for what I think that should be done: \ {{{ \ //inside jQuery.event \ dndHooks: { \ props: jQuery.event.mouseHooks.props.concat( "dataTransfer" ), \ filter: jQuery.event.mouseHooks.filter \ } \ \ \ //outside jQuery.event \ $.each( ("dragstart dragenter dragover dragleave drag drop dragend").split(" "), function ( i, name ) { \ jQuery.event.fixHooks[ name ] = jQuery.event.dndHooks; \ }); \ }}} \ 1323024100821824
_comment6: I think the drag and drop events are more important than adding 3 lines to the top of `fix`. In my opinion there should be a hooks object for drag and drop, which will copy all the properties from `mouseHooks`, adding the `dataTransfer` property. Then I think all the drag events as names each with a value of the drag and drop hooks object should be added to `fixHooks`. \ \ Here is a possible example for what I think that should be done: \ {{{ \ jQuery.event.dndHooks = { \ props: jQuery.event.mouseHooks.props.concat( "dataTransfer" ), \ filter: jQuery.event.mouseHooks.filter \ }; \ \ $.each( ("dragstart dragenter dragover dragleave drag drop dragend").split(" "), function ( i, name ) { \ jQuery.event.fixHooks[ name ] = jQuery.event.dndHooks; \ }); \ }}} \ 1323025755834053
_comment7: I think the drag and drop events are more important than adding 3 lines to the top of `fix`. In my opinion there should be a hooks object for drag and drop, which will copy all the properties from `mouseHooks`, adding the `dataTransfer` property. Then I think all the drag events as names each with a value of the drag and drop hooks object should be added to `fixHooks`. \ I think the drag and drop events should be added to fixHooks seperatly and not to `jQuery.fn` because they are not entirely supported in all the browsers jQuery supports. \ \ https://github.com/jquery/jquery/pull/6201323025886295238

I think the drag and drop events are more important than adding 3 lines to the top of fix. In my opinion there should be a hooks object for drag and drop, which will copy all the properties from mouseHooks, adding the dataTransfer property. Then I think all the drag events as names each with a value of the drag and drop hooks object should be added to fixHooks.

I think the drag and drop events should be added to fixHooks separately and not to jQuery.fn because they are not entirely supported in all the browsers jQuery supports.

https://github.com/jquery/jquery/pull/620

Changed December 06, 2011 04:38PM UTC by dmethvin comment:17

description: Previously, drag* events received the same attention that all events did, now they are neglected and will not receive the same copied properties that mouse events do.Previously, drag* events received the same attention that all events did, now they are neglected and will not receive the same copied properties that mouse events do. \
priority: highlow

If we can't provide cross-browser support for drag/drop then we really shouldn't be copying those properties into our cross-browser jQuery.Event object. Originally I thought UI depended on them but that is not the case. Nobody has yet shown a popular plugin or codebase that we're breaking by leaving them out, so I'm hesitant to make jQuery bigger/slower to support them by default.

Changed June 26, 2012 01:33AM UTC by dmethvin comment:18

resolution: → wontfix
status: openclosed

I'll mark this wontfix, the properties are easily available in originalEvent so we haven't closed the door on any functionality.

Changed October 04, 2013 02:35PM UTC by anonymous comment:19

Please reconsider - it makes working with JQuery.Event even more painful than DOM events. Even though drag events are mouse events I can't work with them using the same code as with mouse events since JQuery wrapped drag events will lack many essential properties. It even worse if you want correct typing (i.e. TypeScript).