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

Focus first element in scope if nodeToRestore does not exist

Open devongovett opened this issue 1 year ago • 7 comments

In collaboration with @snowystinger. Builds on #3323.

Closes #3171. Closes #3128.

This fixes a few related issues:

  1. In cases like the Modal unmounting trigger story, focus would not move into a Dialog within a DialogContainer. The underlying reason is that DialogContainer is not a child of the Popover in the JSX tree, so the Popover's FocusScope was containing focus within it when the dialog opened (here). This is fixed by using the currently active scope (i.e. the popover) as the parent scope rather than the parent in the JSX tree, but only if the JSX parent is not already inside the active scope. This way, the focus scope within the dialog appears to be nested within the popover, and focus is allowed to move into it.
  2. In cases where the node to restore is deleted from the DOM, and there's no parent focus scope to walk back up to. For example, if you delete a row from a table in the CRUD story, or in the ActionBar examples, focus is lost to the body. This is fixed by wrapping collection components such as Table in a FocusScope, and walking up the scope tree from the dialog until we find a scope that still exists (the table), and then focusing the first item within that scope.
  3. In a similar case to (2), the ActionGroup in the CRUD story removes its delete button when nothing is selected, so when restoring focus from the delete dialog, focus should go back to the plus button instead. The fix is the same (wrapping ActionGroup in a FocusScope), but this also required tracking the active scope when both contain and restore are false.

Also related to #3039: focus now goes to the collection element when an item is removed rather than lost to the body, but still would be good to focus a more adjacent row at some point.

To do:

  • [ ] Add FocusScope around all collection components where items may be deleted
  • [ ] Figure out what to do about React Aria examples
  • [ ] Tests

devongovett avatar Aug 05 '22 23:08 devongovett

This seems to resolve #3128, closing #3171, but #2451 to fix focus restoration from the ActionBar to the TableView in this ActionBar Example, still fails intermittently to scroll the element to which focus should be restored within the TableView back into view when restoring focus from the Close/Cancel button within the ActionBar.

majornista avatar Aug 08 '22 18:08 majornista

@majornista I think this was fixed by merging in main, which includes https://github.com/adobe/react-spectrum/pull/3168. That will now keep the focused row in the DOM even when scrolled out of view, instead of shifting focus to the collection. I believe what was happening before was that row element was reused when scrolled out of view, and focus was restored back to that instead.

devongovett avatar Aug 08 '22 20:08 devongovett

@majornista I think this was fixed by merging in main, which includes #3168. That will now keep the focused row in the DOM even when scrolled out of view, instead of shifting focus to the collection. I believe what was happening before was that row element was reused when scrolled out of view, and focus was restored back to that instead.

Seems to be working as expected now.

majornista avatar Aug 09 '22 15:08 majornista