primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.04k stars 526 forks source link

Add support for selecting `ActionMenu` items without closing the menu #2303

Closed iansan5653 closed 1 year ago

iansan5653 commented 1 year ago

ActionMenu automatically closes the menu by default when any menu item is selected.

However, there are some instances when this behavior provides a poor user experience. For example, if you have a list item that you want to allow users to reorder, you might have an ActionList with "Move up" and "Move down" buttons. It would be painstaking to move an item more than one slot because the menu would need to be reopened each time. This is especially true for keyboard users, who might have to navigate through the whole menu just to get back to the "Move" items.

In this case, it would be ideal to have some sort of mechanism to keep the menu open.

Existing solution

ActionMenu does support external control over the open state, so this is theoretically already possible, but it's very difficult to actually implement because the onOpenChange handler does not provide any information about the reason for the close event. Here's an example implementation using a ref, and it's pretty ugly 😬:

const PersistentMenu = () => {
  const [open, setOpen] = useState(false)

  const skipNextClose = useRef(false)
  const onOpenChange = (nextOpen: boolean) => {
    if (nextOpen === open) return

    if (!skipNextClose.current) setOpen(nextOpen)
    skipNextClose.current = false
  }

  const onMove = (direction) => {
    console.log("Moved", direction)
    skipNextClose.current = true
  }

  return (
    <ActionMenu open={open} onOpenChange={onOpenChange}>
      <ActionMenu.Button>Menu</ActionMenu.Button>

      <ActionMenu.Overlay>
        <ActionList>
          <ActionList.Item onSelect={() => onMove("up")}>Move up</ActionList.Item>
          <ActionList.Item onSelect={() => onMove("down")}>Move down</ActionList.Item>
        </ActionList>
      </ActionMenu.Overlay>
    </ActionMenu>
  )
}

Proposed solution

I think the most intuitive answer here is to simply not auto-close the menu if the event is defaultPrevented. That would turn the above example into this:

const PersistentMenu = () => {
  const onMove = (event, direction) => {
    event.preventDefault()
    console.log("Moved", direction)
  }

  return (
    <ActionMenu>
      <ActionMenu.Button>Menu</ActionMenu.Button>

      <ActionMenu.Overlay>
        <ActionList>
          <ActionList.Item onSelect={(event) => onMove(event, "up")}>Move up</ActionList.Item>
          <ActionList.Item onSelect={(event) => onMove(event, "down")}>Move down</ActionList.Item>
        </ActionList>
      </ActionMenu.Overlay>
    </ActionMenu>
  )
}

This mimics the native browser behavior closely.

Alternative solution

One alternative would be to standardize a keyboard shortcut that would "hold" the menu open. For example, the menu might never auto-close if the Shift key is held while selecting an item. This has the advantage of being standardized and expected for the user, and gives the user more control (they can always choose whether to keep the menu open). But it does provide less control to the API consumer.

Maybe both of these could be supported?

lindseywild commented 1 year ago

Hi there! I just wanted to pop in from the Accessibility team (this issue popped up somewhere in Slack and I happened to see it) - an element with role="menu cannot remain open when an action is taken. One of the fundamental rules of a "menu" pattern is for it to close. If this functionality is desired, it shouldn't be using role="menu" but I don't have an alternate suggestion at the time. I helped build the Experimental ActionMenu PVC so let me know if you have any questions, or you could pop them into the #accessibility channel πŸ˜„

iansan5653 commented 1 year ago

Interesting, thanks for the info! I dug into the spec for this and just to add more context to support your point:

Enter:

  • When focus is on a menuitem that has a submenu, opens the submenu and places focus on its first item.
  • Otherwise, activates the item and closes the menu.

Do you think that some text indicating this functionality might be enough to resolve the issue? For example, a description saying something like "Press Shift+Enter to active and keep menu open". This would still close the menu under normal functionality, so users will get the expected behavior when they press Enter as usual. But then it would also provide users with an alternative method that may sometimes ease their workflow.

I persist in asking despite the spec because it feels like there is a balance of conflicting accessibility concerns to resolve here - the original motivation for this feature request came up as we are trying to develop accessible alternatives to drag-and-drop in Projects: reordering tabs, rows, columns, etc is impossible at the moment for keyboard-only users. Adding "move left/right/up/down" options to the corresponding ActionMenus for these items makes reordering technically accessible, but the experience is still painful:

  1. Open the menu
  2. Use arrow keys to navigate to the move item
  3. Press Enter
  4. Repeat 1-3 until the item is in the right spot

It is an unsatisfying solution to make the action accessible but still provide a suboptimal experience for some users. Providing them the ability to keep the menu open while rearranging would make this a lot easier:

  1. Open the menu
  2. User arrow keys to navigate to the move item
  3. Press Enter until the item is in the right spot
  4. Close the menu
lindseywild commented 1 year ago

@iansan5653 thanks for digging around for more context πŸ˜„

Ah, I see what you're saying. Thinking through it quickly, I don't think adding text explaining the behavior would work, because how would mouse users trigger the alternate action? I know they could just drag & drop the rows/columns themselves, but if they wanted to use the menu in the fashion you are suggesting, they wouldn't be able to. Also, the core problem is that triggering a menu item should close the menu, so it sounds like you may need a different solution. I appreciate the creativity you're coming up with, though!

I am not a drag & drop expert by any means, and it's a very tricky pattern to get right in an accessible way! I'd encourage you to sign up for an Office Hours slot next week or post your question in the #accessibility channel (is async is required) for some deeper digging into it.

siddharthkp commented 1 year ago

Hi!

+1 to what @lindseywild said

The main issue here is that ActionMenu (role=menu) can't accommodate interactive elements, we might need to drop an abstraction layer down and build it with (Dialog or AnchoredOverlay, not role=menu).

This is an interesting set! What does an accessible list with selection and drag & drop look like inside a dialog. This might help us find the right composable primitives for drag & drop (we don't have any guidance yet 😟)

iansan5653 commented 1 year ago

I guess there are some alternatives if we wanted to avoid the menu altogether, but they present their own challenges:

  1. Provide always-visible buttons for moving things. Unfortunately this is not really a realistic solution from a design perspective as we can't put two new buttons on every tab, column, and row in the application.
  2. Provide keyboard shortcuts that will rearrange the currently focused tab, column, or row. This is more realistic and the shortcuts could be discoverable through the menu. The challenge here is coming up with keyboard shortcuts that are intuitive and don't conflict with system shortcuts. When focus is on a table cell, there would become many different nested contexts reorderable via keyboard shortcut:
    • Operating system virtual desktops
    • Operating system windows
    • Browser tabs
    • Project view tabs
    • Table rows
      • Note: table rows are inside groups, but the groups are not reorderable. We do allow dragging a row from group to group though so we'd likely want a shortcut for that as well.
    • Table columns
iansan5653 commented 1 year ago

Going to close this based on the discussion here and with the accessibility team. We will find an alternate solution!