Menu items do not generate unique ids

[avallee]

🐛 Bug report

Menu item ids are not scoped to their menu. Instead they are copied raw from the menu item's value property.

💥 Steps to reproduce

1. I used the example from https://zagjs.com/components/react/menu

   export function Menu() {
     const service = useMachine(menu.machine, { id: useId() })
   
     const api = menu.connect(service, normalizeProps)
   
     return (
       <div>
         <button {...api.getTriggerProps()}>
           Actions <span {...api.getIndicatorProps()}>▾</span>
         </button>
         <div {...api.getPositionerProps()}>
           <ul {...api.getContentProps()}>
             <li {...api.getItemProps({ value: "edit" })}>Edit</li>
             <li {...api.getItemProps({ value: "duplicate" })}>Duplicate</li>
             <li {...api.getItemProps({ value: "delete" })}>Delete</li>
             <li {...api.getItemProps({ value: "export" })}>Export...</li>
           </ul>
         </div>
       </div>
     )
   }
   
   export function Example() {
     return (<>
       <Menu />
       <Menu />
     </>)
   }
   

2. Inspect the resulting dom. Note how document.querySelectorAll('[id="edit"]') returns two Elements in this example while document.getElementById('edit') only ever returns the first of these elements.

💻 Link to reproduction

CodeSandbox reproduction: https://codesandbox.io/p/sandbox/busy-dust-fwhc9c

🧐 Expected behavior

The id prop on all elements of the page are meant to be unique. However, using identical values for a menu item in different menus results in this requirement being broken.

🧭 Possible Solution

Use scoping for the ids like most other parts of the menu are.

🌍 System information

Software	Version(s)
Zag Version	0.82.2 (via @ark-ui/[email protected])
Browser	Chrome
Operating System	Microsoft Windows NT 10.0.19045.0

📝 Additional information

[avallee]

The culprit id is being set here, lines 63 and 67:

zag/packages/machines/menu/src/menu.connect.ts

Lines 62 to 70 in 9e3d225

	function getItemProps(props: ItemProps) {
	const { value: id, closeOnSelect, valueText } = props
	const itemState = getItemState(props)
	return normalize.element({
	...parts.item.attrs,
	id,
	role: "menuitem",
	"aria-disabled": itemState.disabled,
	"data-disabled": dataAttr(itemState.disabled),

[segunadebayo]

This falls under the "Won't fix" category for now due to the code's design limitations.

We made it this way to allow for:

• composing menu items with tooltip in an intuitive way
• composing nested menu

As a temp. fix, I recommend aliasing item values with a unique id

export function Menu() {
  const uid = useId()
  const service = useMachine(menu.machine, { id: uid})

  const api = menu.connect(service, normalizeProps)

  return (
    <div>
      <button {...api.getTriggerProps()}>
        Actions <span {...api.getIndicatorProps()}>▾</span>
      </button>
      <div {...api.getPositionerProps()}>
        <ul {...api.getContentProps()}>
          <li {...api.getItemProps({ value: `${uid}/edit` })}>Edit</li>
        </ul>
      </div>
    </div>
  )
}

We should mention this in the docs as a caveat. Redesigning this will lead to a breaking change upstream that I'm unprepared for. Thanks for understanding.

[segunadebayo]

Changed my mind. I got a PR to fix this.

Thanks for opening this @avallee

[avallee]

Thanks for the fix!