Skip to content

Commit 2febfa6

Browse files
rkaraivanovrkaraivanov
authored
refactor: Keyboard focus ring controller (#1684)
1 parent 037e5fa commit 2febfa6

File tree

12 files changed

+206
-214
lines changed

12 files changed

+206
-214
lines changed

‎src/components/button-group/toggle-button.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,14 @@ export default class IgcToggleButtonComponent extends LitElement {
3030
};
3131

3232
/* blazorSuppress */
33-
public static register() {
33+
public static register(): void {
3434
registerComponent(IgcToggleButtonComponent);
3535
}
3636

37-
private _kbFocus = addKeyboardFocusRing(this);
37+
private readonly _focusRingManager = addKeyboardFocusRing(this);
3838

3939
@query('[part="toggle"]', true)
40-
private _nativeButton!: HTMLButtonElement;
40+
private readonly _nativeButton!: HTMLButtonElement;
4141

4242
/**
4343
* The value attribute of the control.
@@ -62,32 +62,33 @@ export default class IgcToggleButtonComponent extends LitElement {
6262

6363
/* alternateName: focusComponent */
6464
/** Sets focus on the button. */
65-
public override focus(options?: FocusOptions) {
65+
public override focus(options?: FocusOptions): void {
6666
this._nativeButton.focus(options);
6767
}
6868

6969
/* alternateName: blurComponent */
7070
/** Removes focus from the button. */
71-
public override blur() {
71+
public override blur(): void {
7272
this._nativeButton.blur();
7373
}
7474

7575
/** Simulates a mouse click on the element. */
76-
public override click() {
76+
public override click(): void {
7777
this._nativeButton.click();
7878
}
7979

8080
protected override render() {
8181
return html`
8282
<button
83-
part=${partMap({ toggle: true, focused: this._kbFocus.focused })}
83+
part=${partMap({
84+
toggle: true,
85+
focused: this._focusRingManager.focused,
86+
})}
8487
type="button"
8588
?disabled=${this.disabled}
8689
.ariaLabel=${this.ariaLabel}
8790
aria-pressed=${this.selected}
8891
aria-disabled=${this.disabled}
89-
@click=${this._kbFocus.reset}
90-
@blur=${this._kbFocus.reset}
9192
>
9293
<slot></slot>
9394
</button>

‎src/components/button/button-base.ts‎

Lines changed: 21 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,14 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin<
2828
delegatesFocus: true,
2929
};
3030

31-
private _kbFocus = addKeyboardFocusRing(this);
31+
protected readonly __internals: ElementInternals;
32+
33+
private readonly _focusRingManager = addKeyboardFocusRing(this);
3234

33-
protected __internals: ElementInternals;
3435
protected _disabled = false;
3536

3637
@query('[part="base"]', true)
37-
private _nativeButton!: HTMLButtonElement;
38+
private readonly _nativeButton!: HTMLButtonElement;
3839

3940
/* alternateName: displayType */
4041
/**
@@ -78,19 +79,19 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin<
7879
* @attr [disabled=false]
7980
*/
8081
@property({ type: Boolean, reflect: true })
81-
public get disabled(): boolean {
82-
return this._disabled;
83-
}
84-
8582
public set disabled(value: boolean) {
8683
this._disabled = value;
8784
this.toggleAttribute('disabled', Boolean(this._disabled));
8885
}
8986

87+
public get disabled(): boolean {
88+
return this._disabled;
89+
}
90+
9091
/* blazorCSSuppress */
9192
/* alternateType: object */
9293
/** Returns the HTMLFormElement associated with this element. */
93-
public get form() {
94+
public get form(): HTMLFormElement | null {
9495
return this.__internals.form;
9596
}
9697

@@ -101,51 +102,42 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin<
101102

102103
/* alternateName: focusComponent */
103104
/** Sets focus in the button. */
104-
public override focus(options?: FocusOptions) {
105+
public override focus(options?: FocusOptions): void {
105106
this._nativeButton.focus(options);
106107
}
107108

108109
/** Simulates a mouse click on the element */
109-
public override click() {
110+
public override click(): void {
110111
this._nativeButton.click();
111112
}
112113

113114
/* alternateName: blurComponent */
114115
/** Removes focus from the button. */
115-
public override blur() {
116+
public override blur(): void {
116117
this._nativeButton.blur();
117118
}
118119

119-
protected handleBlur() {
120-
this._kbFocus.reset();
121-
}
122-
123-
protected handleClick() {
124-
this._kbFocus.reset();
125-
switch (this.type) {
126-
case 'submit':
127-
return this.form?.requestSubmit();
128-
case 'reset':
129-
return this.form?.reset();
130-
default:
131-
return;
120+
protected _handleClick(): void {
121+
if (this.type === 'submit') {
122+
this.form?.requestSubmit();
123+
} else if (this.type === 'reset') {
124+
this.form?.reset();
132125
}
133126
}
134127

135-
protected formDisabledCallback(state: boolean) {
128+
protected formDisabledCallback(state: boolean): void {
136129
this._disabled = state;
137130
this.requestUpdate();
138131
}
139132

140133
private renderButton() {
141134
return html`
142135
<button
143-
part=${partMap({ base: true, focused: this._kbFocus.focused })}
136+
part=${partMap({ base: true, focused: this._focusRingManager.focused })}
144137
aria-label=${ifDefined(this.ariaLabel ?? nothing)}
145138
?disabled=${this.disabled}
146139
type=${ifDefined(this.type)}
147-
@click=${this.handleClick}
148-
@blur=${this.handleBlur}
140+
@click=${this._handleClick}
149141
>
150142
${this.renderContent()}
151143
</button>
@@ -155,15 +147,14 @@ export abstract class IgcButtonBaseComponent extends EventEmitterMixin<
155147
private renderLinkButton() {
156148
return html`
157149
<a
158-
part=${partMap({ base: true, focused: this._kbFocus.focused })}
150+
part=${partMap({ base: true, focused: this._focusRingManager.focused })}
159151
role="button"
160152
aria-label=${ifDefined(this.ariaLabel ?? nothing)}
161153
aria-disabled=${this.disabled}
162154
href=${ifDefined(this.href)}
163155
target=${ifDefined(this.target)}
164156
download=${ifDefined(this.download)}
165157
rel=${ifDefined(this.rel)}
166-
@blur=${this.disabled ? nothing : this.handleBlur}
167158
>
168159
${this.renderContent()}
169160
</a>

‎src/components/carousel/carousel-indicator-container.spec.ts‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@ import { elementUpdated, expect, fixture, html } from '@open-wc/testing';
22

33
import { tabKey } from '../common/controllers/key-bindings.js';
44
import { defineComponents } from '../common/definitions/defineComponents.js';
5-
import { simulateClick, simulateKeyboard } from '../common/utils.spec.js';
5+
import { first } from '../common/util.js';
6+
import {
7+
simulateClick,
8+
simulateKeyboard,
9+
simulatePointerDown,
10+
} from '../common/utils.spec.js';
611
import IgcCarouselIndicatorContainerComponent from './carousel-indicator-container.js';
712
import IgcCarouselIndicatorComponent from './carousel-indicator.js';
813

@@ -26,12 +31,8 @@ describe('Carousel Indicator Container', () => {
2631
let buttons: HTMLButtonElement[];
2732

2833
beforeEach(async () => {
29-
container = await fixture<IgcCarouselIndicatorContainerComponent>(
30-
createIndicatorContainerComponent()
31-
);
32-
buttons = container.querySelectorAll(
33-
'button'
34-
) as unknown as HTMLButtonElement[];
34+
container = await fixture(createIndicatorContainerComponent());
35+
buttons = Array.from(container.querySelectorAll('button'));
3536
});
3637

3738
it('is correctly initialized', () => {
@@ -56,7 +57,7 @@ describe('Carousel Indicator Container', () => {
5657
</div>`
5758
);
5859

59-
simulateKeyboard(buttons[0], tabKey);
60+
simulateKeyboard(first(buttons), tabKey);
6061
await elementUpdated(container);
6162

6263
expect(container).shadowDom.to.equal(
@@ -67,7 +68,7 @@ describe('Carousel Indicator Container', () => {
6768
});
6869

6970
it('should remove `focused` part on click', async () => {
70-
simulateKeyboard(buttons[0], tabKey);
71+
simulateKeyboard(first(buttons), tabKey);
7172
await elementUpdated(container);
7273

7374
expect(container).shadowDom.to.equal(
@@ -76,7 +77,8 @@ describe('Carousel Indicator Container', () => {
7677
</div>`
7778
);
7879

79-
simulateClick(buttons[0]);
80+
simulatePointerDown(first(buttons));
81+
simulateClick(first(buttons));
8082
await elementUpdated(container);
8183

8284
expect(container).shadowDom.to.equal(
@@ -87,7 +89,7 @@ describe('Carousel Indicator Container', () => {
8789
});
8890

8991
it('it should remove `focused` part on focusout', async () => {
90-
simulateKeyboard(buttons[0], tabKey);
92+
simulateKeyboard(first(buttons), tabKey);
9193
await elementUpdated(container);
9294

9395
expect(container).shadowDom.to.equal(
@@ -96,7 +98,7 @@ describe('Carousel Indicator Container', () => {
9698
</div>`
9799
);
98100

99-
buttons[0].dispatchEvent(new FocusEvent('focusout', { bubbles: true }));
101+
first(buttons).dispatchEvent(new FocusEvent('focusout', { bubbles: true }));
100102
await elementUpdated(container);
101103

102104
expect(container).shadowDom.to.equal(
@@ -107,7 +109,7 @@ describe('Carousel Indicator Container', () => {
107109
});
108110

109111
it('it should not remove `focused` part on focusout if the target receiving focus is an `igc-carousel-indicator`', async () => {
110-
simulateKeyboard(buttons[0], tabKey);
112+
simulateKeyboard(first(buttons), tabKey);
111113
await elementUpdated(container);
112114

113115
expect(container).shadowDom.to.equal(
@@ -116,14 +118,14 @@ describe('Carousel Indicator Container', () => {
116118
</div>`
117119
);
118120

119-
const indicator = await fixture<IgcCarouselIndicatorComponent>(
121+
const indicator = await fixture(
120122
html`<igc-carousel-indicator>
121123
<span>0</span>
122124
<span slot="active">1</span>
123125
</igc-carousel-indicator>`
124126
);
125127

126-
buttons[0].dispatchEvent(
128+
first(buttons).dispatchEvent(
127129
new FocusEvent('focusout', { bubbles: true, relatedTarget: indicator })
128130
);
129131
await elementUpdated(container);

‎src/components/carousel/carousel-indicator-container.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,19 @@ export default class IgcCarouselIndicatorContainerComponent extends LitElement {
2222
public static override styles = [styles, shared];
2323

2424
/* blazorSuppress */
25-
public static register() {
25+
public static register(): void {
2626
registerComponent(IgcCarouselIndicatorContainerComponent);
2727
}
2828

29-
private _kbFocus = addKeyboardFocusRing(this);
29+
private readonly _focusRingManager = addKeyboardFocusRing(this);
3030

31-
private handleFocusOut(event: FocusEvent) {
31+
private _handleFocusOut(event: FocusEvent): void {
3232
const target = event.relatedTarget as Element;
3333

34-
if (!target?.matches(IgcCarouselIndicatorComponent.tagName)) {
35-
this._kbFocus.reset();
34+
if (target?.matches(IgcCarouselIndicatorComponent.tagName)) {
35+
// Stop the event from hitting the _focusRingManager handler redrawing
36+
// the keyboard focus styles
37+
event.stopPropagation();
3638
}
3739
}
3840

@@ -41,10 +43,9 @@ export default class IgcCarouselIndicatorContainerComponent extends LitElement {
4143
<div
4244
part=${partMap({
4345
base: true,
44-
focused: this._kbFocus.focused,
46+
focused: this._focusRingManager.focused,
4547
})}
46-
@click=${this._kbFocus.reset}
47-
@focusout=${this.handleFocusOut}
48+
@focusout=${this._handleFocusOut}
4849
>
4950
<slot></slot>
5051
</div>

0 commit comments

Comments
 (0)