From 77005b57986ae70a943ef457e0f6dcfb8f12490a Mon Sep 17 00:00:00 2001 From: Jacques Ikot Date: Mon, 29 Sep 2025 01:14:08 +0100 Subject: [PATCH] fix: tab navigation not working in Fixed Layout due to event listener timing issue (#41256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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](https://theappsmith.slack.com/archives/C0341RERY4R/p1758112042665109) ## 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): ```typescript 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): ```typescript 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" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: ab9af8404302eb19c243dea583160bc9e74f33aa > 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. --- .../hooks/useWidgetFocus/useWidgetFocus.tsx | 92 +++++++++++-------- 1 file changed, 55 insertions(+), 37 deletions(-) diff --git a/app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx b/app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx index 50e2ee3a96..bc3772cda0 100644 --- a/app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx +++ b/app/client/src/utils/hooks/useWidgetFocus/useWidgetFocus.tsx @@ -1,52 +1,70 @@ +import { useCallback, useRef } from "react"; import { useSelector } from "react-redux"; -import { useCallback, useEffect, useRef } from "react"; +import { getIsAutoLayout } from "selectors/canvasSelectors"; import { handleTab } from "./handleTab"; import { CANVAS_WIDGET } from "./tabbable"; -import { getIsAutoLayout } from "selectors/canvasSelectors"; function useWidgetFocus(): (instance: HTMLElement | null) => void { const ref = useRef(); const isAutoLayout = useSelector(getIsAutoLayout); - // This is a callback that will be called when the ref is set - const setRef = useCallback((node: HTMLElement | null) => { - if (node === null) return; - - if (ref.current === node) return; - - ref.current = node; - - return ref; - }, []); - - useEffect(() => { - if (isAutoLayout) return; - - if (!ref.current) return; - - const handleKeyDown = (event: KeyboardEvent) => { - if (event.key === "Tab") handleTab(event); - }; - - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const handleClick = (event: any) => { - const target = event.target as HTMLElement; - - if (target.matches(CANVAS_WIDGET)) { - target.focus(); + const attachEventListeners = useCallback( + (element: HTMLElement) => { + if (isAutoLayout) { + return () => {}; // Return empty cleanup function } - }; - ref.current.addEventListener("keydown", handleKeyDown); - ref.current.addEventListener("click", handleClick); + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === "Tab") { + handleTab(event); + } + }; - return () => { - ref?.current && ref.current.removeEventListener("keydown", handleKeyDown); - ref?.current && ref.current.removeEventListener("click", handleClick); - }; - }, []); + // TODO: Fix this the next time the file is edited + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const handleClick = (event: any) => { + const target = event.target as HTMLElement; + + if (target.matches(CANVAS_WIDGET)) { + target.focus(); + } + }; + + element.addEventListener("keydown", handleKeyDown); + element.addEventListener("click", handleClick); + + // Return cleanup function + return () => { + element.removeEventListener("keydown", handleKeyDown); + element.removeEventListener("click", handleClick); + }; + }, + [isAutoLayout], + ); + + // This is a callback that will be called when the ref is set + const setRef = useCallback( + (node: HTMLElement | null) => { + if (node === 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; + }, + [attachEventListeners], + ); return setRef; }