From 2b4bb41e137983af1c88214b0c621c7f49306cea Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Mon, 17 Jun 2024 12:27:34 -0700 Subject: [PATCH 1/2] fix(compact-select): Prevent CompactSelect from stealing focus back to itself after clicking into another element --- static/app/components/compactSelect/control.tsx | 10 +++++++++- static/app/components/compactSelect/index.spec.tsx | 11 ++++++++++- static/app/utils/useOverlay.tsx | 9 ++++++--- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/static/app/components/compactSelect/control.tsx b/static/app/components/compactSelect/control.tsx index 009b83f6454788..20e243a0b85fe3 100644 --- a/static/app/components/compactSelect/control.tsx +++ b/static/app/components/compactSelect/control.tsx @@ -247,6 +247,7 @@ export function Control({ children, ...wrapperProps }: ControlProps) { + const wrapperRef = useRef(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[]>([]); @@ -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(); + } }); }, }); diff --git a/static/app/components/compactSelect/index.spec.tsx b/static/app/components/compactSelect/index.spec.tsx index cbfcf09f26e840..51a1018d4ebaca 100644 --- a/static/app/components/compactSelect/index.spec.tsx +++ b/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,10 @@ 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(); + await waitFor(() => { + expect(screen.getByRole('button', {name: 'Option Three'})).toHaveFocus(); + }); }); describe('ListBox', function () { diff --git a/static/app/utils/useOverlay.tsx b/static/app/utils/useOverlay.tsx index ee96a89720ca81..db50b586d29238 100644 --- a/static/app/utils/useOverlay.tsx +++ b/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(); From 44ec94352ae0cb8bad3e3ca3d713b3665d749301 Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Mon, 17 Jun 2024 16:30:22 -0700 Subject: [PATCH 2/2] Add some awaits in tests --- static/app/components/compactSelect/index.spec.tsx | 3 --- static/app/components/events/eventTags/eventTagsTree.spec.tsx | 4 ++-- .../events/highlights/highlightsDataSection.spec.tsx | 2 +- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/static/app/components/compactSelect/index.spec.tsx b/static/app/components/compactSelect/index.spec.tsx index 51a1018d4ebaca..253a05caa889ac 100644 --- a/static/app/components/compactSelect/index.spec.tsx +++ b/static/app/components/compactSelect/index.spec.tsx @@ -103,9 +103,6 @@ describe('CompactSelect', function () { await userEvent.click(screen.getByRole('button', {name: 'Option Three'})); expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument(); expect(await screen.findByRole('option', {name: 'Option Three'})).toBeInTheDocument(); - await waitFor(() => { - expect(screen.getByRole('button', {name: 'Option Three'})).toHaveFocus(); - }); }); describe('ListBox', function () { diff --git a/static/app/components/events/eventTags/eventTagsTree.spec.tsx b/static/app/components/events/eventTags/eventTagsTree.spec.tsx index f04fd60aebe124..7d07c787718359 100644 --- a/static/app/components/events/eventTags/eventTagsTree.spec.tsx +++ b/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(); } diff --git a/static/app/components/events/highlights/highlightsDataSection.spec.tsx b/static/app/components/events/highlights/highlightsDataSection.spec.tsx index d95b43e7fbfcac..9a638ef87652e9 100644 --- a/static/app/components/events/highlights/highlightsDataSection.spec.tsx +++ b/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')