Bug 35299

Summary:

[Gtk] mousemove event always has metaKey == true

Product:

WebKit

Reporter:

Grant Gayed <grant_gayed>

Component:

UI Events

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mrobinson, webkit.devdatta, webkit.review.bot

Priority:

Keywords:

PlatformOnly

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
Patch	mrobinson: review-
Patch after incorporating review comments.	mrobinson: review-
Combined patch for EventSender and metaKey state for mouse move	none
Patch after incorporating review comments.	none

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.