35299 – [Gtk] mousemove event always has metaKey == true

RESOLVED FIXED 35299

[Gtk] mousemove event always has metaKey == true

https://bugs.webkit.org/show_bug.cgi?id=35299

Summary [Gtk] mousemove event always has metaKey == true

Grant Gayed

Reported 2010-02-23 09:11:27 PST

- observed with WebKitGTK 1.1.21 - view the page below in GtkLauncher, which just listens for mousemove and shows an alert with the event's metaKey value - mouse into the browser and note that alerts are shown with 'true' even though a modifier key is not being held -> (I'm not actually sure which key should set this value to true on a standard keyboard, I think Command does this on OS X) <html> <head> <script language="JavaScript"> function mousehandler(e) { alert(e.metaKey); } function init() { document.onmousemove = mousehandler; } </script> </head> <body onLoad="init()"> is meta key down? </body> </html>

Attachments
Patch (4.81 KB, patch) 2011-08-28 13:21 PDT, Devdatta Deshpande	mrobinson: review-	Details Formatted Diff Diff
Patch after incorporating review comments. (3.17 KB, patch) 2011-09-18 23:50 PDT, Devdatta Deshpande	mrobinson: review-	Details Formatted Diff Diff
Combined patch for EventSender and metaKey state for mouse move (6.41 KB, patch) 2011-09-19 02:28 PDT, Devdatta Deshpande	no flags	Details Formatted Diff Diff
Patch after incorporating review comments. (6.37 KB, patch) 2011-09-20 22:26 PDT, Devdatta Deshpande	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2010-02-23 09:31:05 PST

This doesn't happen for me in Safari, so it looks Gtk-specific.

Devdatta Deshpande

Comment 2 2011-08-28 13:21:17 PDT

Created attachment 105450 [details] Patch

Martin Robinson

Comment 3 2011-08-28 14:32:16 PDT

Comment on attachment 105450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105450&action=review > Source/WebCore/ChangeLog:5 > + GDK_MOD2_MASK doesn't always mean meta so we can't use it to identify > + the meta key state. Use GDK_META_MASK instead. > + https://bugs.webkit.org/show_bug.cgi?id=35299 The format here should be: bug title bug url review line description > Source/WebCore/ChangeLog:12 > + Test: manual-tests/special-key-states-for-mouse-events.html > + > + * manual-tests/special-key-states-for-mouse-events.html: Added a manual > + test which shows the status of special keys for different mouse events. As long as you are updating EventSender, it makes sense to make an automated test. > Tools/ChangeLog:7 > + GDK_MOD2_MASK doesn't always mean meta so we can't use it to identify > + the meta key state. Use GDK_META_MASK instead. > + https://bugs.webkit.org/show_bug.cgi?id=35299 > + > + Reviewed by NOBODY (OOPS!). See above. > Tools/DumpRenderTree/gtk/EventSender.cpp:278 > - // Currently the metaKey as defined in WebCore/platform/gtk/MouseEventGtk.cpp > - // is GDK_MOD2_MASK. This code must be kept in sync with that file. > else if (JSStringIsEqualToUTF8CString(string, "metaKey")) Why remove this comment instead of updating it?

Devdatta Deshpande

Comment 4 2011-09-18 23:50:43 PDT

Created attachment 107809 [details] Patch after incorporating review comments. Updated patch

Devdatta Deshpande

Comment 5 2011-09-19 00:21:15 PDT

(In reply to comment #3) > (From update of attachment 105450 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105450&action=review > > > Source/WebCore/ChangeLog:5 > > + GDK_MOD2_MASK doesn't always mean meta so we can't use it to identify > > + the meta key state. Use GDK_META_MASK instead. > > + https://bugs.webkit.org/show_bug.cgi?id=35299 > > The format here should be: > > bug title > bug url > > review line > > description > Modified the change log as per review comments. > > Source/WebCore/ChangeLog:12 > > + Test: manual-tests/special-key-states-for-mouse-events.html > > + > > + * manual-tests/special-key-states-for-mouse-events.html: Added a manual > > + test which shows the status of special keys for different mouse events. > > As long as you are updating EventSender, it makes sense to make an automated test. > The metaKey attribute in EventSender was not in sync with the mouse and keyboard events from WebCore. I have added a different bug for this <https://bugs.webkit.org/show_bug.cgi?id=68324>. Also, none of the ports uses modifier keys for mouse move in EventSender, hence a manual test case is added to test modifier keys for mouse move event. > > Tools/ChangeLog:7 > > + GDK_MOD2_MASK doesn't always mean meta so we can't use it to identify > > + the meta key state. Use GDK_META_MASK instead. > > + https://bugs.webkit.org/show_bug.cgi?id=35299 > > + > > + Reviewed by NOBODY (OOPS!). > > See above. > > > Tools/DumpRenderTree/gtk/EventSender.cpp:278 > > - // Currently the metaKey as defined in WebCore/platform/gtk/MouseEventGtk.cpp > > - // is GDK_MOD2_MASK. This code must be kept in sync with that file. > > else if (JSStringIsEqualToUTF8CString(string, "metaKey")) > > Why remove this comment instead of updating it? Updated the comment in the patch for Bug 68324.

Martin Robinson

Comment 6 2011-09-19 00:38:23 PDT

Comment on attachment 105450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105450&action=review >>> Source/WebCore/ChangeLog:12 >>> + test which shows the status of special keys for different mouse events. >> >> As long as you are updating EventSender, it makes sense to make an automated test. > > The metaKey attribute in EventSender was not in sync with the mouse and keyboard events from WebCore. I have added a different bug for this <https://bugs.webkit.org/show_bug.cgi?id=68324>. > > Also, none of the ports uses modifier keys for mouse move in EventSender, hence a manual test case is added to test modifier keys for mouse move event. I think it makes sense to add this to the EventSender and make the test GTK+ only for now. Manual tests don't protect us from regressions. I'm not sure why you use a separate bug to fix EventSender, since this patch changes it below. I think it's better as one patch, since you can make the fix and test it all at once.

Martin Robinson

Comment 7 2011-09-19 00:39:11 PDT

Comment on attachment 107809 [details] Patch after incorporating review comments. This patch should fix EventSender and use an automated test.

Devdatta Deshpande

Comment 8 2011-09-19 02:28:57 PDT

Created attachment 107820 [details] Combined patch for EventSender and metaKey state for mouse move I have combined both EventSender and mouse move patch into one and added an automated test under platform/gtk to test both the changes.

Martin Robinson

Comment 9 2011-09-20 15:27:37 PDT

Comment on attachment 107820 [details] Combined patch for EventSender and metaKey state for mouse move View in context: https://bugs.webkit.org/attachment.cgi?id=107820&action=review > LayoutTests/platform/gtk/fast/events/event-sender-metakey.html:16 > + if(true == failed) > + return; > + if(e.metaKey != metaKey) > + failed = true; > + } You're missing spaces after both 'if's here. > LayoutTests/platform/gtk/fast/events/event-sender-metakey.html:40 > + resultDiv.innerHTML = (true == failed) ? "FAIL" : "PASS"; Why not just this resultDiv.innerHTML = failed ? "FAIL" : "PASS"; ?

Devdatta Deshpande

Comment 10 2011-09-20 22:26:36 PDT

Created attachment 108108 [details] Patch after incorporating review comments.

Martin Robinson

Comment 11 2011-09-21 08:55:33 PDT

Comment on attachment 108108 [details] Patch after incorporating review comments. Thank you!

Devdatta Deshpande

Comment 12 2011-10-21 05:10:46 PDT

Hey Martin, Can you please help me commit this patch? I am unable to commit this as I am not a commiter. Thanks.

WebKit Review Bot

Comment 13 2011-10-21 10:27:14 PDT

Comment on attachment 108108 [details] Patch after incorporating review comments. Clearing flags on attachment: 108108 Committed r98108: <http://trac.webkit.org/changeset/98108>

WebKit Review Bot

Comment 14 2011-10-21 10:27:19 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS Linux

Product WebKit

Component UI Events

Assignee

Nobody

Reported

2010-02-23 09:11 PST

Modified

2011-10-21 10:27 PDT History

CC List

3 users Show

URL

Keywords PlatformOnly

Depends on

Blocks