Skip to content

Commit 4727fb5

Browse files
ndeshevndeshev
authored
refactor(ui5-range-slider): refactor ui5-range-slider component (#13316)
* feat(ui5-range-slider): refactor ui5-range-slider component Extracted reusable SliderHandle and SliderScale components from RangeSlider for better modularity and code reuse with Slider Refactored RangeSliderTemplate to use composition with the new sub-components instead of inline markup Added progress bar ARIA and focus properties to SliderScale to support RangeSlider's range selection feature Consolidated and cleaned up CSS theme parameters across all themes (Horizon, Fiori 3, HCB/HCW variants) Removed aria-keyshortcuts attributes from the handles to avoid duplicated speech output Removed the wrong aria-describedby references Fixes various minor issues FIXES: #13348
1 parent c765630 commit 4727fb5

64 files changed

Lines changed: 2574 additions & 3054 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎packages/main/cypress/specs/RangeSlider.cy.tsx‎

Lines changed: 1865 additions & 1815 deletions
Large diffs are not rendered by default.

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

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -512,28 +512,6 @@ describe("Accessibility", () => {
512512
.invoke('css', 'padding', '100px')
513513
})
514514

515-
it("aria-keyshortcuts should not be set on regular slider", () => {
516-
cy.mount(
517-
<Slider min={0} max={20}></Slider>
518-
);
519-
520-
cy.get("[ui5-slider]")
521-
.shadow()
522-
.find("[ui5-slider-handle]")
523-
.should("not.have.attr", "aria-keyshortcuts");
524-
});
525-
526-
it("aria-keyshortcuts should be set on slider with editable tooltips", () => {
527-
cy.mount(
528-
<Slider editableTooltip={true} min={0} max={20}></Slider>
529-
);
530-
531-
cy.get("[ui5-slider]")
532-
.shadow()
533-
.find("[ui5-slider-handle]")
534-
.should("have.attr", "aria-keyshortcuts");
535-
});
536-
537515
it("should apply associated label text as aria-label on the slider element", () => {
538516
const labelText = "label for slider";
539517
cy.mount(

‎packages/main/cypress/specs/VerticalAlignment.cy.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,6 @@ describe("Vertical Alignment", () => {
7272
cy.get("#container").should("have.css", "height", "44px");
7373
cy.get("#container2").should("have.css", "height", "44px");
7474
cy.get("#container3").should("have.css", "height", "48px");
75-
cy.get("#container4").should("have.css", "height", "53px");
75+
cy.get("#container4").should("have.css", "height", "44px");
7676
});
7777
});

‎packages/main/src/RangeSlider.ts‎

Lines changed: 126 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
isEnd,
1212
isF2,
1313
} from "@ui5/webcomponents-base/dist/Keys.js";
14+
import { getAssociatedLabelForTexts } from "@ui5/webcomponents-base/dist/util/AccessibilityTextsHelper.js";
1415
import SliderBase from "./SliderBase.js";
1516
import RangeSliderTemplate from "./RangeSliderTemplate.js";
1617
import type SliderTooltip from "./SliderTooltip.js";
@@ -93,7 +94,7 @@ type AffectedValue = "startValue" | "endValue";
9394
languageAware: true,
9495
formAssociated: true,
9596
template: RangeSliderTemplate,
96-
styles: [SliderBase.styles, rangeSliderStyles],
97+
styles: [rangeSliderStyles],
9798
})
9899
class RangeSlider extends SliderBase implements IFormInputElement {
99100
/**
@@ -106,7 +107,7 @@ class RangeSlider extends SliderBase implements IFormInputElement {
106107
@property({ type: Number })
107108
set startValue(value: number) {
108109
this._startValue = value;
109-
this.tooltipStartValue = value.toString();
110+
this.tooltipStartValue = value?.toString() ?? "";
110111
}
111112

112113
get startValue(): number {
@@ -123,7 +124,7 @@ class RangeSlider extends SliderBase implements IFormInputElement {
123124
@property({ type: Number })
124125
set endValue(value: number) {
125126
this._endValue = value;
126-
this.tooltipEndValue = value.toString();
127+
this.tooltipEndValue = value?.toString() ?? "";
127128
}
128129

129130
get endValue(): number {
@@ -145,6 +146,9 @@ class RangeSlider extends SliderBase implements IFormInputElement {
145146
@property({ type: Boolean })
146147
rangePressed = false;
147148

149+
@property({ type: Boolean })
150+
_progressFocused = false;
151+
148152
@property({ type: Boolean })
149153
_isStartValueValid = false;
150154

@@ -169,6 +173,7 @@ class RangeSlider extends SliderBase implements IFormInputElement {
169173
_lastValidStartValue: string;
170174
_lastValidEndValue: string;
171175
_areInputValuesSwapped = false;
176+
_onDocumentClickBound: (e: MouseEvent) => void;
172177

173178
@i18n("@ui5/webcomponents")
174179
static i18nBundle: I18nBundle;
@@ -192,6 +197,29 @@ class RangeSlider extends SliderBase implements IFormInputElement {
192197
this._stateStorage.endValue = undefined;
193198
this._lastValidStartValue = this.min.toString();
194199
this._lastValidEndValue = this.max.toString();
200+
this._onDocumentClickBound = this._onDocumentClick.bind(this);
201+
}
202+
203+
onEnterDOM() {
204+
document.addEventListener("mousedown", this._onDocumentClickBound, true);
205+
}
206+
207+
onExitDOM() {
208+
document.removeEventListener("mousedown", this._onDocumentClickBound, true);
209+
}
210+
211+
/**
212+
* Handles document-level clicks to clear progress focus when clicking outside.
213+
* @private
214+
*/
215+
_onDocumentClick(e: MouseEvent) {
216+
const clickedInside = e.composedPath().includes(this);
217+
218+
if (!clickedInside) {
219+
if (this._tooltipsOpen) {
220+
this._tooltipsOpen = false;
221+
}
222+
}
195223
}
196224

197225
get _ariaDisabled() {
@@ -326,6 +354,7 @@ class RangeSlider extends SliderBase implements IFormInputElement {
326354
this._setAffectedValue(undefined);
327355
this._startValueInitial = undefined;
328356
this._endValueInitial = undefined;
357+
this._progressFocused = false;
329358

330359
if (this.showTooltip && !(e.relatedTarget as HTMLInputElement)?.hasAttribute("ui5-slider-tooltip")) {
331360
this._tooltipsOpen = false;
@@ -358,7 +387,9 @@ class RangeSlider extends SliderBase implements IFormInputElement {
358387
this._endValueAtBeginningOfAction = this.endValue;
359388

360389
if (isEscape(e)) {
361-
this.update(undefined, this._startValueInitial, this._endValueInitial);
390+
if (this._startValueInitial !== undefined && this._endValueInitial !== undefined) {
391+
this.update(undefined, this._startValueInitial, this._endValueInitial);
392+
}
362393
return;
363394
}
364395

@@ -387,13 +418,14 @@ class RangeSlider extends SliderBase implements IFormInputElement {
387418

388419
// Update a single value if one of the handles is focused or the range if not already at min or max
389420
const ctor = this.constructor as typeof RangeSlider;
421+
const stepPrecision = ctor._getDecimalPrecisionOfNumber(this._effectiveStep);
390422
if (affectedValue && !this._isPressInCurrentRange) {
391423
const propValue = this[affectedValue as keyof RangeSlider] as number;
392-
const newValue = ctor.clipValue(newValueOffset + propValue, min, max);
424+
const newValue = Number(ctor.clipValue(newValueOffset + propValue, min, max).toFixed(stepPrecision));
393425
this.update(affectedValue, newValue, undefined);
394426
} else if ((newValueOffset < 0 && this.startValue > min) || (newValueOffset > 0 && this.endValue < max)) {
395-
const newStartValue = ctor.clipValue(newValueOffset + this.startValue, min, max);
396-
const newEndValue = ctor.clipValue(newValueOffset + this.endValue, min, max);
427+
const newStartValue = Number(ctor.clipValue(newValueOffset + this.startValue, min, max).toFixed(stepPrecision));
428+
const newEndValue = Number(ctor.clipValue(newValueOffset + this.endValue, min, max).toFixed(stepPrecision));
397429
this.update(affectedValue, newStartValue, newEndValue);
398430
}
399431

@@ -415,7 +447,9 @@ class RangeSlider extends SliderBase implements IFormInputElement {
415447
this._setAffectedValue("endValue");
416448
}
417449

418-
if (this.shadowRoot!.activeElement === this._progressBar) {
450+
// Progress bar is inside SliderScale's shadow DOM, so check the nested activeElement
451+
const sliderScale = this.shadowRoot!.querySelector<HTMLElement>("[ui5-slider-scale]");
452+
if (sliderScale?.shadowRoot?.activeElement === this._progressBar) {
419453
this._setAffectedValue(undefined);
420454
}
421455

@@ -430,8 +464,9 @@ class RangeSlider extends SliderBase implements IFormInputElement {
430464
_homeEndForSelectedRange(e: KeyboardEvent, affectedValue: string, min: number, max: number) {
431465
const newValueOffset = this._handleActionKeyPressBase(e, affectedValue);
432466
const ctor = this.constructor as typeof RangeSlider;
433-
const newStartValue = ctor.clipValue(newValueOffset + this.startValue, min, max);
434-
const newEndValue = ctor.clipValue(newValueOffset + this.endValue, min, max);
467+
const stepPrecision = ctor._getDecimalPrecisionOfNumber(this._effectiveStep);
468+
const newStartValue = Number(ctor.clipValue(newValueOffset + this.startValue, min, max).toFixed(stepPrecision));
469+
const newEndValue = Number(ctor.clipValue(newValueOffset + this.endValue, min, max).toFixed(stepPrecision));
435470

436471
this.update(undefined, newStartValue, newEndValue);
437472
}
@@ -481,15 +516,30 @@ class RangeSlider extends SliderBase implements IFormInputElement {
481516
return;
482517
}
483518

484-
// Calculate the new value from the press position of the event
519+
// Pre-calculate whether the press is in the current range before handleDownBase
520+
// This is needed so focusInnerElement() knows where to focus
521+
const ctor = this.constructor as typeof RangeSlider;
522+
const pageX = ctor.getPageXValueFromEvent(e);
523+
const tempValue = ctor.getValueFromInteraction(e, this._effectiveStep, this._effectiveMin, this._effectiveMax, this.getBoundingClientRect(), this.directionStart);
524+
const isInRange = tempValue >= this.startValue && tempValue <= this.endValue;
525+
const startHandle = this._startHandle;
526+
const endHandle = this._endHandle;
527+
const inStartHandle = startHandle && pageX >= startHandle.getBoundingClientRect().left && pageX <= startHandle.getBoundingClientRect().right;
528+
const inEndHandle = endHandle && pageX >= endHandle.getBoundingClientRect().left && pageX <= endHandle.getBoundingClientRect().right;
529+
485530
const newValue = this.handleDownBase(e);
486531

487-
// Determine the rest of the needed details from the start of the interaction.
488-
this._saveInteractionStartData(e, newValue);
532+
if (isInRange && !inStartHandle && !inEndHandle) {
533+
this._setIsPressInCurrentRange(true);
534+
this._progressFocused = true;
535+
this.rangePressed = true;
536+
} else {
537+
this._progressFocused = false;
538+
this.rangePressed = false;
539+
}
489540

490-
this.rangePressed = this._isPressInCurrentRange;
541+
this._saveInteractionStartData(e, newValue);
491542

492-
// Do not yet update the RangeSlider if press is in range or over a handle.
493543
if (this._isPressInCurrentRange || this._handeIsPressed) {
494544
this._handeIsPressed = false;
495545
return;
@@ -509,7 +559,7 @@ class RangeSlider extends SliderBase implements IFormInputElement {
509559
* @private
510560
*/
511561
_saveInteractionStartData(e: TouchEvent | MouseEvent, newValue: number) {
512-
const progressBarDom = this.shadowRoot!.querySelector(".ui5-slider-progress")!.getBoundingClientRect();
562+
const progressBarDom = this._progressBar?.getBoundingClientRect();
513563

514564
// Save the state of the value properties on the start of the interaction
515565
this._startValueAtBeginningOfAction = this.startValue;
@@ -521,7 +571,9 @@ class RangeSlider extends SliderBase implements IFormInputElement {
521571
// Which element of the Range Slider is pressed and which value property to be modified on further interaction
522572
this._pressTargetAndAffectedValue(this._initialPageXPosition, newValue);
523573
// Use the progress bar to save the initial coordinates of the start-handle when the interaction begins.
524-
this._initialStartHandlePageX = this.directionStart === "left" ? progressBarDom.left : progressBarDom.right;
574+
if (progressBarDom) {
575+
this._initialStartHandlePageX = this.directionStart === "left" ? progressBarDom.left : progressBarDom.right;
576+
}
525577
}
526578

527579
/**
@@ -606,8 +658,8 @@ class RangeSlider extends SliderBase implements IFormInputElement {
606658
* @private
607659
*/
608660
_pressTargetAndAffectedValue(clientX: number, value: number) {
609-
const startHandle = this.shadowRoot!.querySelector(".ui5-slider-handle--start")!;
610-
const endHandle = this.shadowRoot!.querySelector(".ui5-slider-handle--end")!;
661+
const startHandle = this._startHandle;
662+
const endHandle = this._endHandle;
611663

612664
// Check if the press point is in the bounds of any of the Range Slider handles
613665
const handleStartDomRect = startHandle.getBoundingClientRect();
@@ -690,16 +742,16 @@ class RangeSlider extends SliderBase implements IFormInputElement {
690742
const affectedValue = this._valueAffected;
691743

692744
if (this._isPressInCurrentRange || !affectedValue) {
693-
this._progressBar.focus();
745+
this._progressBar?.focus();
694746
}
695747

696748
if ((affectedValue === "startValue" && !isReversed) || (affectedValue === "endValue" && isReversed)) {
697-
this._startHandle.focus();
749+
this._startHandle?.focus();
698750
this.bringToFrontTooltip("start");
699751
}
700752

701753
if ((affectedValue === "endValue" && !isReversed) || (affectedValue === "startValue" && isReversed)) {
702-
this._endHandle.focus();
754+
this._endHandle?.focus();
703755
this.bringToFrontTooltip("end");
704756
}
705757
}
@@ -732,7 +784,11 @@ class RangeSlider extends SliderBase implements IFormInputElement {
732784
const ctor = this.constructor as typeof RangeSlider;
733785
startValue = ctor.clipValue(startValue, min, max - selectedRange);
734786

735-
return [startValue, startValue + selectedRange];
787+
const stepPrecision = ctor._getDecimalPrecisionOfNumber(this._effectiveStep);
788+
const endValue = Number((startValue + selectedRange).toFixed(stepPrecision));
789+
startValue = Number(startValue.toFixed(stepPrecision));
790+
791+
return [startValue, endValue];
736792
}
737793

738794
/**
@@ -830,6 +886,12 @@ class RangeSlider extends SliderBase implements IFormInputElement {
830886
}
831887

832888
_onTooltipChange(e: CustomEvent) {
889+
// Skip if this is a focusout change event triggered by the swap focus change
890+
if (this._areInputValuesSwapped) {
891+
this._areInputValuesSwapped = false;
892+
return;
893+
}
894+
833895
const tooltip = e.target as SliderTooltip;
834896
const isStart = tooltip.hasAttribute("data-sap-ui-start-value");
835897
const inputValue = parseFloat(e.detail.value as string);
@@ -849,9 +911,11 @@ class RangeSlider extends SliderBase implements IFormInputElement {
849911
const clampedValue = Math.min(this.max, Math.max(this.min, inputValue));
850912

851913
if (isStart) {
914+
this.tooltipStartValueState = ValueState.None;
852915
this.startValue = clampedValue;
853916
this._lastValidStartValue = clampedValue.toString();
854917
} else {
918+
this.tooltipEndValueState = ValueState.None;
855919
this.endValue = clampedValue;
856920
this._lastValidEndValue = clampedValue.toString();
857921
}
@@ -898,10 +962,12 @@ class RangeSlider extends SliderBase implements IFormInputElement {
898962
}
899963

900964
_onTooltipOpen() {
901-
const ctor = this.constructor as typeof RangeSlider;
902-
const stepPrecision = ctor._getDecimalPrecisionOfNumber(this._effectiveStep);
903-
this.tooltipStartValue = this.startValue.toFixed(stepPrecision);
904-
this.tooltipEndValue = this.endValue.toFixed(stepPrecision);
965+
if (!this.startValue || !this.endValue) {
966+
return;
967+
}
968+
969+
this.tooltipStartValue = this.startValue.toString();
970+
this.tooltipEndValue = this.endValue.toString();
905971
}
906972

907973
_onTooltipInput(e: CustomEvent) {
@@ -1004,15 +1070,16 @@ class RangeSlider extends SliderBase implements IFormInputElement {
10041070
}
10051071

10061072
get _startHandle() {
1007-
return this.shadowRoot!.querySelector<HTMLElement>(".ui5-slider-handle--start")!;
1073+
return this.shadowRoot!.querySelector<HTMLElement>("[ui5-slider-handle][handle-type='Start']")!;
10081074
}
10091075

10101076
get _endHandle() {
1011-
return this.shadowRoot!.querySelector<HTMLElement>(".ui5-slider-handle--end")!;
1077+
return this.shadowRoot!.querySelector<HTMLElement>("[ui5-slider-handle][handle-type='End']")!;
10121078
}
10131079

10141080
get _progressBar() {
1015-
return this.shadowRoot!.querySelector<HTMLElement>(".ui5-slider-progress")!;
1081+
const sliderScale = this.shadowRoot!.querySelector<HTMLElement>("[ui5-slider-scale]");
1082+
return sliderScale?.shadowRoot?.querySelector<HTMLElement>(".ui5-slider-progress") ?? null;
10161083
}
10171084

10181085
get _ariaLabelledByStartHandleText() {
@@ -1023,6 +1090,35 @@ class RangeSlider extends SliderBase implements IFormInputElement {
10231090
return this.accessibleName ? ["ui5-slider-accName", "ui5-slider-endHandleDesc"].join(" ").trim() : "ui5-slider-endHandleDesc";
10241091
}
10251092

1093+
/**
1094+
* @private
1095+
*/
1096+
get _ariaLabelStartHandle() {
1097+
return this._getAriaLabelHandle(this._ariaHandlesText.startHandleText || "");
1098+
}
1099+
1100+
/**
1101+
* @private
1102+
*/
1103+
get _ariaLabelEndHandle() {
1104+
return this._getAriaLabelHandle(this._ariaHandlesText.endHandleText || "");
1105+
}
1106+
1107+
_getAriaLabelHandle(handleDescription: string) {
1108+
const associatedLabelText = getAssociatedLabelForTexts(this);
1109+
const hasAccessibleName = !!this.accessibleName;
1110+
1111+
let labelText = hasAccessibleName
1112+
? `${this.accessibleName} ${handleDescription}`
1113+
: handleDescription;
1114+
1115+
if (!hasAccessibleName && associatedLabelText) {
1116+
labelText = `${associatedLabelText} ${labelText}`;
1117+
}
1118+
1119+
return labelText;
1120+
}
1121+
10261122
get _ariaLabelledByInputText() {
10271123
return RangeSlider.i18nBundle.getText(SLIDER_TOOLTIP_INPUT_LABEL);
10281124
}

0 commit comments

Comments
 (0)