Skip to content

Commit

Permalink
fix Popover focus (#1602)
Browse files Browse the repository at this point in the history
Fixed focus behavior by changing props in `FloatingFocusManager` and adding `FloatingPortal`; also added fallback `id`/`aria-labelledby` to ensure the dialog is always labelled when it receives focus. Correct behavior described in Accessibility section of docs.

Also adjusted some props.
  • Loading branch information
mayank99 authored Oct 2, 2023
1 parent 32d08f4 commit 6d1f6b3
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 32 deletions.
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.
4 changes: 4 additions & 0 deletions apps/storybook/src/Table.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2195,27 +2195,31 @@ export const CustomFilter = () => {
handleChange(value === 'on', '3');
}}
checked={filter === '3'}
autoFocus={filter === '3'} // moving focus to checked radio button when filter dialog opens
/>
<Radio
label="Contains '5'"
onChange={({ target: { value } }) => {
handleChange(value === 'on', '5');
}}
checked={filter === '5'}
autoFocus={filter === '5'}
/>
<Radio
label="Contains '7'"
onChange={({ target: { value } }) => {
handleChange(value === 'on', '7');
}}
checked={filter === '7'}
autoFocus={filter === '7'}
/>
<Radio
label='No filter'
onChange={({ target: { value } }) => {
handleChange(value === 'on', '');
}}
checked={filter === ''}
autoFocus={filter === ''}
/>
</BaseFilter>
);
Expand Down
4 changes: 2 additions & 2 deletions packages/itwinui-react/src/core/DropdownMenu/DropdownMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type {
PortalProps,
} from '../utils/index.js';
import { Menu } from '../Menu/Menu.js';
import { Popover, usePopover } from '../Popover/Popover.js';
import { usePopover } from '../Popover/Popover.js';

export type DropdownMenuProps = {
/**
Expand All @@ -37,7 +37,7 @@ export type DropdownMenuProps = {
*/
children: React.ReactNode;
} & Pick<
React.ComponentProps<typeof Popover>,
Parameters<typeof usePopover>[0],
'visible' | 'onVisibleChange' | 'placement' | 'matchWidth'
> &
React.ComponentPropsWithoutRef<'ul'> &
Expand Down
90 changes: 60 additions & 30 deletions packages/itwinui-react/src/core/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ import {
useFocus,
safePolygon,
useRole,
FloatingPortal,
} from '@floating-ui/react';
import type { SizeOptions, Placement } from '@floating-ui/react';
import {
Box,
cloneElementWithRef,
useControlledState,
useId,
useMergedRefs,
} from '../utils/index.js';
import type { PolymorphicForwardRefComponent } from '../utils/index.js';
Expand All @@ -54,12 +56,10 @@ type PopoverOptions = {
onVisibleChange?: (visible: boolean) => void;
/**
* If true, the popover will close when clicking outside it.
*
* @default true
*/
closeOnOutsideClick?: boolean;
/**
* Whether the popover should match the width of the trigger.
*/
matchWidth?: boolean;
};

// keep public api small to start with
Expand Down Expand Up @@ -101,6 +101,10 @@ type PopoverInternalProps = {
*/
trigger?: Partial<Record<'hover' | 'click' | 'focus', boolean>>;
role?: 'dialog' | 'menu' | 'listbox';
/**
* Whether the popover should match the width of the trigger.
*/
matchWidth?: boolean;
};

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -193,32 +197,49 @@ export const usePopover = (options: PopoverOptions & PopoverInternalProps) => {
// ----------------------------------------------------------------------------

type PopoverPublicProps = {
/**
* Content displayed inside the popover.
*/
content?: React.ReactNode;
/**
* Element that triggers the popover. Should usually be a button.
*/
children?: React.ReactNode;
/**
* Whether the popover adds recommended CSS (background-color, box-shadow, etc.) to itself.
*
* @default false
*/
applyBackground?: boolean;
} & PortalProps &
PopoverOptions;

/**
* A utility component to help with positioning of floating content.
* Built on top of [`floating-ui`](https://floating-ui.com/)
* A utility component to help with positioning of floating content relative to a trigger.
* Built on top of [`floating-ui`](https://floating-ui.com/).
*
* @see https://itwinui.bentley.com/docs/popover
*
* @example
* <Popover content='This is a popover'>
* <Button>Show popover</Button>
* </Popover>
*/
export const Popover = React.forwardRef((props, forwardedRef) => {
const {
portal = true,
//
// popover options
visible,
placement,
placement = 'bottom-start',
onVisibleChange,
closeOnOutsideClick,
matchWidth,
closeOnOutsideClick = true,
//
// dom props
className,
children,
content,
applyBackground,
applyBackground = false,
...rest
} = props;

Expand All @@ -227,7 +248,6 @@ export const Popover = React.forwardRef((props, forwardedRef) => {
placement,
onVisibleChange,
closeOnOutsideClick,
matchWidth,
role: 'dialog',
});

Expand All @@ -239,36 +259,46 @@ export const Popover = React.forwardRef((props, forwardedRef) => {
setPopoverElement,
);

const triggerId = `${useId()}-trigger`;
const hasAriaLabel = !!props['aria-labelledby'] || !!props['aria-label'];

return (
<>
{cloneElementWithRef(children, (children) => ({
id: children.props.id || triggerId,
...popover.getReferenceProps(children.props),
ref: popover.refs.setReference,
}))}

{popover.open ? (
<Portal portal={portal}>
<ThemeProvider
portalContainer={popoverElement} // portal nested popovers into this one
>
<FloatingFocusManager
context={popover.context}
modal={false}
initialFocus={-1}
returnFocus
<FloatingPortal>
<ThemeProvider
portalContainer={popoverElement} // portal nested popovers into this one
>
<Box
className={cx(
{ 'iui-popover-surface': applyBackground },
className,
)}
{...popover.getFloatingProps(rest)}
ref={popoverRef}
<FloatingFocusManager
context={popover.context}
modal={false}
initialFocus={popover.refs.floating}
>
{content}
</Box>
</FloatingFocusManager>
</ThemeProvider>
<Box
className={cx(
{ 'iui-popover-surface': applyBackground },
className,
)}
aria-labelledby={
!hasAriaLabel
? popover.refs.domReference.current?.id
: undefined
}
{...popover.getFloatingProps(rest)}
ref={popoverRef}
>
{content}
</Box>
</FloatingFocusManager>
</ThemeProvider>
</FloatingPortal>
</Portal>
) : null}
</>
Expand Down

0 comments on commit 6d1f6b3

Please sign in to comment.