Closed Bug 606885 Opened 9 years ago Closed 3 years ago

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

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: beckyang, Assigned: stone)

Details

Attachments

(3 files, 5 obsolete files)

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"
Attached file For reproduce problem
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)
Attachment #679172 - Attachment mime type: text/plain → text/html
Anybody here? Is it hard to fix? Sorry, I'm webdeveloper, not C++ one ((
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?
Bug still exists, can't get ctrlKey, shiftKey in dragStart or dragEnd
I'm a webdeveloper also C++, How can I help?
(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
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.
Summary: DragEnd event does not reflect real ctrl/shift/alt key status → drop/drag/dragend events do not reflect real ctrl/shift/alt key status
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Assignee: nobody → sshih
Separate the patches for different platforms since they should be reviewed by different reviewers
Separate the patches for different platforms since they should be reviewed by different reviewers
Attachment #8837953 - Flags: review?(bugs)
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).
Yeah I think I agree with Enn.
Attachment #8837953 - Flags: review?(bugs)
But if there is some reason for the approach you took, please explain and ask review again.
Attachment #8837953 - Attachment is obsolete: true
Attachment #8837955 - Attachment is obsolete: true
Attachment #8837956 - Attachment is obsolete: true
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.
Flags: needinfo?(enndeakin)
(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.
Flags: needinfo?(enndeakin)
Attachment #8838384 - Attachment is obsolete: true
Attachment #8839395 - Flags: review?(enndeakin)
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.
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.
Attachment #8839395 - Flags: review?(enndeakin) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f60181551b
Fire drag events with keyboard modifiers. r=enn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4f60181551b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.