diff --git a/.changeset/all-paths-dance.md b/.changeset/all-paths-dance.md new file mode 100644 index 0000000000..1daeb35d68 --- /dev/null +++ b/.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. diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/open_project/filterable_tree_view/custom_segmented_control/dark_high_contrast.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/open_project/filterable_tree_view/custom_segmented_control/dark_high_contrast.png index 6cd8569021..3ad3aa47b8 100644 Binary files a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/open_project/filterable_tree_view/custom_segmented_control/dark_high_contrast.png and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/open_project/filterable_tree_view/custom_segmented_control/dark_high_contrast.png differ diff --git a/app/components/primer/alpha/tree_view/tree_view.ts b/app/components/primer/alpha/tree_view/tree_view.ts index 4843db9f67..ccf8f88b92 100644 --- a/app/components/primer/alpha/tree_view/tree_view.ts +++ b/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 diff --git a/app/components/primer/open_project/filterable_tree_view.ts b/app/components/primer/open_project/filterable_tree_view.ts index 64e0606417..8564293874 100644 --- a/app/components/primer/open_project/filterable_tree_view.ts +++ b/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) diff --git a/previews/primer/alpha/tree_view_preview.rb b/previews/primer/alpha/tree_view_preview.rb index 9d450567e5..6e671bbb26 100644 --- a/previews/primer/alpha/tree_view_preview.rb +++ b/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) diff --git a/previews/primer/alpha/tree_view_preview/doubled_path.html.erb b/previews/primer/alpha/tree_view_preview/doubled_path.html.erb new file mode 100644 index 0000000000..6f40084a12 --- /dev/null +++ b/previews/primer/alpha/tree_view_preview/doubled_path.html.erb @@ -0,0 +1,12 @@ +
+ <%= 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 %> +
diff --git a/previews/primer/alpha/tree_view_preview/form_input.html.erb b/previews/primer/alpha/tree_view_preview/form_input.html.erb index 049d4fc77f..fc9da996a4 100644 --- a/previews/primer/alpha/tree_view_preview/form_input.html.erb +++ b/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 %> diff --git a/test/system/alpha/tree_view_test.rb b/test/system/alpha/tree_view_test.rb index 6263205dbf..97a9651b25 100644 --- a/test/system/alpha/tree_view_test.rb +++ b/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