fix(ui5-range-slider): fire input event with correct values after swapping

packages/main/src/RangeSlider.ts

@@ -215,6 +215,14 @@ class RangeSlider extends SliderBase {
 	 *
 	 */
 	onBeforeRendering() {
+		if (this.startValue > this.endValue) {
+			const affectedValue = this._valueAffected === "startValue" ? "endValue" : "startValue";
+
+			this._swapValues();
+			this._setAffectedValue(affectedValue);
+			this.update(affectedValue, this.startValue, this.endValue);
+		}
+
 		if (!this.isCurrentStateOutdated()) {
 			return;
 		}
@@ -309,12 +317,20 @@ class RangeSlider extends SliderBase {
 	*/
 	_onkeyup() {
 		super._onkeyup();
-
-		this._swapValues();
 		this._setAffectedValue(undefined);
+
+		if (this.startValue !== this._startValueAtBeginningOfAction || this.endValue !== this._endValueAtBeginningOfAction) {
+			this.fireEvent("change");
+		}
+
+		this._startValueAtBeginningOfAction = undefined;
+		this._endValueAtBeginningOfAction = undefined;
 	}
 
 	_handleActionKeyPress(e: KeyboardEvent) {
+		this._startValueAtBeginningOfAction = this.startValue;
+		this._endValueAtBeginningOfAction = this.endValue;
+
 		if (isEscape(e)) {
 			this.update(undefined, this._startValueInitial, this._endValueInitial);
 			return;
@@ -409,7 +425,7 @@ class RangeSlider extends SliderBase {
 			this.updateStateStorageAndFireInputEvent("endValue");
 			this._updateHandlesAndRange(0);
 		} else {
-			const newValue = startValue;
+			const newValue = endValue && affectedValue === "endValue" ? endValue : startValue;
 			this._updateHandlesAndRange(newValue || 0);
 
 			if (affectedValue === "startValue") {
@@ -533,21 +549,19 @@ class RangeSlider extends SliderBase {
 	}
 
 	_handleUp() {
-		this._swapValues();
 		this._setAffectedValueByFocusedElement();
 		this._setAffectedValue(undefined);
 
-		this._startValueAtBeginningOfAction = undefined;
-		this._endValueAtBeginningOfAction = undefined;
-		this._setIsPressInCurrentRange(false);
+		if (this.startValue !== this._startValueAtBeginningOfAction || this.endValue !== this._endValueAtBeginningOfAction) {
+			this.fireEvent("change");
+		}
 
+		this._setIsPressInCurrentRange(false);
 		this.handleUpBase();
 
 		this.rangePressed = false;
-
-		if (this.startValue !== this._startValueAtBeginningOfAction || this.endValue !== this._endValueAtBeginningOfAction) {
-			this.fireEvent("change");
-		}
+		this._startValueAtBeginningOfAction = undefined;
+		this._endValueAtBeginningOfAction = undefined;
 	}
 
 	/**
@@ -782,24 +796,26 @@ class RangeSlider extends SliderBase {
 	 */
 	_swapValues() {
 		const affectedValue = this._valueAffected;
+		if (!affectedValue) {
+			return;
+		}
 
 		if (affectedValue === "startValue" && this.startValue > this.endValue) {
 			const prevEndValue = this.endValue;
 			this.endValue = this.startValue;
 			this.startValue = prevEndValue;
-
-			this._setValuesAreReversed();
-			this.focusInnerElement();
 		}
 
 		if (affectedValue === "endValue" && this.endValue < this.startValue) {
 			const prevStartValue = this.startValue;
 			this.startValue = this.endValue;
 			this.endValue = prevStartValue;
-
-			this._setValuesAreReversed();
-			this.focusInnerElement();
 		}
+
+		this._setValuesAreReversed();
+		this._updateHandlesAndRange(this[affectedValue]);
+		this.focusInnerElement();
+		this.syncUIAndState();
 	}
 
 	/**

packages/main/src/SliderBase.ts

@@ -331,26 +331,6 @@ class SliderBase extends UI5Element {
 
 	/**
 	 * Prevent focus out when inner element within the component is currently being in process of focusing in.
-	 * In theory this can be achieved either if the shadow root is focusable and 'delegatesFocus' attribute of
-	 * the .attachShadow() customElement method is set to true, or if we forward it manually.
-
-	 * As we use lit-element as base of our core UI5 element class that 'delegatesFocus' property is not set to 'true' and
-	 * we have to manage the focus here. If at some point in the future this changes, the focus delegating logic could be
-	 * removed as it will become redundant.
-	 *
-	 * When we manually set the focus on mouseDown to the first focusable element inside the shadowDom,
-	 * that inner focus (shadowRoot.activeElement) is set a moment before the global document.activeElement
-	 * is set to the customElement (ui5-slider) causing a 'race condition'.
-	 *
-	 * In order for a element within the shadowRoot to be focused, the global document.activeElement MUST be the parent
-	 * customElement of the shadow root, in our case the ui5-slider component. Because of that after our focusin of the handle,
-	 * a focusout event fired by the browser immidiatly after, resetting the focus. Focus out must be manually prevented
-	 * in both initial focusing and switching the focus between inner elements of the component cases.
-
-	 * Note: If we set the focus to the handle with a timeout or a bit later in time, on a mouseup or click event it will
-	 * work fine and we will avoid the described race condition as our host customElement will be already finished focusing.
-	 * However, that does not work for us as we need the focus to be set to the handle exactly on mousedown,
-	 * because of the nature of the component and its available drag interactions.
 	 *
 	 * @private
 	 */

packages/main/test/pages/RangeSlider.html

@@ -56,7 +56,14 @@ <h2>Event Testing Slider</h2>
 				<span>endValue in the change event: </span>
 				<span id="change-event-endValue"></span>
 			</div>
-
+			<div>
+				<span>startValue in the input event: </span>
+				<span id="input-event-startValue"></span>
+			</div>
+			<div>
+				<span>endValue in the input event: </span>
+				<span id="input-event-endValue"></span>
+			</div>
 			<h2>Event Testing Result Slider</h2>
 			<ui5-range-slider id="test-result-slider" start-value="1" end-value="2"></ui5-range-slider>
 		</section>
@@ -76,7 +83,11 @@ <h2>Event Testing Result Slider</h2>
 			eventTargetSlider.addEventListener("ui5-change", (e) => {
 				document.getElementById("change-event-startValue").innerText = e.target.startValue;
 				document.getElementById("change-event-endValue").innerText = e.target.endValue;
+			});
 
+			eventTargetSlider.addEventListener("ui5-input", (e) => {
+				document.getElementById("input-event-startValue").innerText = e.target.startValue;
+				document.getElementById("input-event-endValue").innerText = e.target.endValue;
 			});
 		</script>
 	</body>

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

@@ -303,14 +303,47 @@ describe("Testing events", () => {
 		const rangeSlider = await browser.$("#test-slider");
 		const firstHandle = await rangeSlider.shadow$(".ui5-slider-handle--start");
 
-		await rangeSlider.setProperty("endValue", 3);
-		await firstHandle.dragAndDrop({ x: 300, y: 1 });
+		await firstHandle.click();
+		await firstHandle.keys("ArrowRight");
+		await firstHandle.keys("ArrowRight");
 
 		const changeEventStartValue = await browser.execute(() => document.querySelector("#change-event-startValue").innerText);
 		const changeEventEndValue = await browser.execute(() => document.querySelector("#change-event-endValue").innerText);
 
-		assert.strictEqual(changeEventStartValue, "3", "Values are swapped prior to the firing of change event");
-		assert.strictEqual(changeEventEndValue, "4", "Values are swapped prior to the firing of change event");
+		assert.strictEqual(changeEventStartValue, "2", "Values are swapped prior to the firing of change event");
+		assert.strictEqual(changeEventEndValue, "3", "Values are swapped prior to the firing of change event");
+	});
+
+	it("Should not fire change event if the values are the same after interaction", async () => {
+		await browser.url(`test/pages/RangeSlider.html`);
+
+		const rangeSlider = await browser.$("#test-slider");
+		const firstHandle = await rangeSlider.shadow$(".ui5-slider-handle--start");
+
+		await rangeSlider.setProperty("startValue", 0);
+		await firstHandle.click();
+		await firstHandle.keys("Home");
+
+		const changeEventStartValue = await browser.execute(() => document.querySelector("#change-event-startValue").innerText);
+
+		assert.strictEqual(changeEventStartValue, "", "Change event is not fired if no value is changed");
+	});
+
+	it("Should fire input event with correctly swiped values", async () => {
+		await browser.url(`test/pages/RangeSlider.html`);
+
+		const rangeSlider = await browser.$("#test-slider");
+		const firstHandle = await rangeSlider.shadow$(".ui5-slider-handle--start");
+
+		await firstHandle.click();
+		await firstHandle.keys("ArrowRight");
+		await firstHandle.keys("ArrowRight");
+
+		const inputEventStartValue = await browser.execute(() => document.querySelector("#input-event-startValue").innerText);
+		const inputEventEndValue = await browser.execute(() => document.querySelector("#input-event-endValue").innerText);
+
+		assert.strictEqual(inputEventStartValue, "2", "The input event is fired with the correct values");
+		assert.strictEqual(inputEventEndValue, "3", "The input event is fired with the correct values");
 	});
 
 	it("Should not fire change event after user interaction is finished if the current value is the same as the one at the start of the action", async () => {
@@ -886,20 +919,23 @@ describe("Accessibility: Testing keyboard handling", async () => {
 	});
 
 	it("When one handle come across the other and the values are swapped the focus must be switched between the handles", async () => {
+		await browser.url(`test/pages/RangeSlider.html`);
+
 		const rangeSlider = await browser.$("#basic-range-slider");
 		const startHandle = await rangeSlider.shadow$(".ui5-slider-handle--start");
 		const endHandle = await rangeSlider.shadow$(".ui5-slider-handle--end");
 
-		await rangeSlider.setProperty("endValue", 20);
 		await startHandle.click();
 		await browser.keys("End");
+		await browser.pause(300);
 
 		let innerFocusedElement = await browser.custom$("activeElement", "#basic-range-slider");
 
-		assert.strictEqual(await rangeSlider.getProperty("endValue"), 100, "The original end-value is set to min and switched as a start-value");
+		assert.strictEqual(await rangeSlider.getProperty("endValue"), 100, "The original start-value is set to min and switched as a end-value");
 		assert.strictEqual(await browser.$(innerFocusedElement).getAttribute("class"), await endHandle.getAttribute("class"), "Range Slider second handle now has the shadowDom focus");
 
 		await browser.keys("Home");
+		await browser.pause(300);
 
 		innerFocusedElement = await browser.custom$("activeElement", "#basic-range-slider");