Skip to content

fix(sort): add aria-sort to host when sorted #6891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 29, 2018
Merged

fix(sort): add aria-sort to host when sorted #6891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/lib/sort/sort-header-intl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import {Injectable, SkipSelf, Optional} from '@angular/core';
import {Subject} from 'rxjs';
import {SortDirection} from './sort-direction';

/**
* To modify the labels and text displayed, create a new instance of MatSortHeaderIntl and
Expand All @@ -26,11 +25,6 @@ export class MatSortHeaderIntl {
sortButtonLabel = (id: string) => {
return `Change sorting for ${id}`;
}

/** A label to describe the current sort (visible only to screenreaders). */
sortDescriptionLabel = (id: string, direction: SortDirection) => {
return `Sorted by ${id} ${direction == 'asc' ? 'ascending' : 'descending'}`;
}
}
/** @docs-private */
export function MAT_SORT_HEADER_INTL_PROVIDER_FACTORY(parentIntl: MatSortHeaderIntl) {
Expand Down
4 changes: 0 additions & 4 deletions src/lib/sort/sort-header.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,3 @@
</div>
</div>
</div>

<span class="cdk-visually-hidden" *ngIf="_isSorted()">
&nbsp;{{_intl.sortDescriptionLabel(id, _sort.direction)}}
</span>
13 changes: 13 additions & 0 deletions src/lib/sort/sort-header.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export interface ArrowViewStateTransition {
'(mouseenter)': '_setIndicatorHintVisible(true)',
'(longpress)': '_setIndicatorHintVisible(true)',
'(mouseleave)': '_setIndicatorHintVisible(false)',
'[attr.aria-sort]': '_getAriaSortAttribute()',
'[class.mat-sort-header-disabled]': '_isDisabled()',
},
encapsulation: ViewEncapsulation.None,
Expand Down Expand Up @@ -263,4 +264,16 @@ export class MatSortHeader extends _MatSortHeaderMixinBase implements MatSortabl
_isDisabled() {
return this._sort.disabled || this.disabled;
}

/**
* Gets the aria-sort attribute that should be applied to this sort header. If this header
* is not sorted, returns null so that the attribute is removed from the host element. Aria spec
* says that the aria-sort property should only be present on one header at a time, so removing
* ensures this is true.
*/
_getAriaSortAttribute() {
if (!this._isSorted()) { return null; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be none when it's not sorted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec, For each table or grid, authors SHOULD apply aria-sort to only one header at a time. so by returning null, the attribute is removed altogether rather than being set to 'none' since it's possible another header now has aria-sort applied


return this._sort.direction == 'asc' ? 'ascending' : 'descending';
}
}
17 changes: 17 additions & 0 deletions src/lib/sort/sort.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,23 @@ describe('MatSort', () => {
expect(header._showIndicatorHint).toBeFalsy();
});

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

component.sort('defaultA');
fixture.detectChanges();
expect(sortHeaderElement.getAttribute('aria-sort')).toBe('ascending');

component.sort('defaultA');
fixture.detectChanges();
expect(sortHeaderElement.getAttribute('aria-sort')).toBe('descending');

component.sort('defaultA');
fixture.detectChanges();
expect(sortHeaderElement.getAttribute('aria-sort')).toBe(null);
});

it('should re-render when the i18n labels have changed',
inject([MatSortHeaderIntl], (intl: MatSortHeaderIntl) => {
const header = fixture.debugElement.query(By.directive(MatSortHeader)).nativeElement;
Expand Down
14 changes: 7 additions & 7 deletions src/lib/table/table.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('MatTable', () => {
component.sort.sort(component.sortHeader);
fixture.detectChanges();
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a ascending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['a_1', 'b_1', 'c_1'],
['a_2', 'b_2', 'c_2'],
['a_3', 'b_3', 'c_3'],
Expand All @@ -171,7 +171,7 @@ describe('MatTable', () => {
component.sort.sort(component.sortHeader);
fixture.detectChanges();
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['a_3', 'b_3', 'c_3'],
['a_2', 'b_2', 'c_2'],
['a_1', 'b_1', 'c_1'],
Expand All @@ -189,7 +189,7 @@ describe('MatTable', () => {
component.sort.direction = '';
component.sort.sort(component.sortHeader);
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['a_1', 'b_1', 'c_1'],
['a_3', 'b_3', 'c_3'],
['a_2', 'b_2', 'c_2'],
Expand All @@ -204,7 +204,7 @@ describe('MatTable', () => {

// Expect that empty string row comes before the other values
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a ascending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['', 'b_1', 'c_1'],
['a_2', 'b_2', 'c_2'],
['a_3', 'b_3', 'c_3'],
Expand All @@ -214,7 +214,7 @@ describe('MatTable', () => {
component.sort.sort(component.sortHeader);
fixture.detectChanges();
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['a_3', 'b_3', 'c_3'],
['a_2', 'b_2', 'c_2'],
['', 'b_1', 'c_1'],
Expand All @@ -229,7 +229,7 @@ describe('MatTable', () => {
component.sort.sort(component.sortHeader);
fixture.detectChanges();
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a ascending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['', 'b_1', 'c_1'],
['a_2', 'b_2', 'c_2'],
['a_3', 'b_3', 'c_3'],
Expand All @@ -240,7 +240,7 @@ describe('MatTable', () => {
component.sort.sort(component.sortHeader);
fixture.detectChanges();
expectTableToMatchContent(tableElement, [
['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
['Column A', 'Column B', 'Column C'],
['a_3', 'b_3', 'c_3'],
['a_2', 'b_2', 'c_2'],
['', 'b_1', 'c_1'],
Expand Down