From 6c854f49585faff1d7b321bc72cbc700a9302bd3 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 4 Mar 2022 08:19:36 +0100 Subject: [PATCH] fix(material/button): avoid setting a tabindex on all link buttons (#22901) Fixes that the button component was always setting a `tabindex="0"` when it's placed on a link element. This is unnecessary, because links are in the tab order by default. We actually had a test that has an assertion against this, but it was passing by accident because we weren't running change detection before making the asserting. --- .../mdc-button/button-base.ts | 2 +- .../mdc-button/button.spec.ts | 18 +++++++++++---- src/material/button/button.spec.ts | 22 +++++++++++++------ src/material/button/button.ts | 2 +- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/material-experimental/mdc-button/button-base.ts b/src/material-experimental/mdc-button/button-base.ts index 9d6806a1de71..ee30a73b0fef 100644 --- a/src/material-experimental/mdc-button/button-base.ts +++ b/src/material-experimental/mdc-button/button-base.ts @@ -141,7 +141,7 @@ export const MAT_ANCHOR_HOST = { // Note that we ignore the user-specified tabindex when it's disabled for // consistency with the `mat-button` applied on native buttons where even // though they have an index, they're not tabbable. - '[attr.tabindex]': 'disabled ? -1 : (tabIndex || 0)', + '[attr.tabindex]': 'disabled ? -1 : tabIndex', '[attr.aria-disabled]': 'disabled.toString()', // MDC automatically applies the primary theme color to the button, but we want to support // an unthemed version. If color is undefined, apply a CSS class that makes it easy to diff --git a/src/material-experimental/mdc-button/button.spec.ts b/src/material-experimental/mdc-button/button.spec.ts index b19b59c8b9da..80fdc8d1b86e 100644 --- a/src/material-experimental/mdc-button/button.spec.ts +++ b/src/material-experimental/mdc-button/button.spec.ts @@ -170,11 +170,13 @@ describe('MDC-based MatButton', () => { let fixture = TestBed.createComponent(TestApp); let testComponent = fixture.debugElement.componentInstance; let buttonDebugElement = fixture.debugElement.query(By.css('a'))!; - expect(buttonDebugElement.nativeElement.getAttribute('tabIndex')).toBe(null); + fixture.detectChanges(); + + expect(buttonDebugElement.nativeElement.hasAttribute('tabindex')).toBe(false); testComponent.isDisabled = true; fixture.detectChanges(); - expect(buttonDebugElement.nativeElement.getAttribute('tabIndex')).toBe('-1'); + expect(buttonDebugElement.nativeElement.getAttribute('tabindex')).toBe('-1'); }); it('should add aria-disabled attribute if disabled', () => { @@ -219,18 +221,26 @@ describe('MDC-based MatButton', () => { fixture.componentInstance.tabIndex = 3; fixture.detectChanges(); - expect(buttonElement.getAttribute('tabIndex')) + expect(buttonElement.getAttribute('tabindex')) .withContext('Expected custom tabindex to be set') .toBe('3'); testComponent.isDisabled = true; fixture.detectChanges(); - expect(buttonElement.getAttribute('tabIndex')) + expect(buttonElement.getAttribute('tabindex')) .withContext('Expected custom tabindex to be overwritten when disabled.') .toBe('-1'); }); + it('should not set a default tabindex on enabled links', () => { + const fixture = TestBed.createComponent(TestApp); + const buttonElement = fixture.debugElement.query(By.css('a'))!.nativeElement; + fixture.detectChanges(); + + expect(buttonElement.hasAttribute('tabindex')).toBe(false); + }); + describe('change detection behavior', () => { it('should not run change detection for disabled anchor but should prevent the default behavior and stop event propagation', () => { const appRef = TestBed.inject(ApplicationRef); diff --git a/src/material/button/button.spec.ts b/src/material/button/button.spec.ts index e4c5bcd179a9..8c7bf0d84d4d 100644 --- a/src/material/button/button.spec.ts +++ b/src/material/button/button.spec.ts @@ -181,14 +181,14 @@ describe('MatButton', () => { }); it('should remove tabindex if disabled', () => { - const fixture = TestBed.createComponent(TestApp); - const testComponent = fixture.debugElement.componentInstance; - const buttonDebugElement = fixture.debugElement.query(By.css('a'))!; - expect(buttonDebugElement.nativeElement.getAttribute('tabIndex')).toBe(null); + let fixture = TestBed.createComponent(TestApp); + let testComponent = fixture.debugElement.componentInstance; + let buttonDebugElement = fixture.debugElement.query(By.css('a'))!; + expect(buttonDebugElement.nativeElement.hasAttribute('tabindex')).toBe(false); testComponent.isDisabled = true; fixture.detectChanges(); - expect(buttonDebugElement.nativeElement.getAttribute('tabIndex')).toBe('-1'); + expect(buttonDebugElement.nativeElement.getAttribute('tabindex')).toBe('-1'); }); it('should add aria-disabled attribute if disabled', () => { @@ -233,18 +233,26 @@ describe('MatButton', () => { fixture.componentInstance.tabIndex = 3; fixture.detectChanges(); - expect(buttonElement.getAttribute('tabIndex')) + expect(buttonElement.getAttribute('tabindex')) .withContext('Expected custom tabindex to be set') .toBe('3'); testComponent.isDisabled = true; fixture.detectChanges(); - expect(buttonElement.getAttribute('tabIndex')) + expect(buttonElement.getAttribute('tabindex')) .withContext('Expected custom tabindex to be overwritten when disabled.') .toBe('-1'); }); + it('should not set a default tabindex on enabled links', () => { + const fixture = TestBed.createComponent(TestApp); + const buttonElement = fixture.debugElement.query(By.css('a'))!.nativeElement; + fixture.detectChanges(); + + expect(buttonElement.hasAttribute('tabindex')).toBe(false); + }); + describe('change detection behavior', () => { it('should not run change detection for disabled anchor but should prevent the default behavior and stop event propagation', () => { const appRef = TestBed.inject(ApplicationRef); diff --git a/src/material/button/button.ts b/src/material/button/button.ts index ca1100b324eb..e0f126eff617 100644 --- a/src/material/button/button.ts +++ b/src/material/button/button.ts @@ -162,7 +162,7 @@ export class MatButton // Note that we ignore the user-specified tabindex when it's disabled for // consistency with the `mat-button` applied on native buttons where even // though they have an index, they're not tabbable. - '[attr.tabindex]': 'disabled ? -1 : (tabIndex || 0)', + '[attr.tabindex]': 'disabled ? -1 : tabIndex', '[attr.disabled]': 'disabled || null', '[attr.aria-disabled]': 'disabled.toString()', '[class._mat-animation-noopable]': '_animationMode === "NoopAnimations"',