feat(Select/Dropdown/MenuContainer): arrow key handling to focus items

[adamviktora]

Closes #11131

Potential problem: if consumers pass a component prop to MenuItem other than button / input / a, it won't focus the item. They can always override the onArrowUpDownKeyDown themselves, but I am wondering whether we should provide some prop to specify, what is the html element they want to focus.

[patternfly-build]

Preview: https://patternfly-react-pr-11132.surge.sh

A11y report: https://patternfly-react-pr-11132-a11y.surge.sh

[thatblindgeye]

Potential problem: if consumers pass a component prop to MenuItem other than button / input / a, it won't focus the item. They can always override the onArrowUpDownKeyDown themselves, but I am wondering whether we should provide some prop to specify, what is the html element they want to focus.

Yeah, this is something that was in the back of my mind. Allowing a consumer to customize the query string for elements that can be possibly focused (either auto on open, or via this arrow key handler). Adding a prop for that might require tweaking either the auto-focus logic or this, though, since the default query strings are just slightly different (querying a string that includes "li" for each query vs querying within an already captured <li> element).

I think the question then is if it makes sense to have both a callback to customize onKeydown handling for the arrow keys, and a prop to customize the query string. Via a query string a consumer could try to focus anything that's inside the menu (but not in the actual menu list, e.g. a menu with search filter).

It probably doesn't hurt to have both; maybe the a prop for a custom query string could be a more "basic" customization, and this callback prop could be a more "advanced" customization. cc @tlabaj wdyt?

[tlabaj]

Potential problem: if consumers pass a component prop to MenuItem other than button / input / a, it won't focus the item. They can always override the onArrowUpDownKeyDown themselves, but I am wondering whether we should provide some prop to specify, what is the html element they want to focus.

Yeah, this is something that was in the back of my mind. Allowing a consumer to customize the query string for elements that can be possibly focused (either auto on open, or via this arrow key handler). Adding a prop for that might require tweaking either the auto-focus logic or this, though, since the default query strings are just slightly different (querying a string that includes "li" for each query vs querying within an already captured <li> element).

I think the question then is if it makes sense to have both a callback to customize onKeydown handling for the arrow keys, and a prop to customize the query string. Via a query string a consumer could try to focus anything that's inside the menu (but not in the actual menu list, e.g. a menu with search filter).

It probably doesn't hurt to have both; maybe the a prop for a custom query string could be a more "basic" customization, and this callback prop could be a more "advanced" customization. cc @tlabaj wdyt?

@thatblindgeye Can the query string be an argument of the callback with the default being li? Or am I not following correctly?

[thatblindgeye]

@tlabaj so with this addition, we would have 2 instances where we're attempting to get an element to focus via a querySelector string: once for the automatic focusing when shouldFocusFirstItemOnOpen is true, and the new addition of when an Arrow key is pressed as part of this new callback prop. I was thinking that a possible new prop to customize a single query string could be used in both instances; adding an arg to the callback would only work for that callback, while the autofocus logic would still be hardcoded to the query string we dictate internally.

If we were to implement a query string prop, at that point I was just wondering if this custom callback prop would be as useful, or if we should only run our default logic for when Up/Down arrow is pressed without the ability to customize. Within Select and Dropdown, the query string should be capturing anything within .pf-v6-c-menu (so that could be actual MenuItems, or a non-menu item like a search/filter input). If a consumer wants to build their own thing using Menu components themselves, as long as they're using MenuContainer (that has the same focus logic as Select and dropdown), the case should be the same.

Like I said it doesn't really hurt to have both. A query string prop is a bit simpler of a customization, whereas this callback prop could provide even more fine tuned control. I'm thinking more whether 2 props would be necessary immediately or if we should try just adding 1 and seeing what consumers think.

[kmcfaul]

The query selector is only looking at the first and last li currently, so this behavior breaks if the first/last option is disabled. To skip disabled options, a single querySelectorAll('li button:not(:disabled),input:not(:disabled),a:not([aria-disabled="true"])') call instead of looking for the li separately should work, and then we'd grab the first/last of that resulting array.

To your question at the top, I think it's fine to not have a direct prop to customize the focusable element. The user can override the toggle keydown behavior, and has access to the menu ref to do their own query. I'd prefer to try to keep the API streamlined, and we can always add a prop for it later down the line if we get requests for it.

[kmcfaul]

I think this prop can be generalized to onToggleKeydown since it could be used to add any key behavior.

The description could also be updated to Callback to override the toggle keydown behavior. By default, when the toggle has focus and the menu is open, pressing the up/down arrow keys will focus a valid non-disabled menu item - the first item for the down arrow key and last item for the up arrow key. The double when is slightly confusing to me.

Same goes for MenuContainer and Select.

[kmcfaul]

Is there any chance we would want this to be a wider covering variant prop? Ideally we don't want to have this prop and later find we need to also add isCheckbox, isMultiTypeahead, etc. If we go with variant, the prop description can indicate that the prop affects default keyboard behavior based on its value.

What does everyone think?

[kmcfaul]

@tlabaj @thatblindgeye

[thatblindgeye]

That sounds good to me

[tlabaj]

@thatblindgeye in response to the prop conversation above. I understand now and the prop is fine. Like Katie said, we can add an argument to callback at a later time if it is requested.

[adamviktora]

I had to keep the querySelector looking for li, the reason for it were the Menus with actions which would focus the action button instead of the main menu item button when focusing the last item.

[kmcfaul]

Updated behavior lgtm. Only open question left is if we want to leave the isTypeahead prop as is or rename it to variant. I don't think that's blocking though and we can update it later if necessary in the next breaking change release.

[thatblindgeye]

Looks good. Tested with the Talon program for voice input and saying "Arrow Up/Down" focuses things as expected nicely.

We should ideally have a followup where we show a more custom callback being used, possibly justupdating the custom inline filter menu example.

[tlabaj]

lgtm another than changing the isTypeahead prop to `variant. Like Katie said, it allows for further expansion later on. Not a blocker for me though.

[tlabaj]

@thatblindgeye @kmcfaul this would be good to get into v5 correct? It could help with adoption...

[thatblindgeye]

@tlabaj yeah most likely since we reversed the default value of that autofocus prop there, too.

[tlabaj]

@adamviktora can you open a PR for the same fix in v5 please.

[patternfly-build]

Your changes have been released in:

• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]

Thanks for your contribution! 🎉