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.

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

Better support for useDocumentEventListeners on Menu components #545

Merged
merged 1 commit into from
Feb 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,27 @@ export const GridKeyboardNavigationContext = React.createContext();
* {leftElement: React.MutableRefObject, rightElement: React.MutableRefObject})[]} positions - the positions of the navigable items
* @param {*} wrapperRef - a reference for a wrapper element which contains all the referenced elements
*/
export const useGridKeyboardNavigationContext = (positions, wrapperRef) => {
export const useGridKeyboardNavigationContext = (positions, wrapperRef, { disabled } = { disabled: false }) => {
const directionMaps = useMemo(() => getDirectionMaps(positions), [positions]);
const upperContext = useContext(GridKeyboardNavigationContext);

const onWrapperFocus = useCallback(
e => {
const keyboardDirection = e?.detail?.keyboardDirection;
if (!keyboardDirection) {
if (!keyboardDirection || disabled) {
return;
}
const oppositeDirection = getOppositeDirection(keyboardDirection);
const elementToFocus = getOutmostElementInDirection(directionMaps, oppositeDirection);
focusElementWithDirection(elementToFocus, keyboardDirection);
},
[directionMaps]
[directionMaps, disabled]
);
useEventListener({ eventName: "focus", callback: onWrapperFocus, ref: wrapperRef });

const onOutboundNavigation = useCallback(
(elementRef, direction) => {
if (disabled) return;
const maybeNextElement = getNextElementToFocusInDirection(directionMaps[direction], elementRef);
if (maybeNextElement) {
elementRef.current?.blur();
Expand All @@ -44,7 +45,7 @@ export const useGridKeyboardNavigationContext = (positions, wrapperRef) => {
// nothing on that direction - try updating the upper context
upperContext?.onOutboundNavigation(wrapperRef, direction);
},
[directionMaps, upperContext, wrapperRef]
[directionMaps, upperContext, wrapperRef, disabled]
);
return { onOutboundNavigation };
};
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ describe("GridKeyboardNavigationContext", () => {
expect(ref4.current.dispatchEvent).not.toHaveBeenCalled();
});

it("should do nothing if onOutboundNavigation is called when disabled", () => {
const positions = [{ leftElement: ref2, rightElement: ref4 }];
const keyboardDirection = NAV_DIRECTIONS.RIGHT;
const { result } = renderHookForTest(positions, true);

result.current.onOutboundNavigation(ref2, keyboardDirection);

expect(ref2.current.blur).not.toHaveBeenCalled();
expect(ref2.current.dispatchEvent).not.toHaveBeenCalled();
expect(ref4.current.blur).not.toHaveBeenCalled();
expect(ref4.current.dispatchEvent).not.toHaveBeenCalled();
});

it("should call the upper context's onOutboundNavigation if there is no element in that direction", () => {
const positions = [{ leftElement: ref2, rightElement: ref4 }];
const keyboardDirection = NAV_DIRECTIONS.UP;
Expand All @@ -72,9 +85,22 @@ describe("GridKeyboardNavigationContext", () => {
);
});

function renderHookForTest(positions) {
it("should do nothing if the wrapper element is focused, and the hook is disabled", () => {
const positions = [{ leftElement: ref2, rightElement: ref4 }];
const keyboardDirection = NAV_DIRECTIONS.LEFT;
renderHookForTest(positions, true);

focusElementWithDirection(wrapperRef, keyboardDirection);

expect(ref2.current.blur).not.toHaveBeenCalled();
expect(ref2.current.dispatchEvent).not.toHaveBeenCalled();
expect(ref4.current.blur).not.toHaveBeenCalled();
expect(ref4.current.dispatchEvent).not.toHaveBeenCalled();
});

function renderHookForTest(positions, disabled = false) {
wrapperRef = createElementRef();
return renderHook(() => useGridKeyboardNavigationContext(positions, wrapperRef));
return renderHook(() => useGridKeyboardNavigationContext(positions, wrapperRef, { disabled }));
}

function renderHookWithContext(positions, contextValue) {
Expand Down
12 changes: 6 additions & 6 deletions src/components/Menu/Menu/__stories__/menu.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const menuTemplate = args => (

export const menuSizesTemplate = args => [
<DialogContentContainer key="small">
<Menu size={Menu.sizes.SMALL}>
<Menu {...args} size={Menu.sizes.SMALL}>
<MenuTitle caption="Small menu" />
<MenuDivider />
<MenuItem title="Menu item 1" />
Expand All @@ -44,7 +44,7 @@ export const menuSizesTemplate = args => [
</Menu>
</DialogContentContainer>,
<DialogContentContainer key="md">
<Menu size={Menu.sizes.MEDIUM}>
<Menu {...args} size={Menu.sizes.MEDIUM}>
<MenuTitle caption="Medium menu" />
<MenuDivider />
<MenuItem title="Menu item 1" />
Expand All @@ -53,7 +53,7 @@ export const menuSizesTemplate = args => [
</Menu>
</DialogContentContainer>,
<DialogContentContainer key="lg">
<Menu size={Menu.sizes.LARGE}>
<Menu {...args} size={Menu.sizes.LARGE}>
<MenuTitle caption="Large menu" />
<MenuDivider />
<MenuItem title="Menu item 1" />
Expand All @@ -75,7 +75,7 @@ export const menuWithIconsTemplate = args => (

export const menuWithSubMenuTemplate = args => (
<DialogContentContainer>
<Menu>
<Menu {...args}>
<MenuItem title="Menu item" icon={Activity} />
<MenuItem title='Hover me to see the sub menu"' icon={Activity}>
<Menu>
Expand All @@ -91,7 +91,7 @@ export const menuWithSubMenuTemplate = args => (

export const menuWith2DepthSubMenuTemplate = args => (
<DialogContentContainer>
<Menu>
<Menu {...args}>
<MenuItem title="Menu item" icon={Favorite} />
<MenuItem title="Hover me to see the sub menu" icon={Activity}>
<Menu>
Expand All @@ -114,7 +114,7 @@ export const menuWith2DepthSubMenuTemplate = args => (
export const menuWithGridItems = args => (
<div className={classes["menu-long-story-wrapper"]}>
<DialogContentContainer>
<Menu>
<Menu {...args}>
<MenuItem title="Menu item" icon={Favorite} />
<MenuTitle caption="Top level grid item" />
<MenuItem title="Hover me to see the sub menu" icon={Activity}>
Expand Down
17 changes: 13 additions & 4 deletions src/components/Menu/MenuGridItem/MenuGridItem.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const MenuGridItem = forwardRef(
setActiveItemIndex,
setSubMenuIsOpenByIndex,
isUnderSubMenu,
disabled
disabled,
useDocumentEventListeners
},
ref
) => {
Expand All @@ -49,7 +50,13 @@ const MenuGridItem = forwardRef(
);
const { focusWithinProps } = useFocusWithin({ onFocusWithinChange });

useFocusGridItemByActiveStatus({ wrapperRef: componentRef, childRef, activeItemIndex, index });
useFocusGridItemByActiveStatus({
wrapperRef: componentRef,
childRef,
activeItemIndex,
index,
useDocumentEventListeners
});

const keyboardContext = useMenuGridItemNavContext({
wrapperRef: mergedRef,
Expand Down Expand Up @@ -97,7 +104,8 @@ MenuGridItem.propTypes = {
getPreviousSelectableIndex: PropTypes.func,
index: PropTypes.number, // the index of this item
isUnderSubMenu: PropTypes.bool, // true if this item is under a submenu, and not a top-level menu
setSubMenuIsOpenByIndex: PropTypes.func
setSubMenuIsOpenByIndex: PropTypes.func,
useDocumentEventListeners: PropTypes.bool
};

MenuGridItem.defaultProps = {
Expand All @@ -112,7 +120,8 @@ MenuGridItem.defaultProps = {
index: undefined,
setSubMenuIsOpenByIndex: undefined,
getNextSelectableIndex: undefined,
getPreviousSelectableIndex: undefined
getPreviousSelectableIndex: undefined,
useDocumentEventListeners: false
};

export default MenuGridItem;
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ Since <code>MenuGridItem</code> should be used only inside a <code>Menu</code>,
<>Also, the referenced element should have a <code>tabIndex</code> value (probably -1).</>,
<>MenuGridItem will pass the <code>disabled</code> prop to the child. The child should handle this prop and disable interactions.</>,
<>To support a "disabled" mode, the child must have a prop named <code>disabled</code> (it will be automatically detected).</>,
<>NOTE: Due to technical limitations, <code>useDocumentEventListeners</code> is not fully supported.</>,
]} />

<Tip title="Looking for a single button in a menu?">Check the MenuItem or MenuItemButton components</Tip>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,21 @@ describe("useFocusGridItemByActiveStatus", () => {
expect(element.blur).toHaveBeenCalledTimes(1);
});

it("it should do nothing if useDocumentEventListeners and index != activeItemIndex", () => {
renderHook(() =>
useFocusGridItemByActiveStatus({
index: 0,
activeItemIndex: 1,
wrapperRef,
childRef,
useDocumentEventListeners: true
})
);

expect(element.blur).not.toHaveBeenCalled();
expect(GridKeyboardNavigationContextHelperModule.focusElementWithDirection).not.toHaveBeenCalled();
});

it("it should blur the wrapper element if activeItemIndex changes from the given index to a different one", () => {
const props = { index: 0, activeItemIndex: 0, wrapperRef, childRef };
const { rerender } = renderHook(() => useFocusGridItemByActiveStatus(props));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,22 @@ import { useMemo, useEffect } from "react";
import { focusElementWithDirection } from "../../GridKeyboardNavigationContext/helper";
import { useLastNavigationDirection } from "../Menu/hooks/useLastNavigationDirection";

export const useFocusGridItemByActiveStatus = ({ wrapperRef, childRef, index, activeItemIndex }) => {
export const useFocusGridItemByActiveStatus = ({
wrapperRef,
childRef,
index,
activeItemIndex,
useDocumentEventListeners = false
}) => {
const { lastNavigationDirectionRef } = useLastNavigationDirection();
const isActive = useMemo(() => index === activeItemIndex, [activeItemIndex, index]);

useEffect(() => {
if (useDocumentEventListeners) return;
if (isActive) {
focusElementWithDirection(childRef, lastNavigationDirectionRef.current);
} else {
wrapperRef?.current?.blur?.();
}
}, [childRef, isActive, lastNavigationDirectionRef, wrapperRef]);
}, [childRef, isActive, lastNavigationDirectionRef, wrapperRef, useDocumentEventListeners]);
};
6 changes: 4 additions & 2 deletions src/components/Menu/MenuItem/MenuItem.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,11 @@ const MenuItem = forwardRef(
}, [shouldShowSubMenu, childElement, useDocumentEventListeners]);

useEffect(() => {
if (useDocumentEventListeners) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most important part

if (isActive) {
referenceElement?.focus();
}
}, [isActive, referenceElement]);
}, [isActive, referenceElement, useDocumentEventListeners]);

const closeSubMenu = useCallback(
(options = {}) => {
Expand Down Expand Up @@ -262,7 +263,8 @@ const MenuItem = forwardRef(
isVisible: shouldShowSubMenu,
isSubMenu: true,
onClose: closeSubMenu,
ref: childRef
ref: childRef,
useDocumentEventListeners
})}
</DialogContentContainer>
)}
Expand Down