From d0468bd4ba5a94cbc32efffa6fbf6696702e6150 Mon Sep 17 00:00:00 2001
From: ilhan orhan <ilhan.myumyun@sap.com>
Date: Tue, 23 Feb 2021 16:48:00 +0200
Subject: [PATCH] fix(ui5-icon): fix click event fired twice (#2858)

We used to fire a custom event, but did not stop the native one and end up with firing two "click" events.
Now, the native one is stopped properly (we need to fire the custom one, so the noConflict ui5-click is also fired). Also, one additional fix has been performed - the content no longer scrolls when the SPACE key is pressed over "interactive" icon.

FIXES: #2857
---
 packages/main/src/Icon.js             | 14 +++++++++++---
 packages/main/test/pages/Icon.html    | 16 ++++++++++++++++
 packages/main/test/specs/Icon.spec.js | 12 ++++++++++--
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/packages/main/src/Icon.js b/packages/main/src/Icon.js
index 96bed613c6ca..24c1b18c347a 100644
--- a/packages/main/src/Icon.js
+++ b/packages/main/src/Icon.js
@@ -198,9 +198,17 @@ class Icon extends UI5Element {
 	}
 
 	_onkeydown(event) {
-		if (this.interactive && isEnter(event)) {
+		if (!this.interactive) {
+			return;
+		}
+
+		if (isEnter(event)) {
 			this.fireEvent("click");
 		}
+
+		if (isSpace(event)) {
+			event.preventDefault(); // prevent scrolling
+		}
 	}
 
 	_onkeyup(event) {
@@ -211,8 +219,8 @@ class Icon extends UI5Element {
 
 	_onclick(event) {
 		if (this.interactive) {
-			event.preventDefault();
-			// Prevent the native event and fire custom event because otherwise the noConfict event won't be thrown
+			// prevent the native event and fire custom event to ensure the noConfict "ui5-click" is fired
+			event.stopPropagation(); 
 			this.fireEvent("click");
 		}
 	}
diff --git a/packages/main/test/pages/Icon.html b/packages/main/test/pages/Icon.html
index 6f9ef42b1651..117f8334f2c1 100644
--- a/packages/main/test/pages/Icon.html
+++ b/packages/main/test/pages/Icon.html
@@ -37,6 +37,16 @@
 	<ui5-icon name="add-employee" class="icon-red icon-small"></ui5-icon>
 	<ui5-icon show-tooltip name="message-error"></ui5-icon>
 
+	<h3>Interactive Icon</h3>
+	<ui5-icon
+		id="myInteractiveIcon"
+		interactive
+		name="add-equipment"
+		style="width: 50px; height: 50px">
+	</ui5-icon>
+	<br>
+	<ui5-input id="click-event-2"></ui5-input>
+
 	<br>
 	<ui5-icon name="add-employee"></ui5-icon>
 	<br>
@@ -89,7 +99,9 @@
 		var icon = document.getElementById("interactive-icon"),
 			nonInteractiveIcon = document.getElementById("non-interactive-icon"),
 			input = document.getElementById("click-event"),
+			input2 = document.getElementById("click-event-2"),
 			inputValue = 0;
+			inputValue2 = 0;
 
 		icon.addEventListener("ui5-click", function() {
 			input.value = ++inputValue;
@@ -98,6 +110,10 @@
 		nonInteractiveIcon.addEventListener("ui5-click", function() {
 			input.value = ++inputValue;
 		});
+
+		myInteractiveIcon.addEventListener("click", function() {
+			input2.value = ++inputValue2;
+		});
 	</script>
 
 	<script type="module">
diff --git a/packages/main/test/specs/Icon.spec.js b/packages/main/test/specs/Icon.spec.js
index 4050762abf87..65a96503d25f 100644
--- a/packages/main/test/specs/Icon.spec.js
+++ b/packages/main/test/specs/Icon.spec.js
@@ -14,7 +14,7 @@ describe("Icon general interaction", () => {
 			"Built-in tooltip is correct");
 	});
 
-	it("Tests if clicked event is thrown for interactive icons", () => {
+	it("Tests noConflict 'ui5-click' event is thrown for interactive icons", () => {
 		const iconRoot = browser.$("#interactive-icon").shadow$(".ui5-icon-root");
 		const input = browser.$("#click-event");
 
@@ -28,7 +28,7 @@ describe("Icon general interaction", () => {
 		assert.strictEqual(input.getAttribute("value"), "3", "Space throws event");
 	});
 
-	it("Tests if clicked event is not thrown for non interactive icons", () => {
+	it("Tests noConflict 'ui5-click' event is not thrown for non interactive icons", () => {
 		const iconRoot = browser.$("#non-interactive-icon");
 		const input = browser.$("#click-event");
 
@@ -41,4 +41,12 @@ describe("Icon general interaction", () => {
 		iconRoot.keys("Space");
 		assert.strictEqual(input.getAttribute("value"), "3", "Space throws event");
 	});
+
+	it("Tests native 'click' event thrown", () => {
+		const icon = browser.$("#myInteractiveIcon");
+		const input = browser.$("#click-event-2");
+
+		icon.click();
+		assert.strictEqual(input.getAttribute("value"), "1", "Mouse click throws event");
+	});
 });