LibWeb+UI: Detect and handle left vs. right modifier keys

Our handling of left vs. right modifiers keys (shift, ctrl, etc.) was largely not to spec. This patch adds explicit UIEvents::KeyCode values for these keys, and updates the UI to match native key events to these keys (as best as we are able).
Author: https://github.com/trflynn89 Commit: https://github.com/LadybirdBrowser/ladybird/commit/4fcaeabe1a6 Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/1699
2025-04-24 05:25:13 +00:00 · 2024-10-09 08:56:55 -04:00 · 2024-10-09 08:56:55 -04:00 · 4fcaeabe1a · 2024-10-09 17:10:55 +00:00
commit 4fcaeabe1a
parent 448754d95d
9 changed files with 103 additions and 60 deletions
--- a/Ladybird/AppKit/UI/Event.mm
+++ b/Ladybird/AppKit/UI/Event.mm
@ -226,8 +226,8 @@ static Web::UIEvents::KeyCode ns_key_code_to_key_code(unsigned short key_code, W
    case kVK_ANSI_Semicolon: return Web::UIEvents::KeyCode::Key_Semicolon;
    case kVK_ANSI_Slash: return Web::UIEvents::KeyCode::Key_Slash;
    case kVK_CapsLock: return Web::UIEvents::KeyCode::Key_CapsLock;
-    case kVK_Command: return Web::UIEvents::KeyCode::Key_Super;
-    case kVK_Control: return Web::UIEvents::KeyCode::Key_Control;
+    case kVK_Command: return Web::UIEvents::KeyCode::Key_LeftSuper;
+    case kVK_Control: return Web::UIEvents::KeyCode::Key_LeftControl;
    case kVK_Delete: return Web::UIEvents::KeyCode::Key_Backspace;
    case kVK_DownArrow: return Web::UIEvents::KeyCode::Key_Down;
    case kVK_End: return Web::UIEvents::KeyCode::Key_End;
@ -247,16 +247,16 @@ static Web::UIEvents::KeyCode ns_key_code_to_key_code(unsigned short key_code, W
    case kVK_ForwardDelete: return Web::UIEvents::KeyCode::Key_Delete;
    case kVK_Home: return Web::UIEvents::KeyCode::Key_Home;
    case kVK_LeftArrow: return Web::UIEvents::KeyCode::Key_Left;
-    case kVK_Option: return Web::UIEvents::KeyCode::Key_Alt;
+    case kVK_Option: return Web::UIEvents::KeyCode::Key_LeftAlt;
    case kVK_PageDown: return Web::UIEvents::KeyCode::Key_PageDown;
    case kVK_PageUp: return Web::UIEvents::KeyCode::Key_PageUp;
    case kVK_Return: return Web::UIEvents::KeyCode::Key_Return;
    case kVK_RightArrow: return Web::UIEvents::KeyCode::Key_Right;
-    case kVK_RightCommand: return Web::UIEvents::KeyCode::Key_Super; // FIXME: We do not distinguish left-vs-right.
-    case kVK_RightControl: return Web::UIEvents::KeyCode::Key_Control; // FIXME: We do not distinguish left-vs-right.
-    case kVK_RightOption: return Web::UIEvents::KeyCode::Key_Alt; // FIXME: We do not distinguish left-vs-right.
+    case kVK_RightCommand: return Web::UIEvents::KeyCode::Key_RightSuper;
+    case kVK_RightControl: return Web::UIEvents::KeyCode::Key_RightControl;
+    case kVK_RightOption: return Web::UIEvents::KeyCode::Key_RightAlt;
    case kVK_RightShift: return Web::UIEvents::KeyCode::Key_RightShift;
-    case kVK_Shift: return Web::UIEvents::KeyCode::Key_Shift;
+    case kVK_Shift: return Web::UIEvents::KeyCode::Key_LeftShift;
    case kVK_Space: return Web::UIEvents::KeyCode::Key_Space;
    case kVK_Tab: return Web::UIEvents::KeyCode::Key_Tab;
    case kVK_UpArrow: return Web::UIEvents::KeyCode::Key_Up;
--- a/Ladybird/Qt/WebContentView.cpp
+++ b/Ladybird/Qt/WebContentView.cpp
@ -244,8 +244,6 @@ static Web::UIEvents::KeyModifier get_modifiers_from_qt_key_event(QKeyEvent cons
        modifiers |= Web::UIEvents::KeyModifier::Mod_Super;
    if (event.modifiers().testFlag(Qt::ShiftModifier))
        modifiers |= Web::UIEvents::KeyModifier::Mod_Shift;
-    if (event.modifiers().testFlag(Qt::AltModifier))
-        modifiers |= Web::UIEvents::KeyModifier::Mod_AltGr;
    if (event.modifiers().testFlag(Qt::KeypadModifier))
        modifiers |= Web::UIEvents::KeyModifier::Mod_Keypad;
    return modifiers;
@ -264,8 +262,12 @@ static Web::UIEvents::KeyCode get_keycode_from_qt_key_event(QKeyEvent const& eve
        Web::UIEvents::KeyCode serenity_key;
    };

+    // FIXME: Qt does not differentiate between left-and-right modifier keys. Unfortunately, it seems like we would have
+    //        to inspect event.nativeScanCode() / event.nativeVirtualKey() to do so, which has platform-dependent values.
+    //        For now, we default to left keys.
+
    // https://doc.qt.io/qt-6/qt.html#Key-enum
-    constexpr Mapping mappings[] = {
+    static constexpr Mapping mappings[] = {
        { Qt::Key_0, Web::UIEvents::Key_0 },
        { Qt::Key_1, Web::UIEvents::Key_1 },
        { Qt::Key_2, Web::UIEvents::Key_2 },
@ -277,7 +279,7 @@ static Web::UIEvents::KeyCode get_keycode_from_qt_key_event(QKeyEvent const& eve
        { Qt::Key_8, Web::UIEvents::Key_8 },
        { Qt::Key_9, Web::UIEvents::Key_9 },
        { Qt::Key_A, Web::UIEvents::Key_A },
-        { Qt::Key_Alt, Web::UIEvents::Key_Alt },
+        { Qt::Key_Alt, Web::UIEvents::Key_LeftAlt },
        { Qt::Key_Ampersand, Web::UIEvents::Key_Ampersand },
        { Qt::Key_Apostrophe, Web::UIEvents::Key_Apostrophe },
        { Qt::Key_AsciiCircum, Web::UIEvents::Key_Circumflex },
@ -296,7 +298,7 @@ static Web::UIEvents::KeyCode get_keycode_from_qt_key_event(QKeyEvent const& eve
        { Qt::Key_CapsLock, Web::UIEvents::Key_CapsLock },
        { Qt::Key_Colon, Web::UIEvents::Key_Colon },
        { Qt::Key_Comma, Web::UIEvents::Key_Comma },
-        { Qt::Key_Control, Web::UIEvents::Key_Control },
+        { Qt::Key_Control, Web::UIEvents::Key_LeftControl },
        { Qt::Key_D, Web::UIEvents::Key_D },
        { Qt::Key_Delete, Web::UIEvents::Key_Delete },
        { Qt::Key_Dollar, Web::UIEvents::Key_Dollar },
@ -334,7 +336,7 @@ static Web::UIEvents::KeyCode get_keycode_from_qt_key_event(QKeyEvent const& eve
        { Qt::Key_Less, Web::UIEvents::Key_LessThan },
        { Qt::Key_M, Web::UIEvents::Key_M },
        { Qt::Key_Menu, Web::UIEvents::Key_Menu },
-        { Qt::Key_Meta, Web::UIEvents::Key_Super },
+        { Qt::Key_Meta, Web::UIEvents::Key_LeftSuper },
        { Qt::Key_Minus, Web::UIEvents::Key_Minus },
        { Qt::Key_N, Web::UIEvents::Key_N },
        { Qt::Key_NumberSign, Web::UIEvents::Key_Hashtag },
@ -362,8 +364,8 @@ static Web::UIEvents::KeyCode get_keycode_from_qt_key_event(QKeyEvent const& eve
        { Qt::Key_Shift, Web::UIEvents::Key_LeftShift },
        { Qt::Key_Slash, Web::UIEvents::Key_Slash },
        { Qt::Key_Space, Web::UIEvents::Key_Space },
-        { Qt::Key_Super_L, Web::UIEvents::Key_Super },
-        { Qt::Key_Super_R, Web::UIEvents::Key_Super },
+        { Qt::Key_Super_L, Web::UIEvents::Key_LeftSuper },
+        { Qt::Key_Super_R, Web::UIEvents::Key_RightSuper },
        { Qt::Key_SysReq, Web::UIEvents::Key_SysRq },
        { Qt::Key_T, Web::UIEvents::Key_T },
        { Qt::Key_Tab, Web::UIEvents::Key_Tab },
--- a/Tests/LibWeb/Text/expected/UIEvents/KeyEvent-functional-keys.txt
+++ b/Tests/LibWeb/Text/expected/UIEvents/KeyEvent-functional-keys.txt
@ -0,0 +1,13 @@
+key=CapsLock code=CapsLock
+key=Escape code=Escape
+key=Enter code=Enter
+key=  code=Space
+key=Tab code=Tab
+key=Alt code=AltLeft
+key=Alt code=AltRight
+key=Control code=ControlLeft
+key=Control code=ControlRight
+key=Shift code=ShiftLeft
+key=Shift code=ShiftRight
+key=Meta code=MetaLeft
+key=Meta code=MetaRight
--- a/Tests/LibWeb/Text/expected/UIEvents/special-keys.txt
+++ b/Tests/LibWeb/Text/expected/UIEvents/special-keys.txt
@ -1 +0,0 @@
-Escape
--- a/Tests/LibWeb/Text/input/UIEvents/KeyEvent-functional-keys.html
+++ b/Tests/LibWeb/Text/input/UIEvents/KeyEvent-functional-keys.html
@ -0,0 +1,32 @@
+<input id="input" />
+<script src="../include.js"></script>
+<script>
+    const FUNCTIONAL_KEYS = [
+        "CapsLock",
+        "Escape",
+        "Return",
+        "Space",
+        "Tab",
+        "LeftAlt",
+        "RightAlt",
+        "LeftControl",
+        "RightControl",
+        "LeftShift",
+        "RightShift",
+        "LeftSuper",
+        "RightSuper",
+    ];
+
+    test(() => {
+        let input = document.getElementById("input");
+
+        input.addEventListener("keydown", e => {
+            println(`key=${e.key} code=${e.code}`);
+            e.preventDefault();
+        });
+
+        FUNCTIONAL_KEYS.forEach(key => {
+            internals.sendKey(input, key);
+        });
+    });
+</script>
--- a/Tests/LibWeb/Text/input/UIEvents/special-keys.html
+++ b/Tests/LibWeb/Text/input/UIEvents/special-keys.html
@ -1,17 +0,0 @@
-<input id="input" />
-<script src="../include.js"></script>
-<script>
-    const SPECIAL_KEYS = ["Escape"];
-
-    test(() => {
-        let input = document.getElementById("input");
-
-        input.addEventListener("keydown", e => {
-            println(`${e.key}`);
-        });
-
-        SPECIAL_KEYS.forEach(key => {
-            internals.sendKey(input, "Escape");
-        });
-    });
-</script>
--- a/Userland/Libraries/LibWeb/Page/EventHandler.cpp
+++ b/Userland/Libraries/LibWeb/Page/EventHandler.cpp
@ -1072,7 +1072,7 @@ EventResult EventHandler::handle_keydown(UIEvents::KeyCode key, u32 modifiers, u
        return EventResult::Handled;
    case UIEvents::KeyCode::Key_Left:
    case UIEvents::KeyCode::Key_Right:
-        if (modifiers > UIEvents::KeyModifier::Mod_Alt && modifiers != (UIEvents::KeyModifier::Mod_Alt | UIEvents::KeyModifier::Mod_AltGr))
+        if (modifiers && modifiers != UIEvents::KeyModifier::Mod_Alt)
            break;
        if (modifiers)
            document->page().traverse_the_history_by_delta(key == UIEvents::KeyCode::Key_Left ? -1 : 1);
@ -1081,7 +1081,7 @@ EventResult EventHandler::handle_keydown(UIEvents::KeyCode key, u32 modifiers, u
        return EventResult::Handled;
    case UIEvents::KeyCode::Key_PageUp:
    case UIEvents::KeyCode::Key_PageDown:
-        if (modifiers > UIEvents::KeyModifier::Mod_None)
+        if (modifiers != UIEvents::KeyModifier::Mod_None)
            break;
        document->window()->scroll_by(0, key == UIEvents::KeyCode::Key_PageUp ? -page_scroll_distance : page_scroll_distance);
        return EventResult::Handled;
--- a/Userland/Libraries/LibWeb/UIEvents/KeyCode.h
+++ b/Userland/Libraries/LibWeb/UIEvents/KeyCode.h
@ -21,10 +21,11 @@ namespace Web::UIEvents {
    __ENUMERATE_KEY_CODE(Return, "Return", 0x0D)                     \
    __ENUMERATE_KEY_CODE(LeftShift, "LeftShift", 0x10)               \
    __ENUMERATE_KEY_CODE(RightShift, "RightShift", 0xB0)             \
-    __ENUMERATE_KEY_CODE(Control, "Ctrl", 0x11)                      \
-    __ENUMERATE_KEY_CODE(RightControl, "RightCtrl", 0xB1)            \
-    __ENUMERATE_KEY_CODE(Alt, "Alt", 0x12)                           \
-    __ENUMERATE_KEY_CODE(RightAlt, "Alt", 0xB2)                      \
+    __ENUMERATE_KEY_CODE(LeftControl, "LeftControl", 0x11)           \
+    __ENUMERATE_KEY_CODE(RightControl, "RightControl", 0xB1)         \
+    __ENUMERATE_KEY_CODE(LeftAlt, "LeftAlt", 0x12)                   \
+    __ENUMERATE_KEY_CODE(RightAlt, "RightAlt", 0xB2)                 \
+    __ENUMERATE_KEY_CODE(AltGr, "AltGr", 0xE1)                       \
    __ENUMERATE_KEY_CODE(PauseBreak, "PauseBreak", 0x13)             \
    __ENUMERATE_KEY_CODE(CapsLock, "CapsLock", 0x14)                 \
    __ENUMERATE_KEY_CODE(Escape, "Escape", 0x1B)                     \
@ -123,7 +124,8 @@ namespace Web::UIEvents {
    __ENUMERATE_KEY_CODE(Backtick, "`", 0x8C)                        \
    __ENUMERATE_KEY_CODE(NumLock, "NumLock", 0x90)                   \
    __ENUMERATE_KEY_CODE(ScrollLock, "ScrollLock", 0x91)             \
-    __ENUMERATE_KEY_CODE(Super, "Super", 0x92)                       \
+    __ENUMERATE_KEY_CODE(LeftSuper, "LeftSuper", 0x92)               \
+    __ENUMERATE_KEY_CODE(RightSuper, "RightSuper", 0xAC)             \
    __ENUMERATE_KEY_CODE(BrowserSearch, "BrowserSearch", 0x93)       \
    __ENUMERATE_KEY_CODE(BrowserFavorites, "BrowserFavorites", 0x94) \
    __ENUMERATE_KEY_CODE(BrowserHome, "BrowserHome", 0x95)           \
@ -154,11 +156,7 @@ enum KeyCode : u8 {
 #define __ENUMERATE_KEY_CODE(name, ui_name, code) Key_##name = code,
    ENUMERATE_KEY_CODES
 #undef __ENUMERATE_KEY_CODE
-
-        Key_Shift
-    = Key_LeftShift,
 };
-size_t const key_code_count = Key_Menu + 1;

 constexpr KeyCode key_code_from_string(StringView key_name)
 {
@ -177,9 +175,8 @@ enum KeyModifier {
    Mod_Ctrl = (1 << 1),
    Mod_Shift = (1 << 2),
    Mod_Super = (1 << 3),
-    Mod_AltGr = (1 << 4),
-    Mod_Keypad = (1 << 5),
-    Mod_Mask = Mod_Alt | Mod_Ctrl | Mod_Shift | Mod_Super | Mod_AltGr | Mod_Keypad,
+    Mod_Keypad = (1 << 4),
+    Mod_Mask = Mod_Alt | Mod_Ctrl | Mod_Shift | Mod_Super | Mod_Keypad,

    Is_Press = 0x80,

--- a/Userland/Libraries/LibWeb/UIEvents/KeyboardEvent.cpp
+++ b/Userland/Libraries/LibWeb/UIEvents/KeyboardEvent.cpp
@ -59,11 +59,14 @@ static unsigned long determine_key_code(KeyCode platform_key, u32 code_point)
        return 9;
    case KeyCode::Key_Return:
        return 13;
-    case KeyCode::Key_Shift:
+    case KeyCode::Key_LeftShift:
+    case KeyCode::Key_RightShift:
        return 16;
-    case KeyCode::Key_Control:
+    case KeyCode::Key_LeftControl:
+    case KeyCode::Key_RightControl:
        return 17;
-    case KeyCode::Key_Alt:
+    case KeyCode::Key_LeftAlt:
+    case KeyCode::Key_RightAlt:
        return 18;
    case KeyCode::Key_CapsLock:
        return 20;
@ -143,15 +146,20 @@ static ErrorOr<Optional<String>> get_event_named_key(KeyCode platform_key)
        return "Unidentified"_string;

    // 3.2. Modifier Keys, https://www.w3.org/TR/uievents-key/#keys-modifier
-    case KeyCode::Key_Alt:
-        return "AltLeft"_string;
+    case KeyCode::Key_LeftAlt:
    case KeyCode::Key_RightAlt:
-        return "AltRight"_string;
+        return "Alt"_string;
+    case KeyCode::Key_AltGr:
+        return "AltGraph"_string;
    case KeyCode::Key_CapsLock:
        return "CapsLock"_string;
-    case KeyCode::Key_Control:
+    case KeyCode::Key_LeftControl:
+    case KeyCode::Key_RightControl:
        return "Control"_string;
-    case KeyCode::Key_Super:
+    // FIXME: Fn
+    // FIXME: FnLock
+    case KeyCode::Key_LeftSuper:
+    case KeyCode::Key_RightSuper:
        return "Meta"_string;
    case KeyCode::Key_NumLock:
        return "NumLock"_string;
@ -454,24 +462,28 @@ static ErrorOr<String> get_event_code(KeyCode platform_key, unsigned modifiers)
        return "Slash"_string;

    // 3.1.2. Functional Keys, https://www.w3.org/TR/uievents-code/#key-alphanumeric-functional
-    case KeyCode::Key_Alt:
+    case KeyCode::Key_LeftAlt:
        return "AltLeft"_string;
    case KeyCode::Key_RightAlt:
        return "AltRight"_string;
+    case KeyCode::Key_AltGr:
+        return "AltGraph"_string;
    case KeyCode::Key_Backspace:
        return "Backspace"_string;
    case KeyCode::Key_CapsLock:
        return "CapsLock"_string;
    case KeyCode::Key_Menu:
        return "ContextMenu"_string;
-    case KeyCode::Key_Control:
+    case KeyCode::Key_LeftControl:
        return "ControlLeft"_string;
    case KeyCode::Key_RightControl:
        return "ControlRight"_string;
    case KeyCode::Key_Return:
        return "Enter"_string;
-    case KeyCode::Key_Super:
-        return "Meta"_string; // FIXME: Detect left vs. right key.
+    case KeyCode::Key_LeftSuper:
+        return "MetaLeft"_string;
+    case KeyCode::Key_RightSuper:
+        return "MetaRight"_string;
    case KeyCode::Key_LeftShift:
        return "ShiftLeft"_string;
    case KeyCode::Key_RightShift:
@ -611,11 +623,16 @@ static DOMKeyLocation get_event_location(KeyCode platform_key, unsigned modifier
    if ((modifiers & Mod_Keypad) != 0)
        return DOMKeyLocation::Numpad;

-    // FIXME: Detect left vs. right for Control and Alt keys.
    switch (platform_key) {
+    case KeyCode::Key_LeftAlt:
+    case KeyCode::Key_LeftControl:
    case KeyCode::Key_LeftShift:
+    case KeyCode::Key_LeftSuper:
        return DOMKeyLocation::Left;
+    case KeyCode::Key_RightAlt:
+    case KeyCode::Key_RightControl:
    case KeyCode::Key_RightShift:
+    case KeyCode::Key_RightSuper:
        return DOMKeyLocation::Right;
    default:
        break;