[SelectPanel] Fix issue causing only one server-rendered item to appear selected (#3010)

Co-authored-by: camertron <camertron@users.noreply.github.com>

Author: camertron

.changeset/brown-tables-do.md

@@ -0,0 +1,8 @@
+---
+'@primer/view-components': patch
+---
+
+Fixes several SelectPanel bugs:
+
+1. If multiple server-rendered items are checked, the panel will only show one item checked.
+2. If no `data-value` attributes are provided, panels in single-select mode will allow multiple items to be checked.

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/dark.png

@@ -713,11 +713,12 @@ export class SelectPanelElement extends HTMLElement {
           itemContent.setAttribute(this.ariaSelectionType, 'false')
         }
       } else if (value && !this.#selectedItems.has(value) && this.isItemChecked(item)) {
-        this.#hasLoadedData = true
         this.#addSelectedItem(item)
       }
     }
 
+    this.#hasLoadedData = true
+
     if (!this.noResults) return
 
     if (this.#inErrorState()) {
@@ -860,7 +861,8 @@ export class SelectPanelElement extends HTMLElement {
     // interfere with events fired by menu items whose behavior is specified outside the library.
     if (this.selectVariant !== 'multiple' && this.selectVariant !== 'single') return
 
-    const checked = !this.isItemChecked(item)
+    const currentlyChecked = this.isItemChecked(item)
+    const checked = !currentlyChecked
 
     const activationSuccess = this.dispatchEvent(
       new CustomEvent('beforeItemActivated', {
@@ -875,22 +877,21 @@ export class SelectPanelElement extends HTMLElement {
     const itemContent = this.#getItemContent(item)
 
     if (this.selectVariant === 'single') {
-      const value = this.selectedItems[0]?.value
-      const element = this.visibleItems.find(el => this.#getItemContent(el)?.getAttribute('data-value') === value)
+      // disallow unchecking checked item in single-select mode
+      if (!currentlyChecked) {
+        for (const el of this.items) {
+          this.#getItemContent(el)?.setAttribute(this.ariaSelectionType, 'false')
+        }
 
-      if (element) {
-        this.#getItemContent(element)?.setAttribute(this.ariaSelectionType, 'false')
-      }
+        this.#selectedItems.clear()
 
-      this.#selectedItems.clear()
+        if (checked) {
+          this.#addSelectedItem(item)
+          itemContent?.setAttribute(this.ariaSelectionType, 'true')
+        }
 
-      // Only check, never uncheck here. Single-select mode does not allow unchecking a checked item.
-      if (checked) {
-        this.#addSelectedItem(item)
-        itemContent?.setAttribute(this.ariaSelectionType, 'true')
+        this.#setDynamicLabel()
       }
-
-      this.#setDynamicLabel()
     } else {
       // multi-select mode allows unchecking a checked item
       itemContent?.setAttribute(this.ariaSelectionType, `${checked}`)

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/dark_colorblind.png

@@ -5,7 +5,7 @@ class SelectPanelItemsController < ApplicationController
   SELECT_PANEL_ITEMS = [
     { value: 1, title: "Photon torpedo", description: "Starship-mounted missile" },
     { value: 2, title: "Bat'leth", description: "The Klingon warrior's preferred means of achieving honor" },
-    { value: 3, selected: true, title: "Phaser", description: "The iconic handheld laser beam" },
+    { value: 3, title: "Phaser", description: "The iconic handheld laser beam" },
     { value: 4, title: "Lightsaber", description: "An elegant weapon for a more civilized age", recent: true },
     { value: 5, title: "Proton pack", description: "Ghostbusting equipment" },
     { value: 6, title: "Sonic screwdriver", description: "The Time Lord's multi-purpose tool" },
@@ -42,9 +42,13 @@ def index
                 []
               end
 
-    if unselect_items?
-      results.map! do |result|
-        result.merge(selected: false)
+    if allows_selection?
+      results = results.map(&:dup)
+      results.each do |result|
+        if selected_items.any? { |item| result[:title].downcase.include?(item) }
+          result[:selected] = true
+          break if single_select?
+        end
       end
     end
 
@@ -91,11 +95,23 @@ def key_for(uuid)
     "#{COOKIE_PREFIX}#{uuid}"
   end
 
-  def select_items?
-    params.fetch(:select_items, "true") == "true"
+  def selected_items
+    params.fetch(:selected_items, "").downcase.split(",").map(&:strip)
   end
 
-  def unselect_items?
-    !select_items?
+  def select_variant
+    @select_variant ||= params.fetch(:select_variant, Primer::Alpha::SelectPanel::DEFAULT_SELECT_VARIANT).to_sym
+  end
+
+  def single_select?
+    select_variant == :single
+  end
+
+  def multi_select?
+    select_variant == :multiple
+  end
+
+  def allows_selection?
+    Primer::Alpha::SelectPanel::SELECT_VARIANT_OPTIONS.include?(select_variant)
   end
 end

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/dark_dimmed.png

@@ -18,7 +18,7 @@ class SelectPanelPreview < ViewComponent::Preview
       # @param open_on_load toggle
       # @param anchor_align [Symbol] select [start, center, end]
       # @param anchor_side [Symbol] select [outside_bottom, outside_top, outside_left, outside_right]
-      # @param select_items toggle
+      # @param selected_items text
       def playground(
         title: "Sci-fi equipment",
         subtitle: "Various tools from your favorite shows",
@@ -33,11 +33,11 @@ def playground(
         open_on_load: false,
         anchor_align: :start,
         anchor_side: :outside_bottom,
-        select_items: true
+        selected_items: "Phaser"
       )
         render_with_template(locals: {
           subtitle: subtitle,
-          select_items: select_items,
+          selected_items: selected_items,
           system_arguments: {
             title: title,
             size: size,
@@ -65,8 +65,6 @@ def default(open_on_load: false)
         })
       end
 
-      # @!group Fetch strategies
-
       # @label Local fetch
       #
       # @snapshot interactive
@@ -87,8 +85,9 @@ def eventually_local_fetch(open_on_load: false)
       #
       # @snapshot interactive
       # @param open_on_load toggle
-      def remote_fetch(open_on_load: false)
-        render_with_template(locals: { open_on_load: open_on_load })
+      # @param selected_items text
+      def remote_fetch(open_on_load: false, selected_items: "Phaser")
+        render_with_template(locals: { open_on_load: open_on_load, selected_items: selected_items })
       end
 
       # @label Local fetch (no results)
@@ -115,8 +114,6 @@ def remote_fetch_no_results(open_on_load: false)
         render_with_template(locals: { open_on_load: open_on_load })
       end
 
-      # @!endgroup
-
       # @label Single select
       #
       # @snapshot interactive
@@ -245,6 +242,14 @@ def multiselect_form(open_on_load: false, route_format: :html)
       def list_of_links(open_on_load: false)
         render_with_template(locals: { open_on_load: open_on_load })
       end
+
+      # @label No values
+      #
+      # @snapshot interactive
+      # @param open_on_load toggle
+      def no_values(open_on_load: false)
+        render_with_template(locals: { open_on_load: open_on_load })
+      end
     end
   end
 end

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/dark_high_contrast.png

@@ -0,0 +1,19 @@
+<% subject_id = SecureRandom.hex %>
+
+<%= render(Primer::Alpha::SelectPanel.new(
+  data: { interaction_subject: subject_id },
+  select_variant: :single,
+  fetch_strategy: :local,
+  open_on_load: open_on_load
+)) do |panel| %>
+  <% panel.with_show_button { "Choose item" } %>
+  <% panel.with_item(label: "Item 1") %>
+  <% panel.with_item(label: "Item 2") %>
+  <% panel.with_item(label: "Item 3") %>
+  <% panel.with_item(label: "Item 4") %>
+  <% panel.with_footer(show_divider: true) do %>
+    I'm a footer!
+  <% end %>
+<% end %>
+
+<%= render partial: "primer/alpha/select_panel_preview/interaction_subject_js", locals: { subject_id: subject_id } %>

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/light.png

@@ -9,7 +9,7 @@
     select_variant: :single,
     show_results: !simulate_no_results,
     fail: simulate_failure,
-    select_items: select_items
+    selected_items: selected_items
   ),
   select_variant: :single,
   fetch_strategy: :remote,

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/light_colorblind.png

@@ -2,7 +2,10 @@
 
 <%= render(Primer::Alpha::SelectPanel.new(
   data: { interaction_subject: subject_id },
-  src: select_panel_items_path,
+  src: select_panel_items_path(
+    select_variant: :multiple,
+    selected_items: selected_items,
+  ),
   select_variant: :multiple,
   fetch_strategy: :remote,
   open_on_load: open_on_load

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/eventually_local_fetch/light_high_contrast.png

@@ -7,10 +7,10 @@
   open_on_load: open_on_load
 )) do |panel| %>
   <% panel.with_show_button { "Choose item" } %>
-  <% panel.with_item(label: "Item 1", item_id: :item1) %>
-  <% panel.with_item(label: "Item 2", item_id: :item2) %>
-  <% panel.with_item(label: "Item 3", item_id: :item3) %>
-  <% panel.with_item(label: "Item 4", item_id: :item4) %>
+  <% panel.with_item(label: "Item 1", item_id: :item1, content_arguments: { data: { value: 1 } }) %>
+  <% panel.with_item(label: "Item 2", item_id: :item2, content_arguments: { data: { value: 2 } }) %>
+  <% panel.with_item(label: "Item 3", item_id: :item3, content_arguments: { data: { value: 3 } }) %>
+  <% panel.with_item(label: "Item 4", item_id: :item4, content_arguments: { data: { value: 4 } }) %>
   <% panel.with_item(label: "Disabled", disabled: true, item_id: :disabled) %>
   <% panel.with_footer(show_divider: true) do %>
     I'm a footer!

.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/select_panel/no_values/dark.png

@@ -266,6 +266,18 @@ def test_pressing_enter_in_filter_input_navigates_if_first_item_is_link
       assert_current_path "https://github.com"
     end
 
+    def test_selecting_without_data_values
+      visit_preview(:no_values)
+
+      click_on_invoker_button
+      click_on_first_item
+      assert_selector "[aria-selected=true]", text: "Item 1", count: 1, visible: :hidden
+
+      click_on_invoker_button
+      click_on_second_item
+      assert_selector "[aria-selected=true]", text: "Item 2", count: 1, visible: :hidden
+    end
+
     ########## SINGLE SELECT TESTS ############
 
     def test_single_select_item_checked
@@ -471,7 +483,7 @@ def test_single_select_remembers_only_one_checked_item_and_ignores_checked_items
 
     def test_single_select_handles_all_options_unselected_by_default
       # playground is single-select
-      visit_preview(:playground, select_items: false)
+      visit_preview(:playground, selected_items: "")
 
       wait_for_items_to_load do
         click_on_invoker_button
@@ -632,6 +644,18 @@ def test_multi_select_does_not_allow_server_to_check_items_on_filter_if_selectio
       assert_selector "[aria-checked=true]", text: "Photon torpedo"
     end
 
+    def test_multi_select_allows_server_to_check_multiple_items
+      # "ph" should match two items, i.e. the server should respond with two checked items
+      visit_preview(:remote_fetch, selected_items: "ph")
+
+      wait_for_items_to_load do
+        click_on_invoker_button
+      end
+
+      assert_selector "[aria-checked=true]", text: "Phaser"
+      assert_selector "[aria-checked=true]", text: "Photon torpedo"
+    end
+
     ########## JAVASCRIPT API TESTS ############
 
     def test_disable_item_via_js_api