Ensure modifier change is forwarded after the key sending to input method.

Same as real hardware wl_keyboard, key should be sent before modifier
change. For example, Left Ctrl press and release should produce
key events in the order of Control_L and Control+Control_L.

autotests/integration/inputmethod_test.cpp

@@ -32,11 +32,13 @@
 #include <KWaylandServer/surface_interface.h>
 
 #include <KWayland/Client/compositor.h>
+#include <KWayland/Client/keyboard.h>
 #include <KWayland/Client/output.h>
 #include <KWayland/Client/region.h>
+#include <KWayland/Client/seat.h>
 #include <KWayland/Client/surface.h>
 #include <KWayland/Client/textinput.h>
-#include <KWayland/Client/seat.h>
+#include <linux/input-event-codes.h>
 
 using namespace KWin;
 using namespace KWayland::Client;
@@ -59,6 +61,7 @@ private Q_SLOTS:
     void testSwitchFocusedSurfaces();
     void testV3Styling();
     void testDisableShowInputPanel();
+    void testModifierForwarding();
 
 private:
     void touchNow() {
@@ -462,6 +465,77 @@ void InputMethodTest::testDisableShowInputPanel()
     QVERIFY(!InputMethod::self()->isActive());
 }
 
+void InputMethodTest::testModifierForwarding()
+{
+    // Create an xdg_toplevel surface and wait for the compositor to catch up.
+    QScopedPointer<KWayland::Client::Surface> surface(Test::createSurface());
+    QScopedPointer<Test::XdgToplevel> shellSurface(Test::createXdgToplevelSurface(surface.data()));
+    AbstractClient *client = Test::renderAndWaitForShown(surface.data(), QSize(1280, 1024), Qt::red);
+    QVERIFY(client);
+    QVERIFY(client->isActive());
+    QCOMPARE(client->frameGeometry().size(), QSize(1280, 1024));
+
+    Test::TextInputV3 *textInputV3 = new Test::TextInputV3();
+    textInputV3->init(Test::waylandTextInputManagerV3()->get_text_input(*(Test::waylandSeat())));
+    textInputV3->enable();
+
+    QSignalSpy inputMethodActiveSpy(InputMethod::self(), &InputMethod::activeChanged);
+    QSignalSpy inputMethodActivateSpy(Test::inputMethod(), &Test::MockInputMethod::activate);
+    // just enabling the text-input should not show it but rather on commit
+    QVERIFY(!InputMethod::self()->isActive());
+    textInputV3->commit();
+    QVERIFY(inputMethodActiveSpy.count() || inputMethodActiveSpy.wait());
+    QVERIFY(InputMethod::self()->isActive());
+    QVERIFY(inputMethodActivateSpy.wait());
+    auto context = Test::inputMethod()->context();
+    QScopedPointer<KWayland::Client::Keyboard> keyboardGrab(new KWayland::Client::Keyboard);
+    keyboardGrab->setup(zwp_input_method_context_v1_grab_keyboard(context));
+    QSignalSpy modifierSpy(keyboardGrab.get(), &Keyboard::modifiersChanged);
+    // Wait for initial modifiers update
+    QVERIFY(modifierSpy.wait());
+
+    quint32 timestamp = 1;
+
+    QSignalSpy keySpy(keyboardGrab.get(), &Keyboard::keyChanged);
+    bool keyChanged = false;
+    bool modifiersChanged = false;
+    // We want to verify the order of two signals, so SignalSpy is not very useful here.
+    auto keyChangedConnection = connect(keyboardGrab.get(), &Keyboard::keyChanged, [&keyChanged, &modifiersChanged]() {
+        QVERIFY(!modifiersChanged);
+        keyChanged = true;
+    });
+    auto modifiersChangedConnection = connect(keyboardGrab.get(), &Keyboard::modifiersChanged, [&keyChanged, &modifiersChanged]() {
+        QVERIFY(keyChanged);
+        modifiersChanged = true;
+    });
+    kwinApp()->platform()->keyboardKeyPressed(KEY_LEFTCTRL, timestamp++);
+    QVERIFY(keySpy.count() == 1 || keySpy.wait());
+    QVERIFY(modifierSpy.count() == 2 || modifierSpy.wait());
+    disconnect(keyChangedConnection);
+    disconnect(modifiersChangedConnection);
+
+    kwinApp()->platform()->keyboardKeyPressed(KEY_A, timestamp++);
+    QVERIFY(keySpy.count() == 2 || keySpy.wait());
+    QVERIFY(modifierSpy.count() == 2 || modifierSpy.wait());
+
+    // verify the order of key and modifiers again. Key first, then modifiers.
+    keyChanged = false;
+    modifiersChanged = false;
+    keyChangedConnection = connect(keyboardGrab.get(), &Keyboard::keyChanged, [&keyChanged, &modifiersChanged]() {
+        QVERIFY(!modifiersChanged);
+        keyChanged = true;
+    });
+    modifiersChangedConnection = connect(keyboardGrab.get(), &Keyboard::modifiersChanged, [&keyChanged, &modifiersChanged]() {
+        QVERIFY(keyChanged);
+        modifiersChanged = true;
+    });
+    kwinApp()->platform()->keyboardKeyReleased(KEY_LEFTCTRL, timestamp++);
+    QVERIFY(keySpy.count() == 3 || keySpy.wait());
+    QVERIFY(modifierSpy.count() == 3 || modifierSpy.wait());
+    disconnect(keyChangedConnection);
+    disconnect(modifiersChangedConnection);
+}
+
 WAYLANDTEST_MAIN(InputMethodTest)
 
 #include "inputmethod_test.moc"

src/inputmethod.cpp

@@ -105,7 +105,9 @@ void InputMethod::init()
             connect(textInputV3, &TextInputV3Interface::enabledChanged, this, &InputMethod::textInputInterfaceV3EnabledChanged);
         }
 
-        connect(input()->keyboard()->xkb(), &Xkb::modifierStateChanged, this, &InputMethod::forwardModifiers);
+        connect(input()->keyboard()->xkb(), &Xkb::modifierStateChanged, this, [this]() {
+            m_hasPendingModifiers = true;
+        });
     }
 }
 
@@ -550,8 +552,13 @@ void InputMethod::modifiers(quint32 serial, quint32 mods_depressed, quint32 mods
     xkb->updateModifiers(mods_depressed, mods_latched, mods_locked, group);
 }
 
-void InputMethod::forwardModifiers()
+void InputMethod::forwardModifiers(ForwardModifiersForce force)
 {
+    const bool sendModifiers = m_hasPendingModifiers || force == Force;
+    m_hasPendingModifiers = false;
+    if (!sendModifiers) {
+        return;
+    }
     auto xkb = input()->keyboard()->xkb();
     if (m_keyboardGrab) {
         m_keyboardGrab->sendModifiers(waylandServer()->display()->nextSerial(),
@@ -719,7 +726,7 @@ void InputMethod::installKeyboardGrab(KWaylandServer::InputMethodGrabV1 *keyboar
     auto xkb = input()->keyboard()->xkb();
     m_keyboardGrab = keyboardGrab;
     keyboardGrab->sendKeymap(xkb->keymapContents());
-    forwardModifiers();
+    forwardModifiers(Force);
 }
 
 void InputMethod::updateModifiersMap(const QByteArray &modifiers)

src/inputmethod.h

@@ -43,6 +43,8 @@ class KWIN_EXPORT InputMethod : public QObject
 {
     Q_OBJECT
 public:
+    enum ForwardModifiersForce { NoForce = 0, Force = 1 };
+
     ~InputMethod() override;
 
     void init();
@@ -63,6 +65,8 @@ public:
     KWaylandServer::InputMethodGrabV1 *keyboardGrab();
     bool shouldShowOnActive() const;
 
+    void forwardModifiers(ForwardModifiersForce force);
+
 Q_SIGNALS:
     void activeChanged(bool active);
     void enabledChanged(bool enabled);
@@ -102,7 +106,6 @@ private:
     void updateModifiersMap(const QByteArray &modifiers);
 
     bool touchEventTriggered() const;
-    void forwardModifiers();
     void resetPendingPreedit();
 
     struct {
@@ -122,6 +125,8 @@ private:
     uint m_inputMethodCrashes = 0;
     QString m_inputMethodCommand;
 
+    bool m_hasPendingModifiers = false;
+
     KWIN_SINGLETON(InputMethod)
 };
 

src/keyboard_input.cpp

@@ -7,15 +7,16 @@
     SPDX-License-Identifier: GPL-2.0-or-later
 */
 #include "keyboard_input.h"
+#include "abstract_client.h"
 #include "input_event.h"
 #include "input_event_spy.h"
+#include "inputmethod.h"
 #include "keyboard_layout.h"
 #include "keyboard_repeat.h"
-#include "abstract_client.h"
 #include "modifier_only_shortcuts.h"
-#include "utils.h"
 #include "screenlockerwatcher.h"
 #include "toplevel.h"
+#include "utils.h"
 #include "wayland_server.h"
 #include "workspace.h"
 // KWayland
@@ -247,6 +248,7 @@ void KeyboardInputRedirection::processKey(uint32_t key, InputRedirection::Keyboa
     m_input->processFilters(std::bind(&InputEventFilter::keyEvent, std::placeholders::_1, &event));
 
     m_xkb->forwardModifiers();
+    InputMethod::self()->forwardModifiers(InputMethod::NoForce);
 
     if (event.modifiersRelevantForGlobalShortcuts() == Qt::KeyboardModifier::NoModifier && type != QEvent::KeyRelease) {
         m_keyboardLayout->checkLayoutChange(previousLayout);