react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Add support for overriding default keyboard actions

Open zzzze opened this issue 1 year ago • 5 comments

The keyboard navigation implementation in react-aria is excellently designed, but it lacks customization options. This limitation makes it challenging for me to utilize the existing logic of react-aria in my keyboard-first application. Moreover, the keyboard logic is embedded deeply within the hooks, making alterations difficult.

In my pull request, I have introduced a KeyboardEventHandler that allows users to choose whether to override the default keyboard operation logic. It provides two levels of customization:

  1. Adding only navigation triggering keys while still utilizing the default navigation logic, such as returning the built-in KeyboardAction: 'nav-down'.
  2. Full customization of the keyboard logic, for example, by returning an empty string ''.

This second level of customization has proven to be incredibly useful in scenarios where I use react-window for virtual scrolling. In such cases, the pagination logic, among other things, deviates from the default, rendering it unusable without extensive customization.

zzzze avatar Feb 06 '24 10:02 zzzze

@zzzze This is a interesting use case, I had originally thought you could create a new keyboard delegate but you want to customize the keyboard actions themselves. Personally, I think this could be fine if the actions were supplementary to the original actions AND didn't heavily deviate/conflict with the keyboard actions mentioned by the APG. Could you expand on your customizations that you mentioned in the latter part of your PR description? I wasn't sure if the stories you added were illustrative of what you are trying to accomplish in your own app or not.

LFDanLu avatar Feb 16 '24 22:02 LFDanLu

@LFDanLu You are correct; this PR indeed supplements the original operations without affecting the original behavior. This PR stems from my own needs as I am building a keyboard-first application, and I have a personal preference for Vim-style navigation. Therefore, I wanted to add additional navigation operations besides the arrow keys. For example, Ctrl+J would act as ArrowDown, and Ctrl+K as ArrowUp. As for the "custom operation," I now believe it should return "noop" instead of an empty string. The action "noop" is not in the original list of actions, so it indicates that the default keyboard operation should not execute. The reason for introducing "noop" is because I am using react-window for virtual scrolling, and since the ScrollToItem operation is internal logic of react-window, the original PageUp operation does not work.

Furthermore, returning "noop" can actually be used to cancel the default keyboard operation. This may conflict with APG, but I believe developers should be aware of what they are doing, so it's up to the developers to decide whether they want to do this.

zzzze avatar Feb 18 '24 12:02 zzzze

@zzzze I see, thank you for the additional details. I'll have to discuss this further with the team, especially the last bit regarding allowing the default keyboard operations to be overridden which I feel will be the only part of contention.

LFDanLu avatar Feb 20 '24 18:02 LFDanLu

I have a different use case but could potentially benefit from this change as well. I am currently trying to integrate the GridList component with the ariakit's composite component. Essentially, GridList elements would function as results from an input field above (similar to a combobox). The current keyboard behavior being so embedded in the hooks does not allow me to easily connect to an external virtual focus manager. I'd love to be able to define custom onKeyUp behavior that plays nice with third party libraries.

TylerGauntlett avatar Apr 01 '24 15:04 TylerGauntlett

@zzzze Hey, sorry about the delay but the team talked about this today. We aren't opposed to the overall overriding and extending of the keyboard actions though we'll want to be quite careful in how we document this since it is an advanced use case.

We did however want to explore a different api than what you've added here, that being adding/modifying the keyboard delegates so that they can accept a similar kind of keyboard handler override that you've done in useSelectableCollection and then refactoring useSelectableCollection so it would call that delegate keyboard handler and pass whatever key it received from it to navigateKey.

It'd involve creating a base class for the KeyboardDelegates along with some abstract functions for extending classes to implement.

There will need to be some experimentation done there since it might cause some issues in Virtualizer/other places that have also use keyboardDelegate, but this approach would ideally allow users to create their own keyboard delegates to override the keyboard actions and/or extend from the base level keyboard delegates if they only wanted to add/override a single action.

If you are interested, definitely feel free to try this out!

LFDanLu avatar Apr 11 '24 22:04 LFDanLu

There's a number of conflicts now, I'm going to close this and we can continue the discussion in the Issue I've opened for this https://github.com/adobe/react-spectrum/issues/6839 and in other PRs, or we can reopen this one if you're still interested. Thanks for starting this!

snowystinger avatar Aug 07 '24 06:08 snowystinger