Summary: | [GTK] Handle caps lock indicator in event modifiers | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | berto, bugs-noreply, commit-queue, gustavo, mcatanzaro, mrobinson | ||||
Priority: | P2 | Keywords: | Gtk, LayoutTestFailure | ||||
Version: | WebKit Local Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2017-02-12 01:59:15 PST
Created attachment 301299 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 301299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301299&action=review > LayoutTests/ChangeLog:9 > + Add platform specific results for fast/events/special-key-events-in-input-text.html. This patch fixes the caps > + lock key case, but we still have different results in the PrintScreen case. What's different about the PrintScreen case? Do we not ever want to fix it? > Source/WebCore/platform/PlatformKeyboardEvent.h:169 > + static bool modifiersContainCapsLock(unsigned); This header is already a mess, so I guess it's OK, but I dislike adding a special function here just for CapsLock. I'd prefer a generic function: static bool modifiersContainModifier(unsigned, PlatformEvent::Modifier) that you would pass PlatformEvent::Modifier::CapsLockKey to. Even if it's currently only used for CapsLock. But your choice. > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:1325 > + // In X11 GDK_LOCK_MASK could be CapsLock or ShiftLock. What GTK+ does in the X11 backend is checking if This code really confused me. I wrote a decent-length WTF comment to ask you to explain why it would ever be impossible for GTK+ to generate GDK_KEY_Caps_Lock. The key is to understand that it depends on the nutty modifier mapping of the X server. So I'd add that to your comment: "In X11 GDK_LOCK_MASK could be CapsLock or ShiftLock, depending on the modifier mapping of the X server." (In reply to comment #3) > Comment on attachment 301299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301299&action=review > > > LayoutTests/ChangeLog:9 > > + Add platform specific results for fast/events/special-key-events-in-input-text.html. This patch fixes the caps > > + lock key case, but we still have different results in the PrintScreen case. > > What's different about the PrintScreen case? Do we not ever want to fix it? -INPUT - keydown - false,false,false,false,false - PrintScreen - KeyA - PrintScreen - 44 - 0. Value: "". -INPUT - keyup - false,false,false,false,false - PrintScreen - KeyA - PrintScreen - 44 - 0. Value: "". +INPUT - keydown - false,false,false,false,false - PrintScreen - PrintScreen - PrintScreen - 44 - 0. Value: "". +INPUT - keyup - false,false,false,false,false - PrintScreen - PrintScreen - PrintScreen - 44 - 0. Value: "". I don't think there's anything to fix there, KeyA doesn't look like the right key code for PrintScreen. > > Source/WebCore/platform/PlatformKeyboardEvent.h:169 > > + static bool modifiersContainCapsLock(unsigned); > > This header is already a mess, so I guess it's OK, but I dislike adding a > special function here just for CapsLock. I'd prefer a generic function: > > static bool modifiersContainModifier(unsigned, PlatformEvent::Modifier) > > that you would pass PlatformEvent::Modifier::CapsLockKey to. Even if it's > currently only used for CapsLock. But your choice. There are other similar static functions there and currentCapsLockState(). > > Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:1325 > > + // In X11 GDK_LOCK_MASK could be CapsLock or ShiftLock. What GTK+ does in the X11 backend is checking if > > This code really confused me. I wrote a decent-length WTF comment to ask you > to explain why it would ever be impossible for GTK+ to generate > GDK_KEY_Caps_Lock. The key is to understand that it depends on the nutty > modifier mapping of the X server. So I'd add that to your comment: > > "In X11 GDK_LOCK_MASK could be CapsLock or ShiftLock, depending on the > modifier mapping of the X server." Ok. Committed r212205: <http://trac.webkit.org/changeset/212205> |