Bug 168186 – [GTK] Handle caps lock indicator in event modifiers

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are not handling it, causing test fast/events/special-key-events-in-input-text.html to fail.

[Carlos Garcia Campos]

Created attachment 301299 [details]
Patch

[WebKit Commit Bot]

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

[Michael Catanzaro]

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."

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r212205: <http://trac.webkit.org/changeset/212205>