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.

Sign up for GitHub

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

Jump to bottom

feat: make Toggle support keyboard #417

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: make Toggle support keyboard #417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c36827
feat: make Toggle support keyboard
peterpeterparker Apr 29, 2024
5ee5f58
test: not called
peterpeterparker Apr 29, 2024
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
32 changes: 29 additions & 3 deletions src/lib/components/Toggle.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,38 @@
const dispatch = createEventDispatcher();

const id = nextElementId("toggle-");

let input: HTMLInputElement | undefined;

const onKeyDown = ({ code }: KeyboardEvent) => {
if (code !== "Space") {
return;
}

input?.click();
Copy link
Collaborator

@dskloetd dskloetd Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For a normal checkbox, one would be able to tab to it to focus it and press space to toggle it, right?
Why is that not possible here, given that the Toggle component is implemented with a checkbox?
Is it because the checkbox has display: none?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it because the checkbox has display: none?

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be solved by making the checkbox visibility: hidden and/or height: 0 instead, or by giving it tabindex="0"?

I think it would be preferable if we could rely on the default browser behavior instead of implementing a keydown listener.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure it is a better solution. If we do so, assuming it would work out, then we would need extra CSS and maybe even a state too to highlight correctly focus, hover and other feedback of the related UI components. One or the other...

Copy link
Member Author

Choose a reason for hiding this comment

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

Also probably more development effort.

Copy link
Member Author

@peterpeterparker peterpeterparker Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That said, here I just realized disabled is not handled and should been added to handle the keyboard event in such case.

Let me know if you rather like going CSS etc. and we close the PR (and comeback to this when we've got time) or otherwise I also add a if to handle disable state and improve this PR.

I'm fine either way, your call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either way is fine with me.

};

let toggle = checked;
peterpeterparker marked this conversation as resolved.
Show resolved Hide resolved

const onInput = (checked: boolean) => {
dispatch("nnsToggle", checked);
toggle = checked;
};
</script>

<div class="toggle" class:disabled>
<div
class="toggle"
class:disabled
tabindex="0"
role="button"
on:keydown={onKeyDown}
aria-pressed={toggle}
>
<input
bind:this={input}
type="checkbox"
{id}
on:input={({ currentTarget }) =>
dispatch("nnsToggle", currentTarget.checked)}
on:input={({ currentTarget }) => onInput(currentTarget.checked)}
{checked}
aria-label={ariaLabel}
{disabled}
Expand All @@ -37,6 +61,8 @@
align-items: center;
margin-top: 1px;

width: fit-content;
peterpeterparker marked this conversation as resolved.
Show resolved Hide resolved

&.disabled {
opacity: var(--toggle-disabled-opacity, 0.25);
}
Expand Down
38 changes: 37 additions & 1 deletion src/tests/lib/components/Toggle.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Toggle from "$lib/components/Toggle.svelte";
import { fireEvent, render } from "@testing-library/svelte";
import { fireEvent, render, waitFor } from "@testing-library/svelte";

describe("Toggle", () => {
const props = {
Expand Down Expand Up @@ -53,4 +53,40 @@ describe("Toggle", () => {

expect(onToggle).toBeCalled();
});

it("should toggle checked with keyboard", () => {
const { component, container } = render(Toggle, { props });

const toggle = container.querySelector("div.toggle") as HTMLDivElement;

const onToggle = vi.fn();
component.$on("nnsToggle", onToggle);

fireEvent.keyDown(toggle, { code: "Space" });
peterpeterparker marked this conversation as resolved.
Show resolved Hide resolved

expect(onToggle).toBeCalled();
});

it("should reflect toggle state on aria pressed", async () => {
const { container } = render(Toggle, { props });

const toggle = container.querySelector("div.toggle") as HTMLDivElement;

expect(toggle.getAttribute("aria-pressed")).toEqual("false");

fireEvent.keyDown(toggle, { code: "Space" });

await waitFor(() =>
expect(toggle.getAttribute("aria-pressed")).toEqual("true"),
);
});

it("should have an accessible toggle", () => {
const { container } = render(Toggle, { props });

const toggle = container.querySelector("div.toggle") as HTMLDivElement;

expect(toggle.getAttribute("role")).toEqual("button");
expect(toggle.getAttribute("tabindex")).toEqual("0");
});
});
Loading