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

Jump to bottom
Merged
merged 1 commit into from
Mar 29, 2018
Merged

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

merged 1 commit into from
Mar 29, 2018

Conversation

andrewseguin
Copy link
Contributor

@andrewseguin andrewseguin commented Sep 6, 2017 •
edited
Loading

Apply the aria-sort label to the sort header rather than include an additional visually hidden div to describe the sort.

Screenreaders will read this and append the phrase "Sorted ascending" or "Sorted descending" after reading the header content. Similar to what we had previously.

Up for debate: this property should only be applied to elements with role columnheader or rowheader. Should we only apply this property in those cases? If so, do we add in the sortDescriptionLabel to be displayed in the cases where the roles are not defined?

https://www.w3.org/TR/wai-aria/states_and_properties#aria-sort

Closes #6874

@andrewseguin andrewseguin added Accessibility This issue is related to accessibility (a11y) md-table labels Sep 6, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 6, 2017
jelbourn
jelbourn reviewed Sep 6, 2017
View reviewed changes
src/lib/sort/sort-header.ts Outdated
@@ -46,6 +46,7 @@ import {getMdSortHeaderNotContainedWithinMdSortError} from './sort-errors';
styleUrls: ['sort-header.css'],
host: {
'(click)': '_sort.sort(this)',
'[attr.aria-sort]': '_getAriaSortAttribute()',
Copy link
Member

Choose a reason for hiding this comment

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

What does the reader announce when we have both the aria-sort and the description?

Could you try this in NVDA and VoiceOver as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's JAWS with different options (with/without aria-sort/visually-hidden div)

With aria-sort, without visually-hidden: change sorting for userid button sorted ascending
Without aria-sort, with visually-hidden: change sorting for userid button sorted by userid ascending
With both: change sorting for userid button sorted ascending sorted by userid ascending sorted ascending

Checking out NVDA and VoiceOver...

Copy link
Contributor Author

@andrewseguin andrewseguin Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

NVDA was similar, wasn't able to get VoiceOver to play nice.

src/lib/sort/sort-header.ts
* 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

src/lib/sort/sort-header.ts Outdated
@@ -113,4 +114,16 @@ export class MdSortHeader implements MdSortable {
_isSorted() {
return this._sort.active == this.id && this._sort.direction;
}

/**
* Returns the aria-sort attribute that should be applied to this sort header. If this header
Copy link
Member

Choose a reason for hiding this comment

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

Prefer Gets to Returns

crisbeto
crisbeto approved these changes Sep 7, 2017
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

jelbourn
jelbourn reviewed Sep 8, 2017
View reviewed changes
src/lib/sort/sort-header.ts Outdated
@@ -83,15 +84,16 @@ export class MdSortHeader implements MdSortable {
get _id() { return this.id; }
set _id(v: string) { this.id = v; }

constructor(public _intl: MdSortHeaderIntl,
constructor(public intl: MdSortHeaderIntl,
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean for this to be part of the public api now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to find the discussion about it, but can't find it. It seems people already are using it and would like to have it part of the public API. Wrong PR to put this in, but curious what you think about allow people to access it directly

@jelbourn
Copy link
Member

Looks like some lint errors

@andrewseguin
Copy link
Contributor Author

Fixed lint issues

@andrewseguin andrewseguin added the P2 The issue is important to a large percentage of users, with a workaround label Oct 20, 2017
@andrewseguin andrewseguin added this to the 5.0 milestone Nov 28, 2017
@jelbourn jelbourn removed this from the 5.0 milestone Nov 28, 2017
@andrewseguin andrewseguin added target: minor This PR is targeted for the next minor release target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Dec 11, 2017
@andrewseguin
Copy link
Contributor Author

Marking as a major release due to the removal of some HTML elements

@josephperrott
Copy link
Member

@andrewseguin please rebase

josephperrott
josephperrott approved these changes Mar 27, 2018
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewseguin
Copy link
Contributor Author

Thanks, rebased

@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Mar 27, 2018
@josephperrott josephperrott merged commit 63f713f into angular:master Mar 29, 2018
@andrewseguin andrewseguin deleted the sort-aria branch April 17, 2018 17:23
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto approved these changes

@josephperrott josephperrott josephperrott approved these changes

@tinayuangao tinayuangao Awaiting requested review from tinayuangao

Assignees
No one assigned
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Sort] Sort headers add noise for accessible users
5 participants
@andrewseguin @jelbourn @josephperrott @crisbeto @googlebot