fix(ui5-toolbar): correct event handlers attachment (#10546)

The Toolbar component now attaches event listeners directly in the template, replacing the previous onEnterDOM approach. This ensures handlers are available immediately after rendering, resolving race conditions and improving test reliability. Event handling is now managed by the abstract items themselves, rather than the toolbar mediating events. Items fire a new internal "close-overflow" event to notify the toolbar when the overflow menu should close.
SAP · Jan 20, 2025 · f6d6c67 · f6d6c67
1 parent 1e01523
commit f6d6c67
Show file tree

Hide file tree

Showing 11 changed files with 122 additions and 155 deletions.
diff --git a/docs/internal/Toolbar.md b/docs/internal/Toolbar.md
@@ -105,42 +105,4 @@ import ToolbarButton from "./dist/ToolbarButton.js";
   <ui5-toolbar-button text="Button 1" disabled></ui5-toolbar-button>
   <ui5-toolbar-button text="Button 2"></ui5-toolbar-button>
 </ui5-toolbar>
-```
-
-## Events
-
-Abstract items can provide a map of events through the `subscribedEvents` getter. The toolbar will actively monitor these events on the physical items, and when triggered, it will also fire the information to the corresponding abstract item. This mechanism proves useful when the abstract item requires synchronization of changes or interactions with the physical items. Importantly, events described as public offer benefits to consumers of the abstract items informing them about interactions with the physical elements. Additionally, the map contains information about the popover, such as `preventClosing: true`, which ensures that the popover remains open when this event is triggered by the physical item.
-
-A good example is the Map of the `ui5-toolbar-select`:
-
-```javascript
-get subscribedEvents() {
-  const map = new Map();
-
-  map.set("click", { preventClosing: true });
-  map.set("change", { preventClosing: false });
-  map.set("open", { preventClosing: true });
-  map.set("close", { preventClosing: true });
-
-  return map;
-}
-```
-
-The `ui5-toolbar-select` then waits for the toolbar to fire the `change` event, in order to notify (synchronize) its `options` slots:
-
-```ts
-_onEventHandler(e: Event): void {
-  if (e.type === "change") {
-    // update options
-    const selectedOption = (e as CustomEvent<ToolbarSelectChangeEventDetail>).detail.selectedOption;
-    const selectedOptionIndex = Number(selectedOption?.getAttribute("data-ui5-external-action-item-index"));
-    this.options.forEach((option: Option, index: number) => {
-      if (index === selectedOptionIndex) {
-        option.setAttribute("selected", "");
-      } else {
-        option.removeAttribute("selected");
-      }
-    });
-  }
-}
 ```
diff --git a/packages/main/cypress/specs/Toolbar.cy.ts b/packages/main/cypress/specs/Toolbar.cy.ts
@@ -122,4 +122,53 @@ describe("Toolbar general interaction", () => {
 			.find(".ui5-tb-overflow-btn-hidden")
 			.should("exist", "hidden class attached to tb button, meaning it's not shown as expected");
 	});
+
+	it("Should call event handlers on abstract item", () => {
+		cy.mount(html`
+			<ui5-toolbar>
+				<ui5-toolbar-button text="Button 1"></ui5-toolbar-button>
+				<ui5-toolbar-select>
+					<ui5-toolbar-select-option>1</ui5-toolbar-select-option>
+					<ui5-toolbar-select-option selected>2</ui5-toolbar-select-option>
+					<ui5-toolbar-select-option>3</ui5-toolbar-select-option>
+				</ui5-toolbar-select>
+			</ui5-toolbar>
+		`);
+
+		cy.get("ui5-toolbar-button[text='Button 1']")
+			.then(button => {
+				button.get(0).addEventListener("click", cy.stub().as("clicked"));
+			});
+
+		cy.get("ui5-button", { includeShadowDom: true }).contains("Button 1")
+			.click();
+
+		cy.get("@clicked")
+			.should("have.been.calledOnce");
+
+		cy.get("ui5-toolbar-select")
+			.then(select => {
+				select.get(0).addEventListener("ui5-click", cy.stub().as("clicked"));
+				select.get(0).addEventListener("ui5-change", cy.stub().as("changed"));
+				select.get(0).addEventListener("ui5-open", cy.stub().as("opened"));
+				select.get(0).addEventListener("ui5-close", cy.stub().as("closed"));
+			});
+
+		cy.get("ui5-select", { includeShadowDom: true })
+			.click();
+
+		cy.get("@clicked")
+			.should("have.been.calledOnce");
+		cy.get("@opened")
+			.should("have.been.calledOnce");
+
+		cy.get("ui5-option", { includeShadowDom: true })
+			.first()
+			.click();
+
+		cy.get("@changed")
+			.should("have.been.calledOnce");
+		cy.get("@closed")
+			.should("have.been.calledOnce");
+	});
 });
diff --git a/packages/main/src/Toolbar.ts b/packages/main/src/Toolbar.ts
@@ -158,7 +158,7 @@ class Toolbar extends UI5Element {
 	items!: Array<ToolbarItem>
 
 	_onResize!: ResizeObserverCallback;
-	_onInteract!: EventListener;
+	_onCloseOverflow!: EventListener;
 	itemsToOverflow: Array<ToolbarItem> = [];
 	itemsWidth = 0;
 	minContentWidth = 0;
@@ -180,7 +180,7 @@ class Toolbar extends UI5Element {
 		super();
 
 		this._onResize = this.onResize.bind(this);
-		this._onInteract = (e: Event) => this.onInteract(e as CustomEvent);
+		this._onCloseOverflow = this.closeOverflow.bind(this);
 	}
 
 	/**
@@ -197,14 +197,6 @@ class Toolbar extends UI5Element {
 			+ calculateCSSREMValue(toolbarComputedStyle, getScopedVarName("--_ui5-toolbar-padding-right"));
 	}
 
-	get subscribedEvents() {
-		return this.items
-			.map((item: ToolbarItem) => Array.from(item.subscribedEvents.keys()))
-			.flat()
-			// remove duplicates
-			.filter((value, index, self) => self.indexOf(value) === index);
-	}
-
 	get alwaysOverflowItems() {
 		return this.items.filter((item: ToolbarItem) => item.overflowPriority === ToolbarItemOverflowBehavior.AlwaysOverflow);
 	}
@@ -490,57 +482,16 @@ class Toolbar extends UI5Element {
 		this.processOverflowLayout();
 	}
 
-	onInteract(e: CustomEvent) {
-		e.stopImmediatePropagation();
-		const target = e.target as HTMLElement;
-		const item = target.closest<ToolbarItem>(".ui5-tb-item") || target.closest<ToolbarItem>(".ui5-tb-popover-item");
-
-		if (target === this.overflowButtonDOM) {
-			this.toggleOverflow();
-			return;
-		}
-
-		if (!item) {
-			return;
-		}
-
-		const refItemId = target.getAttribute("data-ui5-external-action-item-id");
-
-		if (refItemId) {
-			const abstractItem = this.getItemByID(refItemId);
-			const eventType = e.type;
-			const eventTypeNonPrefixed: string = e.type.replace("ui5-", "");
-			const prevented = !abstractItem?.fireEvent(eventTypeNonPrefixed, { ...e.detail, targetRef: target });
-			const eventOptions = abstractItem?.subscribedEvents.get(eventType) || abstractItem?.subscribedEvents.get(eventTypeNonPrefixed);
-
-			if (prevented || abstractItem?.preventOverflowClosing || eventOptions?.preventClosing) {
-				return;
-			}
-
-			this.closeOverflow();
-		}
-	}
-
 	/**
 	 * Private members
 	 */
 
 	attachListeners() {
-		const popover = this.getOverflowPopover();
-
-		this.subscribedEvents.forEach((e: string) => {
-			this.itemsDOM?.addEventListener(e, this._onInteract);
-			popover?.addEventListener(e, this._onInteract);
-		});
+		this.addEventListener("close-overflow", this._onCloseOverflow);
 	}
 
 	detachListeners() {
-		const popover = this.getOverflowPopover();
-
-		this.subscribedEvents.forEach((e: string) => {
-			this.itemsDOM?.removeEventListener(e, this._onInteract);
-			popover?.removeEventListener(e, this._onInteract);
-		});
+		this.removeEventListener("close-overflow", this._onCloseOverflow);
 	}
 
 	onToolbarItemChange() {

diff --git a/packages/main/src/ToolbarButton.ts b/packages/main/src/ToolbarButton.ts
@@ -5,7 +5,6 @@ import type { ButtonAccessibilityAttributes } from "./Button.js";
 import type ButtonDesign from "./types/ButtonDesign.js";
 
 import ToolbarItem from "./ToolbarItem.js";
-import type { IEventOptions } from "./ToolbarItem.js";
 import ToolbarButtonTemplate from "./ToolbarButtonTemplate.js";
 import ToolbarPopoverButtonTemplate from "./ToolbarPopoverButtonTemplate.js";
 
@@ -47,11 +46,7 @@ type ToolbarButtonAccessibilityAttributes = ButtonAccessibilityAttributes;
 	bubbles: true,
 	cancelable: true,
 })
-
 class ToolbarButton extends ToolbarItem {
-	eventDetails!: ToolbarItem["eventDetails"] & {
-		"click": void
-	}
 	/**
 	 * Defines if the action is disabled.
 	 *
@@ -180,10 +175,12 @@ class ToolbarButton extends ToolbarItem {
 		return ToolbarPopoverButtonTemplate;
 	}
 
-	get subscribedEvents(): Map<string, IEventOptions> {
-		const map = new Map();
-		map.set("click", { preventClosing: false });
-		return map;
+	onClick(e: Event) {
+		e.stopImmediatePropagation();
+		const prevented = !this.fireDecoratorEvent("click", { targetRef: e.target as HTMLElement });
+		if (!prevented && !this.preventOverflowClosing) {
+			this.fireDecoratorEvent("close-overflow");
+		}
 	}
 }
 

diff --git a/packages/main/src/ToolbarButtonTemplate.tsx b/packages/main/src/ToolbarButtonTemplate.tsx
@@ -21,6 +21,7 @@ export default function ToolbarButtonTemplate(this: ToolbarButton) {
 			hidden={this.hidden}
 			data-ui5-external-action-item-id={this._id}
 			data-ui5-stable={this.stableDomRef}
+			onClick={(...args) => this.onClick(...args)}
 		>
 			{this.text}
 		</Button>

diff --git a/packages/main/src/ToolbarItem.ts b/packages/main/src/ToolbarItem.ts
@@ -1,13 +1,22 @@
 import UI5Element from "@ui5/webcomponents-base/dist/UI5Element.js";
 import type { TemplateFunction } from "@ui5/webcomponents-base/dist/renderer/executeTemplate.js";
 import property from "@ui5/webcomponents-base/dist/decorators/property.js";
+import event from "@ui5/webcomponents-base/dist/decorators/event-strict.js";
 
 import type ToolbarItemOverflowBehavior from "./types/ToolbarItemOverflowBehavior.js";
 
 type IEventOptions = {
 	preventClosing: boolean;
 }
 
+type ToolbarItemEventDetail = {
+	targetRef: HTMLElement;
+}
+
+@event("close-overflow", {
+	bubbles: true,
+})
+
 /**
  * @class
  *
@@ -21,7 +30,8 @@ type IEventOptions = {
 class ToolbarItem extends UI5Element {
 	// strictEvents: needed for parent class
 	eventDetails!: {
-		click: void
+		click: ToolbarItemEventDetail,
+		"close-overflow": void;
 	}
 	/**
 	 * Property used to define the access of the item to the overflow Popover. If "NeverOverflow" option is set,
@@ -101,18 +111,13 @@ class ToolbarItem extends UI5Element {
 		throw new Error("Popover template must be defined");
 	}
 
-	/**
-	 * Returns the events that the item is subscribed to.
-	 * @protected
-	 */
-	get subscribedEvents(): Map<string, IEventOptions> {
-		return new Map();
-	}
-
 	get stableDomRef() {
 		return this.getAttribute("stable-dom-ref") || `${this._id}-stable-dom-ref`;
 	}
 }
 
-export type { IEventOptions };
+export type {
+	IEventOptions,
+	ToolbarItemEventDetail,
+};
 export default ToolbarItem;
diff --git a/packages/main/src/ToolbarPopoverButtonTemplate.tsx b/packages/main/src/ToolbarPopoverButtonTemplate.tsx
@@ -16,6 +16,7 @@ export default function ToolbarPopoverButtonTemplate(this: ToolbarButton) {
 			class="ui5-tb-popover-button ui5-tb-popover-item"
 			data-ui5-external-action-item-id={this._id}
 			data-ui5-stable={this.stableDomRef}
+			onClick={(...args) => this.onClick(...args)}
 		>
 			{this.text}
 		</Button>

diff --git a/packages/main/src/ToolbarPopoverSelectTemplate.tsx b/packages/main/src/ToolbarPopoverSelectTemplate.tsx
@@ -11,6 +11,10 @@ export default function ToolbarPopoverSelectTemplate(this: ToolbarSelect) {
 			disabled={this.disabled}
 			accessibleName={this.accessibleName}
 			accessibleNameRef={this.accessibleNameRef}
+			onClick={(...args) => this.onClick(...args)}
+			onClose={(...args) => this.onClose(...args)}
+			onOpen={(...args) => this.onOpen(...args)}
+			onChange={(...args) => this.onChange(...args)}
 		>
 			{this.options.map((option, index) => (
 				<Option