feat: Add support for grid edit mode w/ nested interactive widgets

[nwidynski]

Closes #6037 #5005 #2328 #4957 #5637 #5179 #7121 #1214 #4674 #5003

This PR adds edit mode to @react-aria/grid and RAC <Table /> in addition to generalized support for nested interactive (collection) components inside grid cells or items.

It also introduces a new useGroup() and useGroupState() hook meant to propagate validation and input state to nested interactive components, allowing for more convenience when controlling large sections of an application.

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[nwidynski]

This doesn't touch useGridCell() for now, but should be good to go for a first round of review 👍

[nwidynski]

As @devongovett suggested, i implemented this for grid cells and added support for edit mode into RAC table.
Test suite is passing, but no new tests added just for now - i added a couple examples to showcase behavior.

Sorry for the large diff - most of it is boilerplate from the new package though 😓

[LFDanLu]

Partial review, will see about wrapping it up soon. Looks promising so far!

[LFDanLu]

Will see about getting this into our upcoming test session on Monday as well

[nwidynski]

Pushed my last updates, completing what I had in mind for this PR 👍

As mentioned briefly, biggest change from the last review is the introduction of a GridManager which extends the SelectionManager by the grid editing and navigation state. This allows us to access the editing state to prevent features like useTypeSelect() from interrupting our editing. I also made sure selection doesn't intervene with interactions on editable cells.

Another key insight was that, after the edit mode changes, we could remove isKeyboardNavigationDisabled by just switching the navigation mode to tab whenever we need to disable arrow key navigation. This way we can ensure a great UX, since we only cater towards these two interaction models. I don't really see a need for disabling the grid navigation completely anymore, so i deprecated the flag.

Lastly i extended support for the disabled state to rows and columns and adjusted the GridKeyboardDelegate accordingly. There are a bunch of other small features and bug fixes along the way, but thats pretty much it.

I'm looking forward to your test session feedback to finalize and get started on tests for this beast 😅 !

[LFDanLu]

@nwidynski Thanks for all the work you've done here! The team took a closer look today and are a bit concerned about the extent of the changes made. The overall behavior seems promising but we would like to perhaps pare it down into smaller pieces so it is easier to incrementally update the behavior/understand the full ramifications of the impact of these changes.

With that in mind, we took some time to implement what we thought was the base amount of changes outlined in #7215 (comment) to support textfields in GridList, you can see those in (WIP) Update GridList so it supports textfields with this simple story. At the moment, that implementation doesn't have "memory" of the last focused element in the GridList and instead opts to focus the last child element of your last focused cell when shift tabbing back into the collection. Any thoughts? Some of the issues you noted in #7215 (comment) still apply but we are debating if saving the "memory" of the last focused element should still apply for non Tab cases or not and thus if it is important to save the "memory" of the last focused element at all.

[nwidynski]

@LFDanLu That's unfortunate, but I understand. I originally set out to complete the full spec outlined in #2328, but increasingly became aware of how much change this required while implementing 😬.

As to your draft in #7360 , I figure this is addressing the proposed solution in #7125 (comment)? If so, this does not really capture the scope of our original issue #7121, where we proposed preventing all navigation when in tab mode, not limited to having an input element as the activeElement. Our use-case was not related to editing in the first place, but rather nested interactive widgets, which would remain nearly unchanged after your PR.

As for my thoughts, I feel like the proposed solution in #7125 (comment) is heading down a direction where we shift responsibility for nested interactive widgets down to children or possibly even the consumer to figure out himself, which I wouldn't agree with. In the current state I do not see much of a point in #7360, especially when type select remains active.

In regards to the "memory", I think maintaining a stable tab sequence forwards and backwards should be the case for both arrow and tab navigation behavior, but I as well focused only on tab in this PR. Ideally, I would aim for keyboardNavigationBehavior to only control navigation inside cells, while leaving cell movement to arrow navigation. The arrow navigation mode should pretty much be a simulated tab movement action, hence why i also proposed changes to make getFocusableTreeWalker(ref.current, {tabbable: true}).

With current knowledge, I would also advise to follow the notion in #7125 (comment) to leave useSelectableCollection() mostly alone and handle this inside items/cells. Your current approach does not work when re-entering the collection from the bottom with a relatedTarget === null, which you can test by removing the adjacent <input /> elements. This can be the case in an iframe or in mobile browsers when the address bar is positioned at the bottom, e.g. Safari on iOS. The only incentive of solving this issue inside useSelectableCollection would be a potentially more stable setFocusedKey() for programmatic edit mode.

Regardless, I would definitely still advocate for trying to bring full edit mode capabilities to RAC, because I know a ton of people are awaiting this, just as we are. I think it's the libraries responsibility to handle such a common use-case and I definitely think it's possible to offer full edit mode support out of the box with the current architecture. I can only offer to get this done as digestable as possible through chunking this PR into the following:

• Implement multitab for useGridListItem() as outlined in feat: implement multi-tab-stop #7215
• Implement multitab for useGridCell() & useTableCell() as outlined in feat: implement multi-tab-stop #7215
• Deprecate isKeyboardNavigationDisabled flag in favor of setKeyboardNavigationBehavior('tab')
• Add Document key prefix to support nested collection widgets
• Add useGridEditState(), GridManager and useGridEditAnnouncement()
• Add support for isDisabled state to useGridCell() & useTableColumnHeader() and adjust GridKeyboardDelegate
• Add support for isEditable & isReadOnly state to useGridCell() & useTableColumnHeader()
• Add support for W3C edit mode interactions to useGridCell() & useTableCell()
• Add useGroup(), useGroupState() and group state propagation to RAC <Group />, <Button/> & <Input />
• Add grid edit mode support to RAC <Table />, <Column /> & <Cell />

My intention is only to get this shipped as quickly as possible, so I'm willing to jump through a couple hoops to get there. I'm afraid this will take months if someone from the community doesn't pick this up, since you guys are understandably busy on S2. Let me know how you guys would like to proceed with this 👍

[LFDanLu]

@nwidynski Gotcha, thank you for the detailed response! As you mentioned, the team was moving towards having the children of the cell be responsible for stopping propagation but I can revisit this assumption to them in our meeting tomorrow and see if we may want to pivot there. As for "memory", we have prior precedence doing such tracking in useToolbar, but there have been differing opinions as to what the behavior should be when tabbing back into the collection for specific cases like "blur happened due to change in browser window/mouse click/iframe" which I'll see about getting a consensus from the team about what behavior we want to go with.

As for edit mode as a whole, historically when we tried to implement it ourselves I recall that @devongovett still had concerns as to how it should work on mobile touch devices, where perhaps it wouldn't be clear enough to the end user when/how edit mode would be triggered and whether or not there would be confusion around that. I don't quite remember all the details but hopefully I'll be able to comeback with a more detailed answer after tomorrow's meeting.

Again, thank you so much for all the work done here, it is greatly appreciated! It quite evident that you've done a lot of good work providing a solution that also matches the ARIA spec, it is just that we need to be extra careful since these are quite hefty changes and as a library we need to make sure that we are ready to accept and maintain this behavior for the foreseeable future.

[devongovett]

this does not really capture the scope of our original issue #7121, where we proposed preventing all navigation when in tab mode, not limited to having an input element as the activeElement

Could you explain why you want that? Obviously there are issues with using the arrow keys on certain components such as inputs conflicting with grid navigation, which Daniel's PR has solved, but I don't see any particular issue with still allowing arrow key navigation when focused on an element that does not use the arrow keys. It seems nice to me that this approach allows us to support arbitrary elements such as inputs inside grid cells without needing edit mode at all, while not impacting the behavior of any existing applications.

heading down a direction where we shift responsibility for nested interactive widgets down to children or possibly even the consumer to figure out himself

This is already the case in many ways throughout the library. For example, if you have a button inside a table row, pressing Enter while focused on the button triggers the press event only on that button and does not trigger row selection. That is because the button stops event propagation so the enter key event never reaches the row. The overall principle here is that once the deepest element in the DOM tree handles an event, it should stop propagation to prevent parent elements from also handling it. Without this, we would lose the ability to compose arbitrary components together – the parent would need to know about all of the possible children in order to decide whether to handle an event or not. You can see an example of that in Daniel's PR which does this for native input elements (as a nicety), but we couldn't possibly know all of the other arbitrary components that might handle arrow key navigation without leaving some responsibility to them to stop propagation.

[devongovett]

btw, we definitely want to support a full edit mode in the future in addition to what Daniel proposed, we're just looking to break up the work a bit and hopefully solve some immediate issues that might not require it first. As Daniel mentioned, many of the interactions for edit mode (especially on touch devices) have not yet been defined.

[nwidynski]

Thanks for getting back on this so quickly 👍

TLDR;

[...] but I don't see any particular issue with still allowing arrow key navigation when focused on an element that does not use the arrow keys. It seems nice to me that this approach allows us to support arbitrary elements such as inputs inside grid cells without needing edit mode at all [...]

The flag is called keyboardNavigationBehavior === 'tab', why would it allow grid navigation with arrow still? To me, an implicit intention is phrased when enabling this setting: Items will contain conflicting interactions.

The only difference in our viewpoint is that you guys are trying to shift the decision of when not to navigate to user land, while I rather advocate for users to make the decision of when to navigate - a decision I find much easier to make.

---

Answer

As you mentioned, it's practically impossible for parent elements to predict what content they will render and shifting actions to children is a common pattern in react-aria. What I meant however, was that there is no slot react-aria could use to carry default behavior to unknown, possibly custom, children. This means we shift all of the prevention responsibility down to the actual user land (not RAC, but actual users) to implement, which obviously isn't working out.

Users are attempting, and I think should be able to, render any arbitrary content inside a <GridListItem /> or <Cell />, while expecting the content to work as if outside an item. While this is technically speaking part covered by the W3C edit mode spec, it's not related to an edit interaction per se. As an example, we were attempting to build virtualized grid list items with nested grid lists and tag groups inside (see demo). The solution in #7360 is not helping here...

Leaving user land only an isKeyboardNavigationDisabled flag to control navigation is hardly a great experience, not only because half of the necessary props aren't even exposed, or the fact that it doesn't prevent type select, but because very deep knowledge of the underlying hooks is required to even come close to making an informed decision as to how and when to disable. To give you an idea of what a user currently has to do to make something like our use case work:

• Child wrapper to prevent all leaking events from reaching the item
• Prevent useSelectableCollection() to query data-key of nested collection items
• Wrap the collection with a container to intercept type select in capture phase
• Marshal to last child when re-entering the collection from the bottom
• Child wrapper to implement enter & exit interactions
• a lot of smaller bugs to fix with virtualization scroll

As a result, I'm advocating here to disable all grid navigation interactions when in keyboardNavigationbehavior === 'tab' by default and rather concern with when to re-enable. If you are concerned about interrupting existing behavior, I'm totally happy with introducing yet another keyboardNavigationBehavior === 'isolate' or something similar and make this work.

[nwidynski]

@devongovett Maybe I should further clarify, after sleeping on this a night.

That is because the button stops event propagation so the enter key event never reaches the row.

This is just not true at the moment, since event propagation isn't stopped by the button, but by usePress() in the useSelectableItem(). I understand that in an ideal scenario, each child component would stop propagation, leaving only the elements with native arrow key interactions (e.g. inputs) to deal with as a nicety.

Unfortunately though, pretty much all components leak keyboard events upwards (demo) when not at one point an onKeyDown handler is spread onto them, because usePress() or useKeyboard() otherwise skip stopping propagation.

This effectively sets up the exact scenario you are wanting to prevent, where a parent now needs to know about all possible children to spread an onKeyDown handler onto them and make them stopPropagation(). If the parent doesn't do this, its left for the users to do, which I don't understand why they should have to.

Am I missing something here entirely? Surely we could stop event propagation even without handlers, but that I feel like would be a much more breaking change 👍

[devongovett]

The flag is called keyboardNavigationBehavior === 'tab', why would it allow grid navigation with arrow still?

It is for tabbing within cells, not between them. You still use arrow keys to do that. There are a few different modes as well, for example in some components the cell itself is never actually focused - focus moves immediately to the first focusable child. If we disabled arrow key navigation in that case, the user would be stuck inside the cell.

Unfortunately though, pretty much all components leak keyboard events upwards (demo) when not at one point an onKeyDown handler is spread onto them, because usePress() or useKeyboard() otherwise skip stopping propagation.

I'd say this is more of a bug in TagGroup and/or useSelectableCollection. Ideally those would stop propagation after handling the event. I think we just have not yet encountered nested collection components so this never came up before. We can certainly fix that.

I guess what we're discussing here is defaults: in what we proposed, the user can navigate between cells by default and if the developer wants to prevent that they can stopPropagation. In what you proposed, there's less work for the developer because they don't need to stop propagation to prevent arrow key navigation, but it's also not possible to enable the user to navigate between cells with the arrow keys at all (which might make the UI easier to use).

If you are concerned about interrupting existing behavior, I'm totally happy with introducing yet another keyboardNavigationBehavior === 'isolate' or something similar and make this work.

I guess this still shifts the burden to the developer to know that this is something they need to use, the same way as stopPropagation would.

I think we should consider the ideal user behavior here first, e.g. whether arrow key navigation on elements such as buttons within cells should move focus between cells. Needing to shift tab back out in order to do that might not be expected. We will consult some internal accessibility folks on this before deciding on an API.

[devongovett]

Btw, wanted to add that I see two separate but related features here:

1. Supporting interactive elements within grid cells at all times. A user could tab into the contents of a cell and interact with elements such as inputs that are already present. No mode switching or interactions to enable or disable edit mode needed.
2. A more explicit edit mode, where a cell might contain non-editable data such as text and a user would press Enter/F2 to swap the UI into an editing mode. For example, the text might turn into a textfield, date picker, or whatever other UI might be appropriate for editing that type of value. Then the user could press Enter again to commit their changes, or Escape to cancel. This is the more traditional approach taken by spreadsheets.

What we've mostly been discussing so far is (1), which is much simpler to implement. (2) is more complicated, because the interactions are not well specified for getting into and out of edit mode. For keyboard, the aria spec provides good guidelines, but you would need some way to do the same thing with a mouse (maybe double click? a visual edit button?), a touch screen (long press? a button?), and with a screen reader. You've made a good start here, but this will require further research and testing. I was hoping that we could land a solution to (1) first since it is much simpler, and follow up with (2) in the future once we figure out these interactions.

[nwidynski]

@devongovett Gotcha, I'm fine with postponing the edit mode interactions until the spec for mobile is ready 👍

In my view edit mode interactions marked alpha/prerelease, even only for pointer & keyboard, would be still better than nothing. I'm only saying this from a practicality standpoint, since people will build their UIs regardless, only now likely worse in terms of accessibility. I respect the team's decision to maintain a high quality bar internally however.

I'd say this is more of a bug in TagGroup and/or useSelectableCollection. Ideally those would stop propagation after handling the event. I think we just have not yet encountered nested collection components so this never came up before. We can certainly fix that.

In the meantime, I've worked on a playtest in #7376, which demonstrates interaction event leaks in a lot of RAC’s, not limited to <TagGroup /> or useSelectableCollection(). I was under the impression RAC's were working as intended while working on this PR, so if you regard all of these leaks as bugs, then we got a whole lot of fixing to do 😄

I've also gone ahead and talked through with my team what's actually a roadblock for us at this point, which comes down to the Document key prefix introduced in this PR & disallowTypeahead not being exposed in useGridList() & useGrid(). All other stuff we can temporarily get past ourselves with some inconveniences. I would like to get #7379 shipped as soon as possible and then help out where I can to get Milestone #1 of what you mentioned completed 🚀

PS: I still think preventing all grid navigation in tab mode is less of a breaking change than fixing these event leaks, since consumers might rely on them now that they are in the world 🤣 . As mentioned, an Esc interaction to not have to shift+tab all the way back would be nice to have, and was implemented in this PR.