Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable keyboard interactions for DropdownCore if component is disabled #2250

Merged
merged 6 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curvy-frogs-listen.md
Original file line number Diff line number Diff line change
@@ -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`
3 changes: 1 addition & 2 deletions __docs__/wonder-blocks-dropdown/single-select.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => (
<SingleSelect
{...args}
placeholder="Choose a fruit"
onChange={() => {}}
selectedValue=""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<DropdownCore
initialFocusedIndex={undefined}
onSearchTextChanged={jest.fn()}
// mock the items (3 options)
items={items}
role="listbox"
open={false}
// mock the opener elements
opener={<button />}
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(
<DropdownCore
initialFocusedIndex={undefined}
onSearchTextChanged={jest.fn()}
// mock the items (3 options)
items={items}
role="listbox"
open={false}
// mock the opener elements
opener={<button />}
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);
});
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Thanks for adding all the tests (including the open + disabled case.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
<SelectOpener
onOpenChanged={onOpenMock}
disabled={true}
error={false}
isPlaceholder={false}
light={false}
open={false}
>
{children}
</SelectOpener>,
);

// 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(
<SelectOpener
onOpenChanged={onOpenMock}
disabled={true}
error={false}
isPlaceholder={false}
light={false}
open={false}
>
{children}
</SelectOpener>,
);

// 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(
<SelectOpener
onOpenChanged={onOpenMock}
disabled={true}
error={false}
isPlaceholder={false}
light={false}
open={false}
>
{children}
</SelectOpener>,
);

// 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);
});
});
});
10 changes: 7 additions & 3 deletions packages/wonder-blocks-dropdown/src/components/dropdown-core.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
/**
Expand Down Expand Up @@ -1040,12 +1044,12 @@ class DropdownCore extends React.Component<Props, State> {
}

render(): React.ReactNode {
const {open, opener, style, className} = this.props;
const {open, opener, style, className, disabled} = this.props;

return (
<View
onKeyDown={this.handleKeyDown}
onKeyUp={this.handleKeyUp}
onKeyDown={!disabled ? this.handleKeyDown : undefined}
onKeyUp={!disabled ? this.handleKeyUp : undefined}
style={[styles.menuWrapper, style]}
className={className}
>
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This makes me realize that we should force the listbox/dropdown to be closed when it is disabled even if open/opened is set to true.

Right now, if I use the Default story and turn on both the opened and disabled controls, then the listbox will be shown, but to me it looks incorrect as disabled should take more precedence (IMO). What are your thoughts on that?

https://5e1bf4b385e3fb0020b7073c-nhmqyuvdyi.chromatic.com/iframe.html?args=opened:!true;disabled:!true&id=packages-dropdown-singleselect--default&viewMode=story

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! I've updated it so the derived open state will always be false if the disabled prop is true!

Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,7 @@ export default class MultiSelect extends React.Component<Props, State> {
isFilterable,
"aria-invalid": ariaInvalid,
"aria-required": ariaRequired,
disabled,
} = this.props;
const {open, searchText} = this.state;
const {clearSearch, filter, noResults, someSelected} =
Expand Down Expand Up @@ -599,6 +600,7 @@ export default class MultiSelect extends React.Component<Props, State> {
}}
aria-invalid={ariaInvalid}
aria-required={ariaRequired}
disabled={disabled}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ export default class SingleSelect extends React.Component<Props, State> {
style,
"aria-invalid": ariaInvalid,
"aria-required": ariaRequired,
disabled,
} = this.props;
const {searchText} = this.state;
const allChildren = React.Children.toArray(children).filter(Boolean);
Expand Down Expand Up @@ -484,6 +485,7 @@ export default class SingleSelect extends React.Component<Props, State> {
labels={labels}
aria-invalid={ariaInvalid}
aria-required={ariaRequired}
disabled={disabled}
/>
);
}
Expand Down
Loading