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
261 stars 37 forks source link

Abilities to set focus by passing focusedId props #180

Closed yhy-1 closed 4 months ago

yhy-1 commented 4 months ago

fix the issue in https://github.com/dgreene1/react-accessible-treeview/issues/156

Ke1sy commented 4 months ago

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

Ke1sy commented 4 months ago

I'm not sure, if we need to add any error handling when the focused id node doesn't exist, it looks like it can break the application... But on the other hand this should be handled by the application's ErrorBoundary. image image

Ke1sy commented 4 months ago

It looks like the actual focus doesn't move to the focused node, only visual styles appear on new focused node, and in the result the keyboard navigation doesn't work from the new focused place. image

Ke1sy commented 4 months ago

I noticed that in basic treeview the disabled nodes are focusable with mouse and keyboard, may be can think if we really need this AC: "- shouldn't be able to set focus on a disabled node" image CC: @mellis481

mellis481 commented 4 months ago

I noticed that in basic treeview the disabled nodes are focusable with mouse and keyboard, may be can think if we really need this AC: "- shouldn't be able to set focus on a disabled node")

I agree that disabled nodes shouldn't be focusable. That said, I think we need to keep them focusable b/c of how the tree was originally developed which allows for children of disabled nodes to NOT be disabled (which doesn't make sense). I detailed this issue here: https://github.com/dgreene1/react-accessible-treeview/issues/93.

yhy-1 commented 4 months ago

@Ke1sy @mellis481 I add the check to see if the existing focused ID exists and throw that error in case they set a bad focused ID. If we are saying we should skip the check and just focus if it finds the ID, I can remove it.

yhy-1 commented 4 months ago

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

Hello, I find this statement a bit unclear. If the IDs for the tree aren't hard-coded or if we don't know which ID to pass, how can we determine what the user is trying to focus on? The user already has the ability to set an ID. Is this the issue you're referring to? If so, I can update the example to clarify this point. @Ke1sy

mellis481 commented 4 months ago

@Ke1sy @mellis481 I add the check to see if the existing focused ID exists and throw that error in case they set a bad focused ID. If we are saying we should skip the check and just focus if it finds the ID, I can remove it.

I would recommend we don't throw an error if they pass a bad ID, but rather just don't focus anything.

mellis481 commented 4 months ago

One of the possible difficulties for user to use this new prop could be that the ids of the nodes are not displayed in html even if they are provided by user for each node, so without using the hardcoded id for each node user may not even know how they are calculated inside treeview.

Hello, I find this statement a bit unclear. If the IDs for the tree aren't hard-coded or if we don't know which ID to pass, how can we determine what the user is trying to focus on? The user already has the ability to set an ID. Is this the issue you're referring to? If so, I can update the example to clarify this point. @Ke1sy

@Ke1sy It seems to me you are conflating how the developer will code programmatically set focus to a node and the user being able to set focus. Perhaps this has happened b/c you are focused on the Storybook example where we let the user set the value? In any case, the user won't be setting the node to focus.

CC: @yhy-1