From 0fbd580d022861185819380a75f3575d910426c0 Mon Sep 17 00:00:00 2001 From: Bea Esguerra Date: Tue, 11 Jun 2024 15:20:25 -0600 Subject: [PATCH 1/5] Remove storybook controls for SingleSelect disabled story. Because the storybook args included opened:false, the select was always closed, which was hiding a bug where the disabled select could still be opened using keyboard arrows --- __docs__/wonder-blocks-dropdown/single-select.stories.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/__docs__/wonder-blocks-dropdown/single-select.stories.tsx b/__docs__/wonder-blocks-dropdown/single-select.stories.tsx index 214a9d078..08b2d2e9b 100644 --- a/__docs__/wonder-blocks-dropdown/single-select.stories.tsx +++ b/__docs__/wonder-blocks-dropdown/single-select.stories.tsx @@ -343,9 +343,8 @@ export const LongOptionLabels: StoryComponentType = { * remains focusable while communicating to screen readers that it is disabled. */ export const Disabled: StoryComponentType = { - render: (args) => ( + render: () => ( {}} selectedValue="" From a1957bc9e2cb31526b00504784e5f1078c471423 Mon Sep 17 00:00:00 2001 From: Bea Esguerra Date: Tue, 11 Jun 2024 15:21:52 -0600 Subject: [PATCH 2/5] Add disabled prop to DropdownCore so that it's interactions can be disabled if it is true. This is needed now that the dropdown opener could potentially not have the disabled attribute set (since we use aria-disabled now on the selectopener so that it can be focusable --- .../src/components/dropdown-core.tsx | 10 +++++++--- .../src/components/multi-select.tsx | 2 ++ .../src/components/single-select.tsx | 2 ++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/wonder-blocks-dropdown/src/components/dropdown-core.tsx b/packages/wonder-blocks-dropdown/src/components/dropdown-core.tsx index d0a45dda7..1e7058e07 100644 --- a/packages/wonder-blocks-dropdown/src/components/dropdown-core.tsx +++ b/packages/wonder-blocks-dropdown/src/components/dropdown-core.tsx @@ -169,6 +169,10 @@ type ExportProps = Readonly<{ * top. The items will be filtered by the input. */ isFilterable?: boolean; + /** + * Whether the dropdown and it's interactions should be disabled. + */ + disabled?: boolean; // Optional props with defaults /** @@ -1040,12 +1044,12 @@ class DropdownCore extends React.Component { } render(): React.ReactNode { - const {open, opener, style, className} = this.props; + const {open, opener, style, className, disabled} = this.props; return ( diff --git a/packages/wonder-blocks-dropdown/src/components/multi-select.tsx b/packages/wonder-blocks-dropdown/src/components/multi-select.tsx index 09e98bbaa..808a58ed8 100644 --- a/packages/wonder-blocks-dropdown/src/components/multi-select.tsx +++ b/packages/wonder-blocks-dropdown/src/components/multi-select.tsx @@ -549,6 +549,7 @@ export default class MultiSelect extends React.Component { isFilterable, "aria-invalid": ariaInvalid, "aria-required": ariaRequired, + disabled, } = this.props; const {open, searchText} = this.state; const {clearSearch, filter, noResults, someSelected} = @@ -599,6 +600,7 @@ export default class MultiSelect extends React.Component { }} aria-invalid={ariaInvalid} aria-required={ariaRequired} + disabled={disabled} /> ); } diff --git a/packages/wonder-blocks-dropdown/src/components/single-select.tsx b/packages/wonder-blocks-dropdown/src/components/single-select.tsx index a599ab2a4..f65d92b9b 100644 --- a/packages/wonder-blocks-dropdown/src/components/single-select.tsx +++ b/packages/wonder-blocks-dropdown/src/components/single-select.tsx @@ -448,6 +448,7 @@ export default class SingleSelect extends React.Component { style, "aria-invalid": ariaInvalid, "aria-required": ariaRequired, + disabled, } = this.props; const {searchText} = this.state; const allChildren = React.Children.toArray(children).filter(Boolean); @@ -484,6 +485,7 @@ export default class SingleSelect extends React.Component { labels={labels} aria-invalid={ariaInvalid} aria-required={ariaRequired} + disabled={disabled} /> ); } From 22bbb14a8cfdc9271640e68a20d0c59cd720612b Mon Sep 17 00:00:00 2001 From: Bea Esguerra Date: Tue, 11 Jun 2024 15:44:42 -0600 Subject: [PATCH 3/5] Add tests to make sure event handlers aren't called when a component is disabled --- .../__tests__/dropdown-core.test.tsx | 76 +++++++++++++ .../__tests__/select-opener.test.tsx | 107 +++++++++++++++++- 2 files changed, 182 insertions(+), 1 deletion(-) diff --git a/packages/wonder-blocks-dropdown/src/components/__tests__/dropdown-core.test.tsx b/packages/wonder-blocks-dropdown/src/components/__tests__/dropdown-core.test.tsx index 13597bd51..3f247da6f 100644 --- a/packages/wonder-blocks-dropdown/src/components/__tests__/dropdown-core.test.tsx +++ b/packages/wonder-blocks-dropdown/src/components/__tests__/dropdown-core.test.tsx @@ -772,4 +772,80 @@ describe("DropdownCore", () => { expect(container).toHaveTextContent("3 items"); }); }); + + describe("onOpenChanged", () => { + it("Should be triggered when the down key is pressed and the menu is closed", async () => { + // Arrange + const onOpenMock = jest.fn(); + + render( + } + onOpenChanged={onOpenMock} + />, + ); + // Act + // Press the button + const button = await screen.findByRole("button"); + // NOTE: we need to use fireEvent here because await userEvent doesn't + // support keyUp/Down events and we use these handlers to override + // the default behavior of the button. + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyDown(button, { + keyCode: 40, + }); + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyUp(button, { + keyCode: 40, + }); + + // Assert + expect(onOpenMock).toHaveBeenCalledTimes(1); + expect(onOpenMock).toHaveBeenCalledWith(true); + }); + + it("Should not be triggered when the dropdown is disabled and the down key is pressed and the menu is closed", async () => { + // Arrange + const onOpenMock = jest.fn(); + + render( + } + onOpenChanged={onOpenMock} + disabled={true} + />, + ); + // Act + // Press the button + const button = await screen.findByRole("button"); + // NOTE: we need to use fireEvent here because await userEvent doesn't + // support keyUp/Down events and we use these handlers to override + // the default behavior of the button. + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyDown(button, { + keyCode: 40, + }); + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyUp(button, { + keyCode: 40, + }); + + // Assert + expect(onOpenMock).toHaveBeenCalledTimes(0); + }); + }); }); diff --git a/packages/wonder-blocks-dropdown/src/components/__tests__/select-opener.test.tsx b/packages/wonder-blocks-dropdown/src/components/__tests__/select-opener.test.tsx index f3cad178d..8c7e461f8 100644 --- a/packages/wonder-blocks-dropdown/src/components/__tests__/select-opener.test.tsx +++ b/packages/wonder-blocks-dropdown/src/components/__tests__/select-opener.test.tsx @@ -5,7 +5,7 @@ import {userEvent} from "@testing-library/user-event"; import SelectOpener from "../select-opener"; describe("SelectOpener", () => { - describe("onClick", () => { + describe("onOpenChanged", () => { const children = "text"; it("should trigger using the mouse", async () => { // Arrange @@ -111,5 +111,110 @@ describe("SelectOpener", () => { // Assert expect(onOpenMock).toHaveBeenCalledTimes(1); }); + + it("should not trigger using the mouse if it is disabled", async () => { + // Arrange + const onOpenMock = jest.fn(); + + render( + + {children} + , + ); + + // Act + // Press the button. + await userEvent.click(await screen.findByRole("button")); + + // Assert + expect(onOpenMock).toHaveBeenCalledTimes(0); + }); + + it("should not trigger by pressing {Space} if it is disabled", async () => { + // Arrange + const onOpenMock = jest.fn(); + + render( + + {children} + , + ); + + // Act + // Press the button. + const button = await screen.findByRole("button"); + // NOTE: we need to use fireEvent here because await userEvent doesn't + // support keyUp/Down events and we use these handlers to override + // the default behavior of the button. + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyDown(button, { + key: " ", + }); + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyUp(button, { + key: " ", + }); + + // Assert + expect(onOpenMock).toHaveBeenCalledTimes(0); + }); + + it("should not trigger by pressing {Enter} if it is disabled", async () => { + // Arrange + const onOpenMock = jest.fn(); + + render( + + {children} + , + ); + + // Act + // Press the button. + const button = await screen.findByRole("button"); + // NOTE: we need to use fireEvent here because await userEvent doesn't + // support keyUp/Down events and we use these handlers to override + // the default behavior of the button. + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyDown(button, { + key: "Enter", + }); + // NOTE: We need to trigger multiple events to simulate the browser + // behavior of pressing Enter on a button. By default, browsers will + // trigger a click event on keyDown, but we need to trigger it on + // keyUp. + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyDown(button, { + key: "Enter", + }); + // eslint-disable-next-line testing-library/prefer-user-event + fireEvent.keyUp(button, { + key: "Enter", + }); + + // Assert + expect(onOpenMock).toHaveBeenCalledTimes(0); + }); }); }); From 9188844652cde44a648a4a997636b87c2881d417 Mon Sep 17 00:00:00 2001 From: Bea Esguerra Date: Tue, 11 Jun 2024 16:22:34 -0600 Subject: [PATCH 4/5] Add changeset --- .changeset/curvy-frogs-listen.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/curvy-frogs-listen.md diff --git a/.changeset/curvy-frogs-listen.md b/.changeset/curvy-frogs-listen.md new file mode 100644 index 000000000..2267685c1 --- /dev/null +++ b/.changeset/curvy-frogs-listen.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/wonder-blocks-dropdown": patch +--- + +SingleSelect and MultiSelect: Disable keyboard interactions to open the select if the `disabled` prop is set to `true` From 1c3300580a3941dcfec5abe498ab9d238ec94121 Mon Sep 17 00:00:00 2001 From: Bea Esguerra Date: Wed, 12 Jun 2024 09:39:28 -0600 Subject: [PATCH 5/5] Prevent single/multiselect from being in an open state if the disabled prop is true --- .changeset/curvy-frogs-listen.md | 3 ++- .../__tests__/multi-select.test.tsx | 16 ++++++++++++++ .../__tests__/single-select.test.tsx | 21 +++++++++++++++++++ .../src/components/multi-select.tsx | 7 ++++++- .../src/components/single-select.tsx | 7 ++++++- 5 files changed, 51 insertions(+), 3 deletions(-) diff --git a/.changeset/curvy-frogs-listen.md b/.changeset/curvy-frogs-listen.md index 2267685c1..a9c47f90d 100644 --- a/.changeset/curvy-frogs-listen.md +++ b/.changeset/curvy-frogs-listen.md @@ -2,4 +2,5 @@ "@khanacademy/wonder-blocks-dropdown": patch --- -SingleSelect and MultiSelect: Disable keyboard interactions to open the select if the `disabled` prop is set to `true` +SingleSelect and MultiSelect: Disable keyboard interactions to open the select if the `disabled` prop is set to `true`. Prevent select from being in an open state if +`disabled` prop is `true`. diff --git a/packages/wonder-blocks-dropdown/src/components/__tests__/multi-select.test.tsx b/packages/wonder-blocks-dropdown/src/components/__tests__/multi-select.test.tsx index 452b0b36b..3238d6278 100644 --- a/packages/wonder-blocks-dropdown/src/components/__tests__/multi-select.test.tsx +++ b/packages/wonder-blocks-dropdown/src/components/__tests__/multi-select.test.tsx @@ -1623,6 +1623,22 @@ describe("MultiSelect", () => { // Assert expect(multiSelect).not.toHaveAttribute("disabled"); }); + + it("should not be opened if it is disabled and `open` prop is set to true", () => { + // Arrange + + // Act + doRender( + + + + + , + ); + + // Assert + expect(screen.queryByRole("listbox")).not.toBeInTheDocument(); + }); }); describe("a11y > Focusable", () => { diff --git a/packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx b/packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx index cc2898073..552f91e77 100644 --- a/packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx +++ b/packages/wonder-blocks-dropdown/src/components/__tests__/single-select.test.tsx @@ -1166,6 +1166,27 @@ describe("SingleSelect", () => { // Assert expect(singleSelect).not.toHaveAttribute("disabled"); }); + + it("should not be opened if it is disabled and `open` prop is set to true", () => { + // Arrange + + // Act + doRender( + + + + + , + ); + + // Assert + expect(screen.queryByRole("listbox")).not.toBeInTheDocument(); + }); }); describe("a11y > Focusable", () => { diff --git a/packages/wonder-blocks-dropdown/src/components/multi-select.tsx b/packages/wonder-blocks-dropdown/src/components/multi-select.tsx index 808a58ed8..28055ce52 100644 --- a/packages/wonder-blocks-dropdown/src/components/multi-select.tsx +++ b/packages/wonder-blocks-dropdown/src/components/multi-select.tsx @@ -241,7 +241,12 @@ export default class MultiSelect extends React.Component { state: State, ): Partial | null { return { - open: typeof props.opened === "boolean" ? props.opened : state.open, + // open should always be false if select is disabled + open: props.disabled + ? false + : typeof props.opened === "boolean" + ? props.opened + : state.open, }; } diff --git a/packages/wonder-blocks-dropdown/src/components/single-select.tsx b/packages/wonder-blocks-dropdown/src/components/single-select.tsx index f65d92b9b..4c17c0a71 100644 --- a/packages/wonder-blocks-dropdown/src/components/single-select.tsx +++ b/packages/wonder-blocks-dropdown/src/components/single-select.tsx @@ -252,7 +252,12 @@ export default class SingleSelect extends React.Component { state: State, ): Partial | null { return { - open: typeof props.opened === "boolean" ? props.opened : state.open, + // open should always be false if select is disabled + open: props.disabled + ? false + : typeof props.opened === "boolean" + ? props.opened + : state.open, }; }