http://trac.webkit.org/changeset/118001 introduced a distinction for the left and right modifier keys (e.g. VK_LCONTROL and VK_RCONTROL), however the existing checks (one instance is staticKeyIdentifiers(unsigned short keyCode) in WebInputEvent.cpp) still rely on the old, location-unaware constants (e.g. VKEY_CONTROL). To test this, just go to http://quirksmode.org/dom/events/tests/keys.html and inspect the details for the "keydown" event fired upon a bare modifier ("Ctrl", "Alt"...) press.
Created attachment 148358 [details] Patch
Comment on attachment 148358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148358&action=review > LayoutTests/ChangeLog:9 > + * platform/chromium/fast/events/keydown-leftright-keys-with-identifier-expected.txt: Added. > + * platform/chromium/fast/events/keydown-leftright-keys-with-identifier.html: Added. Do these tests need to be in platform/chromium? They look cross platform at first glance.
(In reply to comment #2) > (From update of attachment 148358 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148358&action=review > > > LayoutTests/ChangeLog:9 > > + * platform/chromium/fast/events/keydown-leftright-keys-with-identifier-expected.txt: Added. > > + * platform/chromium/fast/events/keydown-leftright-keys-with-identifier.html: Added. > > Do these tests need to be in platform/chromium? They look cross platform at first glance. Unfortunately, not all ports support the new key event translation scheme (VK_LCONTROL/VK_RCONTROL). I'm seeing the original test disabled on Qt and GTK; not sure if it is worth adding an new, almost identical, test to all platforms and skipping it on some of those. I'm also unable to run it against "try bots" to skip on all appropriate platforms at once, so the build breakage is inevitable. WDYT?
I haven't looked very closely. If it's almost identical, maybe add test cases to it, and enjoy the benefit of it already being skipped on platforms that don't support it?
Created attachment 148540 [details] Patch
(In reply to comment #4) > I haven't looked very closely. If it's almost identical, maybe add test cases to it, and enjoy the benefit of it already being skipped on platforms that don't support it? Done.
(In reply to comment #6) > (In reply to comment #4) > > I haven't looked very closely. If it's almost identical, maybe add test cases to it, and enjoy the benefit of it already being skipped on platforms that don't support it? > > Done. @Alexey: ping? Reusing the same test now.
I don't have further comments, thank you for updating the test!
(In reply to comment #8) > I don't have further comments, thank you for updating the test! OK. Should the patch perhaps be r+'ed? :)
(In reply to comment #9) > (In reply to comment #8) > > I don't have further comments, thank you for updating the test! > > OK. Should the patch perhaps be r+'ed? :) Alexey, I'm sorry if it seemed non-obvious: I wanted you to review this patch since you were the reviewer of the offending change, http://trac.webkit.org/changeset/118001.
Comment on attachment 148540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148540&action=review > LayoutTests/fast/events/keydown-leftright-keys.html:55 > - testKeyEventWithLocation("leftShift", VK_SHIFT, "KEY_LOCATION_LEFT"); > - testKeyEventWithLocation("leftControl", VK_CONTROL, "KEY_LOCATION_LEFT"); > - testKeyEventWithLocation("leftAlt", VK_MENU, "KEY_LOCATION_LEFT"); > + testKeyEventWithLocation("leftShift", VK_SHIFT, "KEY_LOCATION_LEFT", "Shift"); > + testKeyEventWithLocation("leftControl", VK_CONTROL, "KEY_LOCATION_LEFT", "Control"); > + testKeyEventWithLocation("leftAlt", VK_MENU, "KEY_LOCATION_LEFT", "Alt"); > > - testKeyEventWithLocation("rightShift", VK_SHIFT, "KEY_LOCATION_RIGHT"); > - testKeyEventWithLocation("rightControl", VK_CONTROL, "KEY_LOCATION_RIGHT"); > - testKeyEventWithLocation("rightAlt", VK_MENU, "KEY_LOCATION_RIGHT"); > + testKeyEventWithLocation("rightShift", VK_SHIFT, "KEY_LOCATION_RIGHT", "Shift"); > + testKeyEventWithLocation("rightControl", VK_CONTROL, "KEY_LOCATION_RIGHT", "Control"); > + testKeyEventWithLocation("rightAlt", VK_MENU, "KEY_LOCATION_RIGHT", "Alt"); Does this pass on Mac, GTK+, Qt, etc...?
(In reply to comment #11) > (From update of attachment 148540 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148540&action=review > Does this pass on Mac, GTK+, Qt, etc...? I had presumed the location distinction was implemented universally, but I just tested it, and the test failed for me on Win. Should I add the modified copy of the original test to platform/chromium instead?
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 148540 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=148540&action=review > > > Does this pass on Mac, GTK+, Qt, etc...? > > I had presumed the location distinction was implemented universally, but I just tested it, and the test failed for me on Win. Should I add the modified copy of the original test to platform/chromium instead? How about Mac and other ports? Should this test eventually pass on all ports? (e.g. is this exposed to the Web such that behaving differently would cause a compat. issue?)
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 148540 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148540&action=review > > > > > Does this pass on Mac, GTK+, Qt, etc...? > > > > I had presumed the location distinction was implemented universally, but I just tested it, and the test failed for me on Win. Should I add the modified copy of the original test to platform/chromium instead? > > How about Mac and other ports? Should this test eventually pass on all ports? (e.g. is this exposed to the Web such that behaving differently would cause a compat. issue?) It is disabled on Qt (looks like Qt makes it impossible to detect the modifier key location), and for the time being I've only been able to test WebKit Windows, where it fails. Yes, I believe ultimately we want all ports to work in a similar (and correct) way.
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (In reply to comment #11) > > > > (From update of attachment 148540 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148540&action=review > > > > > > > Does this pass on Mac, GTK+, Qt, etc...? > > > > > > I had presumed the location distinction was implemented universally, but I just tested it, and the test failed for me on Win. Should I add the modified copy of the original test to platform/chromium instead? > > > > How about Mac and other ports? Should this test eventually pass on all ports? (e.g. is this exposed to the Web such that behaving differently would cause a compat. issue?) > > It is disabled on Qt (looks like Qt makes it impossible to detect the modifier key location), and for the time being I've only been able to test WebKit Windows, where it fails. Yes, I believe ultimately we want all ports to work in a similar (and correct) way. I have tested a Windows port, which also fails. Do you think the patch should fix all available ports, just skip the test on the ones where it fails, or have a Chromium-specific test for this change?
(In reply to comment #15) > I have tested a Windows port, which also fails. Do you think the patch should fix all available ports, just skip the test on the ones where it fails, or have a Chromium-specific test for this change? Oh, and it fails on Mac, too. Perhaps, let's have a Chromium-specific test, since it is Chromium that is really borken? We are seeing the bug manifestation in the Web Inspector/DevTools, and it is quite annoying (oftentimes the suggest box does not show upon Ctrl+Space keypress).
Ideally we should fix all ports, if not, then we should add the test to Skipped or TestExpectations file a separate bug for each port.
https://code.google.com/p/chromium/issues/detail?id=229033