fix: disable click events for disabled ui5-button (#586)

add pointer-events: none to host component so in case it is disabled,
the browser does not call click handlers registered to it.

add chai for better assertion messages

* move click handler to shadow dom
this way, the component can handle the click first and then the app handler
will have the correct component state in its click handler

* remove event-related qunit tests
With click handlers registered in the shadow dom, the qunit tests
can no longer simulate events correctly.
For these scenarios, the wdio tests will be used anyway.

* fix ie11 mousedown problem in IE11
remove disabled check, CSS takes care of that

Fixes: #572

packages/main/package.json

@@ -103,6 +103,7 @@
     "@wdio/spec-reporter": "^5.4.3",
     "@wdio/sync": "^5.4.13",
     "@webcomponents/webcomponentsjs": "^2.2.7",
+    "chai": "^4.2.0",
     "chromedriver": "^2.45.0",
     "clean-css": "^4.2.1",
     "copy-and-watch": "^0.1.2",

packages/main/src/Button.hbs

@@ -5,7 +5,9 @@
 		data-sap-focus-ref
 		{{> ariaPressed}}
 		dir="{{rtl}}"
-		@focusout={{onfocusout}}
+		@focusout={{_onfocusout}}
+		@click={{_onclick}}
+		@mousedown={{_onmousedown}}
 	>
 		{{#if icon}}
 			<ui5-icon

packages/main/src/Button.js

@@ -207,23 +207,18 @@ class Button extends UI5Element {
 		document.removeEventListener("mouseup", this._deactivate);
 	}
 
-	onclick(event) {
+	_onclick(event) {
 		event.isMarked = "button";
-		if (!this.disabled) {
-			this.fireEvent("press", {});
-			const FormSupport = getFeature("FormSupport");
-			if (FormSupport) {
-				FormSupport.triggerFormSubmit(this);
-			}
+		this.fireEvent("press", {});
+		const FormSupport = getFeature("FormSupport");
+		if (FormSupport) {
+			FormSupport.triggerFormSubmit(this);
 		}
 	}
 
-	onmousedown(event) {
+	_onmousedown(event) {
 		event.isMarked = "button";
-
-		if (!this.disabled) {
-			this._active = true;
-		}
+		this._active = true;
 	}
 
 	onmouseup(event) {
@@ -242,7 +237,7 @@ class Button extends UI5Element {
 		}
 	}
 
-	onfocusout(_event) {
+	_onfocusout(_event) {
 		this._active = false;
 	}
 

packages/main/src/ToggleButton.js

@@ -66,11 +66,9 @@ class ToggleButton extends Button {
 		return [Button.styles, toggleBtnCss];
 	}
 
-	onclick() {
-		if (!this.disabled) {
-			this.pressed = !this.pressed;
-			this.fireEvent("press", { pressed: this.pressed });
-		}
+	_onclick(e) {
+		this.pressed = !this.pressed;
+		this.fireEvent("press", { pressed: this.pressed });
 	}
 
 	get classes() {

packages/main/src/themes/Button.css

@@ -6,6 +6,14 @@ ui5-button:not([hidden]) {
 	display: inline-block;
 }
 
+:host([disabled]) {
+	pointer-events: none;
+}
+
+ui5-button[disabled] {
+	pointer-events: none;
+}
+
 button[dir="rtl"].sapMBtn.sapMBtnWithIcon .sapMBtnText {
 	margin-right: var(--_ui5_button_base_icon_margin);
 	margin-left: 0;

packages/main/src/themes/ToggleButton.css

@@ -6,6 +6,15 @@ ui5-togglebutton:not([hidden]) {
 	display: inline-block;
 }
 
+/* this line is also included from the Button.css, but keep it here in case we start using it for automatic IE11 compilation */
+:host([disabled]) {
+	pointer-events: none;
+}
+
+ui5-togglebutton[disabled] {
+	pointer-events: none;
+}
+
 ui5-togglebutton .sapMBtn::before { /* this is a hack for a bug in IE https:/* github.com/philipwalton/flexbugs/issues/231 */
 	content: '';
 	min-height: inherit;

packages/main/test/sap/ui/webcomponents/main/pages/Button.html

@@ -33,7 +33,8 @@
 	<br />
 	<br />
 
-	<ui5-input id="field"></ui5-input>
+	<ui5-input id="press-counter"></ui5-input>
+	<ui5-input id="click-counter"></ui5-input>
 
 	<ui5-button design="Emphasized">Action<br> Bar<br> Button</ui5-button>
 	<ui5-button>Primary button</ui5-button>
@@ -150,15 +151,24 @@
 
 
 	<script>
-		var input = document.querySelector("#field");
+		var pressCounter = document.querySelector("#press-counter");
+		var clickCounter = document.querySelector("#click-counter");
 		var button = document.querySelector("#button1");
 		var disabledButton = document.querySelector("#button-disabled");
-		var counter = 0;
+		var pressCount = 0;
+		var clickCount = 0;
 
 		[button, disabledButton].forEach(function (el) {
 			el.addEventListener("ui5-press", function(event) {
-				counter += 1;
-				input.value = counter;
+				pressCount += 1;
+				pressCounter.value = pressCount;
+			});
+		});
+
+		[button, disabledButton].forEach(function (el) {
+			el.addEventListener("click", function(event) {
+				clickCount += 1;
+				clickCounter.value = clickCount;
 			});
 		});
 

packages/main/test/sap/ui/webcomponents/main/pages/ToggleButton.html

@@ -9,7 +9,9 @@
 	<meta name="viewport" content="width=device-width, initial-scale=1">
 	<meta http-equiv="X-UA-Compatible" content="IE=edge">
 	<meta charset="utf-8">
-
+	<script>
+		delete Document.prototype.adoptedStyleSheets
+	</script>
 	<script data-id="sap-ui-config" type="application/json">
 		{
 			"language": "EN"
@@ -89,20 +91,33 @@ <h2>ToggleButton states</h2>
 	</div>
 	<div class="result">
 		<h2>Result</h2>
-		<span id="result"></span>
+		<span id="press-result"></span>
+		<span id="click-result"></span>
 	</div>
 </section>
 
 <script>
-	var result = document.getElementById("result");
+	var pressResult = document.getElementById("press-result");
+	var clickResult = document.getElementById("click-result");
 
 	document.getElementById("main-sample").addEventListener("ui5-press", function (event) {
 		var target = event.target;
 
 		if (target.classList.contains("toggleButton")) {
-			result.innerHTML = target.textContent + ": " + event.detail.pressed;
+			pressResult.innerHTML = target.textContent + ": " + event.detail.pressed;
 		}
 	});
+
+	Array.prototype.slice.call(document.querySelectorAll("#main-sample .toggleButton")).forEach(function (el) {
+		el.addEventListener("click", function (event) {
+			var target = event.target;
+
+			if (target.classList.contains("toggleButton")) {
+				clickResult.innerHTML = target.textContent + ": " + (target.pressed);
+			}
+		});
+	});
+
 </script>
 </body>
 </html>

packages/main/test/sap/ui/webcomponents/main/qunit/Button.qunit.js

@@ -50,15 +50,4 @@ TestHelper.ready(function () {
 			done();
 		}.bind(this));
 	});
-
-	QUnit.test("events", function (assert) {
-		var done = assert.async();
-
-		this.button.addEventListener('press', function (event) {
-			assert.ok(true, 'press event is fired');
-			done();
-		});
-
-		this.button.click();
-	});
 });

packages/main/test/sap/ui/webcomponents/main/qunit/test.pages.js

@@ -27,6 +27,5 @@
 		"Table",
 		"TextArea",
 		"Title",
-		"ToggleButton",
 	];
 }));

packages/main/test/specs/Button.spec.js

@@ -1,12 +1,12 @@
-const assert = require("assert");
+const assert = require("chai").assert;
+
 
 describe("Button general interaction", () => {
 	browser.url("http://localhost:8080/test-resources/sap/ui/webcomponents/main/pages/Button.html");
 
 	it("tests press event", () => {
-
 		const button = browser.findElementDeep("#button1");
-		const field = browser.findElementDeep("#field");
+		const field = browser.findElementDeep("#press-counter");
 
 		button.click();
 		button.keys("Space");
@@ -15,14 +15,45 @@ describe("Button general interaction", () => {
 		assert.strictEqual(field.getProperty("value"), "3", "Press should be called 3 times");
 	});
 
-	it("tests clicking on disabled button", () => {
-		const button = browser.findElementDeep("#button-disabled");
-		const field = browser.findElementDeep("#field");
+	it("tests pressing on disabled button", () => {
+		const button = browser.findElementDeep("#button-disabled >>> button");
+
+		assert.throws(() => {
+			button.click();
+		});
+
+		// don't test space and enter, as wdio always fires a click but the browser not.
+		// button.keys("Space");
+		// button.keys("Enter");
+		const field = browser.findElementDeep("#press-counter");
+		assert.strictEqual(field.getProperty("value"), "3", "Press should be called 3 times");
+	});
+
+	it("click should call handler", () => {
+
+		const button = browser.findElementDeep("#button1");
+		const field = browser.findElementDeep("#click-counter");
 
 		button.click();
 		button.keys("Space");
 		button.keys("Enter");
 
-		assert.strictEqual(field.getProperty("value"), "3", "Press should be called 3 times");
+		assert.strictEqual(field.getProperty("value"), "6", "click should be called 3 times");
+	});
+
+	it("click on disabled button should not call handler", () => {
+		const button = browser.findElementDeep("#button-disabled >>> button");
+		const field = browser.findElementDeep("#click-counter");
+
+		assert.throws(() => {
+			button.click();
+			// the button click should throw a protocol error
+			// ERROR webdriver: Request failed due to Error: unknown error: Element <button type="button" data-sap-focus-ref="" class="sapMBtn sapMBtnDisabled sapMBtnDefault" disabled="">...</button> is not clickable at point (395, 26). Other element would receive the click: <body>...</body>
+		});
+		// don't test space and enter, as wdio always fires a click but the browser not.
+		// button.keys("Space");
+		// button.keys("Enter");
+
+		assert.strictEqual(field.getProperty("value"), "6", "click should be called 6 times");
 	});
 });

packages/main/test/specs/ToggleButton.spec.js

@@ -1,11 +1,11 @@
-const assert = require("assert");
+const assert = require("chai").assert;
 
 describe("ToggleButton general interaction", () => {
 	browser.url("http://localhost:8080/test-resources/sap/ui/webcomponents/main/pages/ToggleButton.html");
 
 	it("should fire press event on a normal togglebutton", () => {
 		const toggleButton = $("#toggle-button");
-		const result = $("#result");
+		const result = $("#press-result");
 
 		toggleButton.click();
 		assert.strictEqual(result.getText(), "ToggleButton: true", "Press event is fired with: { pressed: true }");
@@ -18,16 +18,52 @@ describe("ToggleButton general interaction", () => {
 	});
 
 	it("should not fire press event on a disabled togglebutton", () => {
-		const toggleButton = $("#disabled-toggle-button");
-		const result = $("#result");
+		const toggleButton = browser.findElementDeep("#disabled-toggle-button >>> button");
+		const result = $("#press-result");
+
+		assert.throws(() => {
+			toggleButton.click();
+		})
+		assert.strictEqual(result.getText(), "ToggleButton: true", "toggle state should not change");
+
+		// don't test space and enter, as wdio always fires a click but the browser not.
+
+		// toggleButton.keys("Space");
+		// assert.strictEqual(result.getText(), "ToggleButton: true", "2Press event is fired with: { pressed: true }");
+
+		// toggleButton.keys("Enter");
+		// assert.strictEqual(result.getText(), "ToggleButton: true", "3Press event is fired with: { pressed: true }");
+	});
+
+	it("should fire click event on a normal togglebutton", () => {
+		const toggleButton = $("#toggle-button");
+		const result = $("#click-result");
 
 		toggleButton.click();
-		assert.strictEqual(result.getText(), "ToggleButton: true", "Press event is fired with: { pressed: true }");
+		assert.strictEqual(result.getText(), "ToggleButton: false", "click event changed pressed state");
 
 		toggleButton.keys("Space");
-		assert.strictEqual(result.getText(), "ToggleButton: true", "Press event is fired with: { pressed: true }");
+		assert.strictEqual(result.getText(), "ToggleButton: true", "Space triggered click and changed pressed state");
 
 		toggleButton.keys("Enter");
-		assert.strictEqual(result.getText(), "ToggleButton: true", "Press event is fired with: { pressed: true }");
+		assert.strictEqual(result.getText(), "ToggleButton: false", "Enter triggered click and changed pressed state");
+	});
+
+	it("should not fire click event on a disabled togglebutton", () => {
+		const toggleButton = browser.findElementDeep("#disabled-toggle-button >>> button");
+		const result = $("#click-result");
+
+		assert.throws(() => {
+			toggleButton.click();
+		})
+		assert.strictEqual(result.getText(), "ToggleButton: false", "toggle state should not change");
+
+		// don't test space and enter, as wdio always fires a click but the browser not.
+
+		// toggleButton.keys("Space");
+		// assert.strictEqual(result.getText(), "ToggleButton: true", "2Press event is fired with: { pressed: true }");
+
+		// toggleButton.keys("Enter");
+		// assert.strictEqual(result.getText(), "ToggleButton: true", "3Press event is fired with: { pressed: true }");
 	});
 });