479995 - [linux] mozUserCancelled isn't implemented on gtk / pressing ESC while dragging a tab doesn't cancel the drag (bug 465608 isn't fixed)

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Asaf Romano (gone)]

GTK part of bug 471848.

Comment 1

[Neil Deakin]

Just for info: this is an easy fix but requires gtk2.12 while our builds use 2.10

Comment 2

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

Ah, can we just load the symbols we need dynamically and have this work only on 2.12?  Worst case for 2.10 would be no worse than it is currently, right?

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: handle drag-failed signal when available

This also removes the big delay between dropping the tab (on an unreceptive target) and the tab being torn off into a new window.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 381207 [details] [diff] [review]
handle drag-failed signal when available

This applies on top of attachment 380050 [details] [diff] [review] for bug 495184.

Comment 5

[Neil Deakin]

Comment on attachment 381207 [details] [diff] [review]
handle drag-failed signal when available

>-    sDragLm = PR_NewLogModule("nsDragService");
>+        sDragLm = PR_NewLogModule("nsDragService");

Unnecessary indenting change.

>+    if (aResult != MOZ_GTK_DRAG_RESULT_SUCCESS &&
>+        aResult != MOZ_GTK_DRAG_RESULT_NO_TARGET) {
>+
>+        mUserCancelled = PR_TRUE;

There are six possible values of GtkDragResult, only one
(GTK_DRAG_RESULT_USER_CANCELLED) which means the user cancelled, so mUserCancelled should only be set in that case.

If aResult is MOZ_GTK_DRAG_RESULT_NO_TARGET, don't you just want to set dropEffect to DRAGDROP_ACTION_NONE and not check the action?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> (From update of attachment 381207 [details] [diff] [review])
> >-    sDragLm = PR_NewLogModule("nsDragService");
> >+        sDragLm = PR_NewLogModule("nsDragService");
> 
> Unnecessary indenting change.

Are you saying that we should never change the intendation for the sake of
readability?  Or do you think the lack of indentation was intended?

> 
> >+    if (aResult != MOZ_GTK_DRAG_RESULT_SUCCESS &&
> >+        aResult != MOZ_GTK_DRAG_RESULT_NO_TARGET) {
> >+
> >+        mUserCancelled = PR_TRUE;
> 
> There are six possible values of GtkDragResult, only one
> (GTK_DRAG_RESULT_USER_CANCELLED) which means the user cancelled, so
> mUserCancelled should only be set in that case.

We are not able to accurately represent all 6 GtkDragResult values in the DOM
events.  Only GTK_DRAG_RESULT_USER_CANCELLED has a similar name to the
mozUserCancelled attribute, but I thought (most) of the other failures were
cancelled drags of some sort.

GTK_DRAG_RESULT_GRAB_BROKEN would be generated if the user broke the grab
(cancelling the drag), or if an application broke the grab, which would most
likely be in response to a user action.  So GTK_DRAG_RESULT_GRAB_BROKEN seems
to indicate that "the user cancelled the drag".

Regarding the other 2 GtkDragResult values, I've now read the comment on
nsIDOMDataTransfer::mozUserCancelled more carefully:

   Will be true if the user cancelled the drag (typically by pressing Escape).
   This will be false otherwise, including if a drag failed for any other
   reason.

Currently the only user of this attribute seems to be tabbrowser's _onDragEnd,
but what that method really seems to want to know is whether a drop was
performed on an unreceptive destination.  Should we have a different attribute
more suitable for this purpose?

If we don't treat GTK_DRAG_RESULT_TIMEOUT_EXPIRED as a cancelled drag, then
whatever action results from an uncancelled dragend will be performed 5
minutes after the drop that timed out.

GTK_DRAG_RESULT_ERROR is not ever generated with gtk+-2.14.7 at least, but
does it seem right to respond to an unknown error by tearing off a tab?

> If aResult is MOZ_GTK_DRAG_RESULT_NO_TARGET, don't you just want to set
> dropEffect to DRAGDROP_ACTION_NONE and not check the action?

Yes, you are right.  I thought the action would always be none with
GTK_DRAG_RESULT_NO_TARGET, but this is not necessarily the case with Motif
drops.

Comment 7

[Neil Deakin]

(In reply to comment #6)
> Are you saying that we should never change the intendation for the sake of
> readability?  Or do you think the lack of indentation was intended?

Sorry, I must have read the patch too quickly. That change looks ok.

> 
> > 
> > >+    if (aResult != MOZ_GTK_DRAG_RESULT_SUCCESS &&
> > >+        aResult != MOZ_GTK_DRAG_RESULT_NO_TARGET) {
> > >+
> > >+        mUserCancelled = PR_TRUE;
> > 
> > There are six possible values of GtkDragResult, only one
> > (GTK_DRAG_RESULT_USER_CANCELLED) which means the user cancelled, so
> > mUserCancelled should only be set in that case.
> 
> We are not able to accurately represent all 6 GtkDragResult values in the DOM
> events.  Only GTK_DRAG_RESULT_USER_CANCELLED has a similar name to the
> mozUserCancelled attribute, but I thought (most) of the other failures were
> cancelled drags of some sort.
> 

OK, that seems to make sense. The documentation for mozUserCancelled should be updated to indicate that it will also be true if the drag failed in an unexpected way.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: handle drag-failed signal when available v1.1

Set dropEffect to DRAGDROP_ACTION_NONE on GTK_DRAG_RESULT_NO_TARGET,
and update mozUserCancelled documentation.

Comment 9

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/d9a721a24d2a

Comment 10

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

Verified fixed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090703 Minefield/3.6a1pre ID:20090703031447

Comment 11

[Nochum Sossonko [:Natch]]

No tests were checked in with this, perhaps a litmus test can be added?

Comment 12

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

Karl, will this patch work on 1.9.1? If it does, we should ask for approval1.9.1.1.

Nochum, we have to update https://litmus.mozilla.org/show_test.cgi?id=7544 when it has been (will be) fixed on 1.9.1.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 382441 [details] [diff] [review]
handle drag-failed signal when available v1.1

This patch will work on 1.9.1 if was also have the patches in bug 495184.

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 382441 [details] [diff] [review]
handle drag-failed signal when available v1.1

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

Is there a bug for getting an automated test written for this?

Comment 15

[Karl Tomlinson (:karlt)]

status-1.9.1.2-fixed:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8f359d58f649

(In reply to comment #14)
> Is there a bug for getting an automated test written for this?

I don't really have a good picture of how an automated test for this might
work.
Simulating a drag would require sending native pointer motion and button
and keyboard events.

Comment 16

[Nochum Sossonko [:Natch]]

EventUtils.synthesize[DragStart|Drop] ? That *should* be able to test this, but you can't specify an esc during the drag presently...

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
> EventUtils.synthesize[DragStart|Drop] ? That *should* be able to test this, but
> you can't specify an esc during the drag presently...

Thanks.  That would be useful for testing many parts of drag processing.
But the code involved in this bug is actually in processing native events at a level closer to the OS than nsIDOMEventTarget::dispatchEvent(), so using dispatchEvent() would not test this code.

Comment 18

[Nochum Sossonko [:Natch]]

Actually, why can't you just run a drag manually, like what's done in synthesizeDragStart, then in the dragstart handler of the element you're dragging use sendKey("VK_ESC"). I'd give it a go, but I won't have a chance until the weekend at least.

Comment 19

[Neil Deakin]

(In reply to comment #18)
> Actually, why can't you just run a drag manually, like what's done in
> synthesizeDragStart, then in the dragstart handler of the element you're
> dragging use sendKey("VK_ESC").

Because the cancelling of the drag upon pressing of the escape key is done by the operating system. Drags is general cannot be automated as they rely on the operating system's processing, which Mozilla doesn't control nor, in most cases, has any control over. On Mac, for instance, native drag apis will fail unless an actual mouse button event is being processed.

Comment 20

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

Verified fixed on 1.9.1 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2pre) Gecko/20090728 Shiretoko/3.5.2pre ID:20090728032218

Litmus test https://litmus.mozilla.org/show_test.cgi?id=7544 has been updated. Marking in-litmus+. Can we minus in-testsuite?