Skip to content

feat(ui5-toolbar-select): add value prop to ToolbarSelectOption and selectedToolbarOption in change event#13804

Open
NakataCode wants to merge 2 commits into
mainfrom
toolbar-select-option-value
Open

feat(ui5-toolbar-select): add value prop to ToolbarSelectOption and selectedToolbarOption in change event#13804
NakataCode wants to merge 2 commits into
mainfrom
toolbar-select-option-value

Conversation

@NakataCode

Copy link
Copy Markdown
Contributor

The ui5-toolbar-select component provided no reliable way to identify which option was selected from the change event. The e.detail.selectedOption was an internal ui5-option element with no value and no access to the original ui5-toolbar-select-option's data-* attributes.

Changes

  • Add value property to ToolbarSelectOption — mirrors the ui5-option API
  • Pass value through to the internal ui5-option in the template so e.detail.selectedOption.value works correctly
  • Add selectedToolbarOption to the change event detail — references the original ui5-toolbar-select-option element, giving full access to value, data-* attributes, and any other custom metadata
  • Update _clearSiblingsAndSync to use value when set, falling back to textContent for backwards compatibility with options that have no value

Related to: #13609

@NakataCode NakataCode temporarily deployed to netlify-preview July 9, 2026 11:25 — with GitHub Actions Inactive
@sap-ui5-webcomponents-release

Copy link
Copy Markdown

@PetyaMarkovaBogdanova PetyaMarkovaBogdanova self-requested a review July 13, 2026 07:34

@PetyaMarkovaBogdanova PetyaMarkovaBogdanova left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVED (Minor suggestions)

Code Quality: Excellent — clean, focused changes with good symmetry between APIs
Test Coverage: Strong — covers the main use case and backwards compatibility
Backwards Compatibility: Perfect — purely additive
API Design: Well-considered — dual pathways (selectedOption.value and selectedToolbarOption) serve different consumers
The PR successfully solves the stated problem with minimal, well-tested changes. The new value property on ToolbarSelectOption mirrors existing UI5 patterns, and exposing selectedToolbarOption provides an elegant solution for scenarios with duplicate option text.

Suggested edge-case tests:

// Edge Case Tests: Internal Select State Updates
it("Should sync when internal Select value is updated programmatically", () => {
cy.mount(
<>


Option A
Option B
Option C



</>
);

	// Listen for change events
	cy.get("[ui5-toolbar-select]").then($select => {
		$select.get(0).addEventListener("ui5-change", (e: Event) => {
			const ce = e as CustomEvent;
			(document.querySelector("[data-testid='internal-sync-result']") as HTMLInputElement).value = 
				ce.detail.selectedToolbarOption.value;
		});
	});

	// Access internal Select and update its value
	cy.get("[ui5-toolbar-select]").then($toolbarSelect => {
		const internalSelect = ($toolbarSelect.get(0) as any).shadowRoot?.querySelector("[ui5-select]");
		if (internalSelect) {
			// Directly update the internal Select's value
			internalSelect.value = "opt-c";
			// Trigger change event manually
			const changeEvent = new Event("ui5-change", { bubbles: true });
			Object.defineProperty(changeEvent, "detail", {
				value: {
					selectedOption: internalSelect.querySelector('[value="opt-c"]')
				},
				enumerable: true
			});
			internalSelect.dispatchEvent(changeEvent);
		}
	});

	// Wait a bit for event to propagate
	cy.wait(100);

	// Verify the toolbar select processed the change
	cy.get("[data-testid='internal-sync-result']").should("have.prop", "value", "opt-c");
});
it("Should maintain correct selectedToolbarOption when internal Select changes selection", () => {
	cy.mount(
		<>
			<Toolbar>
				<ToolbarSelect id="ts-verify-sync">
					<ToolbarSelectOption id="opt-1" value="value1">First</ToolbarSelectOption>
					<ToolbarSelectOption id="opt-2" value="value2" selected>Second</ToolbarSelectOption>
					<ToolbarSelectOption id="opt-3" value="value3">Third</ToolbarSelectOption>
				</ToolbarSelect>
			</Toolbar>
			<div data-testid="selected-element-id"></div>
		</>
	);

	cy.get("[ui5-toolbar-select]").then($select => {
		$select.get(0).addEventListener("ui5-change", (e: Event) => {
			const ce = e as CustomEvent;
			const selectedToolbarOption = ce.detail.selectedToolbarOption as HTMLElement;
			const testDiv = document.querySelector("[data-testid='selected-element-id']") as HTMLElement;
			testDiv.setAttribute("data-selected-id", selectedToolbarOption.id);
			testDiv.setAttribute("data-selected-value", selectedToolbarOption.value || "");
		});
	});

	// Trigger selection through UI
	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realClick();

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("ArrowDown");

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("Enter");

	// Verify correct element is referenced
	cy.get("[data-testid='selected-element-id']")
		.should("have.attr", "data-selected-id", "opt-3")
		.should("have.attr", "data-selected-value", "value3");
});

it("Should handle rapid sequential selection changes correctly", () => {
	cy.mount(
		<>
			<Toolbar>
				<ToolbarSelect id="ts-rapid">
					<ToolbarSelectOption value="a">Option A</ToolbarSelectOption>
					<ToolbarSelectOption value="b">Option B</ToolbarSelectOption>
					<ToolbarSelectOption value="c" selected>Option C</ToolbarSelectOption>
					<ToolbarSelectOption value="d">Option D</ToolbarSelectOption>
				</ToolbarSelect>
			</Toolbar>
			<input data-testid="final-value" />
			<input data-testid="change-count" />
		</>
	);

	let changeCount = 0;
	cy.get("[ui5-toolbar-select]").then($select => {
		$select.get(0).addEventListener("ui5-change", (e: Event) => {
			changeCount++;
			const ce = e as CustomEvent;
			const resultInput = document.querySelector("[data-testid='final-value']") as HTMLInputElement;
			resultInput.value = ce.detail.selectedToolbarOption.value;
			(document.querySelector("[data-testid='change-count']") as HTMLInputElement).value = changeCount.toString();
		});
	});

	// Make multiple selections in sequence
	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realClick();

	// Arrow down to A
	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("ArrowUp");

	cy.wait(100);

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("ArrowUp");

	cy.wait(100);

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("Enter");

	// Verify final state is correct
	cy.get("[data-testid='final-value']").should("have.prop", "value", "a");
	// Change events should have fired for each navigation/selection
	cy.get("[data-testid='change-count']").then($input => {
		const count = parseInt($input.val() as string);
		expect(count).to.be.greaterThan(0);
	});
});

it("Should correctly identify selectedToolbarOption even when option text doesn't match value", () => {
	cy.mount(
		<>
			<Toolbar>
				<ToolbarSelect id="ts-mismatch">
					<ToolbarSelectOption value="internal_1">Display Text 1</ToolbarSelectOption>
					<ToolbarSelectOption value="internal_2" selected>Display Text 2</ToolbarSelectOption>
					<ToolbarSelectOption value="internal_3">Display Text 3</ToolbarSelectOption>
				</ToolbarSelect>
			</Toolbar>
			<input data-testid="value-match-test" />
			<input data-testid="text-match-test" />
		</>
	);

	cy.get("[ui5-toolbar-select]").then($select => {
		$select.get(0).addEventListener("ui5-change", (e: Event) => {
			const ce = e as CustomEvent;
			const selectedToolbarOption = ce.detail.selectedToolbarOption as any;
			(document.querySelector("[data-testid='value-match-test']") as HTMLInputElement).value = 
				selectedToolbarOption.value || "";
			(document.querySelector("[data-testid='text-match-test']") as HTMLInputElement).value = 
				selectedToolbarOption.textContent?.trim() || "";
		});
	});

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realClick();

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("ArrowDown");

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("Enter");

	// Value should be from the 'value' property, not text content
	cy.get("[data-testid='value-match-test']").should("have.prop", "value", "internal_3");
	cy.get("[data-testid='text-match-test']").should("have.prop", "value", "Display Text 3");
});

it("Should fire change event with correct selectedToolbarOption after internal option element updates", () => {
	cy.mount(
		<>
			<Toolbar>
				<ToolbarSelect id="ts-element-update">
					<ToolbarSelectOption id="dynamic-opt-1" value="dynamic-1">Dynamic 1</ToolbarSelectOption>
					<ToolbarSelectOption id="dynamic-opt-2" value="dynamic-2" selected>Dynamic 2</ToolbarSelectOption>
					<ToolbarSelectOption id="dynamic-opt-3" value="dynamic-3">Dynamic 3</ToolbarSelectOption>
				</ToolbarSelect>
			</Toolbar>
			<input data-testid="element-update-result" />
		</>
	);

	cy.get("[ui5-toolbar-select]").then($select => {
		$select.get(0).addEventListener("ui5-change", (e: Event) => {
			const ce = e as CustomEvent;
			const selectedToolbarOption = ce.detail.selectedToolbarOption as any;
			// Store the id from the actual element reference
			(document.querySelector("[data-testid='element-update-result']") as HTMLInputElement).value = 
				selectedToolbarOption.id || selectedToolbarOption.value || "";
		});
	});

	// Dynamically update an option's value property
	cy.get("#dynamic-opt-3").then($option => {
		$option.get(0).value = "updated-dynamic-3";
	});

	// Now select that updated option
	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realClick();

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("ArrowDown");

	cy.get("[ui5-toolbar]")
		.find("[ui5-toolbar-select]")
		.shadow()
		.find("[ui5-select]")
		.realPress("Enter");

	// Verify we get the element reference with the updated value
	cy.get("[data-testid='element-update-result']").then($input => {
		const result = $input.val() as string;
		// Should be either the id or the updated value
		expect(result === "dynamic-opt-3" || result === "updated-dynamic-3").to.be.true;
	});
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants