Browse Source

fix(compact-select): Prevent CompactSelect from stealing focus back to itself after clicking into another element (#72883)

In certain situations, clicking out of an open `CompactSelect` would
steal focus back to itself.
Malachi Willey 8 months ago
parent
commit
e18471d4f5

+ 9 - 1
static/app/components/compactSelect/control.tsx

@@ -247,6 +247,7 @@ export function Control({
   children,
   ...wrapperProps
 }: ControlProps) {
+  const wrapperRef = useRef<HTMLDivElement>(null);
   // Set up list states (in composite selects, each region has its own state, that way
   // selection values are contained within each region).
   const [listStates, setListStates] = useState<ListState<any>[]>([]);
@@ -358,7 +359,14 @@ export function Control({
         setSearchInputValue('');
         setSearch('');
 
-        triggerRef.current?.focus();
+        // Only restore focus if it's already here or lost to the body.
+        // This prevents focus from being stolen from other elements.
+        if (
+          document.activeElement === document.body ||
+          wrapperRef.current?.contains(document.activeElement)
+        ) {
+          triggerRef.current?.focus();
+        }
       });
     },
   });

+ 7 - 1
static/app/components/compactSelect/index.spec.tsx

@@ -76,6 +76,9 @@ describe('CompactSelect', function () {
     await waitFor(() => {
       expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument();
     });
+    await waitFor(() => {
+      expect(screen.getByRole('button', {name: 'Option One'})).toHaveFocus();
+    });
 
     // Can be dismissed by pressing Escape
     await userEvent.click(screen.getByRole('button', {name: 'Option One'}));
@@ -89,6 +92,9 @@ describe('CompactSelect', function () {
     await waitFor(() => {
       expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument();
     });
+    await waitFor(() => {
+      expect(screen.getByRole('button', {name: 'Option One'})).toHaveFocus();
+    });
 
     // When menu A is open, clicking once on menu B's trigger button closes menu A and
     // then opens menu B
@@ -96,7 +102,7 @@ describe('CompactSelect', function () {
     expect(screen.getByRole('option', {name: 'Option One'})).toBeInTheDocument();
     await userEvent.click(screen.getByRole('button', {name: 'Option Three'}));
     expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument();
-    expect(screen.getByRole('option', {name: 'Option Three'})).toBeInTheDocument();
+    expect(await screen.findByRole('option', {name: 'Option Three'})).toBeInTheDocument();
   });
 
   describe('ListBox', function () {

+ 2 - 2
static/app/components/events/eventTags/eventTagsTree.spec.tsx

@@ -106,10 +106,10 @@ describe('EventTagsTree', function () {
     for (const link of linkDropdowns) {
       await userEvent.click(link);
       expect(
-        screen.getByLabelText('View issues with this tag value')
+        await screen.findByLabelText('View issues with this tag value')
       ).toBeInTheDocument();
       expect(
-        screen.getByLabelText('View other events with this tag value')
+        await screen.findByLabelText('View other events with this tag value')
       ).toBeInTheDocument();
       expect(screen.getByLabelText('Copy tag value to clipboard')).toBeInTheDocument();
     }

+ 1 - 1
static/app/components/events/highlights/highlightsDataSection.spec.tsx

@@ -90,7 +90,7 @@ describe('HighlightsDataSection', function () {
         expect(highlightTagDropdown).toBeInTheDocument();
         await userEvent.click(highlightTagDropdown);
         expect(
-          screen.getByLabelText('View issues with this tag value')
+          await screen.findByLabelText('View issues with this tag value')
         ).toBeInTheDocument();
         expect(
           screen.queryByLabelText('Add to event highlights')

+ 6 - 3
static/app/utils/useOverlay.tsx

@@ -250,10 +250,13 @@ function useOverlay({
         if (interactedOutside.current) {
           onInteractOutside?.();
           interactedOutside.current = false;
-
-          interactOutsideTrigger.current?.focus();
-          interactOutsideTrigger.current?.click();
+          const trigger = interactOutsideTrigger.current;
           interactOutsideTrigger.current = null;
+
+          requestAnimationFrame(() => {
+            trigger?.focus();
+            trigger?.click();
+          });
         }
 
         openState.close();