fix(#3884): useGrid: gracefully handle focus when a focused row is removed from the collection #3885
Conversation
When one or more rows are removed, restore focus to the appropriate cell within the next or previous remaining row. TODO: Address race condition within CRUD example, using mouse, where pressing Delete button within the overlay dialog removes the row and closes the dialog losing focus to the document.body before useTable can restore focus to the appropriate key within the next or previous row. This causes the focus to be restored to the Select All checkbox.
majornista
changed the title
| onPrimaryAction={() => { | ||
| // Add delay so that dialog closes and restores focus | ||
| // to the TableView before removing the row. | ||
| setTimeout(() => list.remove(dialog.item.id), 60); |
There was a problem hiding this comment.
I don't think we're going to want this timeout
Where does focus go without the setTimeout? I think it goes to the first focusable in the scope? or does table move it to the first row?
If table does it, seems like we could consult the cached information we have an move it somewhere better
If the focus scope does it, then maybe the table can become the first focusable thing and when it gets focus, it then forwards it to the appropriate row?
There was a problem hiding this comment.
Without the timeout, focus moves to the first focusable in scope, the Select All checkbox.
There was a problem hiding this comment.
selectionManager.setFocusedKey executes within the useGrid and targets the appropriate key, but useRestoreFocus executes after, when the dialog closes.
There was a problem hiding this comment.
I think I figured this out. The problem is that the FocusScope for Dialogs and Modals always contain focus using useOverlayFocusContain, so that when the user presses the Delete button to execute the primary action that removes the item, while the useEffect within useGrid does reset focus to the appropriate focusedKey, the FocusScope within the Overlay always steals focus back, even if the Overlay is in the process of closing. I think we need to account for the state of the Overlay when calling useOverlayFocusContain, so that an Overlay that is closing doesn't contain focus.
There was a problem hiding this comment.
I think we were doing that at one point, when transitioning out. But I can't find it, maybe it was in some work we did but never merged.
There was a problem hiding this comment.
@snowystinger Is it this one? #3704
There was a problem hiding this comment.
I can confirm that #3704 resolves the issue.
Unfortunately, I have to take that back. With keyboard it was always working, but with mouse interaction, the FocusScope within the overlay is still stealing focus back to the Dialog that is closing.
There was a problem hiding this comment.
No, it was elsewhere, the idea was that when a Popover began transitioning out, then contains would be set to false. I think it was this #3725
…obe/react-spectrum into tableview-restore-focus-on-remove-row
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| let index = Math.min( | ||
| ( | ||
| diff > 1 ? | ||
| Math.max(parentNode.index - diff + 1, 0) : |
There was a problem hiding this comment.
So if you deleted a row and at the same time added 2+ more the index would change but otherwise it stays the same? What was the reason for this?
There was a problem hiding this comment.
Was added to try to address the concerns @LFDanLu raised with the ListBox example: #3885 (comment).
When you remove multiple items, and depending on where focus is in relation to the items selected for removal, one perceives the focus jumping ahead significantly from where it was. This code intends to mitigate that perception. Is there a better approach to this?
There was a problem hiding this comment.
So if you deleted a row and at the same time added 2+ more the index would change but otherwise it stays the same? What was the reason for this?
Also, in this use case the diff would be less than 1, because the new row count would be greater than the cached row count, const diff = cachedRows.length - rows.length;, so we would just use Math.min(parentNode.index, rows.length - 1).
There was a problem hiding this comment.
Ok that makes sense. One weird case I see is if you select two items from the bottom up so focus is on the first one, focus moves to the previous item rather than the next one. For example, select Snake and Kangaroo, delete, and focus ends on Aardvark rather than Danni (which is now at the corresponding index as Snake). If you select the opposite direction so focus is on Snake, then focus moves to Danni instead (rather than Devon which is at the corresponding index).
There was a problem hiding this comment.
I kinda like that behavior since it is restoring focus to the closest item in the direction the user's focus was closest/trending to if that makes sense (albeit that isn't what the code is explicitly doing). There is another odd case that I noted here, so I think it would be good for the team to test this vs the previous implementation for opinions/edge cases and maybe we can make a call on if we are fine with this or if we want to refine some more
There was a problem hiding this comment.
Mac Finder:
Select two, bottom first. Delete. Loses selection entirely.
Select two, top first. Delete. Selects item that replaced first selected index, not the focused one.
Select one. Delete. Selects the item that followed it.
Select two, top first, gap of n items between. Selects item that followed last focused+selected item.
Select two, bottom first, gap of n items between. Selects item that followed last focused+selected item.
Could improve on this by not losing selection in the first case and sending it to the next available item (checking more than one down in otherwords). The rest seems fine to use as the template for how to move?
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } |
Closes #3884, relates to #3039
✅ Pull Request Checklist:
📝 Test Instructions:
See examples with fix here:
🧢 Your Project:
Adobe/Accessibility