lukasbach / react-complex-tree

Unopinionated Accessible Tree Component with Multi-Select and Drag-And-Drop
https://rct.lukasbach.com
MIT License
915 stars 72 forks source link

Indirectly selecting the starting item in a `shift` multiselect action results in an incorrect multiselection #336

Closed tonyketcham closed 5 months ago

tonyketcham commented 5 months ago

Describe the bug When selecting the first item in a shift key held selection of multiple items, if the first item is selected via a ref or by changing the selectedItems view state on a ControlledTree, the starting state does not get overridden. Therefore, the previous item which was directly selected in the tree is incorrectly used as the start of the range.

To Reproduce

  1. Go to https://rct.lukasbach.com/docs/guides/refs
  2. Select "Banana" in the tree
  3. Click the ref button "Select items Apple and Orange"
  4. Hold down the shift key
  5. Select "Desserts" in the tree

Expected selection is: [Apple, Orange, Lemon, Berries, Banana, Meals, Desserts]

Actual selection is: [Banana, Meals, Desserts]

Expected behavior Changes to the tree's selection state which originate outside of the tree component itself should overwrite the starting item in a multiselection range

Screenshots

https://github.com/lukasbach/react-complex-tree/assets/43280336/7ecad96a-cfc3-4284-b184-2b1cce2324e5

https://github.com/lukasbach/react-complex-tree/assets/43280336/20d85ef9-cef2-473b-8025-72a12bc42762

Real-world case

When clicking on a layer in the visual canvas space, the selectedItems view state of the RCT layer tree is updated. Holding down shift to select the end item of a selection range in the tree then results in an incorrect multiselection:

https://github.com/lukasbach/react-complex-tree/assets/43280336/84e6efc7-cbea-4693-b126-3317d487617e

tonyketcham commented 5 months ago

I believe this is the culprit, as setting selection via ref or viewState does not (and probably should not) focus the item in the tree: https://github.com/lukasbach/react-complex-tree/blob/8e317ce14e5bfa6ee7677b63173ed9dceaa07f72/packages/core/src/treeItem/useTreeItemRenderContext.ts#L18

lukasbach commented 5 months ago

I'm not sure there is unwanted behavior here. Shift-clicking an item should always override the selection with all items between the focused item, and the shift-clicked item, which will then become the newly focused item. The last-selected item doesn't necessarily have to be the focused item, you can e.g. click an item to select and focus it, then move the focus with arrow keys to another item without changing the selection. On the docs page of external refs, calling ref.selectItems(items) selects the items, but doesn't change the focus, so shift-clicking afterwards will use the old focus as reference. If you want to programmatically update the selected items and also set the focus, you can use ref.focusItem(item) for that, than subsequent selections will use the new item as reference.

For some reason, the real-world-case video you attached doesn't load for me, so I can't really see how it behaves in your app.

You also mentioned your expected selection would be [Apple, Orange, Lemon, Berries, Banana, Meals, Desserts] instead of [Banana, Meals, Desserts]. That would mean shift-clicking wouldn't override the existing selection, but add the new range to previously selected items. That is the current behavior of shift+control clicking something, where a range is added without unselecting previously selected items. That behavior is currently stored in interaction managers, do you currently use one of the predefined interaction managers or use a custom one?

lukasbach commented 5 months ago

As we've discussed, it is intended that control-clicking always expands the selection starting from the previously focused item. If the selection is changed from outside, and future control-click selections should start from the newly selected items, the focused item should be changed from outside as well. In 2.3.5, I've added an additional setDomFocus argument to all focus-item handlers that can be used to opt out of also setting the DOM focus when the handler is called, to make this use case more convenient. This prop defaults to true in all cases to stay consistent with the previous behavior, but can be explicitly set to false to opt out of that behavior. I'll close this for now, feel free to reopen or let me know if you have further issues with this topic.