fix: tab navigation not working in Fixed Layout due to event listener timing issue

[jacquesikot]

Problem

Tab navigation between input widgets was not working in Fixed Layout applications. Users reported that pressing the Tab key would not move focus to the next input widget in the expected order (top-to-bottom, left-to-right), instead following the browser's default DOM-based tab order.

This issues was raised by an Enterprise user here

Root Cause

The issue was caused by a timing problem in the useWidgetFocus hook:

1. The useEffect hook was running immediately when the component mounted
2. However, the canvas element ref (ref.current) was set later via the React ref callback
3. This caused the event listeners for Tab navigation to never be attached, as ref.current was null when useEffect ran
4. Without the custom Tab event listeners, the browser fell back to its default tab navigation behavior

Solution

Refactored the useWidgetFocus hook to attach event listeners immediately when the ref is set, rather than waiting for a useEffect that runs too early:

Before (Broken):

useEffect(() => {
  if (!ref.current) return; // ❌ Always true - ref not set yet
  
  const handleKeyDown = (event: KeyboardEvent) => {
    if (event.key === "Tab") handleTab(event);
  };
  
  ref.current.addEventListener("keydown", handleKeyDown);
}, []); // ❌ Runs before ref is set

After (Fixed):

const setRef = useCallback((node: HTMLElement | null) => {
  if (node === null) return;
  if (ref.current === node) return;
  
  ref.current = node;
  attachEventListeners(node); // ✅ Attach immediately when ref is set
}, [attachEventListeners]);

Why This Solution Works

1. Correct Timing: Event listeners are now attached immediately when React calls the ref callback with the DOM element
2. No Race Conditions: Eliminates the timing issue between useEffect and ref assignment
3. Maintains Functionality: Preserves all existing tab navigation logic (position-based sorting, modal focus trapping, etc.)
4. Clean Architecture: Separates event listener attachment logic into a reusable callback

Testing

• ✅ Tab navigation now works correctly in Fixed Layout applications
• ✅ Maintains proper top-to-bottom, left-to-right tab order
• ✅ Modal focus trapping continues to work
• ✅ Auto Layout behavior unchanged (tab navigation disabled as intended)
• ✅ No regressions in existing functionality

Files Changed

• app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx - Fixed event listener timing
• app/client/src/utils/hooks/useWidgetFocus/handleTab.ts - Cleaned up (no functional changes)
• app/client/src/utils/hooks/useWidgetFocus/tabbable.ts - Cleaned up (no functional changes)

Automation

/ok-to-test tags="@tag.Widget"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18034264649
Commit: ab9af84
Cypress dashboard.
Tags: @tag.Widget
Spec:

---

Fri, 26 Sep 2025 11:09:55 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

• Yes
• No

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability of focusing widgets on click.
  • More consistent Tab key navigation across widgets.
  • Prevents unintended focus behavior in non–auto-layout mode.

• Refactor

  • Streamlined event listener management for focus and keyboard interactions, improving stability and reducing potential memory leaks.

[coderabbitai]

Walkthrough

Ref handling in useWidgetFocus is refactored to use a dedicated attachEventListeners callback that conditionally binds click and keydown listeners when not in auto-layout mode. The ref setter initializes the element, attaches listeners immediately, and supports cleanup. Imports are updated. Public hook signature remains unchanged.

Changes

Cohort / File(s)	Change Summary
Hook refactor: event listener attachment app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx	Extracts listener logic into attachEventListeners; ref setter updated to set element, attach listeners, and return ref; adds conditional no-op for auto-layout; ensures cleanup for listeners; adjusts imports accordingly.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Component as React Component
  participant Hook as useWidgetFocus
  participant Elem as DOM Element
  participant Canvas as Canvas Widget

  User->>Component: Render
  Component->>Hook: Call useWidgetFocus()
  Hook-->>Component: refCallback(instance)
  Component->>Elem: Assign refCallback
  Note over Hook,Elem: If auto-layout is enabled → skip attaching listeners

  Elem->>Hook: refCallback(elem)
  Hook->>Elem: attachEventListeners(click, keydown)
  Note right of Elem: Cleanup removes listeners on unmount

  User->>Elem: click
  Elem->>Hook: click handler
  Hook->>Canvas: focus()

  User->>Elem: keydown (Tab)
  Elem->>Hook: keydown handler
  Hook->>Canvas: handleTab()

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A ref once tangled, now clean in its pose,
Listeners attach where the focus now goes.
Click, and the canvas awakens to play;
Tab whispers guidance to slide on the way.
Auto-layout watches, choosing repose.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check	⚠️ Warning	The pull request description clearly outlines the problem, root cause, solution, testing, and file changes, but it does not adhere to the repository’s required template: it is missing the top‐level “## Description” heading and the mandatory “Fixes #” or “Fixes ” line, and it does not explicitly list any dependencies as specified in the template.	Please add a “## Description” section header to match the template, include a “Fixes #” or “Fixes ” line referencing the relevant issue, and verify that any required dependencies or external links are explicitly listed according to the template.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and accurately summarizes the primary change: it fixes Tab navigation in Fixed Layout by addressing an event listener timing issue; it is specific, relevant to the changeset, and understandable when scanning history.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/tab-not-working-as-expected-on-input

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx (3)

12-16: Detach listeners on ref change/unmount; wire up cleanup

You correctly attach immediately, but the returned cleanup from attachEventListeners isn’t used. This can leave stale listeners if the ref changes, and it won’t detach on unmount. Capture and invoke the cleanup.

Apply within setRef:

@@
   const setRef = useCallback(
     (node: HTMLElement | null) => {
       if (node === null) {
-        ref.current = null;
-        return;
+        // Detach listeners on unmount
+        cleanupRef.current?.();
+        cleanupRef.current = null;
+        ref.current = null;
+        return;
       }
 
       if (ref.current === node) {
         return;
       }
 
-      ref.current = node;
-
-      // Attach event listeners immediately when ref is set
-      attachEventListeners(node);
-
-      return ref;
+      // Detach from previous node (if any), then attach to the new node
+      cleanupRef.current?.();
+      ref.current = node;
+      // Attach event listeners immediately when ref is set and keep cleanup
+      cleanupRef.current = attachEventListeners(node);
     },
     [attachEventListeners],
   );

Add cleanupRef near your other refs:

// place right after `const ref = useRef<HTMLElement | null>(null);`
const cleanupRef = useRef<(() => void) | null>(null);

Optional: if layout mode can toggle at runtime, ensure listeners reflect it:

import { useEffect } from "react";

useEffect(() => {
  if (!ref.current) return;
  cleanupRef.current?.();
  cleanupRef.current = attachEventListeners(ref.current);
  return () => {
    cleanupRef.current?.();
    cleanupRef.current = null;
  };
}, [attachEventListeners]);

Also applies to: 34-41, 46-67

---

24-33: Type the native click event; drop eslint-disable

Use the DOM MouseEvent type and remove the temporary rule disable.

- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- const handleClick = (event: any) => {
+ const handleClick = (event: MouseEvent) => {
   const target = event.target as HTMLElement;
 
   if (target.matches(CANVAS_WIDGET)) {
     target.focus();
   }
 };

---

18-22: Honor layout toggles without re-mount

attachEventListeners captures isAutoLayout at creation time. If layout switches at runtime, handlers won’t update. Either re-attach on change (via a tiny effect) or read a live ref inside the handlers.

Option A (re-attach, see effect in earlier comment).

Option B (live ref check):

const isAutoLayoutRef = useRef(isAutoLayout);
useEffect(() => { isAutoLayoutRef.current = isAutoLayout; }, [isAutoLayout]);

const handleKeyDown = (event: KeyboardEvent) => {
  if (isAutoLayoutRef.current) return;
  if (event.key === "Tab") handleTab(event);
};

const handleClick = (event: MouseEvent) => {
  if (isAutoLayoutRef.current) return;
  const target = event.target as HTMLElement;
  if (target.matches(CANVAS_WIDGET)) target.focus();
};

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2945431 and ab9af84.

📒 Files selected for processing (1)

• app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx (1)

app/client/src/selectors/editorSelectors.tsx (1)

• getIsAutoLayout (298-301)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
• GitHub Check: client-unit-tests / client-unit-tests
• GitHub Check: client-lint / client-lint
• GitHub Check: client-prettier / prettier-check
• GitHub Check: client-build / client-build

🔇 Additional comments (2)

app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx (2)

61-63: Nice: immediate attachment fixes the timing race

Attaching in the ref callback eliminates the ref/effect race and restores controlled Tab order. Good call.

---

4-4: Import path is correct — getIsAutoLayout is exported from selectors/canvasSelectors.ts

• app/client/src/selectors/canvasSelectors.ts:10 exports getIsAutoLayout.
• app/client/src/selectors/editorSelectors.tsx:298 also exports getIsAutoLayout — consider deduping if accidental.

[rahulbarwal]

Why do we have any here? Can this not be HTMLElement?