From 51829320e3ece2023cef197cc378b1e9b486fb61 Mon Sep 17 00:00:00 2001 From: Radoslav Karaivanov Date: Thu, 8 May 2025 17:59:03 +0300 Subject: [PATCH 1/3] refactor: Keyboard focus ring controller --- src/components/button-group/toggle-button.ts | 16 ++-- src/components/button/button-base.ts | 51 +++++------- .../carousel-indicator-container.spec.ts | 32 ++++---- .../carousel/carousel-indicator-container.ts | 17 ++-- src/components/carousel/carousel.ts | 39 ++++----- src/components/checkbox/checkbox-base.ts | 34 ++++---- src/components/checkbox/checkbox.ts | 28 +++---- src/components/checkbox/switch.ts | 27 +++--- .../common/controllers/focus-ring.spec.ts | 8 +- .../common/controllers/focus-ring.ts | 49 ++++++----- src/components/common/mixins/option.ts | 13 +-- src/components/radio/radio.ts | 82 +++++++++---------- 12 files changed, 180 insertions(+), 216 deletions(-) diff --git a/src/components/button-group/toggle-button.ts b/src/components/button-group/toggle-button.ts index 60ae8d12f..ef6021ddc 100644 --- a/src/components/button-group/toggle-button.ts +++ b/src/components/button-group/toggle-button.ts @@ -30,14 +30,14 @@ export default class IgcToggleButtonComponent extends LitElement { }; /* blazorSuppress */ - public static register() { + public static register(): void { registerComponent(IgcToggleButtonComponent); } - private _kbFocus = addKeyboardFocusRing(this); + private readonly _focusRingManager = addKeyboardFocusRing(this); @query('[part="toggle"]', true) - private _nativeButton!: HTMLButtonElement; + private readonly _nativeButton!: HTMLButtonElement; /** * The value attribute of the control. @@ -62,18 +62,18 @@ export default class IgcToggleButtonComponent extends LitElement { /* alternateName: focusComponent */ /** Sets focus on the button. */ - public override focus(options?: FocusOptions) { + public override focus(options?: FocusOptions): void { this._nativeButton.focus(options); } /* alternateName: blurComponent */ /** Removes focus from the button. */ - public override blur() { + public override blur(): void { this._nativeButton.blur(); } /** Simulates a mouse click on the element. */ - public override click() { + public override click(): void { this._nativeButton.click(); } @@ -82,15 +82,13 @@ export default class IgcToggleButtonComponent extends LitElement { diff --git a/src/components/button/button-base.ts b/src/components/button/button-base.ts index 23ab91134..b1ac722d0 100644 --- a/src/components/button/button-base.ts +++ b/src/components/button/button-base.ts @@ -28,13 +28,14 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin< delegatesFocus: true, }; - private _kbFocus = addKeyboardFocusRing(this); + protected readonly __internals: ElementInternals; + + private readonly _focusRingManager = addKeyboardFocusRing(this); - protected __internals: ElementInternals; protected _disabled = false; @query('[part="base"]', true) - private _nativeButton!: HTMLButtonElement; + private readonly _nativeButton!: HTMLButtonElement; /* alternateName: displayType */ /** @@ -78,19 +79,19 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin< * @attr [disabled=false] */ @property({ type: Boolean, reflect: true }) - public get disabled(): boolean { - return this._disabled; - } - public set disabled(value: boolean) { this._disabled = value; this.toggleAttribute('disabled', Boolean(this._disabled)); } + public get disabled(): boolean { + return this._disabled; + } + /* blazorCSSuppress */ /* alternateType: object */ /** Returns the HTMLFormElement associated with this element. */ - public get form() { + public get form(): HTMLFormElement | null { return this.__internals.form; } @@ -101,38 +102,30 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin< /* alternateName: focusComponent */ /** Sets focus in the button. */ - public override focus(options?: FocusOptions) { + public override focus(options?: FocusOptions): void { this._nativeButton.focus(options); } /** Simulates a mouse click on the element */ - public override click() { + public override click(): void { this._nativeButton.click(); } /* alternateName: blurComponent */ /** Removes focus from the button. */ - public override blur() { + public override blur(): void { this._nativeButton.blur(); } - protected handleBlur() { - this._kbFocus.reset(); - } - - protected handleClick() { - this._kbFocus.reset(); - switch (this.type) { - case 'submit': - return this.form?.requestSubmit(); - case 'reset': - return this.form?.reset(); - default: - return; + protected _handleClick(): void { + if (this.type === 'submit') { + this.form?.requestSubmit(); + } else if (this.type === 'reset') { + this.form?.reset(); } } - protected formDisabledCallback(state: boolean) { + protected formDisabledCallback(state: boolean): void { this._disabled = state; this.requestUpdate(); } @@ -142,13 +135,12 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin< @@ -160,7 +152,7 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin< ${this.renderContent()} diff --git a/src/components/carousel/carousel-indicator-container.spec.ts b/src/components/carousel/carousel-indicator-container.spec.ts index 044a9a50c..dff4ab056 100644 --- a/src/components/carousel/carousel-indicator-container.spec.ts +++ b/src/components/carousel/carousel-indicator-container.spec.ts @@ -2,7 +2,12 @@ import { elementUpdated, expect, fixture, html } from '@open-wc/testing'; import { tabKey } from '../common/controllers/key-bindings.js'; import { defineComponents } from '../common/definitions/defineComponents.js'; -import { simulateClick, simulateKeyboard } from '../common/utils.spec.js'; +import { first } from '../common/util.js'; +import { + simulateClick, + simulateKeyboard, + simulatePointerDown, +} from '../common/utils.spec.js'; import IgcCarouselIndicatorContainerComponent from './carousel-indicator-container.js'; import IgcCarouselIndicatorComponent from './carousel-indicator.js'; @@ -26,12 +31,8 @@ describe('Carousel Indicator Container', () => { let buttons: HTMLButtonElement[]; beforeEach(async () => { - container = await fixture( - createIndicatorContainerComponent() - ); - buttons = container.querySelectorAll( - 'button' - ) as unknown as HTMLButtonElement[]; + container = await fixture(createIndicatorContainerComponent()); + buttons = Array.from(container.querySelectorAll('button')); }); it('is correctly initialized', () => { @@ -56,7 +57,7 @@ describe('Carousel Indicator Container', () => { ` ); - simulateKeyboard(buttons[0], tabKey); + simulateKeyboard(first(buttons), tabKey); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -67,7 +68,7 @@ describe('Carousel Indicator Container', () => { }); it('should remove `focused` part on click', async () => { - simulateKeyboard(buttons[0], tabKey); + simulateKeyboard(first(buttons), tabKey); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -76,7 +77,8 @@ describe('Carousel Indicator Container', () => { ` ); - simulateClick(buttons[0]); + simulatePointerDown(first(buttons)); + simulateClick(first(buttons)); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -87,7 +89,7 @@ describe('Carousel Indicator Container', () => { }); it('it should remove `focused` part on focusout', async () => { - simulateKeyboard(buttons[0], tabKey); + simulateKeyboard(first(buttons), tabKey); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -96,7 +98,7 @@ describe('Carousel Indicator Container', () => { ` ); - buttons[0].dispatchEvent(new FocusEvent('focusout', { bubbles: true })); + first(buttons).dispatchEvent(new FocusEvent('focusout', { bubbles: true })); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -107,7 +109,7 @@ describe('Carousel Indicator Container', () => { }); it('it should not remove `focused` part on focusout if the target receiving focus is an `igc-carousel-indicator`', async () => { - simulateKeyboard(buttons[0], tabKey); + simulateKeyboard(first(buttons), tabKey); await elementUpdated(container); expect(container).shadowDom.to.equal( @@ -116,14 +118,14 @@ describe('Carousel Indicator Container', () => { ` ); - const indicator = await fixture( + const indicator = await fixture( html` 0 1 ` ); - buttons[0].dispatchEvent( + first(buttons).dispatchEvent( new FocusEvent('focusout', { bubbles: true, relatedTarget: indicator }) ); await elementUpdated(container); diff --git a/src/components/carousel/carousel-indicator-container.ts b/src/components/carousel/carousel-indicator-container.ts index 2aea47505..c9e45be94 100644 --- a/src/components/carousel/carousel-indicator-container.ts +++ b/src/components/carousel/carousel-indicator-container.ts @@ -22,17 +22,19 @@ export default class IgcCarouselIndicatorContainerComponent extends LitElement { public static override styles = [styles, shared]; /* blazorSuppress */ - public static register() { + public static register(): void { registerComponent(IgcCarouselIndicatorContainerComponent); } - private _kbFocus = addKeyboardFocusRing(this); + private readonly _focusRingManager = addKeyboardFocusRing(this); - private handleFocusOut(event: FocusEvent) { + private _handleFocusOut(event: FocusEvent): void { const target = event.relatedTarget as Element; - if (!target?.matches(IgcCarouselIndicatorComponent.tagName)) { - this._kbFocus.reset(); + if (target?.matches(IgcCarouselIndicatorComponent.tagName)) { + // Stop the event from hitting the _focusRingManager handler redrawing + // the keyboard focus styles + event.stopPropagation(); } } @@ -41,10 +43,9 @@ export default class IgcCarouselIndicatorContainerComponent extends LitElement {
diff --git a/src/components/carousel/carousel.ts b/src/components/carousel/carousel.ts index 7a256a457..85678d5bb 100644 --- a/src/components/carousel/carousel.ts +++ b/src/components/carousel/carousel.ts @@ -7,7 +7,7 @@ import { state, } from 'lit/decorators.js'; -import { type Ref, createRef, ref } from 'lit/directives/ref.js'; +import { createRef, ref } from 'lit/directives/ref.js'; import { styleMap } from 'lit/directives/style-map.js'; import { themes } from '../../theming/theming-decorator.js'; import IgcButtonComponent from '../button/button.js'; @@ -92,7 +92,7 @@ export default class IgcCarouselComponent extends EventEmitterMixin< public static readonly tagName = 'igc-carousel'; /* blazorSuppress */ - public static register() { + public static register(): void { registerComponent( IgcCarouselComponent, IgcCarouselIndicatorComponent, @@ -104,10 +104,10 @@ export default class IgcCarouselComponent extends EventEmitterMixin< } private static readonly increment = createCounter(); - private _carouselId = `igc-carousel-${IgcCarouselComponent.increment()}`; - private _carouselKeyboardInteractionFocus = addKeyboardFocusRing(this); + private readonly _carouselId = `igc-carousel-${IgcCarouselComponent.increment()}`; + private readonly _focusRingManager = addKeyboardFocusRing(this); - private _internals: ElementInternals; + private readonly _internals: ElementInternals; private _lastInterval!: ReturnType | null; private _hasKeyboardInteractionOnIndicators = false; private _hasMouseStop = false; @@ -117,10 +117,10 @@ export default class IgcCarouselComponent extends EventEmitterMixin< initialValue: this, }); - private _carouselSlidesContainerRef: Ref = createRef(); - private _indicatorsContainerRef: Ref = createRef(); - private _prevButtonRef: Ref = createRef(); - private _nextButtonRef: Ref = createRef(); + private readonly _carouselSlidesContainerRef = createRef(); + private readonly _indicatorsContainerRef = createRef(); + private readonly _prevButtonRef = createRef(); + private readonly _nextButtonRef = createRef(); private get hasProjectedIndicators(): boolean { return this._projectedIndicators.length > 0; @@ -326,7 +326,6 @@ export default class IgcCarouselComponent extends EventEmitterMixin< this._internals.role = 'region'; this._internals.ariaRoleDescription = 'carousel'; - this.addEventListener('pointerdown', this.handlePointerDown); this.addEventListener('pointerenter', this.handlePointerEnter); this.addEventListener('pointerleave', this.handlePointerLeave); @@ -381,15 +380,9 @@ export default class IgcCarouselComponent extends EventEmitterMixin< this.requestUpdate(); } - private handlePointerDown(): void { - if (this._carouselKeyboardInteractionFocus.focused) { - this._carouselKeyboardInteractionFocus.reset(); - } - } - private handlePointerEnter(): void { this._hasMouseStop = true; - if (this._carouselKeyboardInteractionFocus.focused) { + if (this._focusRingManager.focused) { return; } this.handlePauseOnInteraction(); @@ -397,14 +390,14 @@ export default class IgcCarouselComponent extends EventEmitterMixin< private handlePointerLeave(): void { this._hasMouseStop = false; - if (this._carouselKeyboardInteractionFocus.focused) { + if (this._focusRingManager.focused) { return; } this.handlePauseOnInteraction(); } private handleFocusIn(): void { - if (this._carouselKeyboardInteractionFocus.focused || this._hasMouseStop) { + if (this._focusRingManager.focused || this._hasMouseStop) { return; } this.handlePauseOnInteraction(); @@ -417,12 +410,8 @@ export default class IgcCarouselComponent extends EventEmitterMixin< return; } - if (this._carouselKeyboardInteractionFocus.focused) { - this._carouselKeyboardInteractionFocus.reset(); - - if (!this._hasMouseStop) { - this.handlePauseOnInteraction(); - } + if (this._focusRingManager.focused && !this._hasMouseStop) { + this.handlePauseOnInteraction(); } } diff --git a/src/components/checkbox/checkbox-base.ts b/src/components/checkbox/checkbox-base.ts index adac4db69..a195eabef 100644 --- a/src/components/checkbox/checkbox-base.ts +++ b/src/components/checkbox/checkbox-base.ts @@ -39,18 +39,18 @@ export class IgcCheckboxBaseComponent extends FormAssociatedCheckboxRequiredMixi return checkBoxValidators; } - protected _kbFocus = addKeyboardFocusRing(this); + protected readonly _focusRingManager = addKeyboardFocusRing(this); protected override _formValue: FormValue; protected _value!: string; @query('input', true) - protected input!: HTMLInputElement; + protected readonly _input!: HTMLInputElement; @queryAssignedNodes({ flatten: true }) - protected label!: Array; + protected readonly _label!: Array; @state() - protected hideLabel = false; + protected _hideLabel = false; /** * The value attribute of the control. @@ -99,35 +99,35 @@ export class IgcCheckboxBaseComponent extends FormAssociatedCheckboxRequiredMixi }); } - protected override createRenderRoot() { + protected override createRenderRoot(): HTMLElement | DocumentFragment { const root = super.createRenderRoot(); - this.hideLabel = isEmpty(this.label); + this._hideLabel = isEmpty(this._label); root.addEventListener('slotchange', () => { - this.hideLabel = isEmpty(this.label); + this._hideLabel = isEmpty(this._label); }); return root; } /** Simulates a click on the control. */ - public override click() { - this.input.click(); + public override click(): void { + this._input.click(); } /* alternateName: focusComponent */ /** Sets focus on the control. */ - public override focus(options?: FocusOptions) { - this.input.focus(options); + public override focus(options?: FocusOptions): void { + this._input.focus(options); } /* alternateName: blurComponent */ /** Removes focus from the control. */ - public override blur() { - this.input.blur(); + public override blur(): void { + this._input.blur(); } - protected handleClick(event: PointerEvent) { + protected _handleClick(event: PointerEvent): void { event.stopPropagation(); this.checked = !this.checked; @@ -136,11 +136,7 @@ export class IgcCheckboxBaseComponent extends FormAssociatedCheckboxRequiredMixi }); } - protected handleBlur() { - this._kbFocus.reset(); - } - - protected handleFocus() { + protected _handleFocus(): void { this._dirty = true; } } diff --git a/src/components/checkbox/checkbox.ts b/src/components/checkbox/checkbox.ts index afee7b7b3..1a816819e 100644 --- a/src/components/checkbox/checkbox.ts +++ b/src/components/checkbox/checkbox.ts @@ -36,13 +36,13 @@ export default class IgcCheckboxComponent extends IgcCheckboxBaseComponent { protected static styles = [styles, shared]; /* blazorSuppress */ - public static register() { + public static register(): void { registerComponent(IgcCheckboxComponent, IgcValidationContainerComponent); } private static readonly increment = createCounter(); - private inputId = `checkbox-${IgcCheckboxComponent.increment()}`; - private labelId = `checkbox-label-${this.inputId}`; + private readonly _inputId = `checkbox-${IgcCheckboxComponent.increment()}`; + private readonly _labelId = `checkbox-label-${this._inputId}`; /** * Draws the checkbox in indeterminate state. @@ -55,9 +55,9 @@ export default class IgcCheckboxComponent extends IgcCheckboxBaseComponent { return getThemeController(this)?.theme === 'indigo'; } - protected override handleClick(event: PointerEvent) { + protected override _handleClick(event: PointerEvent): void { this.indeterminate = false; - super.handleClick(event); + super._handleClick(event); } protected renderValidatorContainer(): TemplateResult { @@ -92,13 +92,12 @@ export default class IgcCheckboxComponent extends IgcCheckboxBaseComponent { part=${partNameMap({ base: true, checked, - focused: this._kbFocus.focused, + focused: this._focusRingManager.focused, })} - for=${this.inputId} - @pointerdown=${this._kbFocus.reset} + for=${this._inputId} > @@ -119,9 +117,9 @@ export default class IgcCheckboxComponent extends IgcCheckboxBaseComponent { diff --git a/src/components/checkbox/switch.ts b/src/components/checkbox/switch.ts index d82e51f8e..eb4d8c4c0 100644 --- a/src/components/checkbox/switch.ts +++ b/src/components/checkbox/switch.ts @@ -30,27 +30,23 @@ export default class IgcSwitchComponent extends IgcCheckboxBaseComponent { public static styles = [styles, shared]; /* blazorSuppress */ - public static register() { + public static register(): void { registerComponent(IgcSwitchComponent); } private static readonly increment = createCounter(); - private inputId = `switch-${IgcSwitchComponent.increment()}`; - private labelId = `switch-label-${this.inputId}`; + private readonly _inputId = `switch-${IgcSwitchComponent.increment()}`; + private readonly _labelId = `switch-label-${this._inputId}`; protected override render() { const labelledBy = this.getAttribute('aria-labelledby'); const checked = this.checked; return html` -