From d3ac7abaf1b7089b9392ff38d85f3bad29c3b453 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Wed, 4 Sep 2024 17:53:47 +0200 Subject: [PATCH 01/10] feat(sbb-slider): implement FormAssociated interface --- .../slider.snapshot.spec.snap.js | 105 ++++++++++- src/elements/slider/readme.md | 25 ++- src/elements/slider/slider.ts | 171 +++++++++++------- 3 files changed, 220 insertions(+), 81 deletions(-) diff --git a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js index 904233456d..2849bb8e91 100644 --- a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js +++ b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js @@ -6,7 +6,6 @@ snapshots["sbb-slider renders DOM"] = aria-valuemax="100" aria-valuemin="0" aria-valuenow="1" - name="" role="slider" tabindex="0" value="1" @@ -54,7 +53,6 @@ snapshots["sbb-slider renders with properties DOM"] = end-icon="walk-fast-small" max="500" min="0" - name="" role="slider" start-icon="walk-slow-small" tabindex="0" @@ -119,7 +117,6 @@ snapshots["sbb-slider renders disabled DOM"] = end-icon="walk-fast-small" max="500" min="0" - name="" role="slider" start-icon="walk-slow-small" value="100" @@ -183,7 +180,6 @@ snapshots["sbb-slider renders readonly DOM"] = end-icon="walk-fast-small" max="500" min="0" - name="" readonly="" role="slider" start-icon="walk-slow-small" @@ -398,3 +394,104 @@ snapshots["sbb-slider renders readonly A11y tree Firefox"] = `; /* end snapshot sbb-slider renders readonly A11y tree Firefox */ +snapshots["sbb-slider renders in form DOM"] = +` + +`; +/* end snapshot sbb-slider renders in form DOM */ + +snapshots["sbb-slider renders in form Shadow DOM"] = +`
+
+ + +
+ +
+
+
+
+
+
+
+ + +
+
+`; +/* end snapshot sbb-slider renders in form Shadow DOM */ + +snapshots["sbb-slider renders in form A11y tree Chrome"] = +`

+ { + "role": "WebArea", + "name": "", + "children": [ + { + "role": "slider", + "name": "", + "valuetext": "", + "valuemin": 0, + "valuemax": 10, + "orientation": "horizontal", + "value": 1 + }, + { + "role": "slider", + "name": "", + "valuetext": "", + "valuemin": 0, + "valuemax": 10, + "orientation": "horizontal", + "value": 1 + } + ] +} +

+`; +/* end snapshot sbb-slider renders in form A11y tree Chrome */ + +snapshots["sbb-slider renders in form A11y tree Firefox"] = +`

+ { + "role": "document", + "name": "", + "children": [ + { + "role": "slider", + "name": "", + "valuetext": "1", + "value": "1" + }, + { + "role": "slider", + "name": "", + "valuetext": "1", + "value": "1" + } + ] +} +

+`; +/* end snapshot sbb-slider renders in form A11y tree Firefox */ + diff --git a/src/elements/slider/readme.md b/src/elements/slider/readme.md index 8f5f9132b2..32bd5cd129 100644 --- a/src/elements/slider/readme.md +++ b/src/elements/slider/readme.md @@ -70,25 +70,24 @@ The `sbb-slider` has the following behaviour on keypress when focused: ## Properties -| Name | Attribute | Privacy | Type | Default | Description | -| --------------- | ----------------- | ------- | ---------------------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | -| `disabled` | `disabled` | public | `boolean` | `false` | Whether the component is disabled. | -| `endIcon` | `end-icon` | public | `string \| undefined` | | Name of the icon at component's end, which will be forward to the nested `sbb-icon`. | -| `form` | `form` | public | `string \| undefined` | | The
element to associate the inner HTMLInputElement with. | -| `max` | `max` | public | `string \| undefined` | `'100'` | Maximum acceptable value for the inner HTMLInputElement. | -| `min` | `min` | public | `string \| undefined` | `'0'` | Minimum acceptable value for the inner HTMLInputElement. | -| `name` | `name` | public | `string \| undefined` | `''` | Name of the inner HTMLInputElement. | -| `readonly` | `readonly` | public | `boolean \| undefined` | `false` | Readonly state for the inner HTMLInputElement. Since the input range does not allow this attribute, it will be merged with the `disabled` one. | -| `startIcon` | `start-icon` | public | `string \| undefined` | | Name of the icon at component's start, which will be forward to the nested `sbb-icon`. | -| `value` | `value` | public | `string \| undefined` | `''` | Value for the inner HTMLInputElement. | -| `valueAsNumber` | `value-as-number` | public | `number \| undefined` | | Numeric value for the inner HTMLInputElement. | +| Name | Attribute | Privacy | Type | Default | Description | +| --------------- | ----------------- | ------- | ------------------------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `disabled` | `disabled` | public | `boolean` | `false` | Whether the component is disabled. | +| `endIcon` | `end-icon` | public | `string \| undefined` | | Name of the icon at component's end, which will be forward to the nested `sbb-icon`. | +| `form` | - | public | `HTMLFormElement \| null` | | Returns the form owner of internals target element. | +| `max` | `max` | public | `string` | `'100'` | Maximum acceptable value for the inner HTMLInputElement. | +| `min` | `min` | public | `string` | `'0'` | Minimum acceptable value for the inner HTMLInputElement. | +| `name` | `name` | public | `string` | | Name of the form element. Will be read from name attribute. | +| `readonly` | `readonly` | public | `boolean \| undefined` | `false` | Readonly state for the inner HTMLInputElement. Since the input range does not allow this attribute, it will be merged with the `disabled` one. | +| `startIcon` | `start-icon` | public | `string \| undefined` | | Name of the icon at component's start, which will be forward to the nested `sbb-icon`. | +| `value` | `value` | public | `string \| null` | `null` | Value of the form element. If no value is provided, default is the middle point between min and max. | +| `valueAsNumber` | `value-as-number` | public | `number \| null` | | Numeric value for the inner HTMLInputElement. | ## Events | Name | Type | Description | Inherited From | | ----------- | ------------------- | -------------------------------------------------------------------------------- | -------------- | | `didChange` | `CustomEvent` | Deprecated. used for React. Will probably be removed once React 19 is available. | | -| `input` | `InputEvent` | | | ## Slots diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index 6a6b869663..5115d339b7 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -8,7 +8,12 @@ import { SbbConnectedAbortController } from '../core/controllers.js'; import { hostAttributes } from '../core/decorators.js'; import { setOrRemoveAttribute } from '../core/dom.js'; import { EventEmitter, forwardEventToHost } from '../core/eventing.js'; -import { SbbDisabledTabIndexActionMixin } from '../core/mixins.js'; +import { + type FormRestoreReason, + type FormRestoreState, + SbbDisabledTabIndexActionMixin, + SbbFormAssociatedMixin, +} from '../core/mixins.js'; import style from './slider.scss?lit&inline'; @@ -25,29 +30,70 @@ import '../icon.js'; @hostAttributes({ role: 'slider', }) -export class SbbSliderElement extends SbbDisabledTabIndexActionMixin(LitElement) { +export class SbbSliderElement extends SbbDisabledTabIndexActionMixin( + SbbFormAssociatedMixin(LitElement), +) { public static override styles: CSSResultGroup = style; public static readonly events = { didChange: 'didChange', } as const; - /** Value for the inner HTMLInputElement. */ - @property() public value?: string = ''; + /** + * Value of the form element. + * If no value is provided, default is the middle point between min and max. + */ + @property() + public override set value(value: string | null) { + if (value) { + super.value = value; + } else { + super.value = this._getDefaultValue().toString(); + } + setOrRemoveAttribute(this, 'aria-valuenow', this.value); + this._calculateValueFraction(); + } + public override get value(): string | null { + return super.value; + } /** Numeric value for the inner HTMLInputElement. */ - @property({ attribute: 'value-as-number', type: Number }) public valueAsNumber?: number; - - /** Name of the inner HTMLInputElement. */ - @property({ reflect: true }) public name?: string = ''; - - /** The element to associate the inner HTMLInputElement with. */ - @property() public form?: string; + @property({ attribute: 'value-as-number', type: Number }) + public set valueAsNumber(value: number) { + this.value = value?.toString(); + } + public get valueAsNumber(): number | null { + return Number(this.value); + } /** Minimum acceptable value for the inner HTMLInputElement. */ - @property() public min?: string = '0'; + @property() + public set min(value: string) { + if (!value) { + return; + } + + this._min = value; + this._calculateValueFraction(); + } + public get min(): string { + return this._min; + } + private _min: string = '0'; /** Maximum acceptable value for the inner HTMLInputElement. */ - @property() public max?: string = '100'; + @property() + public set max(value: string) { + if (!value) { + return; + } + + this._max = value; + this._calculateValueFraction(); + } + public get max(): string { + return this._max; + } + private _max: string = '100'; /** * Readonly state for the inner HTMLInputElement. @@ -84,66 +130,65 @@ export class SbbSliderElement extends SbbDisabledTabIndexActionMixin(LitElement) super.connectedCallback(); const signal = this._abort.signal; this.addEventListener('keydown', (e) => this._handleKeydown(e), { signal }); - this._handleChange(); + + if (!this.value) { + this.value = this._getDefaultValue(); + } } protected override willUpdate(changedProperties: PropertyValues): void { super.willUpdate(changedProperties); - if (changedProperties.has('value')) { - this._handleChange(Number(this.value)); - } else if (changedProperties.has('valueAsNumber')) { - this._handleChange(Number(this.valueAsNumber)); - } - if (changedProperties.has('min')) { - setOrRemoveAttribute(this, 'aria-valuemin', this.min ?? null); + setOrRemoveAttribute(this, 'aria-valuemin', this.min); } if (changedProperties.has('max')) { - setOrRemoveAttribute(this, 'aria-valuemax', this.max ?? null); + setOrRemoveAttribute(this, 'aria-valuemax', this.max); } if (changedProperties.has('readonly')) { setOrRemoveAttribute(this, 'aria-readonly', this.readonly ? 'true' : null); } } - private _syncValues(newValue: string | number): void { - if (newValue == null) { - return; - } else if (newValue && typeof newValue !== 'number') { - newValue = +newValue; - } + /** + * @internal + */ + public formResetCallback(): void { + this.value = this._getDefaultValue(); + } - this.value = newValue.toString(); - this.valueAsNumber = newValue as number; - setOrRemoveAttribute(this, 'aria-valuenow', this.value || null); + /** + * @internal + */ + public formStateRestoreCallback( + state: FormRestoreState | null, + _reason: FormRestoreReason, + ): void { + this.value = state as string | null; + } + + protected override isDisabledExternally(): boolean { + return this.formDisabled; } /** - * Recalculates the `_valueFraction` on change to correctly display the slider knob and lines. - * The first calculation happens in connectedCallback(...), so since `_rangeInput` is not yet available, - * the `min` and `max` values are used; if `value` is not provided, the default value is halfway between min and max - * (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#value). + * If no value is provided, default is the middle point between min and max + * (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#value) */ - private _handleChange(value: number = this._rangeInput?.valueAsNumber): void { - let min: number, max: number; - if (this._rangeInput) { - min = +this._rangeInput.min; - max = +this._rangeInput.max; - } else { - min = +(this.min as string); - max = +(this.max as string); - value = - this.value && this.value !== '' - ? +this.value - : this.valueAsNumber - ? this.valueAsNumber - : +(this.min as string) + (+(this.max as string) - +(this.min as string)) / 2; + private _getDefaultValue(): string { + return (+this.min + (+this.max - +this.min) / 2).toString(); + } + + private _calculateValueFraction(): void { + if (!this.value) { + return; } + const value = this.valueAsNumber!; + const min = +this.min; + const max = +this.max; + const mathFraction: number = (value - min) / (max - min); - this._valueFraction = - isNaN(mathFraction) || mathFraction < 0 ? 0 : mathFraction > 1 ? 1 : mathFraction; - this._syncValues(value); + this._valueFraction = isNaN(mathFraction) ? 0 : Math.max(0, Math.min(1, mathFraction)); } private async _handleKeydown(event: KeyboardEvent): Promise { @@ -156,26 +201,26 @@ export class SbbSliderElement extends SbbDisabledTabIndexActionMixin(LitElement) } if (event.key === 'Home') { - this._rangeInput.value = this._rangeInput.min; + this._rangeInput.value = this.min; } else if (event.key === 'End') { - this._rangeInput.value = this._rangeInput.max; + this._rangeInput.value = this.max; } else if (event.key === 'ArrowLeft' || event.key === 'ArrowDown') { this._rangeInput.stepDown(); } else if (event.key === 'ArrowRight' || event.key === 'ArrowUp') { this._rangeInput.stepUp(); } else if (event.key === 'PageDown') { - this._rangeInput.stepDown((+this._rangeInput.max - +this._rangeInput.min) / 10); + this._rangeInput.stepDown((+this.max - +this.min) / 10); } else if (event.key === 'PageUp') { - this._rangeInput.stepUp((+this._rangeInput.max - +this._rangeInput.min) / 10); + this._rangeInput.stepUp((+this.max - +this.min) / 10); } else { return; } - this._handleChange(); - this.dispatchEvent( + // We have to manually fire events because programmatic changes don't trigger them + this._rangeInput.dispatchEvent( new InputEvent('input', { bubbles: true, cancelable: true, composed: true }), ); - this._emitChange(new Event('change', { bubbles: true, cancelable: true, composed: true })); + this._rangeInput.dispatchEvent(new Event('change', { bubbles: true })); } /** Emits the change event. */ @@ -197,16 +242,14 @@ export class SbbSliderElement extends SbbDisabledTabIndexActionMixin(LitElement) > this._emitChange(event)} - @input=${() => this._handleChange()} + @input=${() => (this.value = this._rangeInput.value)} ${ref((input?: Element) => (this._rangeInput = input as HTMLInputElement))} />
From 6fe0b1ab7d20eac354e5342299bd10d197e964a3 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Wed, 4 Sep 2024 17:55:16 +0200 Subject: [PATCH 02/10] test(sbb-slider): update snapshot tests --- src/elements/slider/slider.snapshot.spec.ts | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/elements/slider/slider.snapshot.spec.ts b/src/elements/slider/slider.snapshot.spec.ts index ceb62bfbd0..25d366f44c 100644 --- a/src/elements/slider/slider.snapshot.spec.ts +++ b/src/elements/slider/slider.snapshot.spec.ts @@ -99,4 +99,26 @@ describe(`sbb-slider`, () => { testA11yTreeSnapshot(); }); + + describe('renders in form', async () => { + beforeEach(async () => { + const form = await fixture( + html` + + + `, + ); + element = form.querySelector('sbb-slider')!; + }); + + it('DOM', async () => { + await expect(element).dom.to.be.equalSnapshot(); + }); + + it('Shadow DOM', async () => { + await expect(element).shadowDom.to.be.equalSnapshot(); + }); + + testA11yTreeSnapshot(); + }); }); From a7df61cbc76a5c0f07561732c838a1fae7357d16 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Tue, 10 Sep 2024 12:17:08 +0200 Subject: [PATCH 03/10] test(sbb-slider): update tests --- .../checkbox/common/checkbox-common.spec.ts | 2 +- src/elements/slider/slider.scss | 6 +- src/elements/slider/slider.spec.ts | 246 ++++++++++++++---- src/elements/slider/slider.stories.ts | 24 +- src/elements/slider/slider.ts | 30 ++- src/elements/slider/slider.visual.spec.ts | 9 + 6 files changed, 234 insertions(+), 83 deletions(-) diff --git a/src/elements/checkbox/common/checkbox-common.spec.ts b/src/elements/checkbox/common/checkbox-common.spec.ts index 1a1db2d124..a2e349c6d8 100644 --- a/src/elements/checkbox/common/checkbox-common.spec.ts +++ b/src/elements/checkbox/common/checkbox-common.spec.ts @@ -153,7 +153,7 @@ describe(`checkbox common behaviors`, () => { expect(snapshot.required).not.to.be.ok; }); - it('should should restore form state on formStateRestoreCallback()', async () => { + it('should restore form state on formStateRestoreCallback()', async () => { // Mimic tab restoration. Does not test the full cycle as we can not set the browser in the required state. element.formStateRestoreCallback('true', 'restore'); await waitForLitRender(element); diff --git a/src/elements/slider/slider.scss b/src/elements/slider/slider.scss index 0ceaafbb4f..3def0d88ee 100644 --- a/src/elements/slider/slider.scss +++ b/src/elements/slider/slider.scss @@ -31,7 +31,7 @@ } } -:host([disabled]) { +:host(:disabled) { --sbb-slider-icon-color: var(--sbb-color-graphite); --sbb-slider-knob-border-color: var(--sbb-color-smoke); --sbb-slider-knob-border-size: var(--sbb-border-width-2x); @@ -43,7 +43,7 @@ --sbb-slider-knob-border-color: var(--sbb-slider-selected-line-disabled-color); } -:host([disabled]), +:host(:disabled), :host([readonly]) { --sbb-slider-line-color: var(--sbb-slider-line-disabled-color); --sbb-slider-selected-line-color: var(--sbb-slider-selected-line-disabled-color); @@ -128,7 +128,7 @@ } // slider knob resize on click (active / focus) - :host(:not(:is([disabled], [readonly]))) .sbb-slider__range-input:active ~ & { + :host(:not(:is(:disabled, [readonly]))) .sbb-slider__range-input:active ~ & { --sbb-slider-knob-size: var(--sbb-slider-knob-size-active); } } diff --git a/src/elements/slider/slider.spec.ts b/src/elements/slider/slider.spec.ts index 59742eb4a8..d1aa7f97df 100644 --- a/src/elements/slider/slider.spec.ts +++ b/src/elements/slider/slider.spec.ts @@ -7,79 +7,219 @@ import { EventSpy, waitForLitRender } from '../core/testing.js'; import { SbbSliderElement } from './slider.js'; -const keyboardPressTimes = async ( - slider: SbbSliderElement, - key: string, - times = 1, -): Promise => { - slider.focus(); +const keyboardPressTimes = async (element: HTMLElement, key: string, times = 1): Promise => { + element.focus(); for (let i = 0; i < times; i++) { await sendKeys({ press: key }); } - await waitForLitRender(slider); + await waitForLitRender(element); }; describe(`sbb-slider`, () => { + let form: HTMLFormElement; + let fieldSet: HTMLFieldSetElement; + let input: HTMLInputElement; let element: SbbSliderElement; + let elemChangeEvent: EventSpy, + elemInputEvent: EventSpy, + nativeChangeEvent: EventSpy, + nativeInputEvent: EventSpy; beforeEach(async () => { - element = await fixture(html` - + form = await fixture(html` +
+
+ + + +
+
`); + element = form.querySelector('sbb-slider')!; + input = form.querySelector('input')!; + fieldSet = form.querySelector('fieldset')!; + await waitForLitRender(form); + + // event spies + elemChangeEvent = new EventSpy('change', element); + elemInputEvent = new EventSpy('input', element); + nativeChangeEvent = new EventSpy('change', element); + nativeInputEvent = new EventSpy('input', element); }); + function compareToNativeInput(): void { + const formData = new FormData(form); + + expect(element.value, 'compare to native - value').to.be.equal(input.value); + expect(element.valueAsNumber, 'compare to native - valueAsNumber').to.be.equal( + input.valueAsNumber, + ); + expect(formData.get('sbb-slider'), 'compare to native - form value').to.be.equal( + formData.get('slider'), + ); + expect(elemChangeEvent.count, 'compare to native - change counts').to.be.equal( + nativeChangeEvent.count, + ); + expect(elemInputEvent.count, 'compare to native - input counts').to.be.equal( + nativeInputEvent.count, + ); + } + it('renders', async () => { assert.instanceOf(element, SbbSliderElement); }); - it('should decrease value by two on left arrow keypress', async () => { - const changeEvent = new EventSpy('change', element); - await keyboardPressTimes(element, 'ArrowLeft', 2); - expect(changeEvent.count).to.be.greaterThan(0); - expect(element.value).to.be.equal('398'); - expect(element.valueAsNumber).to.be.equal(398); - }); + describe(`slider behaviors`, () => { + it('should set default value', async () => { + element.value = null; + // @ts-expect-error value should not be null + input.value = null; - it('should decrease value by two on down arrow keypress', async () => { - const changeEvent = new EventSpy('change', element); - await keyboardPressTimes(element, 'ArrowDown', 2); - expect(changeEvent.count).to.be.greaterThan(0); - expect(element.value).to.be.equal('398'); - expect(element.valueAsNumber).to.be.equal(398); - }); + await waitForLitRender(form); - it('should increase value by one on right arrow keypress', async () => { - const changeEvent = new EventSpy('change', element); - await keyboardPressTimes(element, 'ArrowRight'); - expect(changeEvent.count).to.be.greaterThan(0); - expect(element.value).to.be.equal('401'); - expect(element.valueAsNumber).to.be.equal(401); - }); + expect(element.value).to.be.equal('50'); + compareToNativeInput(); + }); + + it('should handle invalid values', async () => { + element.value = input.value = 'a'; + await waitForLitRender(form); + + expect(element.value).to.be.equal('50'); + compareToNativeInput(); + + element.max = element.min = 'a'; + input.max = element.min = 'a'; + await waitForLitRender(form); + expect(element.max).to.be.equal('100'); + expect(element.min).to.be.equal('0'); + compareToNativeInput(); + }); + + it('should handle out of bound values', async () => { + element.value = input.value = '150'; + await waitForLitRender(form); + + expect(element.value).to.be.equal('100'); + compareToNativeInput(); + + element.value = input.value = '-20'; + await waitForLitRender(form); + + expect(element.value).to.be.equal('0'); + compareToNativeInput(); + }); + + it('should update value on min/max change', async () => { + element.max = input.max = '30'; + await waitForLitRender(form); + + expect(element.value).to.be.equal('30'); + compareToNativeInput(); + + element.max = input.max = '100'; + element.min = input.min = '60'; + await waitForLitRender(form); + + expect(element.value).to.be.equal('60'); + compareToNativeInput(); + }); + + it('should result as :disabled', async () => { + element.disabled = true; + + await waitForLitRender(form); - it('should increase value by one on up arrow keypress', async () => { - const changeEvent = new EventSpy('change', element); - await keyboardPressTimes(element, 'ArrowUp'); - expect(changeEvent.count).to.be.greaterThan(0); - expect(element.value).to.be.equal('401'); - expect(element.valueAsNumber).to.be.equal(401); + const disabledElements = Array.from(form.querySelectorAll(':disabled')); + expect(disabledElements.includes(element), ':disabled selector').to.be.true; + }); + + it('should result disabled if a fieldSet is', async () => { + fieldSet.disabled = true; + + await waitForLitRender(form); + + const disabledElements = Array.from(form.querySelectorAll(':disabled')); + expect(disabledElements.includes(element), ':disabled selector').to.be.true; + compareToNativeInput(); + }); + + it('should restore form state on formStateRestoreCallback()', async () => { + // Mimic tab restoration. Does not test the full cycle as we can not set the browser in the required state. + element.formStateRestoreCallback('20', 'restore'); + await waitForLitRender(element); + + expect(element.value).to.be.equal('20'); + }); + + it('should reset on form reset', async () => { + element.value = input.value = '20'; + + form.reset(); + await waitForLitRender(form); + + expect(element.value).to.be.equal('40'); + compareToNativeInput(); + }); }); - it('should not change value on arrow keypress if disabled', async () => { - const changeEvent = new EventSpy('change', element); - element.toggleAttribute('disabled', true); - await keyboardPressTimes(element, 'ArrowLeft'); - expect(changeEvent.count).not.to.be.greaterThan(0); - await keyboardPressTimes(element, 'ArrowRight'); - expect(changeEvent.count).not.to.be.greaterThan(0); - await keyboardPressTimes(element, 'ArrowUp'); - expect(changeEvent.count).not.to.be.greaterThan(0); - await keyboardPressTimes(element, 'ArrowDown'); - expect(changeEvent.count).not.to.be.greaterThan(0); + describe('keyboard interaction', () => { + it('should decrease value by two on left arrow keypress', async () => { + await keyboardPressTimes(element, 'ArrowLeft', 2); + await keyboardPressTimes(input, 'ArrowLeft', 2); + + expect(element.value).to.be.equal('38'); + compareToNativeInput(); + }); + + it('should decrease value by two on down arrow keypress', async () => { + await keyboardPressTimes(element, 'ArrowDown', 2); + await keyboardPressTimes(input, 'ArrowDown', 2); + + expect(element.value).to.be.equal('38'); + compareToNativeInput(); + }); + + it('should increase value by one on right arrow keypress', async () => { + await keyboardPressTimes(element, 'ArrowRight'); + await keyboardPressTimes(input, 'ArrowRight'); + + expect(element.value).to.be.equal('41'); + compareToNativeInput(); + }); + + it('should increase value by one on up arrow keypress', async () => { + await keyboardPressTimes(element, 'ArrowUp'); + await keyboardPressTimes(input, 'ArrowUp'); + + expect(element.value).to.be.equal('41'); + compareToNativeInput(); + }); + + it('should not change value on arrow keypress if disabled', async () => { + element.disabled = true; + await waitForLitRender(element); + + await keyboardPressTimes(element, 'ArrowLeft'); + await keyboardPressTimes(element, 'ArrowRight'); + + // disabled by fieldset + element.disabled = false; + fieldSet.disabled = true; + await waitForLitRender(element); + + await keyboardPressTimes(element, 'ArrowUp'); + await keyboardPressTimes(element, 'ArrowDown'); + + expect(elemChangeEvent.count).not.to.be.greaterThan(0); + expect(element.value).to.be.equal('40'); + }); }); }); diff --git a/src/elements/slider/slider.stories.ts b/src/elements/slider/slider.stories.ts index 6cf920f1b9..e7b342e120 100644 --- a/src/elements/slider/slider.stories.ts +++ b/src/elements/slider/slider.stories.ts @@ -21,8 +21,9 @@ const changeEventHandler = (event: Event): void => { .prepend(div); }; -const TemplateSbbSlider = (args: Args): TemplateResult => - html``; +const TemplateSbbSlider = (args: Args): TemplateResult => html` + +`; const TemplateSbbSliderChangeEvent = (args: Args): TemplateResult => html` { }), ); + it( + 'disabled fieldset', + visualDiffDefault.with(async (setup) => { + await setup.withFixture(html` +
${variant.template({ ...defaultArgs, disabled: true })}
+ `); + }), + ); + it( 'readonly', visualDiffDefault.with(async (setup) => { From 9837ceabd0e2dc5417fcd9bc0878a119afde0c90 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Tue, 10 Sep 2024 13:45:55 +0200 Subject: [PATCH 04/10] test(sbb-slider): update tests --- src/elements/slider/slider.spec.ts | 2 +- src/elements/slider/slider.ts | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/elements/slider/slider.spec.ts b/src/elements/slider/slider.spec.ts index d1aa7f97df..e220ac3f5d 100644 --- a/src/elements/slider/slider.spec.ts +++ b/src/elements/slider/slider.spec.ts @@ -110,7 +110,7 @@ describe(`sbb-slider`, () => { expect(element.value).to.be.equal('100'); compareToNativeInput(); - element.value = input.value = '-20'; + element.valueAsNumber = input.valueAsNumber = -20; await waitForLitRender(form); expect(element.value).to.be.equal('0'); diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index 29ec650d8d..ab0205fe07 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -199,9 +199,6 @@ export class SbbSliderElement extends SbbFocusableDisabledActionMixin( } private _calculateValueFraction(): void { - if (!this.value) { - return; - } const value = this.valueAsNumber!; const min = +this.min; const max = +this.max; From 404082deb2a73c63e27cdeee236b104a461760d5 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Wed, 11 Sep 2024 11:39:11 +0200 Subject: [PATCH 05/10] test(sbb-slider): update tests --- src/elements/slider/slider.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/elements/slider/slider.spec.ts b/src/elements/slider/slider.spec.ts index e220ac3f5d..8338d50922 100644 --- a/src/elements/slider/slider.spec.ts +++ b/src/elements/slider/slider.spec.ts @@ -50,8 +50,8 @@ describe(`sbb-slider`, () => { // event spies elemChangeEvent = new EventSpy('change', element); elemInputEvent = new EventSpy('input', element); - nativeChangeEvent = new EventSpy('change', element); - nativeInputEvent = new EventSpy('input', element); + nativeChangeEvent = new EventSpy('change', input); + nativeInputEvent = new EventSpy('input', input); }); function compareToNativeInput(): void { From a5083dabfb57a1089a18f9de34083083192a8bd0 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Fri, 20 Sep 2024 11:08:37 +0200 Subject: [PATCH 06/10] test(sbb-slider): add visual spec --- src/elements/slider/slider.visual.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/elements/slider/slider.visual.spec.ts b/src/elements/slider/slider.visual.spec.ts index 501c29f645..734699f918 100644 --- a/src/elements/slider/slider.visual.spec.ts +++ b/src/elements/slider/slider.visual.spec.ts @@ -74,7 +74,7 @@ describe('sbb-slider', () => { 'disabled fieldset', visualDiffDefault.with(async (setup) => { await setup.withFixture(html` -
${variant.template({ ...defaultArgs, disabled: true })}
+
${variant.template({ ...defaultArgs })}
`); }), ); From 18accc92893fd24aef400a31cc37fcbba974dc05 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Fri, 20 Sep 2024 17:48:26 +0200 Subject: [PATCH 07/10] fix(sbb-slider): pr feedbacks --- src/elements/slider/slider.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index ab0205fe07..be24355663 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -44,10 +44,10 @@ export class SbbSliderElement extends SbbFocusableDisabledActionMixin( */ @property() public override set value(value: string | null) { - if (value && this._isValidNumber(value!)) { - super.value = this._boundBetweenMinMax(value); + if (this._isValidNumber(value)) { + super.value = this._boundBetweenMinMax(value!); } else { - super.value = this._getDefaultValue().toString(); + super.value = this._getDefaultValue(); } setOrRemoveAttribute(this, 'aria-valuenow', this.value); this._calculateValueFraction(); @@ -68,7 +68,7 @@ export class SbbSliderElement extends SbbFocusableDisabledActionMixin( /** Minimum acceptable value for the inner HTMLInputElement. */ @property() public set min(value: string) { - if (!value || !this._isValidNumber(value!)) { + if (!this._isValidNumber(value!)) { return; } @@ -83,7 +83,7 @@ export class SbbSliderElement extends SbbFocusableDisabledActionMixin( /** Maximum acceptable value for the inner HTMLInputElement. */ @property() public set max(value: string) { - if (!value || !this._isValidNumber(value!)) { + if (!this._isValidNumber(value!)) { return; } @@ -187,8 +187,8 @@ export class SbbSliderElement extends SbbFocusableDisabledActionMixin( return (+this.min + (+this.max - +this.min) / 2).toString(); } - private _isValidNumber(value: string): boolean { - return !isNaN(Number(value)); + private _isValidNumber(value: string | null): boolean { + return !!value && !isNaN(Number(value)); } /** From 81b383324bde8ffc00a2d1007a0e323c4de3c712 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Mon, 23 Sep 2024 11:14:17 +0200 Subject: [PATCH 08/10] fix(sbb-slider): fix disabled behavior --- src/elements/slider/slider.spec.ts | 40 +++++++++++++++--------------- src/elements/slider/slider.ts | 12 +++------ 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/elements/slider/slider.spec.ts b/src/elements/slider/slider.spec.ts index 5576d61f7a..652c4fdab3 100644 --- a/src/elements/slider/slider.spec.ts +++ b/src/elements/slider/slider.spec.ts @@ -133,22 +133,39 @@ describe(`sbb-slider`, () => { }); it('should result as :disabled', async () => { - element.disabled = true; + expect(element.tabIndex).to.be.equal(0); + expect(element.getAttribute('aria-disabled')).to.be.null; + element.disabled = true; await waitForLitRender(form); const disabledElements = Array.from(form.querySelectorAll(':disabled')); expect(disabledElements.includes(element), ':disabled selector').to.be.true; + expect(element.tabIndex).to.be.equal(-1); + expect(element.getAttribute('aria-disabled')).to.be.equal('true'); + + element.disabled = false; + await waitForLitRender(element); + + expect(element.tabIndex).to.be.equal(0); + expect(element.getAttribute('aria-disabled')).to.be.null; }); - it('should result disabled if a fieldSet is', async () => { + it('should result :disabled if a fieldSet is', async () => { fieldSet.disabled = true; - await waitForLitRender(form); const disabledElements = Array.from(form.querySelectorAll(':disabled')); expect(disabledElements.includes(element), ':disabled selector').to.be.true; + expect(element.tabIndex).to.be.equal(-1); + expect(element.getAttribute('aria-disabled')).to.be.equal('true'); compareToNativeInput(); + + fieldSet.disabled = false; + await waitForLitRender(element); + + expect(element.tabIndex).to.be.equal(0); + expect(element.getAttribute('aria-disabled')).to.be.null; }); it('should restore form state on formStateRestoreCallback()', async () => { @@ -222,21 +239,4 @@ describe(`sbb-slider`, () => { expect(element.value).to.be.equal('40'); }); }); - - it('should not be focused when disabled', async () => { - expect(element.tabIndex).to.be.equal(0); - expect(element.getAttribute('aria-disabled')).to.be.null; - - element.toggleAttribute('disabled', true); - await waitForLitRender(element); - - expect(element.tabIndex).to.be.equal(-1); - expect(element.getAttribute('aria-disabled')).to.be.equal('true'); - - element.toggleAttribute('disabled', false); - await waitForLitRender(element); - - expect(element.tabIndex).to.be.equal(0); - expect(element.getAttribute('aria-disabled')).to.be.null; - }); }); diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index 759d93443a..400edaa81b 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -146,8 +146,8 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li if (changedProperties.has('readonly')) { setOrRemoveAttribute(this, 'aria-readonly', this.readonly ? 'true' : null); } - if (changedProperties.has('disabled')) { - if (this.disabled) { + if (changedProperties.has('disabled') || changedProperties.has('formDisabled')) { + if (this.disabled || this.formDisabled) { this.setAttribute('aria-disabled', 'true'); this.removeAttribute('tabindex'); } else { @@ -175,10 +175,6 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li this.value = state as string | null; } - protected override isDisabledExternally(): boolean { - return this.formDisabled; - } - /** * If no value is provided, default is the middle point between min and max * (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#value) @@ -212,7 +208,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li event.preventDefault(); } - if (this.disabled || this.readonly) { + if (this.disabled || this.formDisabled || this.readonly) { return; } @@ -260,7 +256,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li tabindex="-1" min=${this.min} max=${this.max} - ?disabled=${this.disabled || this.readonly || nothing} + ?disabled=${this.disabled || this.formDisabled || this.readonly || nothing} value=${this.value || nothing} class="sbb-slider__range-input" type="range" From 31e2421d38a23ffa66910c89535216650e00af79 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Mon, 23 Sep 2024 12:26:42 +0200 Subject: [PATCH 09/10] fix(sbb-slider): fix disabled behavior --- .../__snapshots__/slider.snapshot.spec.snap.js | 13 +------------ src/elements/slider/slider.spec.ts | 17 +++++++---------- src/elements/slider/slider.ts | 18 +++++------------- 3 files changed, 13 insertions(+), 35 deletions(-) diff --git a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js index 2849bb8e91..6c90f997ec 100644 --- a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js +++ b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js @@ -3,8 +3,6 @@ export const snapshots = {}; snapshots["sbb-slider renders DOM"] = ` @@ -173,9 +167,6 @@ snapshots["sbb-slider renders disabled Shadow DOM"] = snapshots["sbb-slider renders readonly DOM"] = ` { element.disabled = true; await waitForLitRender(form); - const disabledElements = Array.from(form.querySelectorAll(':disabled')); + let disabledElements = Array.from(form.querySelectorAll(':disabled')); expect(disabledElements.includes(element), ':disabled selector').to.be.true; - expect(element.tabIndex).to.be.equal(-1); - expect(element.getAttribute('aria-disabled')).to.be.equal('true'); element.disabled = false; await waitForLitRender(element); - expect(element.tabIndex).to.be.equal(0); - expect(element.getAttribute('aria-disabled')).to.be.null; + disabledElements = Array.from(form.querySelectorAll(':disabled')); + expect(disabledElements.includes(element), ':disabled selector').to.be.false; }); it('should result :disabled if a fieldSet is', async () => { fieldSet.disabled = true; await waitForLitRender(form); - const disabledElements = Array.from(form.querySelectorAll(':disabled')); + let disabledElements = Array.from(form.querySelectorAll(':disabled')); expect(disabledElements.includes(element), ':disabled selector').to.be.true; - expect(element.tabIndex).to.be.equal(-1); - expect(element.getAttribute('aria-disabled')).to.be.equal('true'); compareToNativeInput(); fieldSet.disabled = false; await waitForLitRender(element); - expect(element.tabIndex).to.be.equal(0); - expect(element.getAttribute('aria-disabled')).to.be.null; + disabledElements = Array.from(form.querySelectorAll(':disabled')); + expect(disabledElements.includes(element), ':disabled selector').to.be.false; + compareToNativeInput(); }); it('should restore form state on formStateRestoreCallback()', async () => { diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index 400edaa81b..3e6af75cc1 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -29,6 +29,7 @@ import '../icon.js'; @customElement('sbb-slider') @hostAttributes({ role: 'slider', + tabindex: '0', }) export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(LitElement)) { public static override styles: CSSResultGroup = style; @@ -138,22 +139,13 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li super.willUpdate(changedProperties); if (changedProperties.has('min')) { - setOrRemoveAttribute(this, 'aria-valuemin', this.min); + this.internals.ariaValueMin = this.min; } if (changedProperties.has('max')) { - setOrRemoveAttribute(this, 'aria-valuemax', this.max); + this.internals.ariaValueMax = this.max; } if (changedProperties.has('readonly')) { - setOrRemoveAttribute(this, 'aria-readonly', this.readonly ? 'true' : null); - } - if (changedProperties.has('disabled') || changedProperties.has('formDisabled')) { - if (this.disabled || this.formDisabled) { - this.setAttribute('aria-disabled', 'true'); - this.removeAttribute('tabindex'); - } else { - this.removeAttribute('aria-disabled'); - this.setAttribute('tabindex', '0'); - } + this.internals.ariaReadOnly = Boolean(this.readonly).toString(); } } @@ -208,7 +200,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li event.preventDefault(); } - if (this.disabled || this.formDisabled || this.readonly) { + if (this.readonly) { return; } From 0c4d09908e2bc6c257d11b15a8e57da1bf090301 Mon Sep 17 00:00:00 2001 From: Tommmaso Menga Date: Tue, 24 Sep 2024 15:01:26 +0200 Subject: [PATCH 10/10] fix(sbb-slider): pr feedbacks pt.1 --- .../checkbox/common/checkbox-common.spec.ts | 10 ++++---- .../slider.snapshot.spec.snap.js | 23 +++++++++---------- src/elements/slider/slider.snapshot.spec.ts | 9 +++++++- src/elements/slider/slider.spec.ts | 13 ++++------- src/elements/slider/slider.ts | 14 +++++++---- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/elements/checkbox/common/checkbox-common.spec.ts b/src/elements/checkbox/common/checkbox-common.spec.ts index a2e349c6d8..97fdf927fe 100644 --- a/src/elements/checkbox/common/checkbox-common.spec.ts +++ b/src/elements/checkbox/common/checkbox-common.spec.ts @@ -537,11 +537,11 @@ describe(`checkbox common behaviors`, () => { expect(element).not.to.have.attribute('disabled'); } - const disabledElements = Array.from(form.querySelectorAll(':disabled')); - - expect(disabledElements.includes(element), ':disabled selector').to.be.equal( - assertions.disabledSelector, - ); + if (assertions.disabledSelector) { + expect(element).to.match(':disabled'); + } else { + expect(element).not.to.match(':disabled'); + } const snapshot = (await a11ySnapshot({ selector: element.localName, diff --git a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js index 6c90f997ec..4ba68ab540 100644 --- a/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js +++ b/src/elements/slider/__snapshots__/slider.snapshot.spec.snap.js @@ -3,8 +3,7 @@ export const snapshots = {}; snapshots["sbb-slider renders DOM"] = ` @@ -45,11 +44,9 @@ snapshots["sbb-slider renders Shadow DOM"] = snapshots["sbb-slider renders with properties DOM"] = ` diff --git a/src/elements/slider/slider.snapshot.spec.ts b/src/elements/slider/slider.snapshot.spec.ts index 25d366f44c..5460cd385d 100644 --- a/src/elements/slider/slider.snapshot.spec.ts +++ b/src/elements/slider/slider.snapshot.spec.ts @@ -12,7 +12,14 @@ describe(`sbb-slider`, () => { describe('renders', async () => { beforeEach(async () => { - element = await fixture(html``); + element = ( + await fixture(html` +
+ + +
+ `) + ).querySelector('sbb-slider')!; }); it('DOM', async () => { diff --git a/src/elements/slider/slider.spec.ts b/src/elements/slider/slider.spec.ts index 08f24c01b7..3d0e138430 100644 --- a/src/elements/slider/slider.spec.ts +++ b/src/elements/slider/slider.spec.ts @@ -134,34 +134,29 @@ describe(`sbb-slider`, () => { it('should result as :disabled', async () => { expect(element.tabIndex).to.be.equal(0); - expect(element.getAttribute('aria-disabled')).to.be.null; element.disabled = true; await waitForLitRender(form); - let disabledElements = Array.from(form.querySelectorAll(':disabled')); - expect(disabledElements.includes(element), ':disabled selector').to.be.true; + expect(element).to.match(':disabled'); element.disabled = false; await waitForLitRender(element); - disabledElements = Array.from(form.querySelectorAll(':disabled')); - expect(disabledElements.includes(element), ':disabled selector').to.be.false; + expect(element).not.to.match(':disabled'); }); it('should result :disabled if a fieldSet is', async () => { fieldSet.disabled = true; await waitForLitRender(form); - let disabledElements = Array.from(form.querySelectorAll(':disabled')); - expect(disabledElements.includes(element), ':disabled selector').to.be.true; + expect(element).to.match(':disabled'); compareToNativeInput(); fieldSet.disabled = false; await waitForLitRender(element); - disabledElements = Array.from(form.querySelectorAll(':disabled')); - expect(disabledElements.includes(element), ':disabled selector').to.be.false; + expect(element).not.to.match(':disabled'); compareToNativeInput(); }); diff --git a/src/elements/slider/slider.ts b/src/elements/slider/slider.ts index 3e6af75cc1..450bd2c38b 100644 --- a/src/elements/slider/slider.ts +++ b/src/elements/slider/slider.ts @@ -6,7 +6,6 @@ import { styleMap } from 'lit/directives/style-map.js'; import { SbbConnectedAbortController } from '../core/controllers.js'; import { hostAttributes } from '../core/decorators.js'; -import { setOrRemoveAttribute } from '../core/dom.js'; import { EventEmitter, forwardEventToHost } from '../core/eventing.js'; import { type FormRestoreReason, @@ -28,7 +27,6 @@ import '../icon.js'; */ @customElement('sbb-slider') @hostAttributes({ - role: 'slider', tabindex: '0', }) export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(LitElement)) { @@ -48,7 +46,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li } else { super.value = this._getDefaultValue(); } - setOrRemoveAttribute(this, 'aria-valuenow', this.value); + this.internals.ariaValueNow = this.value; this._calculateValueFraction(); } public override get value(): string { @@ -125,6 +123,12 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li private _abort = new SbbConnectedAbortController(this); + public constructor() { + super(); + /** @internal */ + this.internals.role = 'slider'; + } + public override connectedCallback(): void { super.connectedCallback(); const signal = this._abort.signal; @@ -154,7 +158,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li * @internal */ public formResetCallback(): void { - this.value = this.hasAttribute('value') ? this.getAttribute('value') : this._getDefaultValue(); + this.value = this.getAttribute('value') ?? this._getDefaultValue(); } /** @@ -248,7 +252,7 @@ export class SbbSliderElement extends SbbDisabledMixin(SbbFormAssociatedMixin(Li tabindex="-1" min=${this.min} max=${this.max} - ?disabled=${this.disabled || this.formDisabled || this.readonly || nothing} + ?disabled=${this.disabled || this.formDisabled || this.readonly} value=${this.value || nothing} class="sbb-slider__range-input" type="range"