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

MenuItem is not processable with enter key when not Link element #1445

Closed
erm1116 opened this issue Apr 25, 2024 · 3 comments
Closed

MenuItem is not processable with enter key when not Link element #1445

erm1116 opened this issue Apr 25, 2024 · 3 comments

Comments

@erm1116
Copy link
Contributor

erm1116 commented Apr 25, 2024

🐛 Bug report

When using Menu, the Enter key event is not fired for MenuItem.
This causes an issue if you want to do something as a handler for the Click event.

Specifically, the situation is as follows

  • Click event by Enter key is not fired for MenuItem other than a[href].
  • In MenuContent, the keydown event is stopped propagation, and it is not possible to set onKeydown handler independently.

💥 Steps to reproduce

<div {...api.contentProps}>
  <div
    {...api.getItemProps({ value: "value1" })}
    onClick={(e) => {
      alert("div: clicked"); // this is not fired
      e.preventDefault();
    }}
    onKeyDown={(e) => {
      alert("div: enter"); // this is not fired
    }}
  >
    <div
      onKeyDown={(e) => {
        alert("innterDiv: enter"); // this is not fired
      }}
    >
      div
    </div>
  </div>
  <a
    {...api.getItemProps({ value: "value2" })}
    href="#"
    onClick={(e) => {
      alert("link: clicked"); // it is fired because element is matched as a[href]
      e.preventDefault();
    }}
  >
    link(a[href])
  </a>
</div>

In the above example(this is same as codesandbox reproduction code)

  • first div element's click/keydown event handler is not called with enter key
  • second a element's click event handler is called with enter key
CleanShot.2024-04-25.at.14.47.28.mp4

💻 Link to reproduction

CodeSandbox reproduction

🧐 Expected behavior

I expect one of the following

  • Fire the click event even if it is not match with a[href]
  • Can add own keydown event handler

🧭 Possible Solution

🌍 System information

Software Version(s)
Zag Version 0.48.0
Browser Chromium Engine Version 124
Operating System MacOS 14.3.1

📝 Additional information

@segunadebayo
Copy link
Member

This is by design. The Menu uses virtual focus to navigate the items. Meaning the focus remains on the menu content all the time.

When an is selected, the onSelect is the best way to listen for selection. See the docs here
https://zagjs.com/components/react/menu#listening-for-item-selection

Links are a special case because they need to trigger navigation, hence the reason they’re manually clicked.

Use the merge props helper to combine the props of the content with your own custom handler

mergeProps(api.contentProps, { onKeyDown: …})

@erm1116
Copy link
Contributor Author

erm1116 commented Apr 30, 2024

@segunadebayo
I know that by passing onSelect to Menu, I can detect the user's selection.
However, this causes the processing to become redundant when handling each item is selected.
For example, let's assume there is a component like the following. Here, you want to call the API when either edit or delete is selected.

import * as menu from "@zag-js/menu"
import { useMachine, normalizeProps } from "@zag-js/react"
import { useId } from "react"

export function Menu() {
  const [state, send] = useMachine(menu.machine({ id: useId() }))

  const api = menu.connect(state, send, normalizeProps)

  return (
    <div>
      <button {...api.triggerProps}>
        Actions <span {...api.indicatorProps}></span>
      </button>
      <div {...api.positionerProps}>
        <ul {...api.contentProps}>
          <li {...api.getItemProps({ value: "edit" })}>Edit</li>
          <li {...api.getItemProps({ value: "delete" })}>Delete</li>
        </ul>
      </div>
    </div>
  )
}

For this, it is redundant to describe the processing with onSelect.

import * as menu from "@zag-js/menu"
import { useMachine, normalizeProps } from "@zag-js/react"
import { useId } from "react"

export function Menu() {
  const [state, send] = useMachine(menu.machine({ 
    id: useId(),
    onSelect(details) {
      switch(details.value) {
        case "edit":
          callEditAPI()
        case "delete":
          callDeleteAPI()
        // If there are more menu items, this would continue
      }
    }
  }))
  ...
}

So I think we should be able to pass props like onSelectItem to Item.
(Just like the Radix UI MenuItem's onSelect)

export function Menu() {
  ...
  return (
    <div>
      ...
      <div {...api.positionerProps}>
        <ul {...api.contentProps}>
          <li {...api.getItemProps({ value: "edit", onSelectItem: () => callEditAPI() })}>Edit</li>
          <li {...api.getItemProps({ value: "delete", onSelectItem: () => callDeleteAPI() })}>Delete</li>
        </ul>
      </div>
    </div>
  )
}

How do you think this??

@erm1116
Copy link
Contributor Author

erm1116 commented Apr 30, 2024

Perhaps I should have created the issue as a Feature Request instead of a Bug reporting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants