Bug 17113

Summary:

"event.ctrlKey" is always false when dragging an element with "ctrl" key down

Product:

WebKit

Reporter:

fengjin <fengjin>

Component:

WebCore Misc.

Assignee:

Jessie Berlin <jberlin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, jberlin, mitz, rboucher

Priority:

Version:

523.x (Safari 3)

Hardware:

All

OS:

All

Attachments:

Description	Flags
test case	none
Implement returning the correct state of the modifier keys being pressed or not during a drag for Mac, Chromium, Haiku, and Windows platforms.	none
Use the new PlatformKeyboardEvent::getCurrentModifierState() to add the information about which modifier keys are pressed to the drag event	none

fengjin

Reported 2008-01-31 01:54:22 PST

repro: (1) Keeping the ctrl key down, and drag a link (2) Drag it over a "div" element (3) On the onDragOver event of the "div" element, testing the event.ctrlKey property, its always "false". It should be "true" ------- HTML source repro this: ------- <html> <script language="javascript"> window.onload = function() { var oDrop = document.getElementById("divDrop"); oDrop.addEventListener("dragover", onDragOverDiv, false); oDrop.addEventListener("dragenter", onDragOverDiv, false); } function onDragOverDiv(e) { e = e || window.event; e.dataTransfer.dropEffect = "copy"; shwTxt("e.ctrlKey == : " + e.ctrlKey); } function shwTxt(s) { document.getElementById("divOutput").innerHTML += "<br>" + s; } </script> <body> <div id="divDrop">drop on me</div> <a href="www.google.com" id="divDrag">Drag me</a> <div id="divOutput" style="top:200px"></div> </body> </html>

Attachments
test case (682 bytes, text/html) 2008-01-31 03:00 PST, Alexey Proskuryakov	no flags	Details
Implement returning the correct state of the modifier keys being pressed or not during a drag for Mac, Chromium, Haiku, and Windows platforms. (11.67 KB, patch) 2010-03-13 13:18 PST, Jessie Berlin	no flags	Details Formatted Diff Diff
Use the new PlatformKeyboardEvent::getCurrentModifierState() to add the information about which modifier keys are pressed to the drag event (1.82 KB, patch) 2010-03-13 17:36 PST, Jessie Berlin	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2008-01-31 03:00:49 PST

Created attachment 18814 [details] test case Same test, as an attachment.

Alexey Proskuryakov

Comment 2 2008-01-31 03:07:21 PST

Confirmed with r29869.

boucher

Comment 3 2009-10-30 11:57:19 PDT

Should be noted that this isn't just true for the control key. It's all keyboard information (shift, meta, alt, etc.). Section 7.9.3 of HTML5 states: "the mouse and key attributes [must be] set according to the state of the input devices as they would be for user interaction events"

Jessie Berlin

Comment 4 2010-03-13 13:18:12 PST

Created attachment 50662 [details] Implement returning the correct state of the modifier keys being pressed or not during a drag for Mac, Chromium, Haiku, and Windows platforms. Tested on Mac OS X 10.6 As noted in the ChangeLog, did not add in tests here because of a bug with DumpRenderTree: I have written a test for the Mac platform that is not included in this patch and depends on https://bugs.webkit.org/show_bug.cgi?id=36085 (DumpRenderTree eventSender.mouseDown should change NSApp currentEvent return value) being fixed. Should I include it in this fix and just add it to the skip list until that bug is fixed? Also, should I split off this bug into multiple smaller bugs, one per each platform which I did not provide an implementation? (Qt, GTK, EFL, WX)

Jessie Berlin

Comment 5 2010-03-13 13:31:37 PST

Comment on attachment 50662 [details] Implement returning the correct state of the modifier keys being pressed or not during a drag for Mac, Chromium, Haiku, and Windows platforms. Will post a patch soon that usesPlatformKeyboardEvent::getCurrentModifierState() from http://trac.webkit.org/changeset/55909 instead.

Jessie Berlin

Comment 6 2010-03-13 17:36:06 PST

Created attachment 50665 [details] Use the new PlatformKeyboardEvent::getCurrentModifierState() to add the information about which modifier keys are pressed to the drag event As in the last patch, I did not add in tests here (hence the No new tests (OOPS!) in the ChangeLog) because of a bug with DumpRenderTree, similar to the bug https://bugs.webkit.org/show_bug.cgi?id=36085 . At least on the Mac, the mechanism used to implement getCurrentModifierState (GetCurrentKeyModifiers()) does not appear to get modified by the eventSender.mouseDown method. Should I include that failing test in this fix and just add it to the skip list for now? Any ideas on what might be used to make GetCurrentKeyModifiers (which appears to be a Carbon method) use the information from the eventSender.mouseDown method? The other option would be a manual test, but I talked earlier to Krit, and he strongly suggested against that (for the good reason that manual tests are less likely to get run).

mitz

Comment 7 2010-03-13 17:44:56 PST

Comment on attachment 50665 [details] Use the new PlatformKeyboardEvent::getCurrentModifierState() to add the information about which modifier keys are pressed to the drag event Resolving bug 36085 will not affect the testability of this change, because getCurrentModifierState does not use -currentEvent on Mac OS X. I think it cannot use -currentEvent because changes to modifier key state don’t result in an event, but I could be wrong.

Jessie Berlin

Comment 8 2010-03-13 19:08:38 PST

(In reply to comment #7) > (From update of attachment 50665 [details]) > Resolving bug 36085 will not affect the testability of this change Right, resolving that bug won't do anything in this case. It would have helped with my previous patch. However, I am still fairly sure that there is an issue with eventSender.mouseDown that is causing my layout test to fail while my manual testing works. Should I just leave this with no tests? > , because > getCurrentModifierState does not use -currentEvent on Mac OS X. I think it > cannot use -currentEvent because changes to modifier key state don’t result in > an event, but I could be wrong. Do you know of any way to essentially trick getCurrentModifierState into believing that there is a modifier key being held down? As it currently stands, eventSender.mouseDown just sends the new event with the modifier key state set according to the arguments to eventSender.mouseDown to the NSView. If there is a way to get that information to getCurrentModifierState, then I am pretty sure my layout test would work.

Alexey Proskuryakov

Comment 9 2010-03-13 19:22:32 PST

As an aside, it's rather suspicious that PlatformKeyboardEvent::getCurrentModifierState() uses GetCurrentKeyModifiers(), and not GetCurrentEventKeyModifiers(). But I don't think that even the latter will be affected by anything done at AppKit level.

Sam Weinig

Comment 10 2010-03-13 21:16:01 PST

mitz

Comment 11 2010-03-13 21:19:46 PST

(In reply to comment #9) > As an aside, it's rather suspicious that > PlatformKeyboardEvent::getCurrentModifierState() uses GetCurrentKeyModifiers(), > and not GetCurrentEventKeyModifiers(). Why? It is used (in EventHandler.cpp) to fabricate a fake event with the current state of the modifier keys, not their last known state.

Alexey Proskuryakov

Comment 12 2010-03-13 21:33:22 PST

That's I didn't say it was incorrect, just suspicious. Why do we want to use modifier state that isn't event-synchronized?

Jessie Berlin

Comment 13 2010-03-14 08:45:44 PDT

Alexey Proskuryakov

Comment 14 2010-03-15 10:00:16 PDT

Looking at this change though, I'd say that using non-synchronized modifier state is likely incorrect! This difference can become noticeable on a busy machine. Jessie, what do you think?

Jessie Berlin

Comment 15 2010-03-15 10:23:52 PDT

(In reply to comment #14) > Looking at this change though, I'd say that using non-synchronized modifier > state is likely incorrect! This difference can become noticeable on a busy > machine. > > Jessie, what do you think? I could see how on a busy / slow enough machine if we know that GetCurrentEventKeyModifiers remains in sync with the slower execution of this DragController code, then it might be correct to use GetCurrentEventKeyModifiers instead of GetCurrentKeyModifiers (which might report the immediate state instead of the one in sync with the slowed down DragController code). However, do we know that the GetCurrentEventKeyModifiers would necessarily remain in sync with slow execution of the DragController code? Or is this an unrealistic worry? If we don't, then I think either solution would work.

mitz

Comment 16 2010-03-15 10:45:23 PDT

If this code runs as part of handling an event, shouldn't it just pass the event along? I'm confused.

Jessie Berlin

Comment 17 2010-03-15 13:12:55 PDT

(In reply to comment #16) > If this code runs as part of handling an event, shouldn't it just pass the > event along? I'm confused. It doesn't run directly as part of handling the actual dragging event, at least on the Mac. Once DragController is told to start the drag as a result of the EventHandler figuring out that the mouse move event is actually a drag, it hands it off to doSystemDrag. On the Mac, this means that it goes into NSCoreDragManager. When NSCoreDragTrackingProc tells the WebView about the dragging, it doesn't give it an event, it gives it an NSDraggingInfo, in which there is a lot of the information that would be in an event but not the information about the state of the keys that are pressed. As I understand it, that is why there is no event to pass along and DragController has to create a PlatformMouseEvent to work with. We could store the original event when the drag got started and use the information from that, but that might become incorrect if the user let go of some keys in the process of doing the drag. I don't know what event would be the "CurrentEvent" used by GetCurrentEventKeyModifiers, but I was going along the assumption that it would be more recent than the original one that caused DragController to call doSystemDrag in the first place but might be in sync with the DragController code executing when I was responding to Alexey's question. That probably was not an assumption I should have made.

Note You need to log in before you can comment on or make changes to this bug.