|Summary:||"event.ctrlKey" is always false when dragging an element with "ctrl" key down|
|Component:||WebCore Misc.||Assignee:||Jessie Berlin <jberlin>|
|Severity:||Normal||CC:||ap, jberlin, mitz, rboucher|
|Version:||523.x (Safari 3)|
Description fengjin 2008-01-31 01:54:22 PST
Comment 1 Alexey Proskuryakov 2008-01-31 03:00:49 PST
Created attachment 18814 [details] test case Same test, as an attachment.
Comment 3 boucher 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"
Comment 4 Jessie Berlin 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)
Comment 5 Jessie Berlin 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.
Comment 6 Jessie Berlin 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).
Comment 7 mitz 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.
Comment 8 Jessie Berlin 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.
Comment 9 Alexey Proskuryakov 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.
Comment 10 Sam Weinig 2010-03-13 21:16:01 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 Please remove the Oops before landing.
Comment 11 mitz 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.
Comment 12 Alexey Proskuryakov 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?
Comment 13 Jessie Berlin 2010-03-14 08:45:44 PDT
Comment on attachment 50665 [details] Use the new PlatformKeyboardEvent::getCurrentModifierState() to add the information about which modifier keys are pressed to the drag event Committed in r55974: http://trac.webkit.org/changeset/55974
Comment 14 Alexey Proskuryakov 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?
Comment 15 Jessie Berlin 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.
Comment 16 mitz 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.
Comment 17 Jessie Berlin 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.