search

dgreene1 / react-accessible-treeview

A react component that implements the treeview pattern as described by the WAI-ARIA Authoring Practices.
https://dgreene1.github.io/react-accessible-treeview
MIT License
277 stars 37 forks source link

Scroll to leaf nodes instead of the branches to avoid janky scrolling behavior #81

Closed philipp-spiess closed 1 year ago

philipp-spiess commented 1 year ago

We're evaluating the use of react-accessible-treeview for Sourcegraph and one issue that stood out for us that happens primarely when we're dealing with very large nested lists is that this library relies on focus() of the branch lists to move content into the viewport. Since branches can be very long (possibly longer than the total viewport height), this causes some unexpected jumping when using arrow keys to navigate as the user agent is trying to move as much of the node that was focused into the viewport as possible.

You can recreate this problem easily on the react-accessible-treeview preview page, here's a video recording from the storybook to demo the issue:

https://user-images.githubusercontent.com/458591/213458970-3330301a-af68-4bed-99dd-ef705097e955.mov

You can see in the video above that sometimes the leaf that is highlight is in the middle of the viewport and the next leaf is clearly within the viewport as well when the unexpected scrolling happens.

To fix this, I've made some changes so that in addition to the tabbable nodes (the one that the focus should move to), we're also keeping track of all leaf nodes. We can then use focus({ preventScroll: true }) and manually scroll to the leaf node.

Note that I have put the focus after the scrolling so that in the case of preventScroll being unsupported, the behavior is unchanged.

The test is a bit awkward as jsdom doesn't seem to implement scrollIntoView so we have to spy on focus() and stub scrollIntoView(). I’m happy to make changes if you have other ideas though :)

Test plan

https://user-images.githubusercontent.com/458591/213458273-19f33539-16bc-43e2-a0ac-d6180fd8b949.mov

dgreene1 commented 1 year ago

@mellis481 given that the recreation scenario is likely one that is similar to our flagship product’s scenarios (ie large size of items) this seems to be a fix that we would also want to take advantage of. So can you bring in UIEN-3615 into our current support queue and get it assigned to literally anyone on our team? This is a good opportunity to share knowledge of this library with someone who hasn’t been made familiar yet. My expectation is that they fully understand the well described problem (thanks @philipp-spiess) and then evaluates if the test properly recreates that problem and accurately proves it is fixed. One way to do that is to remove the fix and see that the test fails. I would also expect them to verify that no external props/interfaces have changed without the author seeking prior approval.

thanks again @philipp-spiess for the contribution