Close search bar in Connect on blur (#25186)

* Remove closeAndResetInput

* Remove unnecessary input focus from `open`

As the comment in SearchBar explains, the focus wasn't actually achieving
anything. I added it only because I was testing SearchContext in separation
and depended on the input getting focused in the test.

This doesn't reflect how the input is actually used so I adjusted the test.

* Move onFocus on input, add onBlur to input

* Remove nesting from handling outside click

* SearchContext: Rename onInputValueChange to setInputValue

* Do proper type check before calling focus on previouslyActive

* Add a temp workaround for tsc issue

* Make TypeScript 4.8.4 happy

web/packages/teleterm/src/ui/Search/SearchBar.test.tsx

@@ -16,7 +16,7 @@
 
 import React from 'react';
 import userEvent from '@testing-library/user-event';
-import { render, screen, waitFor } from 'design/utils/testing';
+import { render, screen, waitFor, act } from 'design/utils/testing';
 import { makeSuccessAttempt } from 'shared/hooks/useAsync';
 
 import Logger, { NullService } from 'teleterm/logger';
@@ -315,6 +315,43 @@ it('shows a login modal when a request to a cluster from the current workspace f
   expect(screen.getByRole('menu')).toBeInTheDocument();
 });
 
+it('closes on a click on an unfocusable element outside of the search bar', async () => {
+  const user = userEvent.setup();
+  const cluster = makeRootCluster();
+  const resourceSearchResult = {
+    results: [],
+    errors: [],
+    search: 'foo',
+  };
+  const resourceSearch = async () => resourceSearchResult;
+  jest
+    .spyOn(useSearch, 'useResourceSearch')
+    .mockImplementation(() => resourceSearch);
+
+  const appContext = new MockAppContext();
+  appContext.workspacesService.setState(draft => {
+    draft.rootClusterUri = cluster.uri;
+  });
+  appContext.clustersService.setState(draftState => {
+    draftState.clusters.set(cluster.uri, cluster);
+  });
+
+  render(
+    <MockAppContextProvider appContext={appContext}>
+      <SearchBarConnected />
+      <p data-testid="unfocusable-element">Lorem ipsum</p>
+    </MockAppContextProvider>
+  );
+
+  await user.type(screen.getByRole('searchbox'), 'foo');
+  expect(screen.getByRole('menu')).toBeInTheDocument();
+
+  act(() => {
+    screen.getByTestId('unfocusable-element').click();
+  });
+  expect(screen.queryByRole('menu')).not.toBeInTheDocument();
+});
+
 const getMockedSearchContext = (): SearchContext.SearchContext => ({
   inputValue: 'foo',
   filters: [],
@@ -323,10 +360,10 @@ const getMockedSearchContext = (): SearchContext.SearchContext => ({
   isOpen: true,
   open: () => {},
   close: () => {},
-  closeAndResetInput: () => {},
+  closeWithoutRestoringFocus: () => {},
   resetInput: () => {},
   changeActivePicker: () => {},
-  onInputValueChange: () => {},
+  setInputValue: () => {},
   activePicker: pickers.actionPicker,
   inputRef: undefined,
   pauseUserInteraction: async cb => {
@@ -335,4 +372,5 @@ const getMockedSearchContext = (): SearchContext.SearchContext => ({
   addWindowEventListener: () => ({
     cleanup: () => {},
   }),
+  makeEventListener: cb => cb,
 });

web/packages/teleterm/src/ui/Search/SearchBar.tsx

@@ -53,12 +53,14 @@ function SearchBar() {
   const {
     activePicker,
     inputValue,
-    onInputValueChange,
+    setInputValue,
     inputRef,
     isOpen,
     open,
     close,
+    closeWithoutRestoringFocus,
     addWindowEventListener,
+    makeEventListener,
   } = useSearchContext();
   const ctx = useAppContext();
   ctx.clustersService.useState();
@@ -69,23 +71,53 @@ function SearchBar() {
     },
   });
 
+  // Handle outside click when the search bar is open.
   useEffect(() => {
+    if (!isOpen) {
+      return;
+    }
+
     const onClickOutside = e => {
       if (!e.composedPath().includes(containerRef.current)) {
         close();
       }
     };
-    if (isOpen) {
-      const { cleanup } = addWindowEventListener('click', onClickOutside, {
-        capture: true,
-      });
-      return cleanup;
-    }
+
+    const { cleanup } = addWindowEventListener('click', onClickOutside, {
+      capture: true,
+    });
+    return cleanup;
   }, [close, isOpen, addWindowEventListener]);
 
-  function handleOnFocus(e: React.FocusEvent) {
-    open(e.relatedTarget);
-  }
+  // closeIfAnotherElementReceivedFocus handles a scenario where the focus shifts from the search
+  // input to another element on page. It does nothing if there's no other element that receives
+  // focus, i.e. the user clicks on an unfocusable element (for example, the empty space between the
+  // search bar and the profile selector).
+  //
+  // If that element is present though, onBlur takes precedence over onClickOutside. For example,
+  // clicking on a button outside of the search bar will trigger onBlur and will not trigger
+  // onClickOutside.
+  const closeIfAnotherElementReceivedFocus = makeEventListener(
+    (event: FocusEvent) => {
+      const elementReceivingFocus = event.relatedTarget;
+
+      if (!(elementReceivingFocus instanceof Node)) {
+        // event.relatedTarget might be undefined if the user clicked on an element that is not
+        // focusable. The element might or might not be inside the search bar, however we have no way
+        // of knowing that. Instead of closing the search bar, we defer this responsibility to the
+        // onClickOutside handler and return early.
+        //
+        return;
+      }
+
+      const isElementReceivingFocusOutsideOfSearchBar =
+        !containerRef.current.contains(elementReceivingFocus);
+
+      if (isElementReceivingFocusOutsideOfSearchBar) {
+        closeWithoutRestoringFocus(); // without restoring focus
+      }
+    }
+  );
 
   const defaultInputProps = {
     ref: inputRef,
@@ -93,8 +125,12 @@ function SearchBar() {
     placeholder: activePicker.placeholder,
     value: inputValue,
     onChange: e => {
-      onInputValueChange(e.target.value);
+      setInputValue(e.target.value);
     },
+    onFocus: (e: React.FocusEvent) => {
+      open(e.relatedTarget);
+    },
+    onBlur: closeIfAnotherElementReceivedFocus,
     spellCheck: false,
   };
 
@@ -114,7 +150,6 @@ function SearchBar() {
       `}
       justifyContent="center"
       ref={containerRef}
-      onFocus={handleOnFocus}
     >
       {!isOpen && (
         <>
@@ -124,7 +159,11 @@ function SearchBar() {
       )}
       {isOpen && (
         <activePicker.picker
-          // autofocusing cannot be done in `open` function as it would focus the input from closed state
+          // When the search bar transitions from closed to open state, `inputRef.current` within
+          // the `open` function refers to the input element from when the search bar was closed.
+          //
+          // Thus, calling `focus()` on it would have no effect. Instead, we add `autoFocus` on the
+          // input when the search bar is open.
           input={<Input {...defaultInputProps} autoFocus={true} />}
         />
       )}

web/packages/teleterm/src/ui/Search/SearchContext.test.tsx

@@ -146,11 +146,12 @@ describe('addWindowEventListener', () => {
 describe('open', () => {
   it('manages the focus properly when called with no arguments', () => {
     const SearchInput = () => {
-      const { inputRef, open, close } = useSearchContext();
+      const { inputRef, isOpen, open, close } = useSearchContext();
 
       return (
         <>
           <input data-testid="search-input" ref={inputRef} />
+          <div data-testid="is-open">{String(isOpen)}</div>
           <button data-testid="open" onClick={() => open()} />
           <button data-testid="close" onClick={() => close()} />
         </>
@@ -168,13 +169,59 @@ describe('open', () => {
     );
 
     const otherInput = screen.getByTestId('other-input');
-    const searchInput = screen.getByTestId('search-input');
     otherInput.focus();
 
+    expect(screen.getByTestId('is-open')).toHaveTextContent('false');
     screen.getByTestId('open').click();
-    expect(searchInput).toHaveFocus();
+    expect(screen.getByTestId('is-open')).toHaveTextContent('true');
 
     screen.getByTestId('close').click();
     expect(otherInput).toHaveFocus();
   });
 });
+
+describe('close', () => {
+  it('restores focus on the previously active element', () => {
+    const previouslyActive = {
+      focus: jest.fn(),
+    } as unknown as HTMLInputElement;
+    const { result } = renderHook(() => useSearchContext(), {
+      wrapper: ({ children }) => (
+        <SearchContextProvider>{children}</SearchContextProvider>
+      ),
+    });
+
+    act(() => {
+      result.current.open(previouslyActive);
+    });
+
+    act(() => {
+      result.current.close();
+    });
+
+    expect(previouslyActive.focus).toHaveBeenCalledTimes(1);
+  });
+});
+
+describe('closeWithoutRestoringFocus', () => {
+  it('does not restore focus on the previously active element', () => {
+    const previouslyActive = {
+      focus: jest.fn(),
+    } as unknown as HTMLInputElement;
+    const { result } = renderHook(() => useSearchContext(), {
+      wrapper: ({ children }) => (
+        <SearchContextProvider>{children}</SearchContextProvider>
+      ),
+    });
+
+    act(() => {
+      result.current.open(previouslyActive);
+    });
+
+    act(() => {
+      result.current.closeWithoutRestoringFocus();
+    });
+
+    expect(previouslyActive.focus).not.toHaveBeenCalled();
+  });
+});

web/packages/teleterm/src/ui/Search/SearchContext.tsx

@@ -33,17 +33,20 @@ export interface SearchContext {
   inputValue: string;
   filters: SearchFilter[];
   activePicker: SearchPicker;
-  onInputValueChange(value: string): void;
+  setInputValue(value: string): void;
   changeActivePicker(picker: SearchPicker): void;
   isOpen: boolean;
   open(fromElement?: Element): void;
   close(): void;
-  closeAndResetInput(): void;
+  closeWithoutRestoringFocus(): void;
   resetInput(): void;
   setFilter(filter: SearchFilter): void;
   removeFilter(filter: SearchFilter): void;
   pauseUserInteraction(action: () => Promise<any>): Promise<void>;
   addWindowEventListener: AddWindowEventListener;
+  makeEventListener: <EventListener>(
+    eventListener: EventListener
+  ) => EventListener | undefined;
 }
 
 export type AddWindowEventListener = (
@@ -55,6 +58,7 @@ export type AddWindowEventListener = (
 const SearchContext = createContext<SearchContext>(null);
 
 export const SearchContextProvider: FC = props => {
+  // The type of the ref is Element to adhere to the type of document.activeElement.
   const previouslyActive = useRef<Element>();
   const inputRef = useRef<HTMLInputElement>();
   const [isOpen, setIsOpen] = useState(false);
@@ -77,33 +81,39 @@ export const SearchContextProvider: FC = props => {
   const close = useCallback(() => {
     setIsOpen(false);
     setActivePicker(actionPicker);
-    if (previouslyActive.current instanceof HTMLElement) {
-      previouslyActive.current.focus();
+    if (
+      // The Element type is not guaranteed to have the focus function so we're forced to manually
+      // perform the type check.
+      previouslyActive.current
+    ) {
+      // TODO(ravicious): Revert to a regular `focus()` call (#25186@4f9077eb7) once #25683 gets in.
+      previouslyActive.current['focus']?.();
     }
   }, []);
 
-  const closeAndResetInput = useCallback(() => {
+  const closeWithoutRestoringFocus = useCallback(() => {
+    previouslyActive.current = undefined;
     close();
-    setInputValue('');
   }, [close]);
 
   const resetInput = useCallback(() => {
     setInputValue('');
   }, []);
 
-  function open(fromElement?: Element): void {
+  function open(fromElement?: HTMLElement): void {
     if (isOpen) {
       // Even if the search bar is already open, we want to focus on the input anyway. The search
       // input might lose focus due to user interaction while the search bar stays open. Focusing
       // here again makes it possible to use the shortcut to grant the focus to the input again.
+      //
+      // Also note that SearchBar renders two distinct input elements, one when the search bar is
+      // closed and another when its open. During the initial call to this function,
+      // inputRef.current is equal to the element from when the search bar was closed.
       inputRef.current?.focus();
       return;
     }
 
-    // In case `open` was called without `fromElement` (e.g. when using the keyboard shortcut), we
-    // must read `document.activeElement` before we focus the input.
     previouslyActive.current = fromElement || document.activeElement;
-    inputRef.current?.focus();
     setIsOpen(true);
   }
 
@@ -163,6 +173,22 @@ export const SearchContextProvider: FC = props => {
     [isUserInteractionPaused]
   );
 
+  /**
+   * makeEventListener is similar to addWindowEventListener but meant for situations where you want
+   * to add a listener to an element directly. By wrapping the listener in makeEventListener, you
+   * make sure that the listener will be removed when the interaction with the search bar is paused.
+   */
+  const makeEventListener = useCallback(
+    eventListener => {
+      if (isUserInteractionPaused) {
+        return;
+      }
+
+      return eventListener;
+    },
+    [isUserInteractionPaused]
+  );
+
   function setFilter(filter: SearchFilter) {
     // UI prevents adding more than one filter of the same type
     setFilters(prevState => [...prevState, filter]);
@@ -187,7 +213,7 @@ export const SearchContextProvider: FC = props => {
       value={{
         inputRef,
         inputValue,
-        onInputValueChange: setInputValue,
+        setInputValue,
         changeActivePicker,
         activePicker,
         filters,
@@ -197,9 +223,10 @@ export const SearchContextProvider: FC = props => {
         isOpen,
         open,
         close,
-        closeAndResetInput,
+        closeWithoutRestoringFocus,
         pauseUserInteraction,
         addWindowEventListener,
+        makeEventListener,
       }}
       children={props.children}
     />

web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx

@@ -65,7 +65,6 @@ export function ActionPicker(props: { input: ReactElement }) {
     close,
     inputValue,
     resetInput,
-    closeAndResetInput,
     filters,
     removeFilter,
     addWindowEventListener,
@@ -108,17 +107,16 @@ export function ActionPicker(props: { input: ReactElement }) {
         // Overall, the context should probably encapsulate more logic so that the components don't
         // have to worry about low-level stuff such as input state. Input state already lives in the
         // search context so it should be managed from there, if possible.
-        if (action.preventAutoClose === true) {
-          resetInput();
-        } else {
-          closeAndResetInput();
+        resetInput();
+        if (!action.preventAutoClose) {
+          close();
         }
       }
       if (action.type === 'parametrized-action') {
         changeActivePicker(getParameterPicker(action));
       }
     },
-    [changeActivePicker, closeAndResetInput, resetInput]
+    [changeActivePicker, close, resetInput]
   );
 
   const filterButtons = filters.map(s => {

web/packages/teleterm/src/ui/Search/pickers/ParameterPicker.tsx

@@ -37,7 +37,7 @@ interface ParameterPickerProps {
 export function ParameterPicker(props: ParameterPickerProps) {
   const {
     inputValue,
-    closeAndResetInput,
+    close,
     changeActivePicker,
     resetInput,
     addWindowEventListener,
@@ -62,13 +62,13 @@ export function ParameterPicker(props: ParameterPickerProps) {
   const onPick = useCallback(
     (item: string) => {
       props.action.perform(item);
-      if (props.action.preventAutoClose === true) {
-        resetInput();
-      } else {
-        closeAndResetInput();
+
+      resetInput();
+      if (!props.action.preventAutoClose) {
+        close();
       }
     },
-    [closeAndResetInput, resetInput, props.action]
+    [close, resetInput, props.action]
   );
 
   const onBack = useCallback(() => {