From be510149e894fd9f6364377e209e6c7305251bf6 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 11 Sep 2018 19:43:22 +0200 Subject: [PATCH] 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. --- src/lib/list/_list-theme.scss | 2 +- src/lib/list/list.spec.ts | 17 --------- src/lib/list/list.ts | 10 ------ src/lib/list/selection-list.spec.ts | 54 ----------------------------- src/lib/list/selection-list.ts | 10 ++---- 5 files changed, 3 insertions(+), 90 deletions(-) diff --git a/src/lib/list/_list-theme.scss b/src/lib/list/_list-theme.scss index 09f44509c4e..364dd0a4854 100644 --- a/src/lib/list/_list-theme.scss +++ b/src/lib/list/_list-theme.scss @@ -28,7 +28,7 @@ .mat-list-option, .mat-nav-list .mat-list-item { - &:hover, &.mat-list-item-focus { + &:hover, &:focus { background: mat-color($background, 'hover'); } } diff --git a/src/lib/list/list.spec.ts b/src/lib/list/list.spec.ts index 5339f846cd4..4f8f2c06251 100644 --- a/src/lib/list/list.spec.ts +++ b/src/lib/list/list.spec.ts @@ -28,23 +28,6 @@ describe('MatList', () => { TestBed.compileComponents(); })); - it('should add and remove focus class on focus/blur', () => { - let fixture = TestBed.createComponent(ListWithOneAnchorItem); - fixture.detectChanges(); - let listItem = fixture.debugElement.query(By.directive(MatListItem)); - let listItemEl = fixture.debugElement.query(By.css('.mat-list-item')); - - expect(listItemEl.nativeElement.classList).not.toContain('mat-list-item-focus'); - - listItem.componentInstance._handleFocus(); - fixture.detectChanges(); - expect(listItemEl.nativeElement.classList).toContain('mat-list-item-focus'); - - listItem.componentInstance._handleBlur(); - fixture.detectChanges(); - expect(listItemEl.nativeElement.classList).not.toContain('mat-list-item-focus'); - }); - it('should not apply any additional class to a list without lines', () => { let fixture = TestBed.createComponent(ListWithOneItem); let listItem = fixture.debugElement.query(By.css('mat-list-item')); diff --git a/src/lib/list/list.ts b/src/lib/list/list.ts index d2f4c310d7f..81e9b408e9b 100644 --- a/src/lib/list/list.ts +++ b/src/lib/list/list.ts @@ -107,8 +107,6 @@ export class MatListSubheaderCssMatStyler {} // @breaking-change 7.0.0 Remove `mat-list-item-avatar` in favor of `mat-list-item-with-avatar`. '[class.mat-list-item-avatar]': '_avatar || _icon', '[class.mat-list-item-with-avatar]': '_avatar || _icon', - '(focus)': '_handleFocus()', - '(blur)': '_handleBlur()', }, inputs: ['disableRipple'], templateUrl: 'list-item.html', @@ -148,14 +146,6 @@ export class MatListItem extends _MatListItemMixinBase implements AfterContentIn return !this._isNavList || this.disableRipple || this._navList.disableRipple; } - _handleFocus() { - this._element.nativeElement.classList.add('mat-list-item-focus'); - } - - _handleBlur() { - this._element.nativeElement.classList.remove('mat-list-item-focus'); - } - /** Retrieves the DOM element of the component host. */ _getHostElement(): HTMLElement { return this._element.nativeElement; diff --git a/src/lib/list/selection-list.spec.ts b/src/lib/list/selection-list.spec.ts index 2c5c204aadd..86bab1bb0a7 100644 --- a/src/lib/list/selection-list.spec.ts +++ b/src/lib/list/selection-list.spec.ts @@ -52,21 +52,6 @@ describe('MatSelectionList without forms', () => { selectionList = fixture.debugElement.query(By.directive(MatSelectionList)); })); - it('should add and remove focus class on focus/blur', () => { - // Use the second list item, because the first one is always disabled. - const listItem = listOptions[1].nativeElement; - - expect(listItem.classList).not.toContain('mat-list-item-focus'); - - dispatchFakeEvent(listItem, 'focus'); - fixture.detectChanges(); - expect(listItem.className).toContain('mat-list-item-focus'); - - dispatchFakeEvent(listItem, 'blur'); - fixture.detectChanges(); - expect(listItem.className).not.toContain('mat-list-item-focus'); - }); - it('should be able to set a value on a list option', () => { const optionValues = ['inbox', 'starred', 'sent-mail', 'drafts']; @@ -519,45 +504,6 @@ describe('MatSelectionList without forms', () => { }); }); - describe('with single option', () => { - let fixture: ComponentFixture; - let listOption: DebugElement; - let listItemEl: DebugElement; - - beforeEach(async(() => { - TestBed.configureTestingModule({ - imports: [MatListModule], - declarations: [ - SelectionListWithListOptions, - SelectionListWithCheckboxPositionAfter, - SelectionListWithListDisabled, - SelectionListWithOnlyOneOption - ], - }); - - TestBed.compileComponents(); - })); - - beforeEach(async(() => { - fixture = TestBed.createComponent(SelectionListWithOnlyOneOption); - listOption = fixture.debugElement.query(By.directive(MatListOption)); - listItemEl = fixture.debugElement.query(By.css('.mat-list-item')); - fixture.detectChanges(); - })); - - it('should be focused when focus on nativeElements', () => { - dispatchFakeEvent(listOption.nativeElement, 'focus'); - fixture.detectChanges(); - - expect(listItemEl.nativeElement.className).toContain('mat-list-item-focus'); - - dispatchFakeEvent(listOption.nativeElement, 'blur'); - fixture.detectChanges(); - - expect(listItemEl.nativeElement.className).not.toContain('mat-list-item-focus'); - }); - }); - describe('with option disabled', () => { let fixture: ComponentFixture; let listOptionEl: HTMLElement; diff --git a/src/lib/list/selection-list.ts b/src/lib/list/selection-list.ts index f1598c2a2cc..322e4c30c5b 100644 --- a/src/lib/list/selection-list.ts +++ b/src/lib/list/selection-list.ts @@ -85,7 +85,6 @@ export class MatSelectionListChange { '(click)': '_handleClick()', 'tabindex': '-1', '[class.mat-list-item-disabled]': 'disabled', - '[class.mat-list-item-focus]': '_hasFocus', '[class.mat-list-item-with-avatar]': '_avatar || _icon', '[attr.aria-selected]': 'selected.toString()', '[attr.aria-disabled]': 'disabled.toString()', @@ -100,9 +99,6 @@ export class MatListOption extends _MatListOptionMixinBase private _selected = false; private _disabled = false; - /** Whether the option has focus. */ - _hasFocus: boolean = false; - @ContentChild(MatListAvatarCssMatStyler) _avatar: MatListAvatarCssMatStyler; @ContentChild(MatListIconCssMatStyler) _icon: MatListIconCssMatStyler; @ContentChildren(MatLine) _lines: QueryList; @@ -212,12 +208,10 @@ export class MatListOption extends _MatListOptionMixinBase } _handleFocus() { - this._hasFocus = true; this.selectionList._setFocusedOption(this); } _handleBlur() { - this._hasFocus = false; this.selectionList._onTouched(); } @@ -392,9 +386,9 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu /** Removes an option from the selection list and updates the active item. */ _removeOptionFromList(option: MatListOption) { - if (option._hasFocus) { - const optionIndex = this._getOptionIndex(option); + const optionIndex = this._getOptionIndex(option); + if (optionIndex > -1 && this._keyManager.activeItemIndex === optionIndex) { // Check whether the option is the last item if (optionIndex > 0) { this._keyManager.setPreviousItemActive();