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.

Author: crisbeto

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

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);

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);

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"',