[DropdownMenu] DropdownMenu.CheckboxItem asChild keyboard issue

[bengeorge]

Bug report

Current Behavior

Given a CheckboxItem that has asChild={true} and is the first item in the dropdown.

When you use enter to activate the trigger:

• Menu will not open
• onCheckChange is called on the first menu item
• If you add onSelect={(e) => e.preventDefault()} the menu does stay open

When you use space to activate the trigger:

• Menu will open
• onCheckChange is called on the first menu item

Expected behavior

Menu should open and first item should not have onCheckChange called.

Reproducible example

CodeSandbox Template

Steps to reproduce in this codesandbox:

• Focus the trigger
• enter, esc, enter, esc

You should see the menu open and close and the first item toggles on / off as you do this.

Suggested solution

Additional context

Your environment

Software	Name(s)	Version
Radix Package(s)	dropdown-menu	0.1.6
React	n/a
Browser	chrome	99.0.4844.82
Assistive tech
Node	n/a
npm/yarn
Operating System	windows

[bengeorge]

If you can think of a kludge to get this working with 0.1.6 I would be really grateful.

[jjenzz]

this is because focus is moved during keydown so the enter keyup is firing on your button which triggers the button click.

it is likely an issue for any item type, for example, the menu would not open at all with this set up:

<DropdownMenu.Content>
  <DropdownMenu.Item asChild>
    <button>Item</button>
  </DropdownMenu.Item>
</DropdownMenu.Content>

a simple "kludge" for now would be to use a div instead of a button (like the item we provide) because the component converts it to a role="menuitem", so it will be losing button semantics anyway.

[bengeorge]

Was this issue closed because it is fixed ? I can't see a related PR so just wondering.

[benoitgrelard]

As the comment above suggests, there is no fix needed from the library's standpoint.
When using asChild you should slot it an element of the same type the library normally uses if you want to avoid unforeseen issues.

[bengeorge]

When using asChild you should slot it an element of the same type the library normally uses if you want to avoid unforeseen issues.

Thanks for that explanation, I understand now.

Is there a nice way to know which element we should be using in conjunction with a particular asChild usage ?

[benoitgrelard]

The best way to know right now I suppose is to see what's rendered by default, or look at the code.
Not really obvious, we have actually been thinking of adding this as a reference in the docs, whilst explaining that each part can receive all DOM attributes for a given element.

Something like:

This part accepts all the props supported by div

[jjenzz]

When using asChild you should slot it an element of the same type the library normally uses

i thought asChild was to allow consumers to change the underlying node if they need, especially for DropdownMenu where some like to change from div to an a.

i know historically the decision was that if the underlying node is changed then it is up to the consumer to ensure correctly functionality/a11y but this seems quite a big "break" in functionality that isn't easily solved consumer-side (assuming they need to keep button) 😬

[benoitgrelard]

…if you want to avoid unforeseen issues.

I think there are "some" valid cases of changing the underlying elements, but those are quite limited I would say. In the given case, there are no good reason to change to a button that I can think off, given that we already handle all of the accessibility.

[jjenzz]

I think there are "some" valid cases of changing the underlying elements

yep, there are valid cases where unforeseen issues are not to be expected. it's hard for consumers to know which those valid/invalid cases are atm and isn't always possible to change the underlying node for a component they're switching in (third-party libs).

i've been thinking about all of this a lot lately and wondering if asChild shouldn't change the underlying nodeType. if they want a different nodeType, use a more explicit nodeType prop that is documented as "use with severe caution", or an explicit DropdownMenu.AnchorItem part where a node change is valid.

it feels like most of the time, we're just trying to compose functionality/styles and the nodeType is a big part of a11y/semantics that generally shouldn't be toyed with. my first instinct tho is that preventing the underlying nodeType from changing isn't possible/easy, but might be something to think about... 💭

[benoitgrelard]

it's hard for consumers to know which those valid/invalid cases are atm

Yeah…

i've been thinking about all of this a lot lately and wondering if asChild shouldn't change the underlying nodeType. if they want a different nodeType, use a more explicit nodeType prop

I'm not sure that's even possible outside of passing in a React DOM element, how would that work for custom components that wrap an element (things like Link from react-router, etc)

it feels like most of the time, we're just trying to compose functionality/styles and the nodeType is a big part of a11y/semantics that generally shouldn't be toyed with. my first instinct tho is that preventing the underlying nodeType from changing isn't possible/easy, but might be something to think about... 💭

We had talked about using refs to output warnings at runtime in dev mode when the nodeType is changed: #212
Still kinda feels like a good idea to me.