Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#10729 closed bug (wontfix)

rmouseEvent & mouseHooks missing drag* events

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

Description (last modified by dmethvin)

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.

Change History (19)

comment:2 Changed 7 years ago by Rick Waldron

Component: unfiledevent
Milestone: None1.next
Priority: undecidedhigh

comment:3 Changed 7 years ago by dmethvin

#10756 is a duplicate of this ticket.

comment:4 Changed 7 years ago by dmethvin

Milestone: 1.next1.7.1

The patch won't work.

This is a regression so we should address it asap.

comment:5 Changed 7 years ago by dmethvin

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.

comment:6 Changed 7 years ago by Rick Waldron

Agreed

comment:7 Changed 7 years ago by mikesherov

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*"

comment:8 Changed 7 years ago by dmethvin

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.

comment:9 Changed 7 years ago by mikesherov

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

comment:10 Changed 7 years ago by dmethvin

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?

comment:11 Changed 7 years ago by rnice@…

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.

comment:12 Changed 7 years ago by dmethvin

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

comment:13 Changed 7 years ago by rnice@…

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) :)

comment:14 Changed 7 years ago by dmethvin

Cc: scott.gonzalez added

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?

comment:15 Changed 7 years ago by dmethvin

#10816 is a duplicate of this ticket.

comment:16 Changed 7 years ago by Cokegod

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

Last edited 7 years ago by Cokegod (previous) (diff)

comment:17 Changed 7 years ago by dmethvin

Description: modified (diff)
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.

comment:18 Changed 7 years ago by dmethvin

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.

comment:19 Changed 5 years ago by anonymous

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

Note: See TracTickets for help on using tickets.