[DREAM-704] Tree view selection based on path identity breaks use cases where similar paths are allowed (#475)

* Keep the node object after selection instead of fetching it by the path

* Fix single-select toggle-off and form payload disambiguation

  - Fix treeViewNodeChecked/Before events reporting wrong checkedValue on toggle-off (was hardcoded 'true', now reflects actual new state)
  - Align keyboard Space/Enter with click: pressing Space on an already-selected single-select node now deselects it
  - Include data-node-id as nodeId in hidden form input payload so duplicate-path nodes are distinguishable on the server side

* Adapt test to avoid being trapped in the focus zone

* Pass nodeId in filterableTreeView as well

* Generating component snapshots

* Generating component snapshots

---------

Co-authored-by: HDinger <7457313+HDinger@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: HDinger

.changeset/all-paths-dance.md

@@ -0,0 +1,9 @@
+---
+'@openproject/primer-view-components': minor
+---
+
+Fix selection of nodes with the same path in Primer::Alpha::TreeView
+
+When a `[role=treeitem]` element carries a `data-node-id` attribute, that id is now included as `nodeId` in the hidden form input payload (`{path, nodeId?, value?}`). Trees with duplicate-path nodes should set `data-node-id` to a stable unique identifier so the server can distinguish which node was selected.
+
+**Breaking change in `TreeViewElement#checkOnlyAtPath`:** if the given path is not found the method is now a no-op. Previously it would uncheck all active nodes before failing to check the missing node, which could be used as an indirect "clear selection" mechanism. Use explicit `setNodeCheckedValue(node, 'false')` calls if that behaviour is needed.

app/components/primer/alpha/tree_view/tree_view.ts

@@ -179,8 +179,8 @@ export class TreeViewElement extends HTMLElement {
     // behavior for these element types is user- or browser-defined
     if (!(node instanceof HTMLDivElement)) return
 
-    const path = this.getNodePath(node)
-    const nodeInfo = this.infoFromNode(node, 'true')
+    const newCheckedValue = this.getNodeCheckedValue(node) === 'true' ? 'false' : 'true'
+    const nodeInfo = this.infoFromNode(node, newCheckedValue)
 
     const checkSuccess = this.dispatchEvent(
       new CustomEvent('treeViewBeforeNodeChecked', {
@@ -192,10 +192,10 @@ export class TreeViewElement extends HTMLElement {
 
     if (!checkSuccess) return
 
-    if (this.getNodeCheckedValue(node) === 'true') {
+    if (newCheckedValue === 'false') {
       this.setNodeCheckedValue(node, 'false')
     } else {
-      this.checkOnlyAtPath(path)
+      this.#checkNodeOnly(node)
     }
 
     this.dispatchEvent(
@@ -271,7 +271,11 @@ export class TreeViewElement extends HTMLElement {
         } else if (this.selectVariant(node) === 'single') {
           event.preventDefault()
 
-          this.checkOnlyAtPath(this.getNodePath(node))
+          if (this.getNodeCheckedValue(node) === 'true') {
+            this.setNodeCheckedValue(node, 'false')
+          } else {
+            this.#checkNodeOnly(node)
+          }
         } else if (node instanceof HTMLAnchorElement) {
           // simulate click on space
           node.click()
@@ -313,7 +317,7 @@ export class TreeViewElement extends HTMLElement {
   }
 
   get activeNodes() {
-    return document.querySelectorAll('[aria-checked="true"]')
+    return this.querySelectorAll('[aria-checked="true"]')
   }
 
   expandAtPath(path: string[]) {
@@ -352,11 +356,19 @@ export class TreeViewElement extends HTMLElement {
   }
 
   checkOnlyAtPath(path: string[]) {
+    const node = this.nodeAtPath(path)
+    if (!node) return
+
+    this.#checkNodeOnly(node)
+  }
+
+  #checkNodeOnly(node: Element) {
     for (const el of this.activeNodes) {
-      this.uncheckAtPath(this.getNodePath(el))
+      if (el === node) continue
+      this.setNodeCheckedValue(el, 'false')
     }
 
-    this.checkAtPath(path)
+    this.setNodeCheckedValue(node, 'true')
   }
 
   toggleCheckedAtPath(path: string[]) {
@@ -479,10 +491,13 @@ export class TreeViewElement extends HTMLElement {
       newInput.removeAttribute('data-target')
       newInput.removeAttribute('form')
 
-      const payload: {path: string[]; value?: string} = {
+      const payload: {path: string[]; nodeId?: string; value?: string} = {
         path: this.getNodePath(node),
       }
 
+      const nodeId = node.getAttribute('data-node-id')
+      if (nodeId) payload.nodeId = nodeId
+
       const inputValue = this.getFormInputValueForNode(node)
       if (inputValue) payload.value = inputValue
 

app/components/primer/open_project/filterable_tree_view.ts

@@ -119,7 +119,8 @@ export class FilterableTreeViewElement extends HTMLElement {
           }
 
           this.#checkedNodeIds.set(nodeId, nodeInfo.checkedValue)
-          const payload: {path: string[]; value?: string} = {path: nodeInfo.path}
+          const payload: {path: string[]; nodeId?: string; value?: string} = {path: nodeInfo.path}
+          payload.nodeId = nodeId
           const dataValue = node.getAttribute('data-value')
           if (dataValue) payload.value = dataValue
           this.#checkedNodeFormPayloads.set(nodeId, payload)

previews/primer/alpha/tree_view_preview.rb

@@ -197,6 +197,12 @@ def form_input(select_variant: :multiple, expanded: true)
         })
       end
 
+      # @label Doubled paths
+      # @hidden
+      def doubled_path
+        render_with_template
+      end
+
       private
 
       def coerce_bool(value)

previews/primer/alpha/tree_view_preview/doubled_path.html.erb

@@ -0,0 +1,12 @@
+<div style="max-width: 400px">
+  <%= render(Primer::Alpha::TreeView.new) do |tree_view| %>
+    <% tree_view.with_sub_tree(label: "src", expanded: true, select_variant: :single) do |sub_tree| %>
+      <% sub_tree.with_leaf(label: "button.rb", select_variant: :single) %>
+      <% sub_tree.with_leaf(label: "icon_button.rb", current: true, select_variant: :single) %>
+    <% end %>
+
+    <% tree_view.with_leaf(label: "action_menu.rb", select_variant: :single) %>
+    <% tree_view.with_leaf(label: "action_menu.rb", select_variant: :single) %>
+    <% tree_view.with_leaf(label: "action_menu.rb", select_variant: :single) %>
+  <% end %>
+</div>

previews/primer/alpha/tree_view_preview/form_input.html.erb

@@ -1,15 +1,15 @@
 <%= form_with(url: primer_view_components.generic_form_submission_path(format: :json)) do |f| %>
   <%= render(Primer::Alpha::Stack.new) do %>
     <%= render(Primer::Alpha::TreeView.new(form_arguments: { builder: f, name: "folder_structure" })) do |tree| %>
-      <% tree.with_sub_tree(label: "src", expanded: expanded, select_variant: select_variant, value: 0) do |sub_tree| %>
-        <% sub_tree.with_leaf(label: "button.rb", select_variant: select_variant, value: 1, checked: true) %>
-        <% sub_tree.with_leaf(label: "icon_button.rb", current: true, select_variant: select_variant, value: 2) %>
+      <% tree.with_sub_tree(label: "src", expanded: expanded, select_variant: select_variant, value: 0, data: { node_id: "src" }) do |sub_tree| %>
+        <% sub_tree.with_leaf(label: "button.rb", select_variant: select_variant, value: 1, checked: true, data: { node_id: "src-button-rb" }) %>
+        <% sub_tree.with_leaf(label: "icon_button.rb", current: true, select_variant: select_variant, value: 2, data: { node_id: "src-icon-button-rb" }) %>
       <% end %>
 
-      <% tree.with_leaf(label: "action_menu.rb", select_variant: select_variant, value: 3) %>
-      <% tree.with_sub_tree(label: "Docs & legal requirements", select_variant: select_variant, value: 4) do |sub_tree| %>
-        <% sub_tree.with_leaf(label: "Readme.md", select_variant: select_variant, value: 5) %>
-        <% sub_tree.with_leaf(label: "Copyright.md", select_variant: select_variant, value: 6) %>
+      <% tree.with_leaf(label: "action_menu.rb", select_variant: select_variant, value: 3, data: { node_id: "action-menu-rb" }) %>
+      <% tree.with_sub_tree(label: "Docs & legal requirements", select_variant: select_variant, value: 4, data: { node_id: "docs" }) do |sub_tree| %>
+        <% sub_tree.with_leaf(label: "Readme.md", select_variant: select_variant, value: 5, data: { node_id: "docs-readme-md" }) %>
+        <% sub_tree.with_leaf(label: "Copyright.md", select_variant: select_variant, value: 6, data: { node_id: "docs-copyright-md" }) %>
       <% end %>
     <% end %>
 

test/system/alpha/tree_view_test.rb

@@ -652,6 +652,78 @@ def test_fires_check_event_after_single_variant
       assert_equal details[0]["previousCheckedValue"], "false"
     end
 
+    def test_single_select_with_duplicate_paths_selects_clicked_node
+      visit_preview(:doubled_path)
+
+      nodes = all(selector_for("action_menu.rb"))
+      assert_equal 3, nodes.size
+
+      nodes[1].click
+
+      nodes[1].assert_matches_selector("[aria-checked='true']")
+      nodes[0].assert_matches_selector("[aria-checked='false']")
+
+      nodes[0].click
+
+      nodes[0].assert_matches_selector("[aria-checked='true']")
+      nodes[1].assert_matches_selector("[aria-checked='false']")
+    end
+
+    def test_single_select_with_duplicate_paths_selects_focused_node_on_keyboard
+      visit_preview(:doubled_path)
+
+      nodes = all(selector_for("action_menu.rb"))
+      assert_equal 3, nodes.size
+
+      # icon_button.rb has current: true so it owns tabindex=0. Sending keys directly
+      # to a tabindex=-1 node is unreliable: focusZone redirects focus to the aria-current
+      # item on focusin, so Space would land on the wrong node.
+      # Start from icon_button.rb and navigate down with arrow keys instead.
+      find('[aria-current]').send_keys(:down)
+      keyboard.type(:down)
+      keyboard.type(:space)
+
+      nodes[1].assert_matches_selector("[aria-checked='true']")
+      nodes[0].assert_matches_selector("[aria-checked='false']")
+
+      keyboard.type(:up)
+      keyboard.type(:space)
+
+      nodes[0].assert_matches_selector("[aria-checked='true']")
+      nodes[1].assert_matches_selector("[aria-checked='false']")
+    end
+
+    def test_fires_check_event_after_single_variant_toggle_off
+      visit_preview(:default, select_variant: :single)
+
+      activate_at_path("src")
+      assert_path_checked "src"
+
+      details = capture_event("treeViewNodeChecked") do
+        activate_at_path("src")
+      end
+
+      assert_equal 1, details.size
+      assert details[0]["node"]
+      assert_equal details[0]["path"], ["src"]
+      assert_equal details[0]["checkedValue"], "false"
+      assert_equal details[0]["previousCheckedValue"], "true"
+    end
+
+    def test_keyboard_toggles_off_already_selected_single_variant
+      visit_preview(:doubled_path)
+
+      nodes = all(selector_for("action_menu.rb"))
+
+      # Navigate from icon_button.rb (aria-current, tabindex=0) to nodes[0]
+      find('[aria-current]').send_keys(:down)
+      keyboard.type(:space)
+      nodes[0].assert_matches_selector("[aria-checked='true']")
+
+      keyboard.type(:space)
+      nodes[0].assert_matches_selector("[aria-checked='false']")
+    end
+
     def test_fires_event_before_checking
       visit_preview(:default, select_variant: :multiple)
 
@@ -842,8 +914,8 @@ def test_form_submission
       # for some reason the JSON response is wrapped in HTML, I have no idea why
       response = JSON.parse(find("pre").text)
 
-      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
-      assert_equal "{\"path\":[\"action_menu.rb\"],\"value\":\"3\"}", response.dig("form_params", "folder_structure", 1)
+      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"nodeId\":\"src-button-rb\",\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
+      assert_equal "{\"path\":[\"action_menu.rb\"],\"nodeId\":\"action-menu-rb\",\"value\":\"3\"}", response.dig("form_params", "folder_structure", 1)
     end
 
     def test_initial_form_state
@@ -856,7 +928,7 @@ def test_initial_form_state
       # for some reason the JSON response is wrapped in HTML, I have no idea why
       response = JSON.parse(find("pre").text)
 
-      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
+      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"nodeId\":\"src-button-rb\",\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
     end
 
     def test_form_submission_with_single_select_variant
@@ -870,7 +942,7 @@ def test_form_submission_with_single_select_variant
       # for some reason the JSON response is wrapped in HTML, I have no idea why
       response = JSON.parse(find("pre").text)
 
-      assert_equal "{\"path\":[\"action_menu.rb\"],\"value\":\"3\"}", response.dig("form_params", "folder_structure", 0)
+      assert_equal "{\"path\":[\"action_menu.rb\"],\"nodeId\":\"action-menu-rb\",\"value\":\"3\"}", response.dig("form_params", "folder_structure", 0)
     end
 
     def test_initial_form_state_for_single_select
@@ -883,7 +955,19 @@ def test_initial_form_state_for_single_select
       # for some reason the JSON response is wrapped in HTML, I have no idea why
       response = JSON.parse(find("pre").text)
 
-      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
+      assert_equal "{\"path\":[\"src\",\"button.rb\"],\"nodeId\":\"src-button-rb\",\"value\":\"1\"}", response.dig("form_params", "folder_structure", 0)
+    end
+
+    def test_form_payload_includes_node_id_when_present
+      visit_preview(:form_input, expanded: true, select_variant: :single, route_format: :json)
+
+      activate_at_path("action_menu.rb")
+
+      find("button[type=submit]").click
+
+      response = JSON.parse(find("pre").text)
+
+      assert_equal "{\"path\":[\"action_menu.rb\"],\"nodeId\":\"action-menu-rb\",\"value\":\"3\"}", response.dig("form_params", "folder_structure", 0)
     end
 
     def test_single_select_with_special_character
@@ -898,7 +982,7 @@ def test_single_select_with_special_character
       # for some reason the JSON response is wrapped in HTML, I have no idea why
       response = JSON.parse(find("pre").text)
 
-      assert_equal "{\"path\":[\"Docs & legal requirements\"],\"value\":\"4\"}", response.dig("form_params", "folder_structure", 0)
+      assert_equal "{\"path\":[\"Docs & legal requirements\"],\"nodeId\":\"docs\",\"value\":\"4\"}", response.dig("form_params", "folder_structure", 0)
     end
 
     def test_keyboard_focus_moves_to_parent_when_include_fragment_with_role_treeitem_is_collapsed