Closed Bug 471848 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Drag and Drop, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Natch, Assigned: enndeakin)

References

Details

(Keywords: dev-doc-complete, fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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).
Blocks: 465608
Flags: blocking1.9.1?
Neil: any chance I can get you to look at this?
Flags: blocking1.9.1? → blocking1.9.1+
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.
(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.
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
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.
(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.
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?
(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.
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 ;)
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+
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...
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.
(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.
(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.
(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.
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.
Blocks: 465608
No longer blocks: 474442
Version: unspecified → Trunk
Attached patch implement drag cancel flag (obsolete) — Splinter Review
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
OK, I figured out how to do this on Mac as well (thanks Camino!)
Attachment #361559 - Attachment is obsolete: true
Attachment #362001 - Flags: superreview?(roc)
Attachment #362001 - Flags: review?(roc)
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.
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.
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.
Attachment #362213 - Flags: superreview?(roc)
Attachment #362213 - Flags: review?(roc)
Attachment #362001 - Attachment is obsolete: true
Attachment #362001 - Flags: superreview?(roc)
Attachment #362001 - Flags: review?(roc)
(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+
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
Closed: 11 years ago
Resolution: --- → FIXED
Can we get an automated test for this implementation?
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
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.
You need to log in before you can comment on or make changes to this bug.