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

Make disabling handleKeyDown on MenuList possible #17775

Closed
1 task done
afuller611-animas opened this issue Oct 7, 2019 · 16 comments · Fixed by #17037
Closed
1 task done

Make disabling handleKeyDown on MenuList possible #17775

afuller611-animas opened this issue Oct 7, 2019 · 16 comments · Fixed by #17037
Assignees
Labels
component: menu This is the name of the generic UI component, not the React module!

Comments

@afuller611-animas
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

The MenuList component has a handleKeyDown function that is passed to its child
List component as the onKeyDown prop.

MenuList component

This causes a problem because the handleKeyDown function will focus an element in the list when the first letter of an element is pressed. I understand the motivation behind this and it's great for accessibility, but there should be a way to turn this off so a key press can perform other actions.

My codesanbox below will probably make more sense as to why this causes issues.

Examples 🌈

We utilize Multi-Selects that incorporate a nested TextField to filter the MenuItem children.

image

Sandbox Example

Notice that when you type anything into the TextField shown in the screenshot, it won't change the state of searchInput but will focus the element of the letter you typed. If you paste a string into the TextField however (right-click and paste because ctrl+v will just focus vampire) it will filter based on the string input.

Motivation 🔦

The sandbox above is what I'm trying to get working. We upgraded to v4 of MUI recently and this wasn't an issue before. I think having a prop for MenuList that lets you either disable the character focus (the arrow up and arrow down is still nice) or disable the handleKeyDown function altogether would work a charm.

As a side note for anyone wondering, you can access the MenuList props via a TextField as follows:

<TextField select SelectProps={{ MenuProps: { MenuListProps: { } } }} />

@oliviertassinari
Copy link
Member

@afuller611-animas Would you be willing to use #17037 instead of this custom select menu component?

@afuller611-animas
Copy link
Author

@oliviertassinari Very cool demos there. Will the multiple values feature have an option to not use chips, but to customize what is displayed similar to the "renderValue" prop with Selects?

@oliviertassinari
Copy link
Member

@afuller611-animas Yes, at the very least, with the hook API.

@afuller611-animas
Copy link
Author

@oliviertassinari Awesome that sound great. We'll make a workaround for the issue we're seeing here and then keep our eyes out for how that new component will work when it's released. Thanks so much for the quick response.

@eps1lon
Copy link
Member

eps1lon commented Oct 9, 2019

While I don't necessarily see how a textbox should be part of a menu you could make an argument that keyboard navigation should be skipped if event.defaultPrevented is true. You could then call event.preventDefault() in your custom event handler.

@afuller611-animas
Copy link
Author

@eps1lon

Yes that would be very nice to add in. The problem is the onKeyDown prop that you pass to MenuListProps doesn't remove the functionality of the handleKeyDown() function that is in MenuList -- rather, it appends whatever function you pass to onKeyDown to handleKeyDown()

@petrdsvoboda
Copy link
Contributor

I use menu items to open dialogs with inputs for quick actions. But because the Dialog is under Menu in the React tree, key events are captured and used to control the menu. event.defaultPrevented would be an easy fix. I could make a PR if you are okay with adding it.

@eps1lon
Copy link
Member

eps1lon commented Oct 15, 2019

@petrdsvoboda While I would appreciate your contribution I want to caution you against this approach for your particular use case. The issue you are encountering is that events bubble through portals. I would recommend calling stopPropagation on keydown events at the Dialog. If you call preventDefault only do so on some keys. preventing the default on the "Escape" key would also prevent closing the Dialog.

@petrdsvoboda
Copy link
Contributor

@eps1lon Yes, you are right. I had issues with stopping propagation on my Dialog wrapper, but your answer cleared up things for me and that would be the best solution. Thank you.

@iDVB
Copy link

iDVB commented May 2, 2020

@oliviertassinari @afuller611-animas Did a workaround end up being created for this?
We have the same issue as @afuller611-animas's use case is also similar and is to replicate GitHubs branch creator menu.
image

There is a menu with items and an input at the top. When you type into the input the items are replaced with a single item that says Create branch: blah from master
image

As it stands now, I don't know what the work around is? I took a look at the Autocomplete component and I think that is more suited to @afuller611-animas's needs but I don't think it would work for our case.

@oliviertassinari
Copy link
Member

@iDVB The closest we have to this use case is: https://material-ui.com/components/autocomplete/#customized-autocomplete. You should be able to make it match your use case.

@iDVB
Copy link

iDVB commented May 3, 2020

Thanks @oliviertassinari , I looked at that a bit and could not see how I could use it. Seems like the searching of the labels is pretty tightly built into that component and our scenario does not need that.

I managed to get it working but I had to fork both the Menu and MenuList components.... just so I could add an extra prop to the MenuList that I called searchFocus.
image

I then just made it default to false for my needs. However, I assume this could be a prop passed down with Menu as well.

Works but I MUCH rather not have to fork:

@ethankore
Copy link

ethankore commented Jan 4, 2021

FWIW, I was able to mitigate that by using the Popover component instead of the Menu/MenuList components.

@ItayTur
Copy link

ItayTur commented Aug 4, 2021

Found a workaround here

basiclly wrapping the inner input with <MenuItem/> and call event.stopPropagation() on onKeyDown prop.

const stopImmediatePropagation = (e: any) => {
    e.stopPropagation();
    e.preventDefault();
  };

const search = <MUIMenuItem
      value=""
      onClickCapture={stopImmediatePropagation}
      onKeyDown={(event: any) => {
        event.stopPropagation();
      }}
    >
      <Input
        ref={searchRef}
        key="search"
        endAdornment={<SearchIcon />}
        classes={searchClasses}
        placeholder={searchPlaceholder}
        onChange={onSearch}
        value={search}
        autoFocus
      />
    </MUIMenuItem>


<Select>
     {search}
     {menuItems}
</Select>

@Alxmerino
Copy link

The workaround from @ItayTur worked for me! thanks!

@oliviertassinari oliviertassinari added the component: menu This is the name of the generic UI component, not the React module! label Dec 20, 2022
@lbellman
Copy link

@ItayTur thanks for your solution, it worked perfectly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants