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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Gtk
P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mano, Assigned: karlt)

Tracking

({perf, pp, verified1.9.1})

1.9.1 Branch
mozilla1.9.2a1
x86
Linux
perf, pp, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(status1.9.1 .2-fixed)

Details

(Whiteboard: [TSnap])

Attachments

(1 attachment, 1 obsolete attachment)

GTK part of bug 471848.
Flags: blocking1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
OS: Mac OS X → Linux
Priority: -- → P1

Comment 1

9 years ago
Just for info: this is an easy fix but requires gtk2.12 while our builds use 2.10
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?
(Assignee)

Comment 3

9 years ago
Created attachment 381207 [details] [diff] [review]
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.
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
Attachment #381207 - Flags: superreview?(roc)
Attachment #381207 - Flags: review?(enndeakin)
(Assignee)

Comment 4

9 years ago
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.
Attachment #381207 - Flags: superreview?(roc) → superreview+

Comment 5

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

Comment 6

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

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

Updated

9 years ago
Attachment #381207 - Flags: review?(enndeakin)
Flags: wanted1.9.1.x?
(Assignee)

Comment 8

9 years ago
Created attachment 382441 [details] [diff] [review]
handle drag-failed signal when available v1.1

Set dropEffect to DRAGDROP_ACTION_NONE on GTK_DRAG_RESULT_NO_TARGET,
and update mozUserCancelled documentation.
Attachment #381207 - Attachment is obsolete: true
Attachment #382441 - Flags: review?(enndeakin)

Updated

9 years ago
Attachment #382441 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d9a721a24d2a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: perf
Resolution: --- → FIXED
Whiteboard: [TSnap]
(Assignee)

Updated

9 years ago
Attachment #382441 - Flags: approval1.9.1?
Verified fixed with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090703 Minefield/3.6a1pre ID:20090703031447
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → 1.9.1 Branch
No tests were checked in with this, perhaps a litmus test can be added?
Flags: in-litmus?
Attachment #382441 - Flags: approval1.9.1?
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.
(Assignee)

Comment 13

9 years ago
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.
Attachment #382441 - Flags: approval1.9.1.1?
(Assignee)

Updated

9 years ago
Depends on: 495184
Keywords: pp
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?
Attachment #382441 - Flags: approval1.9.1.1? → approval1.9.1.2+
Flags: wanted1.9.1.x?
(Assignee)

Comment 15

9 years ago
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.
status1.9.1: --- → .2-fixed
EventUtils.synthesize[DragStart|Drop] ? That *should* be able to test this, but you can't specify an esc during the drag presently...
(Assignee)

Comment 17

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

9 years ago
(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.
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?
Flags: in-litmus? → in-litmus+
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.