Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

[NG] Adding aria-label to clr-dg-column button. #3384

Merged
merged 14 commits into from
May 31, 2019

Conversation

bdryanovski
Copy link
Contributor

@bdryanovski bdryanovski commented May 23, 2019

Adding accessibility aria-label to clr-dg-column when sorting is enabled
Adding key sorfOf into CommonString
Updating i18n documentation

fixes: #1930

Adding accessability aria-label to clr-dg-column when sorting is enabled
Adding key sorfOf into CommonString
Updating i18n documentation

fixes: vmware-archive#1930

Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Copy link
Contributor

@Jinnie Jinnie left a comment

Choose a reason for hiding this comment

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

As we talked offline, the proposed label format of "Sort on " is problematic, because:

  • We have no control what the user will put there and if it will be readable at all.
  • The column name was already read just a second before announcing it's a button.
  • There are (minor) technical difficulties that require us to work directly with the DOM in implementation specific way, which is undesirable.
    Because of the above, the current implementation that does not read the column name again, looks good to me.

src/website/src/app/documentation/demos/i18n/i18n.demo.ts Outdated Show resolved Hide resolved
src/clr-angular/utils/i18n/common-strings.service.ts Outdated Show resolved Hide resolved
src/clr-angular/data/datagrid/datagrid-column.ts Outdated Show resolved Hide resolved
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Update Copyright

Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
@gnomeontherun
Copy link
Contributor

I think there is probably a second piece to this, where we look at how to use aria-sort to inform the screen reader about what direction the sorting is. https://www.w3.org/WAI/PF/aria/states_and_properties#aria-sort

@Jinnie
Copy link
Contributor

Jinnie commented May 23, 2019

The aria-sort is already in place, but it is responsible for announcing things like: "This column has been sorted in ascending order". From the spec you referenced:

Indicates if items in a table or grid are sorted in ascending or descending order.

It is not responsible for announcing what the button in the header does. It has to be applied to grid or table headers only, it has no notion of the possible buttons inside.
Together with Bozhidar we verified how other solutions like Material are handling it. Material does almost exactly what we do here, other frameworks didn't handle it at all. Bozhidar can say if there were any other options observed.

@gnomeontherun
Copy link
Contributor

Ok, got confused in my review, aria-sort is there.

Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
@bdryanovski
Copy link
Contributor Author

The material solution for this type of issues is something like that:

// https://github.com/angular/components/blob/9eeb4b5aa0473c0815a73bfe3c2ed6164d86a3b3/src/material/sort/sort-header-intl.ts

  /** ARIA label for the sorting button. */
  sortButtonLabel = (id: string) => {
    return `Change sorting for ${id}`;
  }
}

// https://github.com/angular/components/blob/9eeb4b5aa0473c0815a73bfe3c2ed6164d86a3b3/src/material/sort/sort-header.html
  <button class="mat-sort-header-button" type="button"
          [attr.disabled]="_isDisabled() || null"
          [attr.aria-label]="_intl.sortButtonLabel(id)"
          (focus)="_setIndicatorHintVisible(true)"
          (blur)="_setIndicatorHintVisible(false)">
    <ng-content></ng-content>
  </button>

Which is a nice and simple solution, the tricky part is that they have already the name of the column.

<th mat-sort-header="name">Dessert</th>
<th mat-sort-header="calories">Calories</th>

I have hard times getting the content from the table column:

  • First, the ng-template is not rendered at the time when I'm trying to generate the ariaButtonLabel
  • I'm not always sure that the content for columnTitle will be text only. This could lead to some strange results?

On the other topic, aria-sort only tell if the column is ASC or DESC, I think that the issue here is that the button for changing this sorting is not clear on what it will do when pressed. On my tests, I got only the button name and that's a button nothing more - I was not clear at anytime that this will change the sorting order.

The best solution will be to be able to create a string that will say Sorting for ${columnTitle} - but I'm not sure how many hacks will require this.

If someone has some clever idea, don't be shy?

@vmware-archive vmware-archive deleted a comment from vmwclabot May 28, 2019
@vmware-archive vmware-archive deleted a comment from vmwclabot May 28, 2019
@bdryanovski bdryanovski changed the title [WIP] [NG] Adding aria-label to clr-dg-column button. [NG] Adding aria-label to clr-dg-column button. May 28, 2019
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
@netlify
Copy link

netlify bot commented May 28, 2019

Deploy preview for vmware-clarity ready!

Built with commit 81d9c98

https://deploy-preview-3384--vmware-clarity.netlify.com

Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
Copy link
Contributor

@gnomeontherun gnomeontherun left a comment

Choose a reason for hiding this comment

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

Just a few tweaks otherwise pre-approving so you can merge with those changes. Also we need to backport this to v1 branch.

Signed-off-by: Bozhidar Dryanovski <bozhidar.dryanovski@gmail.com>
@vmware-archive vmware-archive deleted a comment from vmwclabot May 30, 2019
@vmware-archive vmware-archive deleted a comment from vmwclabot May 30, 2019
@vmware-archive vmware-archive deleted a comment from vmwclabot May 30, 2019
@vmware-archive vmware-archive deleted a comment from vmwclabot May 30, 2019
@vmware-archive vmware-archive deleted a comment from vmwclabot May 30, 2019
@bdryanovski bdryanovski merged commit f18da6f into vmware-archive:master May 31, 2019
@hippee-lee hippee-lee added this to the 2.0 milestone Jun 5, 2019
@bdryanovski bdryanovski deleted the issue_1930 branch May 27, 2021 09:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The sort buttons do not have meaningful description
5 participants