606885 - drop/drag/dragend events do not reflect real ctrl/shift/alt key status

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Beck Yang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-TW; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-TW; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10

Please verify the steps to reproduce.

This problem can be reproduced in :
 - XULRunner nightly build: v1.9.2.12pre - 20101024035027 for windows.
 - Firefox 3.5.11 for Linux 32bit: Mozilla/5.0 (X11; U; Linux i686; zh-TW; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11


Reproducible: Always

Steps to Reproduce:
1.Open dragend.htm
2.Double click "Try" in the area that background color is AQUA.
3.Move mouse over the area and hold mouse left key (start drag)
4.Hold ctrl key and drop it to green area.
Actual Results:  
The dragEnd/dragOver/dragStart event handling script will output event key information to text area.
The first line is "dragEnd, ctrlKey:false, shiftKey:false"

Expected Results:  
The first line should be "dragEnd, ctrlKey:true, shiftKey:false"

Comment 1

[Beck Yang]

Attachment: For reproduce problem

Comment 2

[danya.postfactum]

Attachment: Simpler testcase (open console to see messages)

Hey guys, this problem is still not fixed.

As I see, event.ctrlKey, event.shiftKey etc don't reflect the real state on drop event (at least) on Linux Firefox 16.

Please see my simple test case to reproduce this issue (open console to see the log, drag and drop the link, try to hold ctrl (shift, alt) while dropping it)

Comment 3

[danya.postfactum]

Anybody here? Is it hard to fix? Sorry, I'm webdeveloper, not C++ one ((

Comment 4

[Nelson]

I can confirm this is still a bug in Firefox 38.0.1.
We're creating a multi-platform solution (Using Firefox as the recommended browser) that allows users to drag & drop items and based on key modifiers, take certain actions. It works in other platforms, but not in Linux.
I'm not a developer that can fix a browser (I don't think) and even if I find in the code where to fix it, how will it get into the main release?

Comment 5

[bevinhex]

Bug still exists, can't get ctrlKey, shiftKey in dragStart or dragEnd
I'm a webdeveloper also C++, How can I help?

Comment 6

[bevinhex]

(In reply to bevinhex from comment #5)
> Bug still exists, can't get ctrlKey, shiftKey in dragStart or dragEnd
> I'm a webdeveloper also C++, How can I help?

Correction: I can detect in dragStart event, not in dragEnd

Comment 7

[Ian Moody [:Kwan] (UTC+0)]

So stumbled upon this in Stylish's instance of the scratchpad/codemirror, which led to https://github.com/codemirror/CodeMirror/issues/4249
However mdn ( https://developer.mozilla.org/en-US/docs/Web/Events/drop ) says drop should have ctrlKey etc, and I can't see anything obvious in the linked spec to say it shouldn't, and chromium behaves correctly in this instance, at least on linux

As far as I can tell the only DragEvents with this problem are drag, drop, and dropend

dragenter, dragexit, dragleave, dragover, and dragstart all correctly report the key modifiers.

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part1: Cache keyboard modifiers in nsDragService to fire drag events with modifiers

The major changes are
1.Cached keyboard modifers in nsDragService when content receiving real drag event (so that dragservice can fire drag events with modifiers)
2.Update keyboard modifiers in nsDragService::StartDragSession and nsDragService::EndDragSession
3.Pass keyboard modifiers when IPC InvokeDragSession and EndDragSession to content

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part2: Cache keyboard modifiers in nsDragService on Windows

Separate the patches for different platforms since they should be reviewed by different reviewers

Comment 10

[Ming-Chou Shih [:stone]]

Attachment: Part3: Cache keyboard modifiers in nsDragService on Mac

Separate the patches for different platforms since they should be reviewed by different reviewers

Comment 11

[Neil Deakin]

Rather than caching the keyboard modifier state, you should just change FireDragEventAtSource and EndDragSession method signatures to pass the modifiers along as arguments.

I'm unclear why you need to change the dragstart case; it should be inheriting the modifiers from the mousedown event. We should instead figure out why that isn't happening (I am assuming it isn't).

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Yeah I think I agree with Enn.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

But if there is some reason for the approach you took, please explain and ask review again.

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: Fire drag events with keyboard modifiers

Comment 15

[Ming-Chou Shih [:stone]]

That's because I can't get the proper keyboard modifiers in the following cases. e.g. We'd call EndDragSession when document's PresShell is null in TabParent::RecvInvokeDragSession or in nsGlobalWindow::EnterModalState. This is the patch I tried to not cache keyboard modifiers. May I know if you have some suggestions? Thanks.

Comment 16

[Neil Deakin]

(In reply to Ming-Chou Shih [:stone] from comment #15)
> That's because I can't get the proper keyboard modifiers in the following
> cases. e.g. We'd call EndDragSession when document's PresShell is null in
> TabParent::RecvInvokeDragSession or in nsGlobalWindow::EnterModalState.

Both of those are drag failed cases so passing 0 for modifiers should be ok.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Fire drag events with keyboard modifiers

Comment 18

[Neil Deakin]

Comment on attachment 8839395 [details] [diff] [review]
Fire drag events with keyboard modifiers

>diff --git a/dom/events/test/test_bug606885.html b/dom/events/test/test_bug606885.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/events/test/test_bug606885.html
>@@ -0,0 +1,78 @@

Note that this test you added doesn't execute any of the code changes, so doesn't actually test this bug. (The test passes without any of the rest of the patch). I'm ok with having the test anyway since it probably verifies something useful, but you might rename the test file and comment in the test to disassociate it with this specific bug.

You won't be able to write a real test for this bug.


>diff --git a/widget/nsIDragService.idl b/widget/nsIDragService.idl
>--- a/widget/nsIDragService.idl
>+++ b/widget/nsIDragService.idl
>@@ -119,22 +119,23 @@ interface nsIDragService : nsISupports
> 
>   /**
>     * Tells the Drag Service to end a drag session. This is called when
>     * an external drag occurs
>     *
>     * If aDoneDrag is true, the drag has finished, otherwise the drag has
>     * just left the window.
>     */
>-  void endDragSession ( in boolean aDoneDrag ) ;
>+  void endDragSession(in boolean aDoneDrag, in unsigned long aKeyModifiers);

You could also mark the new extra argument as [optional] to avoid changing all of tests.

Comment 19

[Neil Deakin]

One note I should say is that one should not use the keyboard modifier state to determine whether a copy vs move should be done as this does not accurately reflect this. The correct way is to use dataTransfer.dropEffect.

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Fire drag events with keyboard modifiers

Followed the review comments to refine the patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e20242e7851516761576802d664a33ce01ae25b

Comment 21

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=200ab7ebe421610f5ef3f96bfb9f186446f93a3d

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f60181551b
Fire drag events with keyboard modifiers. r=enn

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4f60181551b