Merge pull request #1894 from contour-terminal/fix/modifier-keys-no-scroll

Fix modifier-only keys scrolling viewport to bottom

Author: christianparpart

metainfo.xml

@@ -107,6 +107,7 @@
     <release version="0.6.3" urgency="medium" type="development">
       <description>
         <ul>
+          <li>Fixes modifier-only key presses (Shift, Ctrl, Alt, etc.) incorrectly scrolling the viewport to the bottom when Kitty keyboard protocol ReportAllKeysAsEscapeCodes is enabled</li>
           <li>Fixes keyboard input being swallowed in programs like less and bat due to empty-string terminfo capabilities (#1861)</li>
           <li>Fixes ESC key not being sent to the application when CancelSelection input mapping has no mode restriction (#1839)</li>
           <li>Fixes selective erase (DECSED/DECSEL) incorrectly erasing the cursor line instead of the target line when erasing lines without protected characters (#28)</li>

src/vtbackend/InputGenerator.cpp

@@ -526,34 +526,6 @@ constexpr pair<unsigned, char> mapKey(Key key) noexcept
     crispy::unreachable();
 }
 
-constexpr bool isModifierKey(Key key) noexcept
-{
-    // clang-format off
-    switch (key)
-    {
-        case Key::LeftShift:
-        case Key::LeftControl:
-        case Key::LeftAlt:
-        case Key::LeftSuper:
-        case Key::LeftHyper:
-        case Key::LeftMeta:
-        case Key::RightShift:
-        case Key::RightControl:
-        case Key::RightAlt:
-        case Key::RightSuper:
-        case Key::RightHyper:
-        case Key::RightMeta:
-        case Key::IsoLevel3Shift:
-        case Key::IsoLevel5Shift:
-        case Key::CapsLock:
-        case Key::NumLock:
-            return true;
-        default:
-            return false;
-    }
-    // clang-format on
-}
-
 bool ExtendedKeyboardInputGenerator::generateKey(Key key, Modifiers modifiers, KeyboardEventType eventType)
 {
     if (!enabled(eventType))

src/vtbackend/InputGenerator.h

@@ -184,6 +184,38 @@ enum class Key : uint8_t
     // NOLINTEND(readability-identifier-naming)
 };
 
+/// Checks whether the given key is a modifier-only key (e.g. Shift, Control, Alt, etc.).
+///
+/// @param key The key to check.
+/// @return true if the key is a modifier-only key, false otherwise.
+constexpr bool isModifierKey(Key key) noexcept
+{
+    // clang-format off
+    switch (key)
+    {
+        case Key::LeftShift:
+        case Key::LeftControl:
+        case Key::LeftAlt:
+        case Key::LeftSuper:
+        case Key::LeftHyper:
+        case Key::LeftMeta:
+        case Key::RightShift:
+        case Key::RightControl:
+        case Key::RightAlt:
+        case Key::RightSuper:
+        case Key::RightHyper:
+        case Key::RightMeta:
+        case Key::IsoLevel3Shift:
+        case Key::IsoLevel5Shift:
+        case Key::CapsLock:
+        case Key::NumLock:
+            return true;
+        default:
+            return false;
+    }
+    // clang-format on
+}
+
 std::string to_string(Key key);
 
 enum class KeyMode : uint8_t

src/vtbackend/Terminal.cpp

@@ -824,7 +824,8 @@ Handled Terminal::sendKeyEvent(Key key, Modifiers modifiers, KeyboardEventType e
     if (success)
     {
         flushInput();
-        _viewport.scrollToBottom();
+        if (!isModifierKey(key))
+            _viewport.scrollToBottom();
     }
     return Handled { success };
 }

src/vtbackend/Terminal_test.cpp

@@ -88,6 +88,84 @@ TEST_CASE("Terminal.BlinkingCursor", "[terminal]")
     }
 }
 
+TEST_CASE("Terminal.ModifierKeysDoNotScrollViewport", "[terminal]")
+{
+    // Set up a terminal with history capacity to allow scrollback
+    auto mc = MockTerm { PageSize { LineCount(4), ColumnCount(6) }, LineCount(10) };
+    auto& terminal = mc.terminal;
+
+    // Enable ReportAllKeysAsEscapeCodes (kitty keyboard protocol)
+    terminal.keyboardProtocol().enter(vtbackend::KeyboardEventFlag::ReportAllKeysAsEscapeCodes);
+
+    // Fill terminal and generate scrollback history
+    mc.writeToScreen("line1\r\n"
+                     "line2\r\n"
+                     "line3\r\n"
+                     "line4\r\n"
+                     "line5\r\n"
+                     "line6\r\n");
+
+    // Scroll up so viewport is not at bottom
+    terminal.viewport().scrollUp(LineCount(2));
+    REQUIRE(terminal.viewport().scrolled());
+    auto const scrollOffsetBeforeKey = terminal.viewport().scrollOffset();
+
+    SECTION("modifier-only press does not scroll")
+    {
+        mc.resetReplyData();
+        terminal.sendKeyEvent(vtbackend::Key::LeftShift,
+                              vtbackend::Modifiers { vtbackend::Modifier::Shift },
+                              vtbackend::KeyboardEventType::Press,
+                              std::chrono::steady_clock::now());
+
+        // Viewport should remain scrolled
+        CHECK(terminal.viewport().scrolled());
+        CHECK(terminal.viewport().scrollOffset() == scrollOffsetBeforeKey);
+        // Escape sequence should still have been sent to the application
+        CHECK(!mc.replyData().empty());
+    }
+
+    SECTION("non-modifier press does scroll")
+    {
+        mc.resetReplyData();
+        terminal.sendKeyEvent(vtbackend::Key::Enter,
+                              vtbackend::Modifiers { vtbackend::Modifier::None },
+                              vtbackend::KeyboardEventType::Press,
+                              std::chrono::steady_clock::now());
+
+        // Viewport should scroll to bottom
+        CHECK(!terminal.viewport().scrolled());
+    }
+
+    SECTION("various modifier keys")
+    {
+        auto const modifierKeys = std::vector<vtbackend::Key> {
+            vtbackend::Key::LeftShift,      vtbackend::Key::RightShift,     vtbackend::Key::LeftControl,
+            vtbackend::Key::RightControl,   vtbackend::Key::LeftAlt,        vtbackend::Key::RightAlt,
+            vtbackend::Key::LeftSuper,      vtbackend::Key::RightSuper,     vtbackend::Key::LeftHyper,
+            vtbackend::Key::RightHyper,     vtbackend::Key::LeftMeta,       vtbackend::Key::RightMeta,
+            vtbackend::Key::IsoLevel3Shift, vtbackend::Key::IsoLevel5Shift, vtbackend::Key::CapsLock,
+            vtbackend::Key::NumLock,
+        };
+
+        for (auto const modKey: modifierKeys)
+        {
+            INFO("Testing modifier key: " << std::format("{}", modKey));
+
+            // Reset viewport to scrolled position
+            terminal.viewport().scrollUp(LineCount(2));
+            REQUIRE(terminal.viewport().scrolled());
+
+            terminal.sendKeyEvent(modKey,
+                                  vtbackend::Modifiers { vtbackend::Modifier::None },
+                                  vtbackend::KeyboardEventType::Press,
+                                  std::chrono::steady_clock::now());
+
+            CHECK(terminal.viewport().scrolled());
+        }
+    }
+}
+
 TEST_CASE("Terminal.DECCARA", "[terminal]")
 {
     auto mock = MockTerm { ColumnCount(5), LineCount(5) };