fix(sort): sorted state being read out when navigating cells

Reworks the sort header to allow for screen readers to read out its sorted state through `aria-sort`, rather than having to declare an `aria-label` inside it. The drawback of the `aria-label` approach is that the label will also be read out as the user is navigating the table's cells.

Fixes #13012.

Author: crisbeto

src/material/core/focus-indicators/_focus-indicators.scss

@@ -41,7 +41,7 @@
   .mat-focus-indicator.mat-fab::before,
   .mat-focus-indicator.mat-mini-fab::before,
   .mat-focus-indicator.mat-chip::before,
-  .mat-focus-indicator.mat-sort-header-button::before {
+  .mat-focus-indicator.mat-sort-header-container::before {
     margin: -($border-width + 2px);
   }
 

src/material/sort/BUILD.bazel

@@ -21,6 +21,7 @@ ng_module(
     deps = [
         "//src/cdk/a11y",
         "//src/cdk/coercion",
+        "//src/cdk/keycodes",
         "//src/material/core",
         "@npm//@angular/animations",
         "@npm//@angular/common",

src/material/sort/sort-header-intl.ts

@@ -21,7 +21,11 @@ export class MatSortHeaderIntl {
    */
   readonly changes: Subject<void> = new Subject<void>();
 
-  /** ARIA label for the sorting button. */
+  /**
+   * ARIA label for the sorting button.
+   * @deprecated Not used anymore. To be removed.
+   * @breaking-change 8.0.0
+   */
   sortButtonLabel = (id: string) => {
     return `Change sorting for ${id}`;
   }

src/material/sort/sort-header.html

@@ -1,11 +1,28 @@
-<div class="mat-sort-header-container"
+<!--
+  We set the `tabindex` on an element inside the table header, rather than the header itself,
+  because of a bug in NVDA where having a `tabindex` on a `th` breaks keyboard navigation in the
+  table (see https://github.com/nvaccess/nvda/issues/7718). This allows for the header to both
+  be focusable, and have screen readers read out its `aria-sort` state. We prefer this approach
+  over having a button with an `aria-label` inside the header, because the button's `aria-label`
+  will be read out as the user is navigating the table's cell (see #13012).
+
+  The approach is based off of: https://dequeuniversity.com/library/aria/tables/sf-sortable-grid
+-->
+<div class="mat-sort-header-container mat-focus-indicator"
      [class.mat-sort-header-sorted]="_isSorted()"
-     [class.mat-sort-header-position-before]="arrowPosition == 'before'">
-  <button class="mat-sort-header-button mat-focus-indicator" type="button"
-          [attr.disabled]="_isDisabled() || null"
-          [attr.aria-label]="_intl.sortButtonLabel(id)">
+     [class.mat-sort-header-position-before]="arrowPosition == 'before'"
+     [attr.tabindex]="_isDisabled() ? null : 0"
+     role="button">
+
+  <!--
+    TODO(crisbeto): this div isn't strictly necessary, but we have to keep it due to a large
+    number of screenshot diff failures. It should be removed eventually. Note that the difference
+    isn't visible with a shorter header, but once it breaks up into multiple lines, this element
+    causes it to be center-aligned, whereas removing it will keep the text to the left.
+  -->
+  <div class="mat-sort-header-content">
     <ng-content></ng-content>
-  </button>
+  </div>
 
   <!-- Disable animations while a current animation is running -->
   <div class="mat-sort-header-arrow"

src/material/sort/sort-header.scss

@@ -11,26 +11,11 @@ $mat-sort-header-arrow-hint-opacity: 0.38;
   display: flex;
   cursor: pointer;
   align-items: center;
+  letter-spacing: normal;
 
-  .mat-sort-header-disabled & {
-    cursor: default;
-  }
-}
-
-.mat-sort-header-position-before {
-  flex-direction: row-reverse;
-}
-
-.mat-sort-header-button {
-  border: none;
-  background: 0 0;
-  display: flex;
-  align-items: center;
-  padding: 0;
-  cursor: inherit;
+  // Needs to be reset since we don't want an outline around the inner
+  // div which is focusable. We have our own alternate focus styling.
   outline: 0;
-  font: inherit;
-  color: currentColor;
 
   // Usually we could rely on the arrow showing up to be focus indication, but if a header is
   // active, the arrow will always be shown so the user has no way of telling the difference.
@@ -39,14 +24,21 @@ $mat-sort-header-arrow-hint-opacity: 0.38;
     border-bottom: solid 1px currentColor;
   }
 
-  // The `outline: 0` from above works on all browsers, however Firefox also
-  // adds a special `focus-inner` which we have to disable explicitly. See:
-  // https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Firefox
-  &::-moz-focus-inner {
-    border: 0;
+  .mat-sort-header-disabled & {
+    cursor: default;
   }
 }
 
+.mat-sort-header-content {
+  text-align: center;
+  display: flex;
+  align-items: center;
+}
+
+.mat-sort-header-position-before {
+  flex-direction: row-reverse;
+}
+
 .mat-sort-header-arrow {
   height: $mat-sort-header-arrow-container-size;
   width: $mat-sort-header-arrow-container-size;

src/material/sort/sort-header.ts

@@ -22,6 +22,7 @@ import {
 } from '@angular/core';
 import {CanDisable, CanDisableCtor, mixinDisabled} from '@angular/material/core';
 import {FocusMonitor} from '@angular/cdk/a11y';
+import {ENTER, SPACE} from '@angular/cdk/keycodes';
 import {merge, Subscription} from 'rxjs';
 import {MatSort, MatSortable} from './sort';
 import {matSortAnimations} from './sort-animations';
@@ -78,6 +79,7 @@ interface MatSortHeaderColumnDef {
   host: {
     'class': 'mat-sort-header',
     '(click)': '_handleClick()',
+    '(keydown)': '_handleKeydown($event)',
     '(mouseenter)': '_setIndicatorHintVisible(true)',
     '(mouseleave)': '_setIndicatorHintVisible(false)',
     '[attr.aria-sort]': '_getAriaSortAttribute()',
@@ -233,8 +235,7 @@ export class MatSortHeader extends _MatSortHeaderMixinBase
   }
 
   /** Triggers the sort on this sort header and removes the indicator hint. */
-  _handleClick() {
-    if (this._isDisabled()) { return; }
+  _toggleOnInteraction() {
 
     this._sort.sort(this);
 
@@ -253,6 +254,19 @@ export class MatSortHeader extends _MatSortHeaderMixinBase
     this._showIndicatorHint = false;
   }
 
+  _handleClick() {
+    if (!this._isDisabled()) {
+      this._toggleOnInteraction();
+    }
+  }
+
+  _handleKeydown(event: KeyboardEvent) {
+    if (!this._isDisabled() && (event.keyCode === SPACE || event.keyCode === ENTER)) {
+      event.preventDefault();
+      this._toggleOnInteraction();
+    }
+  }
+
   /** Whether this MatSortHeader is currently sorted in either ascending or descending order. */
   _isSorted() {
     return this._sort.active == this.id &&
@@ -297,7 +311,9 @@ export class MatSortHeader extends _MatSortHeaderMixinBase
    * ensures this is true.
    */
   _getAriaSortAttribute() {
-    if (!this._isSorted()) { return null; }
+    if (!this._isSorted()) {
+      return 'none';
+    }
 
     return this._sort.direction == 'asc' ? 'ascending' : 'descending';
   }

src/material/sort/sort.spec.ts

@@ -7,16 +7,15 @@ import {
   wrappedErrorMessage,
 } from '@angular/cdk/testing/private';
 import {Component, ElementRef, ViewChild} from '@angular/core';
-import {async, ComponentFixture, fakeAsync, inject, TestBed, tick} from '@angular/core/testing';
-import {By} from '@angular/platform-browser';
+import {async, ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing';
 import {NoopAnimationsModule} from '@angular/platform-browser/animations';
+import {By} from '@angular/platform-browser';
 import {Observable} from 'rxjs';
 import {map} from 'rxjs/operators';
 import {MatTableModule} from '../table/index';
 import {
   MatSort,
   MatSortHeader,
-  MatSortHeaderIntl,
   MatSortModule,
   Sort,
   SortDirection
@@ -220,40 +219,40 @@ describe('MatSort', () => {
   });
 
   it('should allow for the cycling the sort direction to be disabled per column', () => {
-    const button = fixture.nativeElement.querySelector('#defaultA button');
+    const container = fixture.nativeElement.querySelector('#defaultA .mat-sort-header-container');
 
     component.sort('defaultA');
     expect(component.matSort.direction).toBe('asc');
-    expect(button.getAttribute('disabled')).toBeFalsy();
+    expect(container.getAttribute('tabindex')).toBe('0');
 
     component.disabledColumnSort = true;
     fixture.detectChanges();
 
     component.sort('defaultA');
     expect(component.matSort.direction).toBe('asc');
-    expect(button.getAttribute('disabled')).toBe('true');
+    expect(container.getAttribute('tabindex')).toBeFalsy();
   });
 
   it('should allow for the cycling the sort direction to be disabled for all columns', () => {
-    const button = fixture.nativeElement.querySelector('#defaultA button');
+    const container = fixture.nativeElement.querySelector('#defaultA .mat-sort-header-container');
 
     component.sort('defaultA');
     expect(component.matSort.active).toBe('defaultA');
     expect(component.matSort.direction).toBe('asc');
-    expect(button.getAttribute('disabled')).toBeFalsy();
+    expect(container.getAttribute('tabindex')).toBe('0');
 
     component.disableAllSort = true;
     fixture.detectChanges();
 
     component.sort('defaultA');
     expect(component.matSort.active).toBe('defaultA');
     expect(component.matSort.direction).toBe('asc');
-    expect(button.getAttribute('disabled')).toBe('true');
+    expect(container.getAttribute('tabindex')).toBeFalsy();
 
     component.sort('defaultB');
     expect(component.matSort.active).toBe('defaultA');
     expect(component.matSort.direction).toBe('asc');
-    expect(button.getAttribute('disabled')).toBe('true');
+    expect(container.getAttribute('tabindex')).toBeFalsy();
   });
 
   it('should reset sort direction when a different column is sorted', () => {
@@ -301,29 +300,24 @@ describe('MatSort', () => {
         fixture, ['asc', 'desc'], 'overrideDisableClear');
   });
 
-  it('should apply the aria-labels to the button', () => {
-    const button = fixture.nativeElement.querySelector('#defaultA button');
-    expect(button.getAttribute('aria-label')).toBe('Change sorting for defaultA');
-  });
-
   it('should toggle indicator hint on button focus/blur and hide on click', () => {
     const header = fixture.componentInstance.defaultA;
-    const button = fixture.nativeElement.querySelector('#defaultA button');
+    const container = fixture.nativeElement.querySelector('#defaultA .mat-sort-header-container');
     const focusEvent = createFakeEvent('focus');
     const blurEvent = createFakeEvent('blur');
 
     // Should start without a displayed hint
     expect(header._showIndicatorHint).toBeFalsy();
 
     // Focusing the button should show the hint, blurring should hide it
-    button.dispatchEvent(focusEvent);
+    container.dispatchEvent(focusEvent);
     expect(header._showIndicatorHint).toBeTruthy();
 
-    button.dispatchEvent(blurEvent);
+    container.dispatchEvent(blurEvent);
     expect(header._showIndicatorHint).toBeFalsy();
 
     // Show the indicator hint. On click the hint should be hidden
-    button.dispatchEvent(focusEvent);
+    container.dispatchEvent(focusEvent);
     expect(header._showIndicatorHint).toBeTruthy();
 
     header._handleClick();
@@ -356,7 +350,7 @@ describe('MatSort', () => {
 
   it('should apply the aria-sort label to the header when sorted', () => {
     const sortHeaderElement = fixture.nativeElement.querySelector('#defaultA');
-    expect(sortHeaderElement.getAttribute('aria-sort')).toBe(null);
+    expect(sortHeaderElement.getAttribute('aria-sort')).toBe('none');
 
     component.sort('defaultA');
     fixture.detectChanges();
@@ -368,22 +362,9 @@ describe('MatSort', () => {
 
     component.sort('defaultA');
     fixture.detectChanges();
-    expect(sortHeaderElement.getAttribute('aria-sort')).toBe(null);
+    expect(sortHeaderElement.getAttribute('aria-sort')).toBe('none');
   });
 
-  it('should re-render when the i18n labels have changed',
-    inject([MatSortHeaderIntl], (intl: MatSortHeaderIntl) => {
-      const header = fixture.debugElement.query(By.directive(MatSortHeader))!.nativeElement;
-      const button = header.querySelector('.mat-sort-header-button');
-
-      intl.sortButtonLabel = () => 'Sort all of the things';
-      intl.changes.next();
-      fixture.detectChanges();
-
-      expect(button.getAttribute('aria-label')).toBe('Sort all of the things');
-    })
-  );
-
   it('should not render the arrow if sorting is disabled for that column', fakeAsync(() => {
     const sortHeaderElement = fixture.nativeElement.querySelector('#defaultA');
 
@@ -412,9 +393,9 @@ describe('MatSort', () => {
   it('should have a focus indicator', () => {
     const headerNativeElement =
         fixture.debugElement.query(By.directive(MatSortHeader))!.nativeElement;
-    const buttonNativeElement = headerNativeElement.querySelector('.mat-sort-header-button');
+    const container = headerNativeElement.querySelector('.mat-sort-header-container');
 
-    expect(buttonNativeElement.classList.contains('mat-focus-indicator')).toBe(true);
+    expect(container.classList.contains('mat-focus-indicator')).toBe(true);
   });
 
 });

src/material/sort/testing/shared.spec.ts

@@ -69,20 +69,6 @@ export function runHarnessTests(
     expect(labels).toEqual(['Dessert', 'Calories', 'Fat', 'Carbs', 'Protein']);
   });
 
-  it('should be able to get the aria-label of a header', async () => {
-    const sort = await loader.getHarness(sortHarness);
-    const headers = await sort.getSortHeaders();
-    const labels = await Promise.all(headers.map(header => header.getAriaLabel()));
-
-    expect(labels).toEqual([
-      'Change sorting for name',
-      'Change sorting for calories',
-      'Change sorting for fat',
-      'Change sorting for carbs',
-      'Change sorting for protein'
-    ]);
-  });
-
   it('should get the disabled state of a header', async () => {
     const sort = await loader.getHarness(sortHarness);
     const thirdHeader = (await sort.getSortHeaders())[2];

src/material/sort/testing/sort-header-harness.ts

@@ -13,7 +13,7 @@ import {SortHeaderHarnessFilters} from './sort-harness-filters';
 /** Harness for interacting with a standard Angular Material sort header in tests. */
 export class MatSortHeaderHarness extends ComponentHarness {
   static hostSelector = '.mat-sort-header';
-  private _button = this.locatorFor('.mat-sort-header-button');
+  private _container = this.locatorFor('.mat-sort-header-container');
 
   /**
    * Gets a `HarnessPredicate` that can be used to
@@ -30,7 +30,7 @@ export class MatSortHeaderHarness extends ComponentHarness {
 
   /** Gets the label of the sort header. */
   async getLabel(): Promise<string> {
-    return (await this._button()).text();
+    return (await this._container()).text();
   }
 
   /** Gets the sorting direction of the header. */
@@ -47,9 +47,13 @@ export class MatSortHeaderHarness extends ComponentHarness {
     return '';
   }
 
-  /** Gets the aria-label of the sort header. */
+  /**
+   * Gets the aria-label of the sort header.
+   * @deprecated The sort header no longer has an `aria-label`. This method will be removed.
+   * @breaking-change 11.0.0
+   */
   async getAriaLabel(): Promise<string|null> {
-    return (await this._button()).getAttribute('aria-label');
+    return (await this._container()).getAttribute('aria-label');
   }
 
   /** Gets whether the sort header is currently being sorted by. */
@@ -59,8 +63,7 @@ export class MatSortHeaderHarness extends ComponentHarness {
 
   /** Whether the sort header is disabled. */
   async isDisabled(): Promise<boolean> {
-    const button = await this._button();
-    return (await button.getAttribute('disabled')) != null;
+    return (await this.host()).hasClass('mat-sort-header-disabled');
   }
 
   /** Clicks the header to change its sorting direction. Only works if the header is enabled. */