Opentui: Fix Keypress Event Leaks In Selectors

by Admin 47 views
Keypress Event Leaks Causing Unintended Automatic ITEM_SELECTED in Opentui Selectors

Hey everyone! Let's dive into a tricky issue in Opentui related to keypress events and how they can cause unexpected behavior with ITEM_SELECTED events. This can lead to automatic selections in your selectors without any user input, which is definitely not what we want. Let's break down the problem, explore the cause, and discuss potential solutions.

Description of the Issue

The core problem lies in how SelectRenderable components handle ITEM_SELECTED events. Imagine you have two selectors, s0 and s1. When s0 handles an ITEM_SELECTED event, and the handler then makes selector s1 visible and focuses it, the same keypress event that triggered the selection in s0 can leak into s1. This leakage causes s1 to immediately fire its own ITEM_SELECTED event, automatically selecting its first option. This happens without any additional user interaction, leading to a broken and confusing user experience.

This unintended behavior is particularly problematic in scenarios involving menus, dialogs, or cascading selectors, where the user expects to have explicit control over each selection. The current workaround involves using setTimeout or queueMicrotask to delay the visibility and focus of s1, but this feels like a hack rather than a proper solution. We need to understand why this event propagation is happening and find a more robust way to prevent it. Consider a scenario where a user is navigating a complex menu system using arrow keys and the enter key. They select an option in the first menu (s0) that is supposed to open a sub-menu (s1). Ideally, the sub-menu should appear and be focused, waiting for the user to make a deliberate choice. However, with this keypress leak, the first item in the sub-menu is automatically selected, potentially bypassing important steps or leading the user down an unintended path. This makes the UI feel unpredictable and unresponsive. Furthermore, relying on setTimeout or queueMicrotask introduces timing dependencies that can be difficult to manage and may not be reliable across different environments. A more direct and deterministic solution is needed to ensure that keypress events are properly scoped and consumed by the intended components.

Expected Behavior

Ideally, the following should occur:

  1. Only one widget should receive a keypress: Keypress events should be targeted and not propagate to unintended recipients.
  2. After s0 selection, s1 should become visible and focused: The intended behavior of making s1 visible and focusing it after a selection in s0 should still occur.
  3. s1 should not automatically select an item unless the user presses a new key: s1 should wait for explicit user input before triggering its own ITEM_SELECTED event.

In essence, the user should have full control over each selection, and the UI should behave predictably based on their explicit actions. The focus should shift seamlessly between selectors without triggering unintended selections. This requires a mechanism to prevent the keypress event from being prematurely consumed by the newly focused selector.

Actual Behavior

Unfortunately, the actual behavior deviates significantly from the expected behavior:

  1. The same keypress that selected an item in s0 is immediately routed into s1 as well. This is the root cause of the problem – the event is not properly consumed or stopped after being processed by s0.
  2. s1 fires its own ITEM_SELECTED. As a result of the leaked keypress, s1 mistakenly believes that the user intended to select an item.
  3. s1 selects the first item without user intention. This automatic selection is the most visible symptom of the underlying issue.
  4. This creates broken UI flows (menus, dialogs, cascading selectors, etc.). The unintended selections disrupt the intended flow of the application, leading to a frustrating user experience.

This behavior undermines the principle of explicit user control and makes the UI feel unpredictable and buggy. It's crucial to address this issue to ensure a smooth and intuitive user experience, especially in applications that rely heavily on selector-based interactions.

Reproducing the Issue

To reproduce the issue, you can use the following code snippet:

#!/usr/bin/env bun

import {
  CliRenderer,
  createCliRenderer,
  SelectRenderable,
  type SelectOption,
  type KeyEvent,
  SelectRenderableEvents,
} from "@opentui/core";

function run(renderer: CliRenderer) {
  const s0 = new SelectRenderable(renderer, {
    id: "select_0",
    position: "absolute",
    top: 0,
    left: 0,
    width: 30,
    height: 10,
    options: [
      { name: "a", value: "a", description: "a" },
      { name: "b", value: "b", description: "b" },
      { name: "c", value: "c", description: "c" },
    ],
    showDescription: false,
  });

  const s1 = new SelectRenderable(renderer, {
    id: "s1",
    position: "absolute",
    top: 2,
    left: 35,
    width: 30,
    height: 10,
    options: [
      {
        name: "I",
        value: "I",
        description: "I",
      },
      {
        name: "II",
        value: "II",
        description: "II",
      },
    ],
    showDescription: false,
  });

  s0.on(
    SelectRenderableEvents.ITEM_SELECTED,
    (idnex: number, option: SelectOption) => {
      console.log(`s0 selected index ${idnex} with option:`, option.value);
      switch (option.value) {
        case "a": {
          break;
        }
        case "b": {
          // TODO: ITEM_SELECTED leaks to s1?
          s1.visible = true;
          s1.focus();
          // FIX:
          // setTimeout(() => {
          //   s1.visible = true;
          //   s1.focus();
          // });
          // FIX:
          // queueMicrotask(() => {
          //  s1.visible = true;
          //  s1.focus();
          // });
          break;
        }
        case "c": {
          break;
        }
        default:
          break;
      }
    },
  );
  s0.visible = true;
  s0.focus();
  renderer.root.add(s0);

  s1.on(
    SelectRenderableEvents.ITEM_SELECTED,
    (index: number, option: SelectOption) => {
      console.log(`s1 selected index ${index} with option:`, option.value);
      switch (option.value) {
        case "a": {
          break;
        }
        case "b": {
          s1.visible = true;
          s1.focus();
          break;
        }
        default:
          break;
      }
    },
  );
  s1.visible = false;
  s1.blur();
  renderer.root.add(s1);
}

function setupKeybindings(renderer: CliRenderer) {
  renderer.keyInput.on("keypress", (key: KeyEvent) => {
    if (key.name === "`") {
      renderer.console.toggle();
    }
  });
}

if (import.meta.main) {
  const renderer = await createCliRenderer({
    exitOnCtrlC: true,
  });
  run(renderer);
  setupKeybindings(renderer);
  renderer.start();
}

In this example, s0 and s1 are two SelectRenderable components. When you select option "b" in s0, it makes s1 visible and focuses it. However, due to the keypress event leak, s1 immediately selects its first option without any user input. To see the problem in action, run this code and use the arrow keys and enter key to navigate the first selector. When you select 'b', observe that the second selector appears and immediately selects its first option. This demonstrates the unintended behavior caused by the keypress event leak.

Potential Solutions and Discussion

So, what can we do about this? Here are a few ideas and points to consider:

  • Event Propagation Control: The most direct approach would be to control the propagation of the keypress event. After s0 handles the event, it should prevent the event from bubbling up to other elements. This might involve using methods like stopPropagation() or preventDefault() within the event handler.
  • Focus Management: Carefully manage the focus state of the selectors. Ensure that when s1 is focused, it doesn't immediately react to the keypress that triggered the focus change. This could involve temporarily disabling keypress handling on s1 during the focus transition.
  • Debouncing or Throttling: While setTimeout and queueMicrotask provide a workaround, they are essentially forms of debouncing. A more robust debouncing or throttling mechanism could be implemented to prevent rapid-fire event handling.
  • Centralized Event Handling: Consider a centralized event handling system where all keypress events are routed through a central manager. This manager can then decide which component should receive the event based on the current focus and application state. This approach provides more control and visibility over event flow.

The current workaround of using setTimeout or queueMicrotask is not ideal because it introduces timing dependencies and feels like a hack. We need a more deterministic and reliable solution that directly addresses the root cause of the event leak. By carefully managing event propagation, focus state, and potentially implementing a centralized event handling system, we can prevent this unintended behavior and ensure a more predictable and user-friendly experience with Opentui selectors.

Let's discuss these potential solutions and explore other ideas to find the best way to fix this issue! What are your thoughts, guys?