Skip to content

Commit

Permalink
fix(Future Select): remove redundant focus management (#5015)
Browse files Browse the repository at this point in the history
* Remove redundant focus lock on Select's Popover implementation

* Remove redundant autoFocus on Select's ListBox

* Remove outline when listbox is focused (happens when clicking open the select)
  • Loading branch information
dougmacknz authored Sep 4, 2024
1 parent fe3cbf1 commit 6bb072f
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 21 deletions.
6 changes: 6 additions & 0 deletions .changeset/twelve-pugs-whisper.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@kaizen/components": patch
---

Future Select: remove redundant focus management

Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,7 @@ describe("<FilterSelect>", () => {
render(<FilterSelectWrapper isOpen />)
expect(screen.queryByRole("listbox")).toBeVisible()
await waitFor(() => {
expect(
screen.queryByRole("option", { name: "Regular" })
).toHaveFocus()
expect(screen.queryByRole("listbox")).toHaveFocus()
})
})

Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/__future__/Select/Select.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,11 @@ describe("<Select />", () => {
})

describe("Given the menu is opened", () => {
it("moves the focus to the first focusable element inside the menu initially", async () => {
it("focuses the listbox initially", async () => {
const { getByRole } = render(<SelectWrapper defaultOpen />)
expect(getByRole("listbox")).toBeVisible()
await waitFor(() => {
expect(getByRole("option", { name: "Short black" })).toHaveFocus()
expect(getByRole("listbox")).toHaveFocus()
})
})
it("is closed when hits the escape key", async () => {
Expand Down Expand Up @@ -305,12 +305,12 @@ describe("<Select />", () => {
})
})

it("keeps the focus ring at the first element when hits arrow up key on it", async () => {
it("focuses the last item in the list on up arrow press", async () => {
const { getByRole } = render(<SelectWrapper />)
await user.tab()
await user.keyboard("{ArrowUp}")
await waitFor(() => {
expect(getByRole("option", { name: "Short black" })).toHaveFocus()
expect(getByRole("option", { name: "Magic" })).toHaveFocus()
})
})

Expand Down
11 changes: 1 addition & 10 deletions packages/components/src/__future__/Select/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,16 +191,7 @@ export const Select = <Option extends SelectOption = SelectOption>({
trigger(selectToggleProps, selectToggleProps.ref)
)}
{state.isOpen && (
<Popover
id={popoverId}
portalContainer={portalContainer}
refs={refs}
focusOnProps={{
onEscapeKey: state.close,
onClickOutside: state.close,
noIsolation: true,
}}
>
<Popover id={popoverId} portalContainer={portalContainer} refs={refs}>
<SelectProvider<Option> state={state}>
<SelectPopoverContents menuProps={menuProps}>
{children}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
padding: 0;
display: grid;
max-height: 22rem;
}

&.focus {
outline: none;
}
.listBox:focus-visible {
outline: none;
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const ListBox = <Option extends SelectOption>({
const { state } = useSelectContext<Option>()
const ref = React.useRef<HTMLUListElement>(null)
const { listBoxProps } = useListBox(
{ ...menuProps, disallowEmptySelection: true, autoFocus: "first" },
{ ...menuProps, disallowEmptySelection: true },
state,
ref
)
Expand Down

0 comments on commit 6bb072f

Please sign in to comment.