471848 - Need a way to tell if dragEnded came from a mouseup or from esc

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Nochum Sossonko [:Natch]]

While looking into bug 465608 I realized that there is no way to tell from the event object if it was fired due to a real dragend or if the drag was canceled (via esc).

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Neil: any chance I can get you to look at this?

Comment 2

[Neil Deakin]

If the drag was cancelled or not accepted at the drop point, it will have a dropEffect of 'none'. If the drag was successful, the dropEffect will be set to one of the other values.

I don't know if it's possible to determine specifically if a drop didn't occur due to escape being pressed versus being over an invalid drop target on all platforms. A cursory glance suggests yes on Windows, but no on Mac and Linux.

Comment 3

[Nochum Sossonko [:Natch]]

(In reply to comment #2)
> If the drag was cancelled or not accepted at the drop point, it will have a
> dropEffect of 'none'. If the drag was successful, the dropEffect will be set to
> one of the other values.
> 
> I don't know if it's possible to determine specifically if a drop didn't occur
> due to escape being pressed versus being over an invalid drop target on all
> platforms. A cursory glance suggests yes on Windows, but no on Mac and Linux.

Isn't it possible to _set_ the dropEffect to 'none'? In fact, if I'm reading correctly, I think the tab drag does that now, which would make this test useless.

Comment 4

[Nochum Sossonko [:Natch]]

Specifically, this line makes me think checking for dropEffect=="none" won't work here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?mark=2249#2241

Comment 5

[Honza Bambas (:mayhemer)]

I have spend today short time on this and here are some observations:
- the event properties are exactly the same in both cases
- there is no way to catch the esc key in onkeypressed bound to tabbrowser.xml

The operation is actually upside down - we do something (detach) when the event is not processed by any target. Fix for this is platform dependent and we will probably have to extend the API and drag service implementation.

Comment 6

[Nochum Sossonko [:Natch]]

(In reply to comment #5)

I tried running the dragend code after a timeout so that hopefully the keypress event would get proessed but I ran into a number of problems, one is that I can't get | this | in a timeout. Events seem to be blocked once the drag starts.

Comment 7

[Neil Deakin]

I suppose I wasn't clear in my previous comment. Please don't spend time looking for some workaround in the browser code as you won't find one. The drag operation including if it canceled is handled by the operating system. Windows appears to provide information about whether escape was pressed via IDropSource::QueryContinueDrag. Mac doesn't seem to. Gtk might provide this via the drag-failed signal.

Comment 8

[Honza Bambas (:mayhemer)]

Key events seem to be blocked, at least on mac. I would say the way we handle dragEnd is wrong as the whole way to detach the tab. For example on mac there is no way to recognize if the drag was canceled or not. Only when I drag on the tab content area and press esc I got operation = NSDragOperationNone instead of NSDragOperationGeneric when I do not press esc. When I drag on the button bar or address bar I got operation = NSDragOperationNone even I press or not esc. We want to detach even we drop on tab content or the window bar.

I would say we need to let specific areas (perfect would be any area except the tab bar generically) to handle the drop specifically for "application/x-moz-tabbrowser-tab" data type and set the dropEffect to something else then "none", probably "move". Then we will revert the condition if (dt.dropEffect == "none") to if (dt.dropEffect != "none") and go forward in that case or let the generic code for all browser areas handle the move it's self (what would be most correct).

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #2)
> I don't know if it's possible to determine specifically if a drop didn't occur
> due to escape being pressed versus being over an invalid drop target on all
> platforms. A cursory glance suggests yes on Windows, but no on Mac and Linux.

I don't think we care, do we? All we want to know was if the drop failed. If it did, we shouldn't detach the tab (see bug 465608). Is the issue that we need to teach Windows that a drop outside the window should be considered success?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comments 7 and 8 imply that this is WONTFIX or INVALID, right?

Comment 11

[Honza Bambas (:mayhemer)]

The only way to cancel drag'n'drop is to return and drop on the tab where we started. It's ugly because many people including me are used to press esc. This should just not be blocking 1.9.1 IMO.

Isn't there a way to handle generically the mime type of the tab by the whole browser window or each of its part?

Comment 12

[Neil Deakin]

(In reply to comment #9)
> I don't think we care, do we? All we want to know was if the drop failed.

If the tab was dragged onto say the desktop and dropped, the drag operation stops, but despite that we want to open a new window. If the tab was dragged into the same location on the desktop but escape was pressed, the drag operation stops, but we do not want to create a new window. Whether this is implementable on all platforms is what needs to be determined.

The drop event is not fired at Mozilla when dropping on another window/desktop so we cannot add code in a drop handler. The dragend event is really the only option here.

Comment 13

[Nochum Sossonko [:Natch]]

Is it possible to just not set the dropEffect to "none" and then test for it? To prevent drops, the code should 1) not post a droppable uri (like onemen.one's patch) and/or 2) maybe set the dropEffect to something invalid (is that possible?). I'll play around with this later tonight, if no one beats me to it ;)

Comment 14

[Nochum Sossonko [:Natch]]

IMHO this is far from invalid, unless it's just impossible to implement. Basically, the reason tabbrowser.xml uses the dragend to tear out the tab is simply because this is supposed to be the new default behavior of tab dragging. Any other behavior is specific to certain locations (i.e. the bookmark toolbar or when it's dragging on itself) and each can be dealt with at the desired location.

With that said, the only realistic way to implement this new behavior _and_ account for drag canceling (presumably through escape) is to nuke the dragend handler in favor of a bunch of onDrops all over the place. This has a couple of drawbacks: 1) There will inevitably be some places that are missed. 2) Dragging outside of firefox will not be detectable, so even if the drop _should_ create a new window (i.e. a drop on an invalid target) the new window will not be spawned (because there is no real way to test for invalid targets versus canceled drags).

Finally, even within firefox, and this was the original intention of the bug to a certain degree, dragend doesn't specify if the cancellation is due to invalidity or due to escape. This, afaict, can be fixed and should be fixed IMHO.

Just my .02 from fiddling around a bit.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The chances of this happening for 1.9.1 this late are basically zero, especially given Neil's comments about not being able to get this info on different platforms.

Comment 16

[Nochum Sossonko [:Natch]]

Neil, in reply to comment 7, is there no way to catch the esc on mac? Iow if we can't get the esc from the object itself, can a handler be attached to the esc key once the drag is started and removed once it ends? That way gecko can simulate its own escape canceling. Does the os block the escape event while dragging? There is some stuff going on in draggedImage:endedAt, perhaps that can be used in conjunction with draggedImage:movedTo to determine if the application is a valid drop target, then when endedAt fires with no action specified it could be inferred that escape was hit. Very hacky, and won't work if the destination app really doesn't have an operation associated with the drag...

Comment 17

[Neil Deakin]

OK, it's possible to implement this on Windows, via the isEscape flag passed to nsNativeDragSource::QueryContinueDrag.

On Linux, a drag_failed signal will be raised with a result of GTK_DRAG_RESULT_USER_CANCELLED.

On Mac, I've tested various things but cannot find a way of determining this.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to comment #17)
> On Mac, I've tested various things but cannot find a way of determining this.

Maybe the minimum way from comment 8? It is not 100% reliable, though:

Only when I drag on the tab content area and press esc I got operation = NSDragOperationNone instead of NSDragOperationGeneric when I do not press esc. When I drag on the button bar or address bar I got operation = NSDragOperationNone even I press or not esc.

Comment 19

[Neil Deakin]

(In reply to comment #18)

> Only when I drag on the tab content area and press esc I got operation =
> NSDragOperationNone instead of NSDragOperationGeneric when I do not press esc.

get from where?

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > Only when I drag on the tab content area and press esc I got operation =
> > NSDragOperationNone instead of NSDragOperationGeneric when I do not press esc.
> 
> get from where?

Debugging.

Comment 21

[Neil Deakin]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > 
> > > Only when I drag on the tab content area and press esc I got operation =
> > > NSDragOperationNone instead of NSDragOperationGeneric when I do not press esc.

I mean, where are you retrieving the operation from? what api and from within what code are you getting it?

Comment 22

[Honza Bambas (:mayhemer)]

At event - (void)draggedImage:(NSImage *)anImage endedAt:(NSPoint)aPoint operation:(NSDragOperation)operation, file nsChildView.mm. Value of the operation argument.

Comment 23

[Neil Deakin]

(In reply to comment #22)
> At event - (void)draggedImage:(NSImage *)anImage endedAt:(NSPoint)aPoint
> operation:(NSDragOperation)operation, file nsChildView.mm. Value of the
> operation argument.

Well the operation there will set to whatever operation was set during the previous dragenter/dragover event, which various parts of the Firefox UI happen to set. The tab dragging code currently is presumably setting this in various places as well.

There is no different operation when a drag is not accepted at the location, versus cancelled. On Mac, these seem to both be considered equivalent.

Comment 24

[Nochum Sossonko [:Natch]]

Is it possible to patch Windows and Linux, while noting that it's fragile and doesn't work on Mac, it's not effectively possible to handle escape reasonably without this. So with the current method, the tab drag will be partially incomplete on Mac, while working properly on Windows/Linux.

The reason I say this is: no matter how well bug 465608 is avoided, there will inevitably be some chinks in the armor. For example, with the _dragLeftWindow field we can now see if the drag left the browser window, what if it went to the Library window, error console or what not? Esc would not be handled in those cases. This is especially true due to the fragile way esc and dragLeave interact on Windows, see bug 465608 comment 13.

Comment 25

[Neil Deakin]

Attachment: implement drag cancel flag

This implements a drag was cancelled by the user flag on Windows. For now, I just use the event.detail property which isn't currently used for anything. (event.detail = 1 means user cancelled with the escape key, event.detail = 0 means drag accepted or not a valid drop target)

It turns out that the linux feature which would handle this is only available for gtk2.11 and higher which I don't have.

Comment 26

[Neil Deakin]

Attachment: implement this for windows and mac

OK, I figured out how to do this on Mac as well (thanks Camino!)

Comment 27

[Neil Deakin]

Comment on attachment 362001 [details] [diff] [review]
implement this for windows and mac

This patch supports a flag to indicate that the user canceled a drag. event.detail will be set to 1 for the dragend event.

I chose the detail property as it isn't currently used for anything, and IE doesn't even have this property for drag events.

Maybe it would be better to use a mozUserCancelled property instead? This would require an api change though.

Comment 28

[Nochum Sossonko [:Natch]]

I think this should block final, as 1) there's a patch here and 2) this will make it possible to get rid of another 3.1 blocker and simplify the tab detaching code.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Roc: I know this isn't a blocker, but it's really wanted in order to resolve a niggly issue (see bug 475066) with ESC not cancelling tab-drag operations on Windows.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #29)
> Roc: I know this isn't a blocker, but it's really wanted in order to resolve a
> niggly issue (see bug 475066) with ESC not cancelling tab-drag operations on
> Windows.

Sorry, that should have been bug 465608 - my bad.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is OK, but I really think we should expose it as mozUserCancelled, not through 'detail'.

Exposing this through 'detail' is just as much of an API change, just one that we're more likely to regret later.

Comment 32

[Neil Deakin]

Attachment: other way using mozUserCancelled

Comment 33

[Neil Deakin]

(In reply to comment #31)
> Exposing this through 'detail' is just as much of an API change

It isn't no as using 'detail' doesn't require idl changes and 'detail' doesn't have any use currently.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Using 'detail' is adding API, and so is using "mozUserCancelled". Not having to change the IDL for 'detail' isn't important IMHO.

Comment 35

[Neil Deakin]

Checked this in, so this is done for Windows and Mac.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 362213 [details] [diff] [review]
other way using mozUserCancelled

a191=beltzner

Comment 37

[Asaf Romano (gone)]

Neil: is this landed on the 1.9.1 branch?

I filed bug 479995 for the linux part of this bug.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we get an automated test for this implementation?

Comment 39

[Neil Deakin]

It isn't possible to automate the os part of drag/drop. But there's a manual test for drag/drop that this feature could be added to. Not sure what the status of it is though.

Comment 40

[Asaf Romano (gone)]

ping comment 37

Comment 41

[Asaf Romano (gone)]

191: a0b3c0e6d9be.