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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Drag and Drop
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Natch, Assigned: Neil Deakin)

Tracking

({dev-doc-complete, fixed1.9.1})

Trunk
mozilla1.9.2a1
dev-doc-complete, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
blocking1.9.1 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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).
(Reporter)

Updated

10 years ago
Blocks: 465608
(Reporter)

Updated

10 years ago
Flags: blocking1.9.1?
Neil: any chance I can get you to look at this?
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 2

10 years ago
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.
(Reporter)

Comment 3

10 years ago
(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.
(Reporter)

Comment 4

10 years ago
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
Assignee: nobody → enndeakin
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.
(Reporter)

Comment 6

10 years ago
(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.
(Assignee)

Comment 7

10 years ago
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.
Assignee: enndeakin → nobody
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).
(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?
Comments 7 and 8 imply that this is WONTFIX or INVALID, right?
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?
(Assignee)

Comment 12

10 years ago
(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.
(Reporter)

Comment 13

10 years ago
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 ;)
(Reporter)

Comment 14

10 years ago
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.
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.
Flags: wanted1.9.2+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
(Reporter)

Comment 16

10 years ago
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...
(Assignee)

Comment 17

10 years ago
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.
(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.
(Assignee)

Comment 19

10 years ago
(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?
(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.
(Assignee)

Comment 21

10 years ago
(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?
At event - (void)draggedImage:(NSImage *)anImage endedAt:(NSPoint)aPoint operation:(NSDragOperation)operation, file nsChildView.mm. Value of the operation argument.
(Assignee)

Comment 23

10 years ago
(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.
(Reporter)

Comment 24

10 years ago
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.

Updated

9 years ago
Blocks: 465608
No longer blocks: 474442
Version: unspecified → Trunk
(Assignee)

Comment 25

9 years ago
Created attachment 361559 [details] [diff] [review]
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 26

9 years ago
Created attachment 362001 [details] [diff] [review]
implement this for windows and mac

OK, I figured out how to do this on Mac as well (thanks Camino!)
Attachment #361559 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #362001 - Flags: superreview?(roc)
Attachment #362001 - Flags: review?(roc)
(Assignee)

Comment 27

9 years ago
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.
(Reporter)

Comment 28

9 years ago
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.
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.
Assignee: enndeakin → nobody
Blocks: 474442
No longer blocks: 465608
Version: Trunk → unspecified
(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.

Updated

9 years ago
Assignee: nobody → enndeakin
Blocks: 465608
No longer blocks: 474442
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.
(Assignee)

Comment 32

9 years ago
Created attachment 362213 [details] [diff] [review]
other way using mozUserCancelled
Attachment #362213 - Flags: superreview?(roc)
Attachment #362213 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
Attachment #362001 - Attachment is obsolete: true
Attachment #362001 - Flags: superreview?(roc)
Attachment #362001 - Flags: review?(roc)
(Assignee)

Comment 33

9 years ago
(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.
Using 'detail' is adding API, and so is using "mozUserCancelled". Not having to change the IDL for 'detail' isn't important IMHO.
Attachment #362213 - Flags: superreview?(roc)
Attachment #362213 - Flags: superreview+
Attachment #362213 - Flags: review?(roc)
Attachment #362213 - Flags: review+
(Assignee)

Comment 35

9 years ago
Checked this in, so this is done for Windows and Mac.
Comment on attachment 362213 [details] [diff] [review]
other way using mozUserCancelled

a191=beltzner
Neil: is this landed on the 1.9.1 branch?

I filed bug 479995 for the linux part of this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Can we get an automated test for this implementation?
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
(Assignee)

Comment 39

9 years ago
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.
191: a0b3c0e6d9be.
Keywords: fixed1.9.1
(Assignee)

Updated

9 years ago
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.