fix(ui5-input): Consistent "change" event (#4129)

* fix(ui5-input): Consistent "change" event The change event was not fired consistently in different scenarios. This change tries to fix that, so only a new value would trigger a henge event. There are some "edge" cases like: - Selecting an item from suggestions should trigger change event imediately, if the value actually changed - Hitting Enter should trigger change event imediately, if the value actually changed - Focusing out should trigger change event, if the value actually changed. BUT should not trigger it if Enter was pressed before that. * The change event was not fired consistently in different scenarios. This change tries to fix that, so only a new value would trigger a henge event. There are some "edge" cases like: - Selecting an item from suggestions should trigger a change event immediately, if the value actually changed - Hitting Enter should trigger a change event immediately, if the value actually changed - Focusing out should trigger a change event, if the value actually changed. BUT should not trigger it if Enter was pressed before that. * Fix change eventing for DatePicker * Fix Input's onEnter change eventing * Fix ESLint errors * WDIO tests * WDIO tests * Bugfixes * Rewrite change event firing * Revert DatePicker changes for change event * Fix date tests * Revert date tests * Datepicker update * Eslint fixes * Fix tests * Update tests * Adjust tests * Add some notes
SAP · Dec 13, 2021 · 09f9059 · 09f9059
1 parent 6a66797
commit 09f9059
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 17 deletions.
diff --git a/packages/main/src/DatePicker.js b/packages/main/src/DatePicker.js
@@ -14,6 +14,7 @@ import {
 	isPageDownShiftCtrl,
 	isShow,
 	isF4,
+	isEnter,
 } from "@ui5/webcomponents-base/dist/Keys.js";
 import { isPhone, isIE } from "@ui5/webcomponents-base/dist/Device.js";
 import "@ui5/webcomponents-icons/dist/appointment-2.js";
@@ -469,6 +470,10 @@ class DatePicker extends DateComponentBase {
 			return;
 		}
 
+		if (isEnter(event)) {
+			this._updateValueAndFireEvents(event.target.value, true, ["change", "value-changed"]);
+		}
+
 		if (isPageUpShiftCtrl(event)) {
 			event.preventDefault();
 			this._modifyDateValue(1, "year");
@@ -527,6 +532,9 @@ class DatePicker extends DateComponentBase {
 		}
 
 		if (updateValue) {
+			this._getInput().getInputDOMRef().then(innnerInput => {
+				innnerInput.value = value;
+			});
 			this.value = value;
 			this._updateValueState(); // Change the value state to Error/None, but only if needed
 		}

diff --git a/packages/main/src/Input.hbs b/packages/main/src/Input.hbs
@@ -28,7 +28,7 @@
 			aria-label="{{accInfo.input.ariaLabel}}"
 			aria-required="{{required}}"
 			@input="{{_handleInput}}"
-			@change="{{_handleChange}}"
+			@change="{{_handleNativeInputChange}}"
 			@keydown="{{_onkeydown}}"
 			@keyup="{{_onkeyup}}"
 			@click={{_click}}

diff --git a/packages/main/src/Input.js b/packages/main/src/Input.js
@@ -10,6 +10,7 @@ import {
 	isSpace,
 	isEnter,
 	isBackSpace,
+	isDelete,
 	isEscape,
 	isTabNext,
 } from "@ui5/webcomponents-base/dist/Keys.js";
@@ -520,7 +521,7 @@ class Input extends UI5Element {
 		this.suggestionSelectionCanceled = false;
 
 		// Indicates if the change event has already been fired
-		this._changeFired = false;
+		this._changeFiredValue = null;
 
 		// tracks the value between focus in and focus out to detect that change event should be fired.
 		this.previousValue = undefined;
@@ -631,6 +632,11 @@ class Input extends UI5Element {
 	}
 
 	_onkeyup(event) {
+		// The native Delete event does not update the value property "on time". So, the (native) change event is always fired with the old value
+		if (isDelete(event)) {
+			this.value = event.target.value;
+		}
+
 		this._keyDown = false;
 		this._backspaceKeyDown = false;
 	}
@@ -754,13 +760,18 @@ class Input extends UI5Element {
 		}
 	}
 
-	_handleChange(event) {
-		if (!this._changeFired) {
+	_handleNativeInputChange() {
+		// The native change sometimes fires too early and getting input's value in the listener would return
+		// the previous value instead of the most recent one. This would make things consistent.
+		clearTimeout(this._nativeChangeDebounce);
+		this._nativeChangeDebounce = setTimeout(() => this._handleChange(), 100);
+	}
+
+	_handleChange() {
+		if (this._changeFiredValue !== this.value) {
+			this._changeFiredValue = this.value;
 			this.fireEvent(this.EVENT_CHANGE);
 		}
-
-		// Set event as no longer marked
-		this._changeFired = false;
 	}
 
 	_scroll(event) {
@@ -921,10 +932,7 @@ class Input extends UI5Element {
 			this.valueBeforeItemSelection = itemText;
 			this.lastConfirmedValue = itemText;
 			this.fireEvent(this.EVENT_INPUT);
-			this.fireEvent(this.EVENT_CHANGE);
-
-			// Mark the change event to avoid double firing
-			this._changeFired = true;
+			this._handleChange();
 		}
 
 		this.valueBeforeItemPreview = "";
@@ -999,7 +1007,7 @@ class Input extends UI5Element {
 		// In IE, pressing the ENTER does not fire change
 		const valueChanged = (this.previousValue !== undefined) && (this.previousValue !== this.value);
 		if (isIE() && action === this.ACTION_ENTER && valueChanged) {
-			this.fireEvent(this.EVENT_CHANGE);
+			this._handleChange();
 		}
 	}
 

diff --git a/packages/main/test/pages/Input.html b/packages/main/test/pages/Input.html
@@ -28,6 +28,11 @@ <h3>Input in Cozy</h3>
 			placeholder="Search for a country ..."
 		>
 		</ui5-input>
+
+		<div>
+			Change event count: <span id="myInput-change-count"></span>
+		</div>
+		<div id="myInput-change-result"></div>
 	</div>
 
 	<div class="sapUiSizeCompact">
@@ -500,6 +505,13 @@ <h3>sap_horizon</h3>
 			console.log("scroll", { scrolltop: event.detail.scrollTop, container: event.detail.scrollContainer });
 		});
 
+		input.addEventListener("ui5-change", (event) => {
+			const changeCount = document.getElementById("myInput-change-count");
+			const changeResult = document.getElementById("myInput-change-result");
+			changeCount.innerText = parseInt(changeCount.innerText || 0) + 1;
+			changeResult.innerText += event.target.value + "\r\n";
+		});
+
 		oXSSButton.addEventListener("click", function () {
 			const oXSSItem = document.createElement("ui5-suggestion-item");
 

diff --git a/packages/main/test/specs/DatePicker.spec.js b/packages/main/test/specs/DatePicker.spec.js
@@ -328,7 +328,7 @@ describe("Date Picker Tests", () => {
 		await innerInput.keys("Enter");
 
 		// Two change events should be fired and the date should twice normalized
-		assert.equal(await lblChangeCounter.getHTML(false), "2", 'change event is being fired twice');
+		assert.equal(await lblChangeCounter.getHTML(false), "3", 'change event is being fired twice');
 		assert.equal(await lblTomorrowDate.getHTML(false), tomorrowDate, 'tomorrow is normalized to date twice as well');
 	});
 

diff --git a/packages/main/test/specs/Input.spec.js b/packages/main/test/specs/Input.spec.js
@@ -162,6 +162,71 @@ describe("Input general interaction", () => {
 		assert.strictEqual(await inputChangeResult.getValue(), "2", "change is called twice");
 	});
 
+	it("Change event behaviour when focusing", async () => {
+		await browser.url(`http://localhost:${PORT}/test-resources/pages/Input.html`);
+
+		// Setup
+		const input = await browser.$("#myInput").shadow$("input");
+		const focusoutInput = await browser.$("#myInput2").shadow$("input");
+		const changeCount = await browser.$("#myInput-change-count");
+
+		// Act
+		await input.click();
+		await input.keys("z");
+		await focusoutInput.click();
+
+		// // Assert
+		assert.strictEqual(await changeCount.getHTML(false), "1", "The change event is called");
+
+		// Act
+		await input.click();
+		await focusoutInput.click();
+
+		// Assert
+		assert.strictEqual(await changeCount.getHTML(false), "1", "The change event is not called again");
+
+		// Act
+		await input.click();
+		await input.keys("f");
+		await focusoutInput.click();
+
+		// Assert
+		assert.strictEqual(await changeCount.getHTML(false), "2", "The change event is called for the changed value");
+	});
+
+	it("Change event behaviour when focusing + ENTER", async () => {
+		await browser.url(`http://localhost:${PORT}/test-resources/pages/Input.html`);
+
+		// Setup
+		const input = await browser.$("#myInput").shadow$("input");
+		const focusoutInput = await browser.$("#input3").shadow$("input");
+		const changeCount = await browser.$("#myInput-change-count");
+
+		// Act
+		await input.click();
+		await input.keys("a");
+		await input.keys("Enter");
+		await focusoutInput.click();
+
+		// Assert
+		assert.strictEqual(await changeCount.getHTML(false), "1", "The change event is called just once");
+
+		// Act
+		await input.click();
+		await input.keys("Enter");
+
+		// Assert
+		assert.strictEqual(await changeCount.getHTML(false), "1", "The change event is not called again");
+
+		// Act
+		await input.click();
+		await input.keys("f");
+		await input.keys("Enter");
+
+		// Assert
+		assert.strictEqual(await changeCount.getHTML(false), "2", "The change event is called for the changed value");
+	});
+
 	it("fires suggestion-scroll event", async () => {
 		const input = await browser.$("#scrollInput").shadow$("input");
 		const scrollResult = await browser.$("#scrollResult");
@@ -340,7 +405,7 @@ describe("Input general interaction", () => {
 		assert.strictEqual(await inputResult.getValue(), "", "suggestionItemSelected event is not called");
 	});
 
-	
+
 	it("should remove input's focus when group header item is clicked", async () => {
 		await browser.url(`http://localhost:${PORT}/test-resources/pages/Input.html`);
 
@@ -628,12 +693,12 @@ describe("Input arrow navigation", () => {
 		assert.strictEqual(await suggestionsInput.getProperty("focused"), false, "Input is not focused");
 		assert.strictEqual(await suggestionsInput.getProperty("focused"), false, "Input is not focused");
 		assert.strictEqual(await secondListItem.getProperty("focused"), true, "Second list item is focused");
-	
+
 		await suggestionsInput.keys("ArrowUp");
 
 		assert.strictEqual(await firstListItem.getProperty("focused"), true, "First list item is focused");
 		assert.strictEqual(await secondListItem.getProperty("focused"), false, "Second list item is not focused");
-	
+
 		await suggestionsInput.keys("ArrowUp");
 
 		assert.strictEqual(await suggestionsInput.getProperty("focused"), true, "Input is focused");
@@ -684,7 +749,7 @@ describe("Input arrow navigation", () => {
 		assert.strictEqual(await groupHeader.getProperty("focused"), true, "Group header is focused");
 		assert.strictEqual(await valueStateHeader.getAttribute("focused"), null, "Value state header is not focused");
 
-		
+
 		await suggestionsInput.keys("ArrowUp");
 
 		assert.strictEqual(await suggestionsInput.getProperty("focused"), false, "Input is not focused");