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

Controlled mode is broken #190

Closed urrri-redis closed 3 weeks ago

urrri-redis commented 3 months ago

Describe the bug when I want to control selection, I add external state for selectedIds and pass them to the tree. this is working, when I apply ids for the first time, like in uncontrolled mode. When I'm trying to apply ids, based on onSelect event, then it behaves incorrect and weird

Code That Causes The Issue

function App(){    
  const [sel, setSel] = useState<NodeId[]>([]);
  return (
    <TreeView
          data={nodes} // any hierarchical nodes (at least 1 level with branches and leaves)
          multiSelect
          propagateSelect
          propagateSelectUpwards
          togglableSelect
          selectedIds={sel}
          onSelect={({ element, treeState, ...state }) => {
            // following only to show the problem
            if (!props.isBranch) setSel([element.id]);
          }}
   />
  )
}

To Reproduce Steps to reproduce the behavior:

  1. Add the code to any tree example
  2. Click on any leaf node
  3. Then click on any other leaf node
  4. ! See endless switching between that nodes
  5. to make problem even more visual, wrap setSel call with setTimeout
  6. select some leafs/branches one by one
  7. ! see flickering among all the selections

Note: I also tried to use following setSel([...treeState.selectedIds]), but it also makes flickering, when I select last leaf in branch, when other leaves already selected

Expected behavior if selectedIds prop is not undefined (controlled mode), then component must show passed selection, instead of internal state, even if user doesn't changes selection. Only after change events (onSelect, onNodeSelect) are treated and selectedIds updated, tree should represent new state, based only on values in the prop

Screen capture https://github.com/user-attachments/assets/5f4d0651-7844-493a-bfa7-4933793a041e

Desktop (please complete the following information):

urrri-redis commented 3 months ago

@dgreene1 onSelect is called on all 4 cases

  1. user selected node
  2. related nodes (parent/children) are selected as result of user action (# 1)
  3. node is selected, because selectedIds prop is changed
  4. related nodes (parent/children) are selected as result of selectedIds change (# 3)

only first 2 events should cause onSelect, otherwise you have endless loop. to be backward compatible, you can add some flag, that tells, if this event caused by user action (## 1,2) or controlled state update (## 3,4)

urrri-redis commented 3 months ago

same issue is for the expandedIds/onExpand

yhy-1 commented 3 months ago

Hi, @urrri-redis is there a reason why your not just using the defaultSelectId with the uncontrolled version?

OnSelect and OnSelectNode are applied only after the selection. Would it make more sense to have an onToggle callback for what you're looking for? This would be called before the actual selection had happened. You will need to manage the selectId, included when user deselected. image

stale[bot] commented 1 month ago

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.

stale[bot] commented 3 weeks ago

This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions.