Fixed bug 3249 - keysym.mod is incorrect when mod keys are pressed for SDL_KEYDOWN events

Adam M.

The keysym.mod field does not reflect the state of the modified keys when processing key down events for the modifier keys themselves. The documentation says that it returns the current key modifiers, but they are not current for key down events involving modifier keys. I interpret "current" to mean "equal to SDL_GetModState() at the instant the event is processed/enqueued".

For example, if you depress the Shift key you get a key down event with .mod == 0. However, .mod should not be 0 because a shift key is down. If you then release the Shift key, you get a key up event with .mod == 0. Neither event reports the modifier key.

If you press Shift and then A, .mod is incorrect (== 0) when Shift is pressed, but is correct later when A is pressed (== KMOD_LSHIFT).

You might say this behavior is deliberate, i.e. keysym.mod is the value /before/ the event, not the current value as documented, but that explanation is incorrect because only key down events behave that way. Key up events correctly give the current value, not the value before the event.

Not only is it inconsistent with itself, I think it makes keyboard processing harder.

The problem is near line 740 in SDL_keyboard.c:

if (SDL_KEYDOWN == type) {
    modstate = keyboard->modstate; // SHOULD THIS BE MOVED DOWN?
    switch (keycode) {
    case SDLK_NUMLOCKCLEAR:
        keyboard->modstate ^= KMOD_NUM;
        break;
    case SDLK_CAPSLOCK:
        keyboard->modstate ^= KMOD_CAPS;
        break;
    default:
        keyboard->modstate |= modifier;
        break;
    }
} else {
    keyboard->modstate &= ~modifier;
    modstate = keyboard->modstate;
}

In the key down path, modstate (and thus keysym.mod) ends up being the modifier state /before/ the event, but in the key up path modstate ends up being the modifier state /after/ the event. Personally I think the "modstate = keyboard->modstate" line should just be moved after the entire if/else statement, so that keysym.mod always reflects the current state.

src/events/SDL_keyboard.c

@@ -672,7 +672,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
     int posted;
     SDL_Keymod modifier;
     SDL_Keycode keycode;
-    Uint16 modstate;
     Uint32 type;
     Uint8 repeat;
 
@@ -745,7 +744,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
         break;
     }
     if (SDL_KEYDOWN == type) {
-        modstate = keyboard->modstate;
         switch (keycode) {
         case SDLK_NUMLOCKCLEAR:
             keyboard->modstate ^= KMOD_NUM;
@@ -759,7 +757,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
         }
     } else {
         keyboard->modstate &= ~modifier;
-        modstate = keyboard->modstate;
     }
 
     /* Post the event, if desired */
@@ -771,7 +768,7 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
         event.key.repeat = repeat;
         event.key.keysym.scancode = scancode;
         event.key.keysym.sym = keycode;
-        event.key.keysym.mod = modstate;
+        event.key.keysym.mod = keyboard->modstate;
         event.key.windowID = keyboard->focus ? keyboard->focus->id : 0;
         posted = (SDL_PushEvent(&event) > 0);
     }