Skip to content

feat(Select/Dropdown/MenuContainer): arrow key handling to focus items #11132

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

Merged
kmcfaul merged 8 commits into from
Nov 20, 2024
Merged

feat(Select/Dropdown/MenuContainer): arrow key handling to focus items #11132

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0fe487a
feat(Select): default arrow key handling to focus items
adamviktora Oct 23, 2024
039e095
feat(Dropdown): default arrow key handling to focus items
adamviktora Oct 25, 2024
fd712a7
feat(MenuContainer): default arrow key handling to focus items
adamviktora Oct 25, 2024
fdbc8b3
fix: invoke callback only when toggle is focused and menu opened
adamviktora Oct 29, 2024
513c6f5
fix: query selector, refactor common functionality
adamviktora Nov 12, 2024
cdd6f0f
feat: use general onToggleKeydown instead of onToggleArrowKeydown
adamviktora Nov 12, 2024
de44721
refactor(onToggleArrowKeydownDefault): don't use :has() selector
adamviktora Nov 12, 2024
901e606
fix: remove isTypeahead prop in favor of variant
adamviktora Nov 20, 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
33 changes: 33 additions & 0 deletions packages/react-core/src/components/Dropdown/Dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ export interface DropdownProps extends MenuProps, OUIAProps {
onOpenChange?: (isOpen: boolean) => void;
/** Keys that trigger onOpenChange, defaults to tab and escape. It is highly recommended to include Escape in the array, while Tab may be omitted if the menu contains non-menu items that are focusable. */
onOpenChangeKeys?: string[];
/** Callback to override the default behavior when pressing up/down arrow keys when the toggle has focus and the menu is open. By default non-disabled menu items will receive focus - the first item on arrow down or the last item on arrow up. */
onToggleArrowKeydown?: (event: KeyboardEvent) => void;
Copy link
Contributor

@kmcfaul kmcfaul Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this prop can be generalized to onToggleKeydown since it could be used to add any key behavior.

The description could also be updated to Callback to override the toggle keydown behavior. By default, when the toggle has focus and the menu is open, pressing the up/down arrow keys will focus a valid non-disabled menu item - the first item for the down arrow key and last item for the up arrow key. The double when is slightly confusing to me.

Same goes for MenuContainer and Select.

/** Indicates if the menu should be without the outer box-shadow. */
isPlain?: boolean;
/** Indicates if the menu should be scrollable. */
Expand Down Expand Up @@ -85,6 +87,7 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
toggle,
shouldFocusToggleOnSelect = false,
onOpenChange,
onToggleArrowKeydown,
isPlain,
isScrollable,
innerRef,
Expand Down Expand Up @@ -127,6 +130,23 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
}, [isOpen]);

React.useEffect(() => {
const onToggleArrowKeydownDefault = (event: KeyboardEvent) => {
event.preventDefault();

let listItem: HTMLLIElement;
if (event.key === 'ArrowDown') {
listItem = menuRef.current?.querySelector('li');
} else {
const allItems = menuRef.current?.querySelectorAll('li');
listItem = allItems ? allItems[allItems.length - 1] : null;
}

const focusableElement = listItem?.querySelector(
'button:not(:disabled),input:not(:disabled),a:not([aria-disabled="true"])'
);
focusableElement && (focusableElement as HTMLElement).focus();
};

const handleMenuKeys = (event: KeyboardEvent) => {
// Close the menu on tab or escape if onOpenChange is provided
if (
Expand All @@ -139,6 +159,18 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
toggleRef.current?.focus();
}
}

if (
isOpen &&
toggleRef.current?.contains(event.target as Node) &&
(event.key === 'ArrowDown' || event.key === 'ArrowUp')
) {
if (onToggleArrowKeydown) {
onToggleArrowKeydown(event);
} else {
onToggleArrowKeydownDefault(event);
}
}
};

const handleClick = (event: MouseEvent) => {
Expand All @@ -163,6 +195,7 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
toggleRef,
onOpenChange,
onOpenChangeKeys,
onToggleArrowKeydown,
shouldPreventScrollOnItemFocus,
shouldFocusFirstItemOnOpen,
focusTimeoutDelay
Expand Down
46 changes: 44 additions & 2 deletions packages/react-core/src/components/Menu/MenuContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export interface MenuPopperProps {
/** Flag to prevent the popper from overflowing its container and becoming partially obscured. */
preventOverflow?: boolean;
}

export interface MenuContainerProps {
/** Menu to be rendered */
menu: React.ReactElement<any, string | React.JSXElementConstructor<any>>;
Expand All @@ -33,6 +34,8 @@ export interface MenuContainerProps {
onOpenChange?: (isOpen: boolean) => void;
/** Keys that trigger onOpenChange, defaults to tab and escape. It is highly recommended to include Escape in the array, while Tab may be omitted if the menu contains non-menu items that are focusable. */
onOpenChangeKeys?: string[];
/** Callback to override the default behavior when pressing up/down arrow keys when the toggle has focus and the menu is open. By default non-disabled menu items will receive focus - the first item on arrow down or the last item on arrow up. */
onToggleArrowKeydown?: (event: KeyboardEvent) => void;
/** z-index of the dropdown menu */
zIndex?: number;
/** Additional properties to pass to the Popper */
Expand All @@ -55,10 +58,11 @@ export const MenuContainer: React.FunctionComponent<MenuContainerProps> = ({
toggle,
toggleRef,
onOpenChange,
onToggleArrowKeydown,
zIndex = 9999,
popperProps,
onOpenChangeKeys = ['Escape', 'Tab'],
shouldFocusFirstItemOnOpen = true,
shouldFocusFirstItemOnOpen = false,
shouldPreventScrollOnItemFocus = true,
focusTimeoutDelay = 0
}: MenuContainerProps) => {
Expand All @@ -79,6 +83,23 @@ export const MenuContainer: React.FunctionComponent<MenuContainerProps> = ({
}, [isOpen]);

React.useEffect(() => {
const onToggleArrowKeydownDefault = (event: KeyboardEvent) => {
event.preventDefault();

let listItem: HTMLLIElement;
if (event.key === 'ArrowDown') {
listItem = menuRef.current?.querySelector('li');
} else {
const allItems = menuRef.current?.querySelectorAll('li');
listItem = allItems ? allItems[allItems.length - 1] : null;
}

const focusableElement = listItem?.querySelector(
'button:not(:disabled),input:not(:disabled),a:not([aria-disabled="true"])'
);
focusableElement && (focusableElement as HTMLElement).focus();
};

const handleMenuKeys = (event: KeyboardEvent) => {
// Close the menu on tab or escape if onOpenChange is provided
if (
Expand All @@ -90,6 +111,18 @@ export const MenuContainer: React.FunctionComponent<MenuContainerProps> = ({
toggleRef.current?.focus();
}
}

if (
isOpen &&
toggleRef.current?.contains(event.target as Node) &&
(event.key === 'ArrowDown' || event.key === 'ArrowUp')
) {
if (onToggleArrowKeydown) {
onToggleArrowKeydown(event);
} else {
onToggleArrowKeydownDefault(event);
}
}
};

const handleClick = (event: MouseEvent) => {
Expand All @@ -108,7 +141,16 @@ export const MenuContainer: React.FunctionComponent<MenuContainerProps> = ({
window.removeEventListener('keydown', handleMenuKeys);
window.removeEventListener('click', handleClick);
};
}, [focusTimeoutDelay, isOpen, menuRef, onOpenChange, onOpenChangeKeys, shouldPreventScrollOnItemFocus, toggleRef]);
}, [
focusTimeoutDelay,
isOpen,
menuRef,
onOpenChange,
onOpenChangeKeys,
onToggleArrowKeydown,
shouldPreventScrollOnItemFocus,
toggleRef
]);

return (
<Popper
Expand Down
34 changes: 34 additions & 0 deletions packages/react-core/src/components/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ export interface SelectProps extends MenuProps, OUIAProps {
onOpenChange?: (isOpen: boolean) => void;
/** Keys that trigger onOpenChange, defaults to tab and escape. It is highly recommended to include Escape in the array, while Tab may be omitted if the menu contains non-menu items that are focusable. */
onOpenChangeKeys?: string[];
/** Callback to override the default behavior when pressing up/down arrow keys when the toggle has focus and the menu is open. By default non-disabled menu items will receive focus - the first item on arrow down or the last item on arrow up. */
onToggleArrowKeydown?: (event: KeyboardEvent) => void;
/** Indicates that the Select is used as a typeahead (combobox). Focus won't shift to menu items when pressing up/down arrows. */
isTypeahead?: boolean;
Copy link
Contributor

@kmcfaul kmcfaul Nov 8, 2024

Choose a reason for hiding this comment

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

Is there any chance we would want this to be a wider covering variant prop? Ideally we don't want to have this prop and later find we need to also add isCheckbox, isMultiTypeahead, etc. If we go with variant, the prop description can indicate that the prop affects default keyboard behavior based on its value.

What does everyone think?

Copy link
Contributor

@kmcfaul kmcfaul Nov 8, 2024

Choose a reason for hiding this comment

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

@tlabaj @thatblindgeye

Copy link
Contributor

@thatblindgeye thatblindgeye Nov 12, 2024

Choose a reason for hiding this comment

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

That sounds good to me

/** Indicates if the select should be without the outer box-shadow */
isPlain?: boolean;
/** @hide Forwarded ref */
Expand Down Expand Up @@ -95,6 +99,8 @@ const SelectBase: React.FunctionComponent<SelectProps & OUIAProps> = ({
shouldFocusFirstItemOnOpen = false,
onOpenChange,
onOpenChangeKeys = ['Escape', 'Tab'],
onToggleArrowKeydown,
isTypeahead,
isPlain,
innerRef,
zIndex = 9999,
Expand Down Expand Up @@ -131,6 +137,21 @@ const SelectBase: React.FunctionComponent<SelectProps & OUIAProps> = ({
}, [isOpen]);

React.useEffect(() => {
const onToggleArrowKeydownDefault = (event: KeyboardEvent) => {
event.preventDefault();

let listItem: HTMLLIElement;
if (event.key === 'ArrowDown') {
listItem = menuRef.current?.querySelector('li');
} else {
const allItems = menuRef.current?.querySelectorAll('li');
listItem = allItems ? allItems[allItems.length - 1] : null;
}

const focusableElement = listItem?.querySelector('button:not(:disabled),input:not(:disabled)');
focusableElement && (focusableElement as HTMLElement).focus();
};

const handleMenuKeys = (event: KeyboardEvent) => {
// Close the menu on tab or escape if onOpenChange is provided
if (
Expand All @@ -144,6 +165,18 @@ const SelectBase: React.FunctionComponent<SelectProps & OUIAProps> = ({
toggleRef.current?.focus();
}
}

if (
isOpen &&
toggleRef.current?.contains(event.target as Node) &&
(event.key === 'ArrowDown' || event.key === 'ArrowUp')
) {
if (onToggleArrowKeydown) {
onToggleArrowKeydown(event);
} else if (!isTypeahead) {
onToggleArrowKeydownDefault(event);
}
}
};

const handleClick = (event: MouseEvent) => {
Expand All @@ -168,6 +201,7 @@ const SelectBase: React.FunctionComponent<SelectProps & OUIAProps> = ({
toggleRef,
onOpenChange,
onOpenChangeKeys,
onToggleArrowKeydown,
shouldPreventScrollOnItemFocus,
shouldFocusFirstItemOnOpen,
focusTimeoutDelay
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Select/examples/SelectMultiTypeahead.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ export const SelectMultiTypeahead: React.FunctionComponent = () => {
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
>
<SelectList isAriaMultiselectable id="select-multi-typeahead-listbox">
{selectOptions.map((option, index) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Select/examples/SelectMultiTypeaheadCheckbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ export const SelectMultiTypeaheadCheckbox: React.FunctionComponent = () => {
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
>
<SelectList isAriaMultiselectable id="select-multi-typeahead-checkbox-listbox">
{selectOptions.map((option, index) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Select/examples/SelectMultiTypeaheadCreatable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export const SelectMultiTypeaheadCreatable: React.FunctionComponent = () => {
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
>
<SelectList isAriaMultiselectable id="select-multi-create-typeahead-listbox">
{selectOptions.map((option, index) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Select/examples/SelectTypeahead.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ export const SelectTypeahead: React.FunctionComponent = () => {
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
>
<SelectList id="select-typeahead-listbox">
{selectOptions.map((option, index) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Select/examples/SelectTypeaheadCreatable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ export const SelectTypeaheadCreatable: React.FunctionComponent = () => {
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
>
<SelectList id="select-create-typeahead-listbox">
{selectOptions.map((option, index) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-templates/src/components/Select/MultiTypeaheadSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ export const MultiTypeaheadSelectBase: React.FunctionComponent<MultiTypeaheadSel
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
ref={innerRef}
{...props}
>
Expand Down
2 changes: 1 addition & 1 deletion packages/react-templates/src/components/Select/TypeaheadSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ export const TypeaheadSelectBase: React.FunctionComponent<TypeaheadSelectProps>
!isOpen && closeMenu();
}}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
isTypeahead
ref={innerRef}
{...props}
>
Expand Down