fix(ToolWindows): fix focus issues

The use of `FocusScope` started to fail on the CI machine. It also had the mentioned caveats. That's now replaced with a custom hook.
alirezamirian · Oct 2, 2023 · eda3709 · eda3709
1 parent f62faf9
commit eda3709
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 65 deletions.
diff --git a/packages/example-app/src/Project/Project.tsx b/packages/example-app/src/Project/Project.tsx
@@ -69,9 +69,6 @@ export const Project = ({
                 }}
                 windows={toolWindows}
                 containerProps={shortcutHandlerProps}
-                // To make it not annoying when the whole app is a part of a bigger page. It's fine to disable focus trap,
-                // because the focusable element, the editor, fills the whole main content.
-                allowBlurOnInteractionOutside
               >
                 <FileEditor />
               </DefaultToolWindows>

diff --git a/packages/jui/src/ToolWindows/ToolWindows.cy.tsx b/packages/jui/src/ToolWindows/ToolWindows.cy.tsx
@@ -63,7 +63,7 @@ describe("ToolWindows", () => {
       cy.findByTestId("First window focusable 1").should("have.focus");
     });
 
-    it("Focuses the first focusable element, when a non-focusable area within the tool window is clicked", () => {
+    it("focuses the first focusable element, when a non-focusable area within the tool window is clicked", () => {
       cy.mount(
         <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
           <SimpleToolWindows />
@@ -78,17 +78,55 @@ describe("ToolWindows", () => {
       cy.findByTestId("First window focusable 1").should("have.focus");
     });
 
-    it("keeps focused element focused, when a non-focusable area within in the main area is clicked", () => {
+    it("moves focus to the main area, when a non-focusable area within in the main area is clicked", () => {
       cy.mount(
         <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
           <SimpleToolWindows />
         </ThemeProvider>
       );
-      cy.contains("First window").click();
-      cy.findByTestId("First window focusable 2").click();
-      cy.findByTestId("main content non-focusable").click();
-      cy.focused().should("exist");
-      cy.findByTestId("First window focusable 2").should("have.focus");
+      cy.contains("First window").realClick();
+      cy.findByTestId("First window focusable 2")
+        .realClick()
+        .should("be.focused");
+      cy.findByTestId("main content non-focusable").realClick();
+      cy.findByTestId("main content focusable").should("have.focus");
+    });
+
+    it("keeps main content focused, when a non-focusable element inside is clicked", () => {
+      cy.mount(
+        <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
+          <div>outside area</div>
+          <SimpleToolWindows />
+        </ThemeProvider>
+      );
+      cy.findByTestId("main content focusable").focus().should("have.focus");
+      cy.findByTestId("main content non-focusable").realClick(); // clicking non-focusable element inside tool window
+      cy.findByTestId("main content focusable").should("have.focus");
+    });
+
+    it("looses focus when a non-focusable element outside is clicked", () => {
+      cy.mount(
+        <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
+          <div>outside area</div>
+          <SimpleToolWindows />
+        </ThemeProvider>
+      );
+      cy.findByTestId("main content focusable").focus().should("have.focus");
+      cy.contains("outside area").realClick(); // clicking non-focusable element outside tool window
+      cy.findByTestId("main content focusable").should("not.have.focus");
+    });
+
+    it("allows focusable element outside the ToolWindows get focused.", () => {
+      cy.mount(
+        <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
+          <div>
+            outside area <input data-testid="outside-input" />
+          </div>
+          <SimpleToolWindows />
+        </ThemeProvider>
+      );
+      cy.findByTestId("main content focusable").focus().should("have.focus");
+      cy.findByTestId("outside-input").realClick().should("have.focus");
     });
 
     // NOTE: even when focus is within the tool window, focusing the tool window container (by pressing header,
@@ -105,7 +143,7 @@ describe("ToolWindows", () => {
       cy.findByTestId("First window focusable 1").should("have.focus");
     });
 
-    it("moves focus to the main content when a window is toggled closed ", () => {
+    it("moves focus to the main content when a window is toggled closed", () => {
       cy.mount(
         <ThemeProvider theme={new Theme(darculaThemeJson as any)}>
           <SimpleToolWindows />
@@ -115,9 +153,7 @@ describe("ToolWindows", () => {
       cy.findByTestId("Second window stripe button").click();
       cy.findByTestId("Second window stripe button").click();
 
-      // Focus should to go main content, but it doesn't currently because of a known issue. FIXME in ToolWindows
-      // cy.findByTestId("main content focusable").should('have.focus');
-      cy.findByTestId("First window focusable 1").should("have.focus");
+      cy.findByTestId("main content focusable").should("have.focus");
     });
     /**
      * Known issue. FIXME. See DefaultToolWindow for more info

diff --git a/packages/jui/src/ToolWindows/ToolWindows.tsx b/packages/jui/src/ToolWindows/ToolWindows.tsx
@@ -1,3 +1,4 @@
+import { indexBy } from "ramda";
 import React, {
   CSSProperties,
   ForwardedRef,
@@ -8,7 +9,8 @@ import React, {
   useRef,
   useState,
 } from "react";
-import { FocusScope as AriaFocusScope } from "@react-aria/focus";
+import { useLatest } from "@intellij-platform/core/utils/useLatest";
+
 import { ThreeViewSplitter } from "../ThreeViewSplitter/ThreeViewSplitter";
 import { FocusScope } from "../utils/FocusScope";
 import { FloatToolWindows } from "./FloatToolWindows";
@@ -25,8 +27,7 @@ import { ToolWindowStateProvider } from "./ToolWindowsState/ToolWindowStateProvi
 import { ToolWindowStripe } from "./ToolWindowStripe";
 import { UndockSide } from "./UndockSide";
 import { Anchor, isHorizontalToolWindow } from "./utils";
-import { useLatest } from "@intellij-platform/core/utils/useLatest";
-import { indexBy } from "ramda";
+import { useOnFocusLost } from "./useOnFocusLost";
 import { useInteractOutside } from "@react-aria/interactions";
 
 interface ToolWindow {
@@ -67,13 +68,6 @@ export interface ToolWindowsProps {
    * props to be passed to the container element.
    */
   containerProps?: Omit<HTMLProps<HTMLDivElement>, "as">;
-
-  /**
-   * By default, `ToolWindows` prevents focus from going to `body`, when something blurs. This is especially
-   * useful for global actions handled at the level of ToolWindows, to be able to consistently capture keyboard events.
-   * setting `disableFocusTrap` to true prevents that default behavior.
-   */
-  allowBlurOnInteractionOutside?: boolean;
 }
 
 export interface ToolWindowRefValue {
@@ -111,15 +105,14 @@ export const ToolWindows = React.forwardRef(function ToolWindows(
   {
     hideToolWindowBars = false,
     useWidescreenLayout = false,
-    allowBlurOnInteractionOutside = false,
     height = "100%",
     minHeight = "0",
     toolWindowsState,
     onToolWindowStateChange,
     windows,
     children,
     mainContentMinWidth = 50,
-    containerProps,
+    containerProps = {},
   }: ToolWindowsProps,
   ref: ForwardedRef<ToolWindowRefValue>
 ): React.ReactElement {
@@ -197,18 +190,23 @@ export const ToolWindows = React.forwardRef(function ToolWindows(
     []
   );
 
-  const [interactionOutside, setInteractionOutside] = useState(false);
+  const interactionOutsideRef = useRef(false);
   useInteractOutside({
     ref: containerRef,
-    isDisabled: !allowBlurOnInteractionOutside,
     onInteractOutsideStart: () => {
-      setInteractionOutside(true);
+      interactionOutsideRef.current = true;
     },
     onInteractOutside: () => {
-      setInteractionOutside(false);
+      interactionOutsideRef.current = false;
     },
   });
 
+  useOnFocusLost(({ focusReceivingElement }) => {
+    if (!focusReceivingElement && !interactionOutsideRef.current) {
+      mainContentFocusScopeRef.current?.focus();
+    }
+  }, containerRef);
+
   // TODO: extract component candidate
   const renderStripe = ({
     anchor,
@@ -431,38 +429,20 @@ export const ToolWindows = React.forwardRef(function ToolWindows(
       </>
     );
   };
+
   return (
-    /**
-     * About FocusScope:
-     * When focus is within the ToolWindows, clicking on non-focusable parts of the UI should not make the focus get
-     * lost. That's especially important with the top level actions being handled on a wrapper around ToolWindows.
-     * Because if the focus goes to body, keyboard events are no longer handled, with the way action system is currently
-     * implemented.
-     * AriaFocusScope provides a somewhat accurate behaviour, but it might also be too much, and we can consider
-     * a more light-weight approach. Issues with the current usage of AriaFocus:
-     * - FocusScope traverses the dom tree to find focusable elements, and it might come with considerable performance
-     *   penalty at this place. Something to be investigated more.
-     * - When the focus is lost, e.g. the active tool window closes, FocusScope moves focus to the **first** focusable
-     *  element, whereas in the reference implementation of Tool Windows, the focus goes to the main content (usually
-     *  the editor).
-     *  TODO: investigate alternative approaches for focus handling here.
-     */
-    <AriaFocusScope
-      contain={!(allowBlurOnInteractionOutside && interactionOutside)}
+    <StyledToolWindowOuterLayout.Shell
+      {...containerProps}
+      ref={containerRef}
+      /**
+       * Potential refactoring: hideStripes can also be handled by conditionally
+       * rendering tool window bars, instead of considering it as a feature of
+       * StyledToolWindowOuterLayout
+       **/
+      hideStripes={hideToolWindowBars}
+      style={{ height, minHeight, ...containerProps?.style }}
     >
-      <StyledToolWindowOuterLayout.Shell
-        {...containerProps}
-        ref={containerRef}
-        /**
-         * Potential refactoring: hideStripes can also be handled by conditionally
-         * rendering tool window bars, instead of considering it as a feature of
-         * StyledToolWindowOuterLayout
-         **/
-        hideStripes={hideToolWindowBars}
-        style={{ height, minHeight, ...containerProps?.style }}
-      >
-        {layoutState && renderInnerLayout(layoutState)}
-      </StyledToolWindowOuterLayout.Shell>
-    </AriaFocusScope>
+      {layoutState && renderInnerLayout(layoutState)}
+    </StyledToolWindowOuterLayout.Shell>
   );
 });
diff --git a/packages/jui/src/ToolWindows/useOnFocusLost.tsx b/packages/jui/src/ToolWindows/useOnFocusLost.tsx
@@ -0,0 +1,38 @@
+import { RefObject, useEffect } from "react";
+
+/**
+ * Executes a callback function when focus is lost from the container element. i.e. when the currently focused element
+ * was within the container and:
+ * - The focus is going to an element outside the container, or
+ * - The focus is about to get lost (i.e. go to `document.body`).
+ *
+ * Note: react-aria's `useFocusWithin` (and it's `onBlurWithin` option) can't be used, since (at least currently) it
+ * doesn't cover the scenario where the focus is about to get lost due to the focused element getting unmounted.
+ */
+export function useOnFocusLost(
+  onFocusLost: (args: {
+    focusLosingElement: HTMLElement | null;
+    focusReceivingElement: Element | null;
+  }) => void,
+  containerRef: RefObject<HTMLElement>
+): void {
+  useEffect(() => {
+    const handleBodyFocus = (e: FocusEvent) => {
+      if (
+        e.target instanceof HTMLElement &&
+        containerRef.current?.contains(e.target) &&
+        (!e.relatedTarget || e.relatedTarget instanceof HTMLElement) &&
+        !containerRef.current?.contains(e.relatedTarget)
+      ) {
+        onFocusLost({
+          focusLosingElement: e.target,
+          focusReceivingElement: e.relatedTarget,
+        });
+      }
+    };
+    containerRef.current?.addEventListener("focusout", handleBodyFocus);
+    return () => {
+      containerRef.current?.removeEventListener("focusout", handleBodyFocus);
+    };
+  }, []);
+}
diff --git a/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx b/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx
@@ -217,10 +217,7 @@ describe("DefaultToolWindowActions", () => {
         </ThemeProvider>
       );
       cy.realPress(["Meta", "2"]);
-
-      // Focus should to go main content, but it doesn't currently because of a known issue. FIXME in ToolWindows
-      // cy.findByTestId("main content focusable").should("have.focus");
-      cy.findByTestId("First window").find("input").eq(0).should("have.focus");
+      cy.findByTestId("main content focusable").should("have.focus");
     });
   });
 });