Et2Select: Fix selecting a second search result could remove first one

api/js/etemplate/Et2Select/Et2Select.ts

@@ -18,6 +18,7 @@ import {Et2WithSearchMixin} from "./SearchMixin";
 import {property} from "lit/decorators/property.js";
 import {SlChangeEvent, SlOption, SlSelect} from "@shoelace-style/shoelace";
 import {repeat} from "lit/directives/repeat.js";
+import {classMap} from "lit/directives/class-map.js";
 
 // export Et2WidgetWithSelect which is used as type in other modules
 export class Et2WidgetWithSelect extends RowLimitedMixin(Et2WidgetWithSelectMixin(LitElement))
@@ -703,7 +704,8 @@ export class Et2Select extends Et2WithSearchMixin(Et2WidgetWithSelect)
 	protected _optionTemplate(option : SelectOption) : TemplateResult
 	{
 		// Exclude non-matches when searching
-		if(typeof option.isMatch == "boolean" && !option.isMatch)
+		// unless they're already selected, in which case removing them removes them from value
+		if(typeof option.isMatch == "boolean" && !option.isMatch && !this.getValueAsArray().includes(option.value))
 		{
 			return html``;
 		}
@@ -716,7 +718,12 @@ export class Et2Select extends Et2WithSearchMixin(Et2WidgetWithSelect)
                     part="option"
                     value="${value}"
                     title="${!option.title || this.noLang ? option.title : this.egw().lang(option.title)}"
-                    class="${option.class}" .option=${option}
+                    class=${classMap({
+                        "match": option.isMatch,
+                        "no-match": !option.isMatch,
+                        ...Object.fromEntries((option.class || "").split(" ").map(k => [k, true]))
+                    })}
+                    .option=${option}
                     .selected=${this.getValueAsArray().some(v => v == value)}
                     ?disabled=${option.disabled}
             >

api/js/etemplate/Et2Select/SearchMixin.ts

@@ -14,6 +14,7 @@ import {Et2Tag} from "./Tag/Et2Tag";
 import {StaticOptions} from "./StaticOptions";
 import {dedupeMixin} from "@open-wc/dedupe-mixin";
 import {SlOption} from "@shoelace-style/shoelace";
+import {Et2Textbox} from "../Et2Textbox/Et2Textbox";
 
 // Otherwise import gets stripped
 let keep_import : Et2Tag;
@@ -327,6 +328,16 @@ export const Et2WithSearchMixin = dedupeMixin(<T extends Constructor<LitElement>
 			this._unbindListeners();
 		}
 
+		async getUpdateComplete()
+		{
+			const result = super.getUpdateComplete();
+			if(this._searchInputNode)
+			{
+				await this._searchInputNode.updateComplete;
+			}
+			return result;
+		}
+
 		willUpdate(changedProperties)
 		{
 			super.willUpdate(changedProperties);
@@ -484,7 +495,7 @@ export const Et2WithSearchMixin = dedupeMixin(<T extends Constructor<LitElement>
 			return !this.readonly && (this.search || this.searchUrl.length > 0);
 		}
 
-		protected get _searchInputNode() : HTMLInputElement
+		protected get _searchInputNode() : Et2Textbox
 		{
 			return this._activeControls?.querySelector("#search");
 		}
@@ -1034,7 +1045,7 @@ export const Et2WithSearchMixin = dedupeMixin(<T extends Constructor<LitElement>
 			this.select.appendChild(spinner);
 
 			// Hide clear button
-			let clear_button = <HTMLElement>this._searchInputNode.shadowRoot.querySelector(".input__clear")
+			let clear_button = <HTMLElement>this._searchInputNode?.shadowRoot?.querySelector(".input__clear");
 			if(clear_button)
 			{
 				clear_button.style.display = "none";
@@ -1049,7 +1060,7 @@ export const Et2WithSearchMixin = dedupeMixin(<T extends Constructor<LitElement>
 			return Promise.all([
 				this.localSearch(this._searchInputNode.value, this.searchOptions),
 				this.remoteSearch(this._searchInputNode.value, this.searchOptions)
-			]).then(() =>
+			]).then(async() =>
 			{
 				// Remove spinner
 				spinner.remove();
@@ -1059,6 +1070,7 @@ export const Et2WithSearchMixin = dedupeMixin(<T extends Constructor<LitElement>
 				{
 					clear_button.style.display = "";
 				}
+				await this.updateComplete;
 			});
 		}
 

api/js/etemplate/Et2Select/test/SearchActions.test.ts

@@ -3,7 +3,7 @@
  * Currently just checking to make sure onchange is only called once.
  */
 import {SelectOption} from "../FindSelectOptions";
-import {assert, elementUpdated, fixture, html} from '@open-wc/testing';
+import {assert, elementUpdated, fixture, html, oneEvent} from '@open-wc/testing';
 import * as sinon from 'sinon';
 import {Et2Box} from "../../Layout/Et2Box/Et2Box";
 import {Et2Select} from "../Et2Select";
@@ -14,8 +14,11 @@ let keep_import : Et2Textbox = null;
 // Stub global egw for cssImage to find
 // @ts-ignore
 window.egw = {
+	ajaxUrl: url => url,
+	decodePath: url => url,
 	//image: () => "data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4NCjwhLS0gR2VuZXJhdG9yOiBBZG9iZSBJbGx1c3RyYXRvciAxNS4wLjAsIFNWRyBFeHBvcnQgUGx1Zy1JbiAuIFNWRyBWZXJzaW9uOiA2LjAwIEJ1aWxkIDApICAtLT4NCjwhRE9DVFlQRSBzdmcgUFVCTElDICItLy9XM0MvL0RURCBTVkcgMS4xLy9FTiIgImh0dHA6Ly93d3cudzMub3JnL0dyYXBoaWNzL1NWRy8xLjEvRFREL3N2ZzExLmR0ZCI+DQo8c3ZnIHZlcnNpb249IjEuMSIgaWQ9IkViZW5lXzEiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgeG1sbnM6eGxpbms9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGxpbmsiIHg9IjBweCIgeT0iMHB4Ig0KCSB3aWR0aD0iMzJweCIgaGVpZ2h0PSIzMnB4IiB2aWV3Qm94PSIwIDAgMzIgMzIiIGVuYWJsZS1iYWNrZ3JvdW5kPSJuZXcgMCAwIDMyIDMyIiB4bWw6c3BhY2U9InByZXNlcnZlIj4NCjxwYXRoIGZpbGwtcnVsZT0iZXZlbm9kZCIgY2xpcC1ydWxlPSJldmVub2RkIiBmaWxsPSIjNjk2OTY5IiBkPSJNNi45NDMsMjguNDUzDQoJYzAuOTA2LDAuNzY1LDIuMDk3LDEuMTI3LDMuMjg2LDEuMTA5YzAuNDMsMC4wMTQsMC44NTItMC4wNjgsMS4yNjUtMC4yMDdjMC42NzktMC4xOCwxLjMyOC0wLjQ1LDEuODY2LTAuOTAyTDI5LjQwMywxNC45DQoJYzEuNzcyLTEuNDk4LDEuNzcyLTMuOTI1LDAtNS40MjJjLTEuNzcyLTEuNDk3LTQuNjQ2LTEuNDk3LTYuNDE4LDBMMTAuMTE5LDIwLjM0OWwtMi4zODktMi40MjRjLTEuNDQtMS40NTctMy43NzItMS40NTctNS4yMTIsMA0KCWMtMS40MzgsMS40Ni0xLjQzOCwzLjgyNSwwLDUuMjgxQzIuNTE4LDIzLjIwNiw1LjQ3NCwyNi45NDcsNi45NDMsMjguNDUzeiIvPg0KPC9zdmc+DQo=",
 	lang: i => i + "*",
+	link: l => l,
 	tooltipUnbind: () => {},
 	webserverUrl: "",
 	window: window
@@ -72,8 +75,10 @@ describe("Search actions", () =>
 		element.onchange = change;
 
 		await elementUpdated(element);
-
+		const option = element.select.querySelector("[value='two']");
-		element.select.querySelector("[value='two']").dispatchEvent(new Event("click"));
+		const listener = oneEvent(option, "mouseup");
+		option.dispatchEvent(new Event("mouseup", {bubbles: true}));
+		await listener;
 
 		await elementUpdated(element);
 
@@ -171,3 +176,196 @@ describe("Trigger search", () =>
 		assert(abortSpy.calledOnce, "_handleSearchAbort() was not called");
 	})
 });
+
+async function doSearch(element, search)
+{
+	// we need to explicitly set the value
+	element._searchInputNode.value = search;
+
+	await element.startSearch();
+
+	await elementUpdated(element)
+};
+
+describe("Search results", () =>
+{
+	let element : Et2Select;
+	const remote_results = [
+		{value: "remote_one", label: "remote_one"},
+		{value: "remote_two", label: "remote_two"}
+	];
+	let clickOption = (value) =>
+	{
+		const option = element.select.querySelector("[value='" + value + "']");
+		let listener = oneEvent(option, "mouseup");
+		option.dispatchEvent(new Event("mouseup", {bubbles: true}));
+		return listener;
+	}
+
+	// Setup run before each test
+	beforeEach(async() =>
+	{
+		// Create an element to test with, and wait until it's ready
+		// @ts-ignore
+		element = await fixture<Et2Select>(html`
+            <et2-select label="I'm a select" search>
+                <option value="one">One</option>
+                <option value="two">Two</option>
+                <option value="three">Three</option>
+                <option value="four">Four</option>
+                <option value="five">Five</option>
+                <option value="six">Six</option>
+                <option value="seven">Seven</option>
+            </et2-select>
+		`);
+		element.loadFromXML(element);
+		// Stub egw()
+		sinon.stub(element, "egw").returns(window.egw);
+
+		await element.updateComplete;
+		await element._searchInputNode.updateComplete;
+		await elementUpdated(element);
+	});
+
+	it("Correct local results", async() =>
+	{
+		// Search
+		await doSearch(element, "one")
+		// Check the result is offered
+		const option = element.select.querySelector("[value='one']")
+		assert.isNotNull(option, "Did not find option in result");
+
+		// _only_ that one?
+		assert.sameMembers(Array.from(element.select.querySelectorAll("sl-option")).map(e => e.value), ["one"], "Unexpected search results");
+	});
+	it("Correct remote results", async() =>
+	{
+		// Enable searching
+		element.searchUrl = "test";
+
+		// Fake remote search
+		window.egw.request = sinon.fake
+			.returns(Promise.resolve([remote_results[0]]));
+
+		// Search
+		await doSearch(element, "remote_one")
+
+		// Check the result is offered
+		const option = element.select.querySelector("[value='remote_one']")
+		assert.isNotNull(option, "Did not find option in result");
+
+		// _only_ that one?
+		// N.B. that "one" will stay, since that's the current value
+		assert.sameMembers(Array.from(element.select.querySelectorAll("sl-option.remote")).map(e => e.value), ["remote_one"], "Unexpected search results");
+	});
+	it("Correct local and remote together", async() =>
+	{
+		// Enable searching
+		element.searchUrl = "test";
+
+		// Fake remote search
+		window.egw.request = sinon.fake
+			.returns(Promise.resolve([remote_results[0]]));
+
+		// Search
+		await doSearch(element, "one")
+
+		// Check the result is offered
+		const local_option = element.select.querySelector("[value='one']")
+		assert.isNotNull(local_option, "Did not find local option in result");
+		const remote_option = element.select.querySelector("[value='remote_one']")
+		assert.isNotNull(remote_option, "Did not find remote option in result");
+
+		// _only_ that one?
+		assert.sameMembers(Array.from(element.select.querySelectorAll("sl-option")).map(e => e.value), ["one", "remote_one"], "Unexpected search results");
+	});
+	it("Selected local result is in value", async() =>
+	{
+		// Search
+		await doSearch(element, "one")
+
+		// "Click" that one
+		await clickOption("one");
+		await element.updateComplete;
+
+		assert.equal(element.value, "one", "Selected search result was not in value");
+	});
+	it("Selected remote result in value", async() =>
+	{
+		// Enable searching
+		element.searchUrl = "test";
+
+		// Fake remote search
+		window.egw.request = sinon.fake
+			.returns(Promise.resolve([remote_results[0]]));
+
+		// Search
+		await doSearch(element, "remote_one")
+
+		// Click
+		await clickOption("remote_one");
+		await element.updateComplete;
+
+		assert.equal(element.value, "remote_one", "Selected search result was not in value");
+	});
+	it("Selected multiple remote results in value", async() =>
+	{
+		// Enable multiple
+		element.multiple = true;
+
+		// Enable searching
+		element.searchUrl = "test";
+
+		// Fake remote search
+		window.egw.request = sinon.fake
+			.returns(Promise.resolve(remote_results));
+
+		// Search
+		await doSearch(element, "doesn't matter, we're faking it")
+
+		// Click
+		const values = ["remote_one", "remote_two"];
+		let listener;
+		values.forEach(value =>
+		{
+			listener = clickOption(value);
+		});
+		await listener;
+		await element.updateComplete;
+
+		assert.deepEqual(element.value, values, "Selected search results were not in value");
+	});
+	it("Adding (multiple) remote keeps value", async() =>
+	{
+		const values = ["remote_one", "remote_two"];
+
+		// Enable multiple
+		element.multiple = true;
+
+		// Clear value ("one" was selected automatically)
+		element.value = "";
+		await element.updateComplete;
+
+		// Enable searching
+		element.searchUrl = "test";
+
+		// Fake remote search
+		window.egw.request = sinon.fake
+			.returns(Promise.resolve(remote_results));
+
+		// Search
+		await doSearch(element, "doesn't matter, we're faking it")
+
+		debugger;
+		// Select the first one
+		await clickOption("remote_one");
+		await element.updateComplete;
+
+		// Search & select another one
+		await doSearch(element, "doesn't matter, we're faking it");
+		await clickOption("remote_two");
+		await element.updateComplete;
+
+		assert.deepEqual(element.value, values, "Selected search results were not in value");
+	});
+});