fix multiselect combobox

2026-07-13 15:21:42 -04:00
parent d52151cc1a
commit 5230bd6702
11 changed files with 664 additions and 58 deletions
--- a/go/webui/forms.go
+++ b/go/webui/forms.go
@@ -815,6 +815,11 @@ func FormTimezoneSelector(p FormSelectProps) *vdom.VNode {
 type dropdown struct {
 	f *Floating

+	// multi drives the parts of a row that differ between picking ONE thing and picking
+	// SEVERAL: a multi-select's rows carry a checkbox, because the row's job there is to
+	// show a state you toggle, not an item you pick.
+	multi bool
+
 	search    *vdom.Signal[string]
 	searchRef *vdom.Ref
 	// active is the keyboard-highlighted option, as an index into the FILTERED list.
@@ -830,8 +835,9 @@ type DropdownOptions struct {
 	OnOpenChange func(bool)
 }

-func newDropdown(o DropdownOptions, closeOnSelect bool) *dropdown {
+func newDropdown(o DropdownOptions, multi bool) *dropdown {
 	d := &dropdown{
+		multi:     multi,
 		search:    vdom.NewSignal(""),
 		searchRef: vdom.NewRef(),
 		active:    vdom.NewSignal(-1),
@@ -858,7 +864,6 @@ func newDropdown(o DropdownOptions, closeOnSelect bool) *dropdown {
 			}
 		},
 	})
-	_ = closeOnSelect
 	return d
 }

@@ -963,9 +968,32 @@ func (d *dropdown) optionButton(opt FormSelectOption, idx int, selected, onDark,
 	} else if onClick != nil {
 		mods = append(mods, vdom.On(vdom.EVENT_CLICK, onClick))
 	}
+
+	if d.multi {
+		mods = append(mods, optionCheckbox(selected))
+	}
 	return vdom.Button(append(mods, vdom.Text(opt.Label))...)
 }

+// optionCheckbox is the tick on a multi-select row.
+//
+// It is inert: pointer-events are off and it has no change handler, so the click always
+// lands on the row BUTTON that owns it. A checkbox that could be clicked independently
+// of its row would let the two disagree about what is selected.
+//
+// `checked` is a PROP, not an attribute. The attribute is only the DOM's initial value;
+// once the browser owns the property, writing the attribute again does nothing — so an
+// attribute-driven tick would be right on the first paint and then never change again.
+func optionCheckbox(selected bool) vdom.Mod {
+	return vdom.Input(
+		vdom.Attr("type", "checkbox"),
+		vdom.Attr("tabindex", "-1"),
+		vdom.Attr("aria-hidden", "true"),
+		vdom.Attr("class", "pointer-events-none shrink-0"),
+		vdom.BoolProp("checked", selected),
+	)
+}
+
 func (d *dropdown) noResults(onDark bool) *vdom.VNode {
 	cls := formDropdownNoResults
 	if onDark {
@@ -1015,7 +1043,7 @@ type Combobox struct{ *dropdown }

 // NewCombobox creates a single-select dropdown.
 func NewCombobox(o DropdownOptions) *Combobox {
-	return &Combobox{dropdown: newDropdown(o, true)}
+	return &Combobox{dropdown: newDropdown(o, false)} // picks one — no checkboxes
 }

 // FormComboboxProps configures Combobox.Render.
@@ -1117,11 +1145,23 @@ func NewSearchableSelect(o DropdownOptions) *Combobox { return NewCombobox(o) }
 // click, on Escape, or when its trigger is clicked again.
 //
 // Create it once, alongside your signals — never inside a render function.
-type MultiSelect struct{ *dropdown }
+type MultiSelect struct {
+	*dropdown
+
+	// tagsRef / countRef are the two faces of the trigger: the row of pills, and the
+	// "N items selected" summary that replaces them when they do not fit. Both are
+	// always rendered; which one is visible is decided by measurement, not by a signal.
+	tagsRef  *vdom.Ref
+	countRef *vdom.Ref
+}

 // NewMultiSelect creates a multi-select dropdown.
 func NewMultiSelect(o DropdownOptions) *MultiSelect {
-	return &MultiSelect{dropdown: newDropdown(o, false)}
+	return &MultiSelect{
+		dropdown: newDropdown(o, true), // picks several — every row gets a checkbox
+		tagsRef:  vdom.NewRef(),
+		countRef: vdom.NewRef(),
+	}
 }

 // FormMultiSelectProps configures MultiSelect.Render.
@@ -1154,43 +1194,6 @@ func (m *MultiSelect) Render(p FormMultiSelectProps) *vdom.VNode {
 		}
 	}

-	// The trigger's face: a placeholder, tags, or "N selected" once there are too many
-	// to show without the field growing unboundedly.
-	var face *vdom.VNode
-	switch {
-	case len(selected) == 0:
-		face = vdom.Span(vdom.Attr("class", "text-neutral-500 truncate"),
-			vdom.Text(pick(p.Placeholder, "Select options")))
-	case len(selected) > maxTags:
-		face = vdom.Span(vdom.Attr("class", "truncate"),
-			vdom.Text(strconv.Itoa(len(selected))+" selected"))
-	default:
-		tags := []vdom.Mod{vdom.Attr("class", "flex flex-wrap items-center gap-1 min-w-0")}
-		for _, opt := range selected {
-			ov := opt.Value
-			remove := []vdom.Mod{
-				vdom.Attr("type", "button"),
-				vdom.Attr("class", "bg-transparent border-none p-0 cursor-pointer flex items-center hover:text-neutral-900"),
-				vdom.Attr("aria-label", "Remove "+opt.Label),
-				IconInline("xmark", 10, ""),
-			}
-			if p.OnChange != nil {
-				oc, cur := p.OnChange, p.Value
-				remove = append(remove, vdom.OnEvent(vdom.EVENT_CLICK, func(e vdom.Event) {
-					// The tag sits INSIDE the trigger, so without this the click would
-					// bubble up and toggle the panel open as it removed the tag.
-					e.StopPropagation()
-					oc(formRemoveStr(cur, ov))
-				}))
-			}
-			tags = append(tags, vdom.Span(vdom.Attr("class", formMultiSelectTag),
-				vdom.Text(opt.Label),
-				vdom.Button(remove...),
-			))
-		}
-		face = vdom.Div(tags...)
-	}
-
 	pad := "p-2"
 	if p.Small {
 		pad = "p-1"
@@ -1200,11 +1203,14 @@ func (m *MultiSelect) Render(p FormMultiSelectProps) *vdom.VNode {
 		chevron = "chevron-up"
 	}

+	face := append([]vdom.Mod{vdom.Attr("class", "flex-1 overflow-hidden flex items-center min-w-0")},
+		m.triggerFace(p, selected, maxTags)...)
+
 	trigger := m.f.Trigger(FloatingTriggerProps{
 		Class:        cx(formTriggerBase, "overflow-hidden", formControlH(p.Small), pad),
 		AriaHasPopup: "listbox",
 	},
-		vdom.Div(vdom.Attr("class", "flex-1 overflow-hidden flex items-center min-w-0"), face),
+		vdom.Div(face...),
 		IconInline(chevron, 16, "text-neutral-400"),
 	)
 	if p.Disabled {
@@ -1270,7 +1276,121 @@ func (m *MultiSelect) Render(p FormMultiSelectProps) *vdom.VNode {
 	)
 }

-const formMultiSelectTag = "inline-flex items-center gap-1 rounded-default bg-neutral-100 px-1.5 py-0.5 text-xs text-neutral-700 max-w-full"
+// triggerFace is what the closed field shows: a placeholder, a row of removable pills,
+// or "N items selected" when the pills will not fit.
+//
+// It renders the pills AND the summary every time, and lets a measurement decide which
+// one is displayed — because "will not fit" is a question only the browser can answer,
+// and answering it by writing a signal would re-render the tree that the answer depends
+// on. See collapseFace.
+func (m *MultiSelect) triggerFace(p FormMultiSelectProps, selected []FormSelectOption, maxTags int) []vdom.Mod {
+	if len(selected) == 0 {
+		return []vdom.Mod{vdom.Span(
+			vdom.Attr("class", "text-neutral-500 truncate"),
+			vdom.Text(pick(p.Placeholder, "Select options")),
+		)}
+	}
+
+	// Too many to be worth showing as pills at all. This needs no measurement, so it is
+	// decided here in the markup and holds on the SERVER too — SSR gets the summary
+	// rather than a row of clipped pills it would then have to swap out.
+	tooMany := len(selected) > maxTags
+
+	pillCls := formMultiSelectTag
+	if p.Small {
+		pillCls = cx(pillCls, "px-1.5 text-xs")
+	} else {
+		pillCls = cx(pillCls, "px-2 text-sm")
+	}
+
+	// flex-nowrap + overflow-hidden is what makes the overflow MEASURABLE: the pills lay
+	// out on one line and are clipped, so scrollWidth exceeds clientWidth by exactly the
+	// amount that does not fit. With flex-wrap they would simply wrap and never overflow
+	// horizontally, and nothing would ever collapse.
+	tags := []vdom.Mod{
+		vdom.WithRef(m.tagsRef),
+		vdom.Attr("class", faceCls("flex flex-nowrap items-center gap-1 min-w-0 overflow-hidden", tooMany)),
+	}
+	for _, opt := range selected {
+		ov, label := opt.Value, opt.Label
+		remove := []vdom.Mod{
+			vdom.Attr("type", "button"),
+			vdom.Attr("class", "bg-transparent border-none p-0 cursor-pointer flex items-center hover:text-neutral-900"),
+			vdom.Attr("aria-label", "Remove "+label),
+			IconInline("xmark", 10, ""),
+		}
+		if p.OnChange != nil {
+			oc, cur := p.OnChange, p.Value
+			remove = append(remove, vdom.OnEvent(vdom.EVENT_CLICK, func(e vdom.Event) {
+				// The pill sits INSIDE the trigger, so without this the click would bubble
+				// up and open the panel as it removed the pill.
+				e.StopPropagation()
+				oc(formRemoveStr(cur, ov))
+			}))
+		}
+		tags = append(tags, vdom.Span(vdom.Attr("class", pillCls),
+			vdom.Text(label),
+			vdom.Button(remove...),
+		))
+	}
+
+	count := vdom.Span(
+		vdom.WithRef(m.countRef),
+		vdom.Attr("class", faceCls("truncate", !tooMany)),
+		vdom.Text(itemsSelected(len(selected))),
+	)
+
+	m.collapseFace(tooMany)
+	return []vdom.Mod{vdom.Div(tags...), count}
+}
+
+// collapseFace picks which face is displayed, after the DOM exists.
+//
+// The choice is written with SetStyle, not a signal: a signal write re-renders the
+// whole tree, and this decision is DERIVED from that tree's layout — the render would
+// feed the measurement that triggers the render. Doing it imperatively also means the
+// pills and the summary can both be in the DOM at once without ever both being seen.
+//
+// AfterRender runs after the commit but before the browser paints, so every write here
+// — including the deliberate flash of the pills back into view to measure them — lands
+// in the same frame. Nothing is visible mid-decision.
+func (m *MultiSelect) collapseFace(tooMany bool) {
+	wasmruntime.AfterRender(func() {
+		collapse := tooMany
+		if !collapse {
+			// Measure from a known state. A previous render may have left the pills
+			// display:none, and a hidden element has no layout — it would report no
+			// overflow and we would wrongly un-collapse.
+			wasmruntime.SetStyle(m.tagsRef, "display", "flex")
+			wasmruntime.SetStyle(m.countRef, "display", "none")
+			if !wasmruntime.OverflowsX(m.tagsRef) {
+				return
+			}
+		}
+		wasmruntime.SetStyle(m.tagsRef, "display", "none")
+		wasmruntime.SetStyle(m.countRef, "display", "block")
+	})
+}
+
+// faceCls hides one of the two faces by class, which is what the SERVER renders (it
+// cannot measure, so it ships the unmeasured choice). On the client the inline styles
+// written by collapseFace take over, and they win over a class.
+func faceCls(cls string, hidden bool) string {
+	if hidden {
+		return cx(cls, "hidden")
+	}
+	return cls
+}
+
+// itemsSelected is the collapsed summary: "1 item selected", "4 items selected".
+func itemsSelected(n int) string {
+	if n == 1 {
+		return "1 item selected"
+	}
+	return strconv.Itoa(n) + " items selected"
+}
+
+const formMultiSelectTag = "inline-flex items-center gap-0.5 rounded-default bg-neutral-200 py-0.5 text-neutral-700 whitespace-nowrap leading-none"

 func truncateRunes(s string, max int) string {
 	if max <= 0 {
--- a/go/webui/forms_dropdown_test.go
+++ b/go/webui/forms_dropdown_test.go
@@ -1,6 +1,7 @@
 package webui

 import (
+	"strconv"
 	"strings"
 	"testing"

@@ -183,6 +184,174 @@ func TestDropdownsAreStandalone(t *testing.T) {
 	}
 }

+// ---- the panel rows ----
+
+// A multi-select row shows a CHECKBOX. Its job is to show a state you toggle, not an
+// item you pick, and the tick is the only thing that says which is which.
+//
+// The unified dropdown dropped it: both kinds of row rendered as a bare label, so a
+// multi-select gave no indication of what was already selected.
+func TestMultiSelectOptionsHaveCheckboxes(t *testing.T) {
+	m := NewMultiSelect(DropdownOptions{})
+	m.Open()
+
+	node := m.Render(FormMultiSelectProps{
+		Options: dropdownOptions(),
+		Value:   []string{"res"}, // Research is selected; the other two are not
+	})
+
+	for _, want := range []struct {
+		label   string
+		checked bool
+	}{
+		{"Engineering", false},
+		{"Research", true},
+		{"Networking", false},
+	} {
+		box := findCheckboxIn(optionRow(t, node, want.label))
+		if box == nil {
+			t.Errorf("option %q has no checkbox", want.label)
+			continue
+		}
+		if got := box.Props["checked"]; got != strconv.FormatBool(want.checked) {
+			t.Errorf("option %q: checked=%q, want %v", want.label, got, want.checked)
+		}
+	}
+}
+
+// ...and a combobox row does NOT. It picks one thing; a checkbox would promise a
+// multiple selection it will not honour.
+func TestComboboxOptionsHaveNoCheckboxes(t *testing.T) {
+	c := NewCombobox(DropdownOptions{})
+	c.Open()
+
+	node := c.Render(FormComboboxProps{Options: dropdownOptions(), Value: "res"})
+	if findCheckboxIn(optionRow(t, node, "Research")) != nil {
+		t.Error("a single-select option must not render a checkbox")
+	}
+}
+
+// ---- the collapsed trigger ----
+
+// Past maxTagsBeforeCollapse the field stops showing pills and says how many are
+// selected. Without this the trigger either grows without bound or silently clips the
+// selection, and the user cannot tell how much they picked.
+func TestMultiSelectCollapsesToACount(t *testing.T) {
+	m := NewMultiSelect(DropdownOptions{})
+	html := vdom.RenderHTML(m.Render(FormMultiSelectProps{
+		Options:               manyOptions(6),
+		Value:                 []string{"o1", "o2", "o3", "o4"}, // 4 > the default max of 3
+		MaxTagsBeforeCollapse: 3,
+	}))
+
+	if !strings.Contains(html, "4 items selected") {
+		t.Errorf("collapsed trigger does not say how many are selected:\n%s", html)
+	}
+	// The count is the SERVER's choice too — this needs no measurement, so SSR must not
+	// ship a row of clipped pills that the client then has to swap out.
+	if !strings.Contains(html, `class="flex flex-nowrap items-center gap-1 min-w-0 overflow-hidden hidden"`) {
+		t.Errorf("the pills were not hidden server-side:\n%s", html)
+	}
+}
+
+// One is one ITEM, not one items.
+func TestCollapsedCountIsPluralised(t *testing.T) {
+	if got := itemsSelected(1); got != "1 item selected" {
+		t.Errorf("itemsSelected(1) = %q", got)
+	}
+	if got := itemsSelected(4); got != "4 items selected" {
+		t.Errorf("itemsSelected(4) = %q", got)
+	}
+}
+
+// Under the limit, the pills show and the summary is the hidden one. Both are always in
+// the DOM: which is visible is settled by a measurement after render (collapseFace),
+// and that measurement needs something to measure.
+func TestMultiSelectShowsPillsWhenTheyFit(t *testing.T) {
+	m := NewMultiSelect(DropdownOptions{})
+	html := vdom.RenderHTML(m.Render(FormMultiSelectProps{
+		Options:               dropdownOptions(),
+		Value:                 []string{"eng", "res"},
+		MaxTagsBeforeCollapse: 3,
+	}))
+
+	if !strings.Contains(html, "Engineering") || !strings.Contains(html, "Research") {
+		t.Errorf("the selected pills are missing:\n%s", html)
+	}
+	if !strings.Contains(html, `class="truncate hidden"`) {
+		t.Errorf("the count summary should be the hidden face here:\n%s", html)
+	}
+	// It must be RENDERED though, or there is nothing for collapseFace to reveal.
+	if !strings.Contains(html, "2 items selected") {
+		t.Errorf("the count summary was not rendered at all:\n%s", html)
+	}
+}
+
+// The pills must lay out on ONE line and clip. With flex-wrap they wrap instead, so
+// scrollWidth never exceeds clientWidth, and the overflow collapse can never fire.
+func TestPillsAreNowrapSoOverflowIsMeasurable(t *testing.T) {
+	m := NewMultiSelect(DropdownOptions{})
+	html := vdom.RenderHTML(m.Render(FormMultiSelectProps{
+		Options: dropdownOptions(),
+		Value:   []string{"eng"},
+	}))
+	if strings.Contains(html, "flex-wrap") {
+		t.Error("the pill row wraps; overflow would never be detected")
+	}
+	if !strings.Contains(html, "flex-nowrap") || !strings.Contains(html, "overflow-hidden") {
+		t.Errorf("the pill row must be flex-nowrap + overflow-hidden:\n%s", html)
+	}
+}
+
+func manyOptions(n int) []FormSelectOption {
+	out := make([]FormSelectOption, n)
+	for i := range out {
+		id := strconv.Itoa(i + 1)
+		out[i] = FormSelectOption{Value: "o" + id, Label: "Option " + id}
+	}
+	return out
+}
+
+// optionRow returns the panel row whose label is label.
+func optionRow(t *testing.T, n *vdom.VNode, label string) *vdom.VNode {
+	t.Helper()
+	if row := findRow(n, label); row != nil {
+		return row
+	}
+	t.Fatalf("no option row labelled %q", label)
+	return nil
+}
+
+func findRow(n *vdom.VNode, label string) *vdom.VNode {
+	if n == nil {
+		return nil
+	}
+	if n.Tag == "button" && n.Attrs["role"] == "option" && nodeText(n) == label {
+		return n
+	}
+	for _, c := range n.Children {
+		if row := findRow(c, label); row != nil {
+			return row
+		}
+	}
+	return nil
+}
+
+func findCheckboxIn(n *vdom.VNode) *vdom.VNode {
+	if n == nil {
+		return nil
+	}
+	if n.Tag == "input" && n.Attrs["type"] == "checkbox" {
+		return n
+	}
+	for _, c := range n.Children {
+		if box := findCheckboxIn(c); box != nil {
+			return box
+		}
+	}
+	return nil
+}
+
 // clickOption finds an option button by its label and invokes its click handler.
 func clickOption(t *testing.T, n *vdom.VNode, label string) {
 	t.Helper()