refactor(ui5-slider): slider handle and scale extraction by MapTo0 · Pull Request #13033 · UI5/webcomponents

MapTo0 · 2026-02-05T14:30:02Z

No description provided.

ui5-webcomponents-bot · 2026-02-05T14:38:03Z

🚀 Deployed on https://pr-13033--ui5-webcomponents-preview.netlify.app

ndeshev · 2026-02-12T10:38:11Z

packages/main/src/SliderTemplate.tsx

+				disabled={slider.disabled}
+				aria-orientation="horizontal"
+				part="handle-container"
+				aria-valuemin={slider.min}


Why there is no longer role="slider" accessibility attribute?

Probably, that causes or at least is related the accessibility issue I mentioned in one of my previous comments

ndeshev · 2026-02-12T10:40:19Z

packages/main/src/SliderTemplate.tsx

+				tabIndex={slider.disabled ? -1 : 0}
+				disabled={slider.disabled}
+				aria-orientation="horizontal"
+				part="handle-container"


Previously the part name was just "handle". This will break the apps that already uses it.

ndeshev · 2026-02-12T10:45:15Z

packages/main/src/SliderScale.ts

+	}
+
+	get _tickmarksCount() {
+		return (this.max - this.min) / this.step;


I think it will be good to return if step is 0 to avoid division by 0, in fact, setting the step property to 0 breaks the code in more than one place. in get _allTickmarks() the script throws an exception and at least my browser crashes. I think we should at least avoid uncaught exceptions in case of unwanted property values.

ndeshev · 2026-02-12T10:48:05Z

packages/main/src/SliderScale.ts

+	@property({ type: Number })
+	min = 0;
+
+	@property({ type: Number })


We should document these props at least a little bit :) You've done it in SliderHandle.ts

ndeshev · 2026-02-12T10:51:01Z

packages/main/src/Slider.ts

-			this.storePropertyState("labelInterval");
-		}
+	_handleResize() {
+		// TODO: Remove after refactoring Base and RangeSlider


ndeshev · 2026-02-12T10:53:30Z

packages/main/src/SliderScale.ts

+		} else {
+			// Calculate if labels would overlap based on their estimated width
+			const visibleLabelCount = Math.ceil((tickmarksCount - 1) / this.labelInterval) + 1;
+			const estimatedLabelWidth = 32; // Estimated width in pixels for a label (2rem = ~32px)


Could we somehow use a CSS var instead of hardcoded number, in case it changes depending on a theme or theme density?

niyap

The accessibility part is broken - slider's value is not announced as well as how to increase or decrease the value.
Now, when an invalid value is set, it is not reset anymore. The problem is that that invalid value is set as value of aria-valuenow of the handle and end-value of the scale. If that change is desired, could you please check if it is appropriate from accessibility perspective
According to the UX specification, the first and the last label should always be visible. In such case, what should happen, if you have the following situation: on the test page, slider with id="slider-tickmarks-labels" enhance with step="3" and label-interval="2" -> the last visible one is 16 as it is the last available in the predefined range. Should we visualised 20 at the end as the end of the interval or leave it without label as there is next valid one in out of range.
Tooltips and inputs values are not updated correctly if you just click somewhere over a tickmark of the slider. Works as expected if you drag the handle.
If you set min value greater than the max one, the slider is broken at all. Labels and tickmarks are not rendered. Handle cound not be dragged, tooltip is not updated and etc. Do you know what should we do in such case, I am not sure if it a valid one but if not, we can add some documentation/note about that
In the documentation we state that if the step is equal to 0, slider handle movement is disabled. When negative number or value other than a number, the component fallbacks to its default value. -> currently if you set the step to 0, the slider is even not rendered and а console error is logged. Please, also check when the step value has been changed programmatically.
If you have editable tooltip and you change its value to a invalid one -> value state is changed to error, then you update it again with a valid one and press Enter, the handle is moved to the correct point of the slider, the tooltip now shows a valid value, but the value state is not updated, still remains error. Even on focusout, if you hover over the handle, you will notice that it is still invalid
In RTL mode, the end dot is placed at the beginning and has blue color.

niyap · 2026-02-13T21:22:41Z

packages/main/src/Slider.ts

 	 *
 	 */
 	onBeforeRendering() {
-		if (!this.isCurrentStateOutdated()) {


Please, update the documentation as it describes the deleted logic.

niyap · 2026-02-16T07:30:30Z

packages/main/cypress/specs/Slider.cy.tsx

-		cy.get("@slider").should("have.value", 1, "The current value should be 'stepified' by 7");
-	});
-
-	it("If the step property or the labelInterval are changed, the tickmarks and labels must be updated also", () => {


Isn't it good to have such a test?

niyap · 2026-02-16T07:49:47Z

packages/main/cypress/specs/Slider.cy.tsx

-			.and('have.length', 6);
-	});
-
-	it("If the min and max properties are changed, the tickmarks and labels must be updated also.", () => {


This one as well

niyap · 2026-02-16T12:53:06Z

packages/main/src/Slider.ts

 	 * @formProperty
 	 * @public
 	 */
 	@property({ type: Number })


If we have negative value both for min and max, for example min="-20" and max="-10" and we do not define value. Value is 0 by default which is out of the range, if we have tooltip, it also shows 0 (the default value)(even not marked as invalid value of the range initially.

niyap · 2026-02-16T12:58:26Z

packages/main/src/SliderHandle.ts

+	@property({ type: Boolean })
+	active = false;
+
+	@property()


It will be good if we add some documentation here as well and probably marked it as private

niyap · 2026-02-16T13:18:43Z

packages/main/src/SliderHandle.ts

+	@property()
+	orientation: `${SliderScaleOrientation}` = "Horizontal";
+
+	get _handlePosition() {


Do we need it and use it somewhere, as I noticed that _handlePosition with the same logic is implemented and used in SliderTemplate

niyap · 2026-02-16T14:59:59Z

packages/main/src/SliderScale.ts

+			const visibleLabelCount = Math.ceil((tickmarksCount - 1) / this.labelInterval) + 1;
+			const estimatedLabelWidth = 32; // Estimated width in pixels for a label (2rem = ~32px)
+			const requiredWidth = visibleLabelCount * estimatedLabelWidth;
+			this._labelsOverlapping = containerSize < requiredWidth;


If you have a slider with labels and tickmarks and you update the step programmatically, the labels are not rendered correctly. The last one is not visualised where it should always be. I tested on "Slider with steps, input tooltips, tickmarks and labels" sample, changing the step value on change

niyap · 2026-02-16T21:09:11Z

packages/main/src/SliderScale.ts

+
+	/**
+	 * Vertical orientation
+	 * @public


Should we mark it as public when it is not yet available. Or can we remove vertical as an option and add it in future

niyap · 2026-02-16T22:02:26Z

packages/main/src/SliderTemplate.tsx

+				disabled={slider.disabled}
+				aria-orientation="horizontal"
+				part="handle-container"
+				aria-valuemin={slider.min}


Probably, that causes or at least is related the accessibility issue I mentioned in one of my previous comments

niyap · 2026-02-16T22:26:20Z

packages/main/src/Slider.ts

@@ -263,7 +232,7 @@

 	_onTooltipOpen() {


Check the sample with the float values. Initially on hover, the tooltip shows the value with the two decimals places precision but if you click couple times over the handle (without changing the value), the tooltip value is shown just 10.

refactor(ui5-slider): slider handle extraction

160781b

MapTo0 changed the title ~~refactor(ui5-slider): slider handle extraction~~ refactor(ui5-slider): slider handle and scale extraction Feb 5, 2026

MapTo0 requested review from ndeshev and niyap February 5, 2026 14:30

ui5-webcomponents-bot temporarily deployed to preview February 5, 2026 14:38 Inactive

Merge branch 'main' into slider-refactoring-2

fe25fa2

ui5-webcomponents-bot temporarily deployed to preview February 9, 2026 08:53 Inactive

ndeshev reviewed Feb 12, 2026

View reviewed changes

niyap reviewed Feb 16, 2026

View reviewed changes

Conversation

MapTo0 commented Feb 5, 2026

Uh oh!

ui5-webcomponents-bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeshev Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeshev Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niyap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ui5-webcomponents-bot commented Feb 5, 2026 •

edited

Loading

ndeshev Feb 12, 2026 •

edited

Loading

ndeshev Feb 12, 2026 •

edited

Loading

niyap left a comment •

edited

Loading