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

Author: HDinger

.changeset/all-paths-dance.md

@@ -3,3 +3,7 @@
 ---
 
 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,7 +179,8 @@ export class TreeViewElement extends HTMLElement {
     // behavior for these element types is user- or browser-defined
     if (!(node instanceof HTMLDivElement)) return
 
-    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', {
@@ -191,7 +192,7 @@ export class TreeViewElement extends HTMLElement {
 
     if (!checkSuccess) return
 
-    if (this.getNodeCheckedValue(node) === 'true') {
+    if (newCheckedValue === 'false') {
       this.setNodeCheckedValue(node, 'false')
     } else {
       this.#checkNodeOnly(node)
@@ -270,7 +271,11 @@ export class TreeViewElement extends HTMLElement {
         } else if (this.selectVariant(node) === 'single') {
           event.preventDefault()
 
-          this.#checkNodeOnly(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()
@@ -312,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[]) {
@@ -359,6 +364,7 @@ export class TreeViewElement extends HTMLElement {
 
   #checkNodeOnly(node: Element) {
     for (const el of this.activeNodes) {
+      if (el === node) continue
       this.setNodeCheckedValue(el, 'false')
     }
 
@@ -485,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
 

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: expanded, disabled: disabled, select_variant: select_variant) do |sub_tree| %>
+      <% sub_tree.with_leaf(label: "button.rb", disabled: disabled, select_variant: select_variant) %>
+      <% sub_tree.with_leaf(label: "icon_button.rb", current: true, disabled: disabled, select_variant: select_variant) %>
+    <% end %>
+
+    <% tree_view.with_leaf(label: "action_menu.rb", disabled: disabled, select_variant: select_variant) %>
+    <% tree_view.with_leaf(label: "action_menu.rb", disabled: disabled, select_variant: select_variant) %>
+    <% tree_view.with_leaf(label: "action_menu.rb", disabled: disabled, select_variant: select_variant) %>
+  <% 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 %>
 

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

@@ -6,6 +6,5 @@
     <% end %>
 
     <% tree_view.with_leaf(label: "action_menu.rb", disabled: disabled, select_variant: select_variant) %>
-    <% tree_view.with_leaf(label: "action_menu.rb", disabled: disabled, select_variant: select_variant) %>
   <% end %>
 </div>

test/system/alpha/tree_view_test.rb

@@ -653,37 +653,65 @@ def test_fires_check_event_after_single_variant
     end
 
     def test_single_select_with_duplicate_paths_selects_clicked_node
-      visit_preview(:single_select)
+      visit_preview(:doubled_path)
 
       nodes = all(selector_for("action_menu.rb"))
-      assert_equal 2, nodes.size
+      assert_equal 3, nodes.size
 
       nodes[1].click
 
-      assert_equal "true", nodes[1]["aria-checked"], "The clicked (second) node should be selected"
-      assert_equal "false", nodes[0]["aria-checked"], "The first node with the same path should not be selected"
+      nodes[1].assert_matches_selector("[aria-checked='true']")
+      nodes[0].assert_matches_selector("[aria-checked='false']")
 
       nodes[0].click
 
-      assert_equal "true", nodes[0]["aria-checked"], "The clicked (first) node should be selected"
-      assert_equal "false", nodes[1]["aria-checked"], "The second node with the same path should not be selected"
+      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(:single_select)
+      visit_preview(:doubled_path)
 
       nodes = all(selector_for("action_menu.rb"))
-      assert_equal 2, nodes.size
+      assert_equal 3, nodes.size
 
       nodes[1].send_keys(:space)
 
-      assert_equal "true", nodes[1]["aria-checked"], "The focused (second) node should be selected via keyboard"
-      assert_equal "false", nodes[0]["aria-checked"], "The first node with the same path should not be selected"
+      nodes[1].assert_matches_selector("[aria-checked='true']")
+      nodes[0].assert_matches_selector("[aria-checked='false']")
 
       nodes[0].send_keys(:space)
 
-      assert_equal "true", nodes[0]["aria-checked"], "The focused (first) node should be selected via keyboard"
-      assert_equal "false", nodes[1]["aria-checked"], "The second node with the same path should not be selected"
+      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"))
+      nodes[0].send_keys(:space)
+      nodes[0].assert_matches_selector("[aria-checked='true']")
+
+      nodes[0].send_keys(:space)
+      nodes[0].assert_matches_selector("[aria-checked='false']")
     end
 
     def test_fires_event_before_checking
@@ -876,8 +904,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
@@ -890,7 +918,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
@@ -904,7 +932,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
@@ -917,7 +945,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
@@ -932,7 +972,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