Skip to content

Commit

Permalink
remove initialFocus from Popover (#2168)
Browse files Browse the repository at this point in the history
  • Loading branch information
mayank99 authored Jul 30, 2024
1 parent 2d65521 commit 4d57f9f
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 5 deletions.
5 changes: 5 additions & 0 deletions .changeset/serious-experts-hammer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@itwin/itwinui-react': patch
---

Adjusted focus management in `Popover` so that it allows interactive elements inside the popover to be more easily focused. This more closely matches the behavior of the HTML `<dialog>` element, which focuses the first interactive element inside it.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 1 addition & 5 deletions packages/itwinui-react/src/core/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -401,11 +401,7 @@ export const Popover = React.forwardRef((props, forwardedRef) => {
portalContainer={popoverElement} // portal nested popovers into this one
>
<DisplayContents />
<FloatingFocusManager
context={popover.context}
modal={false}
initialFocus={popover.refs.floating}
>
<FloatingFocusManager context={popover.context} modal={false}>
<Box
className={cx(
{ 'iui-popover-surface': applyBackground },
Expand Down
31 changes: 31 additions & 0 deletions testing/e2e/app/routes/Popover/route.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Popover } from '@itwin/itwinui-react';
import { useSearchParams } from '@remix-run/react';

export default function Page() {
const [searchParams] = useSearchParams();

const shouldImperativeFocus = searchParams.get('imperativeFocus') === 'true';
const shouldFocusInput = searchParams.get('focusInput') === 'true';

return (
<>
<Popover
applyBackground
content={
<div>
<h2
tabIndex={-1}
ref={shouldImperativeFocus ? (el) => el?.focus() : undefined}
>
Popover title
</h2>
<p>Popover content</p>
{shouldFocusInput && <input />}
</div>
}
>
<button>Open</button>
</Popover>
</>
);
}
31 changes: 31 additions & 0 deletions testing/e2e/app/routes/Popover/spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { test, expect } from '@playwright/test';

test.describe('Popover (focus)', () => {
test('should focus the popover by default', async ({ page }) => {
await page.goto('/Popover');
await page.click('button');
await expect(page.getByRole('dialog')).toBeFocused();
});

test('should allow imperative focus', async ({ page }) => {
await page.goto('/Popover?imperativeFocus=true');
await page.click('button');
await expect(page.getByRole('heading')).toBeFocused();
});

test('should prioritize interactive elements inside the popover content (over the popover element)', async ({
page,
}) => {
await page.goto('/Popover?focusInput=true');
await page.click('button');
await expect(page.locator('input')).toBeFocused();
});

test('should prioritize imperative focus over interactive elements', async ({
page,
}) => {
await page.goto('/Popover?focusInput=true&imperativeFocus=true');
await page.click('button');
await expect(page.getByRole('heading')).toBeFocused();
});
});

0 comments on commit 4d57f9f

Please sign in to comment.