Skip to content

Commit be51014

Browse files
crisbetojelbourn
authored andcommitted
refactor(list): remove redundant focus logic (#12965)
Currently both the regular list and the selection list have focus and blur handlers whose job it is to toggle a focused class. This is redundant, because the same can be achieved with a `:focus` selector, without having to go through change detection.
1 parent cbfbade commit be51014

File tree

5 files changed

+3
-90
lines changed

5 files changed

+3
-90
lines changed

src/lib/list/_list-theme.scss

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
.mat-list-option,
3030
.mat-nav-list .mat-list-item {
31-
&:hover, &.mat-list-item-focus {
31+
&:hover, &:focus {
3232
background: mat-color($background, 'hover');
3333
}
3434
}

src/lib/list/list.spec.ts

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,23 +28,6 @@ describe('MatList', () => {
2828
TestBed.compileComponents();
2929
}));
3030

31-
it('should add and remove focus class on focus/blur', () => {
32-
let fixture = TestBed.createComponent(ListWithOneAnchorItem);
33-
fixture.detectChanges();
34-
let listItem = fixture.debugElement.query(By.directive(MatListItem));
35-
let listItemEl = fixture.debugElement.query(By.css('.mat-list-item'));
36-
37-
expect(listItemEl.nativeElement.classList).not.toContain('mat-list-item-focus');
38-
39-
listItem.componentInstance._handleFocus();
40-
fixture.detectChanges();
41-
expect(listItemEl.nativeElement.classList).toContain('mat-list-item-focus');
42-
43-
listItem.componentInstance._handleBlur();
44-
fixture.detectChanges();
45-
expect(listItemEl.nativeElement.classList).not.toContain('mat-list-item-focus');
46-
});
47-
4831
it('should not apply any additional class to a list without lines', () => {
4932
let fixture = TestBed.createComponent(ListWithOneItem);
5033
let listItem = fixture.debugElement.query(By.css('mat-list-item'));

src/lib/list/list.ts

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,6 @@ export class MatListSubheaderCssMatStyler {}
107107
// @breaking-change 7.0.0 Remove `mat-list-item-avatar` in favor of `mat-list-item-with-avatar`.
108108
'[class.mat-list-item-avatar]': '_avatar || _icon',
109109
'[class.mat-list-item-with-avatar]': '_avatar || _icon',
110-
'(focus)': '_handleFocus()',
111-
'(blur)': '_handleBlur()',
112110
},
113111
inputs: ['disableRipple'],
114112
templateUrl: 'list-item.html',
@@ -148,14 +146,6 @@ export class MatListItem extends _MatListItemMixinBase implements AfterContentIn
148146
return !this._isNavList || this.disableRipple || this._navList.disableRipple;
149147
}
150148

151-
_handleFocus() {
152-
this._element.nativeElement.classList.add('mat-list-item-focus');
153-
}
154-
155-
_handleBlur() {
156-
this._element.nativeElement.classList.remove('mat-list-item-focus');
157-
}
158-
159149
/** Retrieves the DOM element of the component host. */
160150
_getHostElement(): HTMLElement {
161151
return this._element.nativeElement;

src/lib/list/selection-list.spec.ts

Lines changed: 0 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,6 @@ describe('MatSelectionList without forms', () => {
5252
selectionList = fixture.debugElement.query(By.directive(MatSelectionList));
5353
}));
5454

55-
it('should add and remove focus class on focus/blur', () => {
56-
// Use the second list item, because the first one is always disabled.
57-
const listItem = listOptions[1].nativeElement;
58-
59-
expect(listItem.classList).not.toContain('mat-list-item-focus');
60-
61-
dispatchFakeEvent(listItem, 'focus');
62-
fixture.detectChanges();
63-
expect(listItem.className).toContain('mat-list-item-focus');
64-
65-
dispatchFakeEvent(listItem, 'blur');
66-
fixture.detectChanges();
67-
expect(listItem.className).not.toContain('mat-list-item-focus');
68-
});
69-
7055
it('should be able to set a value on a list option', () => {
7156
const optionValues = ['inbox', 'starred', 'sent-mail', 'drafts'];
7257

@@ -519,45 +504,6 @@ describe('MatSelectionList without forms', () => {
519504
});
520505
});
521506

522-
describe('with single option', () => {
523-
let fixture: ComponentFixture<SelectionListWithOnlyOneOption>;
524-
let listOption: DebugElement;
525-
let listItemEl: DebugElement;
526-
527-
beforeEach(async(() => {
528-
TestBed.configureTestingModule({
529-
imports: [MatListModule],
530-
declarations: [
531-
SelectionListWithListOptions,
532-
SelectionListWithCheckboxPositionAfter,
533-
SelectionListWithListDisabled,
534-
SelectionListWithOnlyOneOption
535-
],
536-
});
537-
538-
TestBed.compileComponents();
539-
}));
540-
541-
beforeEach(async(() => {
542-
fixture = TestBed.createComponent(SelectionListWithOnlyOneOption);
543-
listOption = fixture.debugElement.query(By.directive(MatListOption));
544-
listItemEl = fixture.debugElement.query(By.css('.mat-list-item'));
545-
fixture.detectChanges();
546-
}));
547-
548-
it('should be focused when focus on nativeElements', () => {
549-
dispatchFakeEvent(listOption.nativeElement, 'focus');
550-
fixture.detectChanges();
551-
552-
expect(listItemEl.nativeElement.className).toContain('mat-list-item-focus');
553-
554-
dispatchFakeEvent(listOption.nativeElement, 'blur');
555-
fixture.detectChanges();
556-
557-
expect(listItemEl.nativeElement.className).not.toContain('mat-list-item-focus');
558-
});
559-
});
560-
561507
describe('with option disabled', () => {
562508
let fixture: ComponentFixture<SelectionListWithDisabledOption>;
563509
let listOptionEl: HTMLElement;

src/lib/list/selection-list.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ export class MatSelectionListChange {
8585
'(click)': '_handleClick()',
8686
'tabindex': '-1',
8787
'[class.mat-list-item-disabled]': 'disabled',
88-
'[class.mat-list-item-focus]': '_hasFocus',
8988
'[class.mat-list-item-with-avatar]': '_avatar || _icon',
9089
'[attr.aria-selected]': 'selected.toString()',
9190
'[attr.aria-disabled]': 'disabled.toString()',
@@ -100,9 +99,6 @@ export class MatListOption extends _MatListOptionMixinBase
10099
private _selected = false;
101100
private _disabled = false;
102101

103-
/** Whether the option has focus. */
104-
_hasFocus: boolean = false;
105-
106102
@ContentChild(MatListAvatarCssMatStyler) _avatar: MatListAvatarCssMatStyler;
107103
@ContentChild(MatListIconCssMatStyler) _icon: MatListIconCssMatStyler;
108104
@ContentChildren(MatLine) _lines: QueryList<MatLine>;
@@ -212,12 +208,10 @@ export class MatListOption extends _MatListOptionMixinBase
212208
}
213209

214210
_handleFocus() {
215-
this._hasFocus = true;
216211
this.selectionList._setFocusedOption(this);
217212
}
218213

219214
_handleBlur() {
220-
this._hasFocus = false;
221215
this.selectionList._onTouched();
222216
}
223217

@@ -392,9 +386,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu
392386

393387
/** Removes an option from the selection list and updates the active item. */
394388
_removeOptionFromList(option: MatListOption) {
395-
if (option._hasFocus) {
396-
const optionIndex = this._getOptionIndex(option);
389+
const optionIndex = this._getOptionIndex(option);
397390

391+
if (optionIndex > -1 && this._keyManager.activeItemIndex === optionIndex) {
398392
// Check whether the option is the last item
399393
if (optionIndex > 0) {
400394
this._keyManager.setPreviousItemActive();

0 commit comments

Comments
 (0)