fix(Menu): Don't call popover.update on open, #115

Also updates use-item-list to the latest version,
allowing to simplify some highlighting logic

package.json

@@ -86,7 +86,7 @@
 		"react-merge-refs": "^1.0.0",
 		"react-popper": "^2.2.3",
 		"react-resize-aware": "^3.0.1",
-		"use-item-list": "0.0.1"
+		"use-item-list": "0.1.1"
 	},
 	"commitlint": {
 		"extends": [

src/Menu/Menu.js

@@ -33,7 +33,6 @@ function Menu({id, menuPlacement = 'bottom', menuPositionFixed, children}) {
 		onOpen: () => {
 			menuListRef.current?.focus();
 			itemList.setHighlightedItem(0);
-			popover.update();
 		},
 		onClose: () => {
 			// If focus is on or within the popover when it's closed,
@@ -64,7 +63,7 @@ function Menu({id, menuPlacement = 'bottom', menuPositionFixed, children}) {
 		const isFocusInMenuOrOnButton =
 			buttonRef.current === document.activeElement ||
 			popoverRef.current === document.activeElement ||
-			popoverRef.current.contains(document.activeElement);
+			popoverRef.current?.contains(document.activeElement);
 
 		if (isFocusInMenuOrOnButton) {
 			if (event.keyCode === KEY_CODES.ARROW_UP) {
@@ -79,16 +78,13 @@ function Menu({id, menuPlacement = 'bottom', menuPositionFixed, children}) {
 				event.keyCode === KEY_CODES.SPACE ||
 				event.keyCode === KEY_CODES.ENTER
 			) {
-				if (itemList.highlightedIndex.current !== null) {
+				const item = itemList.getHighlightedItem();
+				if (item) {
 					event.preventDefault();
 					// Trigger a click on the highlighted item
 					// to select it (unless it's disabled)
-					const item =
-						itemList.items.current[
-							itemList.highlightedIndex.current
-						];
-					if (item && !item.ref.current.ariaDisabled) {
-						item.ref.current.click();
+					if (!item.ref.current?.ariaDisabled) {
+						item.ref.current?.click();
 					}
 				}
 			}

src/Menu/MenuItem.js

@@ -20,6 +20,7 @@ function MenuItem({icon, isDisabled, onClick, children}) {
 		ref: itemRef,
 		text: children,
 		value: children,
+		disabled: isDisabled,
 	});
 
 	return (

src/NavMenu/NavMenu.js

@@ -31,7 +31,6 @@ function NavMenu({id, menuPlacement, menuPositionFixed, children}) {
 	});
 
 	const {isOpen, open, close, toggle} = usePopoverState({
-		onOpen: popover.update,
 		onClose: () => {
 			itemList.clearHighlightedItem();
 
@@ -47,11 +46,8 @@ function NavMenu({id, menuPlacement, menuPositionFixed, children}) {
 		},
 	});
 
-	function onHighlight(index) {
-		const highlightedItem = itemList.items.current[index];
-		if (highlightedItem) {
-			highlightedItem.ref.current.focus();
-		}
+	function onHighlight(item) {
+		item?.ref.current?.focus();
 	}
 
 	function onSelect() {
@@ -65,7 +61,7 @@ function NavMenu({id, menuPlacement, menuPositionFixed, children}) {
 		const isFocusInMenuOrOnButton =
 			buttonRef.current === document.activeElement ||
 			popoverRef.current === document.activeElement ||
-			popoverRef.current.contains(document.activeElement);
+			popoverRef.current?.contains(document.activeElement);
 
 		if (isFocusInMenuOrOnButton) {
 			if (event.keyCode === KEY_CODES.ARROW_UP) {
@@ -80,14 +76,11 @@ function NavMenu({id, menuPlacement, menuPositionFixed, children}) {
 				event.keyCode === KEY_CODES.SPACE ||
 				event.keyCode === KEY_CODES.ENTER
 			) {
-				if (itemList.highlightedIndex.current !== null) {
+				const item = itemList.getHighlightedItem();
+				if (item) {
 					event.preventDefault();
-					const item =
-						itemList.items.current[
-							itemList.highlightedIndex.current
-						];
-					if (item) {
-						item.ref.current.click();
+					if (!event.ctrlKey && !event.metaKey) {
+						item.ref.current?.click();
 					}
 					itemList.selectHighlightedItem();
 				}

src/Select/Option.js

@@ -5,7 +5,7 @@ import * as MenuListUI from '../MenuList';
 
 import {SelectContext} from './Select';
 
-function Option({value, icon, children}) {
+function Option({value, icon, isDisabled, children}) {
 	const itemRef = useRef();
 	const {itemList, setSelectedItem} = useContext(SelectContext);
 	const {
@@ -19,7 +19,7 @@ function Option({value, icon, children}) {
 		ref: itemRef,
 		value: value ?? children,
 		text: children,
-		// disabled: isDisabled,
+		disabled: isDisabled,
 	});
 
 	useEffect(() => {
@@ -44,15 +44,15 @@ function Option({value, icon, children}) {
 			id={id}
 			role="option"
 			aria-selected={selected ? 'true' : null}
-			// aria-disabled={isDisabled ? 'true' : null}
+			aria-disabled={isDisabled ? 'true' : null}
 			onClick={select}
 			onMouseEnter={highlight}
 			onMouseLeave={clearHighlightedItem}
 		>
 			<MenuListUI.Link
 				forwardedAs="span"
 				isActive={selected}
-				// isDisabled={isDisabled}
+				isDisabled={isDisabled}
 				isHighlighted={useHighlighted()}
 			>
 				{icon && <MenuListUI.ItemIcon name={icon} />}
@@ -79,7 +79,7 @@ Option.propTypes = {
 	/**
 	 * Disable the option
 	 */
-	// isDisabled: PropTypes.bool,
+	isDisabled: PropTypes.bool,
 };
 
 // @component

src/Select/README.mdx

@@ -66,7 +66,11 @@ There are known issues around this pattern when using VoiceOver with Safari on m
 					onChange={setSelectedValue}
 				>
 					{options.map(item => (
-						<Option key={item} value={item}>
+						<Option
+							key={item}
+							value={item}
+							isDisabled={item === 'Mango'}
+						>
 							{item}
 						</Option>
 					))}

src/Select/Select.js

@@ -38,15 +38,13 @@ function Select({
 		adaptivePositioning: true,
 	});
 
-	const {isOpen, open, close, toggle} = usePopoverState({
-		onOpen: popover.update,
-	});
+	const {isOpen, open, close, toggle} = usePopoverState();
 
 	const itemList = useItemList({
 		id,
 		selected: value,
-		onSelect: value => {
-			onChange(value);
+		onSelect: selectedItem => {
+			onChange(selectedItem.value);
 			close();
 		},
 	});

src/usePopoverState/index.js

@@ -38,10 +38,10 @@ function usePopoverState({openDelay = 0, onOpen, onClose} = {}) {
 	}
 
 	function close() {
-		setOpen(false);
 		if (onClose) {
 			onClose();
 		}
+		setOpen(false);
 		resetTimeout();
 	}