Updating controlled selectedIds steals focus and gives it to the tree

[Sappharad]

Describe the bug
When trying to use controlled selectedIds to check checkboxes in the tree, every time you change the value of selectedIds the tree suddenly steals keyboard focus from whatever control on the page had it last.

Code That Causes The Issue
I believe this was introduced in 1f9b062 after dispatching controlledSelectMany for the parents if propagateSelect is enabled it will use the regular changeSelectMany, passing in id for lastInteractedWith, which causes focus to be stolen by the tree.

To Reproduce
You can see this in your "Checkbox with controlled selectedIds" example included with the project.

1. Type a node number into the textbox and press enter instead of clicking the submit button.
2. Focus leaves the text field and immediately jumps into the tree.

Expected behavior
The most obvious use for a controlled set of Ids would be if you were applying changes as the result of an action performed elsewhere on the page. I would not expect performing an action elsewhere to move my keyboard focus into the grid.

Desktop (please complete the following information):

• OS: N/A Desktop PC
• Browser Firefox, Chrome
• Version It doesn't matter

Additional context
I was looking to display some data in a tree and I found this control. It seems to fit that need perfectly, but the actual behavior when setting node selections currently makes it an awkward choice.

In the case where I want to use this I have a regular list of items at the top of the page and a tree with properties for those items on top. Switching the item in the list should update the tree to show the properties for the item selected. But if I use the arrow keys to try and navigate the list of items, every time I arrow down on the list and update the tree with the appropriate selections of checkboxes, the tree immediately steals focus making just general browsing of data a challenge. Additionally I have "Check" and "Uncheck all" buttons similar to the uncheck button on the sample page, clicking those buttons also steals focus and sends it to the tree. When someone is using the mouse this isn't a huge problem but it's annoying when you're trying to use the keyboard.

1f9b062 appears to be fixing some kind of problem with focus being lost, but could we fix this new problem just by setting lastInteractedWith to null if the grid does not have focus so that it does not steal focus? In my case, just changing lastInteractedWith to null on line 276 of index.tsx completely fixes the stolen focus problem, but I suspect it's breaking whatever that previous commit was trying to solve.

[dgreene1]

@kpustakhod I’d be interested to hear your thoughts on this?

@mellis481 do we have a need for controlled selectIds at our company?

@Sappharad, in the case of "Checkbox with controlled selectedIds" what would you expect to happen? And can you provide some documentation from some kind of accessibility resource that backs up your proposed expected behavior? If you can, we would accept a PR from you as long as it doesn’t break the tests (especially https://github.com/kpustakhod/react-accessible-treeview/blob/78dfb93c09086fef4fb8bf8c769611203fafc156/src/__tests__/ControlledTree.test.tsx#L305 )

[Sappharad]

Thanks for the quick reply.

In the case of "Checkbox with controlled selectedIds" what would you expect to happen?

I thought I answered this in the expected behavior section of the initial post. Basically, my expected behavior has that if the tree does not have have focus when it is being modified externally, it should not take focus away from the control that had focus. So for the checkbox example, if I press enter in the text field the focus remains in the text field. If someone wants the grid to get focus after they update it externally, they can just call .focus() on the div themselves.

I'm not really basing this on any accessibility documentation, but here is the first document that came up on google:
https://usability.yale.edu/web-accessibility/articles/focus-keyboard-operability
It has the following relevant rules:

• Keyboard focus should be trapped within a component,
• Focus order should be meaningful and promote ease of use.
• When a user initiates a change in focus, there should not be a change in user agent, viewport, content, or an additional change in focus.

In my actual use case described above where I update the tree based on the selection of a list box elsewhere on the page, this violates the 3 rules above. The act of me moving around the list and the tree stealing focus because it was updated when I switched rows is violating the first rule because focus is no longer trapped within the control that was updating the tree. For the second rule the focus order is not meaningful will depend on the usage scenario, but basically you could put your tree and another control anywhere on a page and there could stuff in between them. If you're randomly jumping to the tree and they don't expect that, the focus order is no longer meaningful. Finally, in my scenario with a separate list, if I'm trying to initiate a change of focus on that list (so the tree is updated), the tree stealing focus is an additional change in focus beyond me navigating my list, thus violating that third rule.

--
If the user is interacting directly with the tree then it seems logical to me that the tree should control focus. But if I'm just trying to update the state of it from the outside (either displaying data for a different parent record in a list, or using a "select all" or "unselect all" button) then my keyboard input suddenly ending up inside the tree is weird. Also the tree components on desktop operating systems like Windows and macOS do not behave that way.

I'll see if I can come up with a PR next week, pending any potential counterarguments. Maybe it's something that needs to be tied to a setting if there's a legit reason to keep allowing it to steal focus from other components.

[Sappharad]

After running the tests, I see that the fix I proposed in the opening post won't solve this problem correctly.

It seems like the upward propagation of parent checkboxes relies on recursion to an extent and nulling out lastInteractedWith causes grandparents not to be updated and fails the "SelectedIds should propagate upward when deselection happens via selectedIds" unit test.

I'm going to experiment around to see if I can come up with a better suggestion that does not break anything. I suspect that I'll probably need to do something in useEffect() on Line 417 of index.tsx to prevent focus from being updated if the selected id's were manipulated from controlledSelectedIds.

Sorry for accidentally submitting an incomplete version of this post and closing/reopening the issue a couple minutes ago. Apparently there's a keyboard shortcut for comment & close and I accidentally pressed it.

[Sappharad]

Hi everyone,

It's been over a month now, and you did a release last week while completely ignoring my PR so I'm pinging this thread again to see if you care about this one at all?

Do you not want it? Do you disagree with something, and is there anything I can do about it?

Should I just make a fork of this repo and publish that instead?

Please advise.

Thank you!

[dgreene1]

@Sappharad we’re volunteers, so I’d appreciate a little more kindness and less charged language like “completely ignored.”

No, we didn’t completely ignore your request— we just made a mistake. I actually fought quite valiantly to have your concern added to our company’s internal tracking software (Jira).

I do not know when that Jira ticket will be prioritized. I apologize for forgetting to inform you that we added your request to the backlog.

[Sappharad]

Hi Dan,

Thank you for the context, I do appreciate it. It wasn’t immediately clear to me how the project was being controlled.

Sorry for the frustration.

[mellis481]

Ref: UIEN-4789

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Sappharad]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Activity.

[Sappharad]

Preemptively replying so the bot doesn't mark this as stale tomorrow.

[yhy-1]

https://github.com/dgreene1/react-accessible-treeview/releases/tag/v2.8.3
@Sappharad merge and released

[Sappharad]

@yhy-1 Thank you so much!

Glad to see this in there.