Skip to content

Commit

Permalink
Fix focus trap issue with static Dialog components
Browse files Browse the repository at this point in the history
We need a tick() when the focus trap is enabled, to wait for other updates to be applied first. Noticed this when playing around in the REPL.
  • Loading branch information
rgossiaux committed Feb 28, 2022
1 parent f5875ef commit 01954a0
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 15 deletions.
87 changes: 77 additions & 10 deletions src/lib/components/dialog/dialog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { click, Keys, press } from "$lib/test-utils/interactions";
import Transition from "$lib/components/transitions/TransitionRoot.svelte";
import { tick } from "svelte";
import svelte from "svelte-inline-compile";
import { writable } from "svelte/store";

let mockId = 0;
jest.mock("../../hooks/use-id", () => {
Expand Down Expand Up @@ -134,19 +135,85 @@ describe("Rendering", () => {

it('should be possible to always render the Dialog if we provide it a `static` prop (and enable focus trapping based on `open`)', async () => {
let focusCounter = jest.fn()
render(
TestRenderer, {
allProps: [
[Button, {}, "Trigger"],
[Dialog, { open: true, onClose: console.log, static: true }, [
[P, {}, "Contents"],
[TestTabSentinel, { onFocus: focusCounter }]
]],
]
})
render(svelte`
<button id="trigger" on:click={() => isOpen = !isOpen}>
Trigger
</button>
<Dialog open={true} on:close={console.log} static>
<p>Contents</p>
<div tabindex={0} on:focus={focusCounter} />
</Dialog>
`)

// Wait for the focus to take effect
await tick();

// Let's verify that the Dialog is already there
expect(getDialog()).not.toBe(null)
expect(focusCounter).toHaveBeenCalledTimes(1)
})

it('should be possible to always render the Dialog if we provide it a `static` prop (and toggle focus trapping based on `open`)', async () => {
let focusCounter = jest.fn()
let isOpen = writable(false);
render(svelte`
<button id="trigger" on:click={() => isOpen = !isOpen}>
Trigger
</button>
<Dialog open={$isOpen} on:close={console.log} static>
<p>Contents</p>
<div tabindex={0} on:focus={focusCounter} />
</Dialog>
`)

// Wait for the focus to take effect
await tick();

// Let's verify that the Dialog is already there
expect(getDialog()).not.toBe(null)
expect(focusCounter).toHaveBeenCalledTimes(0)

isOpen.set(true);
// Wait for the store to trigger rerendering
await tick();

// Wait for the focus to take effect
await tick();

// Let's verify that the Dialog is already there
expect(getDialog()).not.toBe(null)
expect(focusCounter).toHaveBeenCalledTimes(1)
})

it('should be possible to always render the Dialog if we provide it a `static` prop (and enable focus trapping based on `open` with an if block)', async () => {
let focusCounter = jest.fn()
let isOpen = writable(false);
render(svelte`
<button id="trigger" on:click={() => isOpen = !isOpen}>
Trigger
</button>
<Dialog open={$isOpen} on:close={console.log} static>
{#if $isOpen}
<p>Contents</p>
<div tabindex={0} on:focus={focusCounter} />
{/if}
</Dialog>
`)

// Wait for the focus to take effect
await tick();

// Let's verify that the Dialog is already there
expect(getDialog()).not.toBe(null)
expect(focusCounter).toHaveBeenCalledTimes(0)

isOpen.set(true);
// Wait for the store to trigger rerendering
await tick();

// Wait for the focus to take effect
await tick();

// Let's verify that the Dialog is already there
expect(getDialog()).not.toBe(null)
expect(focusCounter).toHaveBeenCalledTimes(1)
Expand Down
6 changes: 1 addition & 5 deletions src/lib/components/focus-trap/FocusTrap.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,12 @@
let previousActiveElement: HTMLElement | null = null;
let initial = true;
async function handleFocus() {
if (initial) {
await tick();
initial = false;
}
if (!enabled) return;
if (containers.size !== 1) return;
let { initialFocus } = options;
await tick();
let activeElement = document.activeElement as HTMLElement;
if (initialFocus) {
Expand Down

1 comment on commit 01954a0

@vercel
Copy link

@vercel vercel bot commented on 01954a0 Feb 28, 2022

Choose a reason for hiding this comment

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

Please sign in to comment.