Skip to content
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.

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

Reimpliment accessibility based on WAI-ARIA 1.1 #1023

Merged
merged 1 commit into from Oct 4, 2017
Merged

Conversation

platosha
Copy link
Contributor

@platosha platosha commented Oct 2, 2017

Fixes #1015

  • Use aria-rowcount, aria-colcount on table

  • Use aria-rowindex on rows to indicate row numbers

  • Use aria-selected on cells to indicate selection

  • Use aria-expanded, aria-controls on cells to indicate row details
    toggle states

  • <vaadin-grid-sorter> sets aria-sort on parent cells

  • Add tests for all above and structural grid/row/gridcell roles in HTML

  • Remove hardcoded english textual announcements


This change is Reviewable

@limonte
Copy link
Contributor

limonte commented Oct 3, 2017

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


vaadin-grid.html, line 32 at r1 (raw file):
From this article:

do not use the “application” role for table that has focusable items and is being navigated via the arrow keys, for example a list of e-mail messages where you provide specific information. Other examples are interactive grids, tree grids etc.

In short: The times when you actually will use role “applications” are probably going to be very rare cases!

We probably don't want role="application" on the scroller.


Comments from Reviewable

@platosha
Copy link
Contributor Author

platosha commented Oct 3, 2017

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


vaadin-grid.html, line 32 at r1 (raw file):

Previously, limonte (Limon Monte) wrote…

From this article:

do not use the “application” role for table that has focusable items and is being navigated via the arrow keys, for example a list of e-mail messages where you provide specific information. Other examples are interactive grids, tree grids etc.

In short: The times when you actually will use role “applications” are probably going to be very rare cases!

We probably don't want role="application" on the scroller.

Done.


Comments from Reviewable

@tomivirkki
Copy link
Member

Looks like cell text can no longer be selected in the grid. I think it was a regression in a previous PR = can be fixed in a separate PR if you prefer.


Comments from Reviewable

@limonte
Copy link
Contributor

limonte commented Oct 3, 2017

Tested on NVDA+Chrome, works nice :lgtm:


Reviewed 8 of 11 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@tomivirkki
Copy link
Member

:lgtm:


Reviewed 8 of 11 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tomivirkki
Copy link
Member

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Fixes #1015

- Use `aria-rowcount`, `aria-colcount` on table

- Use `aria-rowindex` on rows to indicate row numbers

- Use `aria-selected` on cells to indicate selection

- Use `aria-expanded`, `aria-controls` on cells to indicate row details
  toggle states

- `<vaadin-grid-sorter>` sets `aria-sort` on parent cells

- Add tests for all above and structural grid/row/gridcell roles in HTML

- Remove hardcoded english textual announcements
@knoobie
Copy link

knoobie commented Oct 3, 2017

I stumbled over this issue, thank you so much for adressing this!
I'm not so familiar with elements code, so take this with a grain of salt.

Did you consider the following things?

  • aria-sort should not be added if the column isn't sortable (Has grid columns that aren't sortable?)
  • aria-sort can?/should? have the value 'other' if more than one column is sorted (Multisorting? like in the Vaadin Framework Grid)
  • Is there a way to allow different selection models for the user? Single / Multiselect / None? If so, setting 'aria-multiselectable=true' as default can be kinda irritating
  • ^ selection = none --> there should be no aria-selected on cells or rows
  • Is there an API so that the selection can either be a row or a cell? Depending on this the aria-selected should be added on rows or cells
  • missing aria-colindex if you can hide columns on the client side

Update after a developer from w3c aria team answered some questions:

  • aria-rowindex and aria-rowcount should be recalculated after a filter was applied
  • aria-rowindex and aria-rowcount should include the header and body rows but not the footer

@platosha
Copy link
Contributor Author

platosha commented Oct 4, 2017

@knoobie thanks for participating in this review and asking questions. I believe it is helpful for us to go through them.

aria-sort should not be added if the column isn't sortable (Has grid columns that aren't sortable?)

Yes, done so. All the columns are not sortable by default, hence don’t have aria-sort by default. Only the column headers using <vaadin-grid-sorter> have aria-sort.

aria-sort can?/should? have the value 'other' if more than one column is sorted (Multisorting? like in the Vaadin Framework Grid)

The value of other is not in use currently.

When multisorting is in use, more than one column header can have aria-sort of ascending/descending. Then also numbers representing the order of the sort also appear in the column header text.

I find ascending/descending making more sense than other even for the multisorting case, because of how it works: multiple sortings are applied, but each one of them is either ascending or descending, no other options.

Is there a way to allow different selection models for the user? Single / Multiselect / None? If so, setting 'aria-multiselectable=true' as default can be kinda irritating
^ selection = none --> there should be no aria-selected on cells or rows

Tricky questions. The grid is agnostic to selection model that developers using the grid use. There are no “selection modes” to choose from, both multiple selection as well as single selection is allowed out-of-the-box.

You can think of it this way: multiple selection is allowed by default in the API, but selection not triggered by default when end users interact with the grid. Developers can trigger selection the way then want by calling the grid APIs. The resulting selection model can be anything: single, multiple selection, both of them in any combination. See grid examples of different selection modes: https://cdn.vaadin.com/vaadin-grid/4.0.0-alpha3/demo/#grid-selection-demos

So the grid does not know whether developers implement single or multiple selection modes. aria-multiselectable="true" might be wrong for some use cases. But as far as I can understand from the WAI-ARIA spec, on the other hand, when aria-multiselectable is not set, the default is that only one item is selectable at a time, which is also wrong for some use cases. I chose to have aria-multiselectable="true" as this represents how things work under-the-hood.

After trying how it works with screen readers, I haven’t noticed any downsides of having aria-multisepectable="true" enabled.

For same reasons all items have aria-selected="false" even though selection is not triggered.

Is there an API so that the selection can either be a row or a cell? Depending on this the aria-selected should be added on rows or cells

API-wise the selection can be only on rows, or in other words, on the grid items.

However, both rows and cells in this PR have aria-selected. Due to how focus and keyboard navigation work in the grid, only the cells are focusable, while the rows are not. Due to how NVDA screen reader works, when a cell is focused, aria-selected from the parent row is not announced. On the other hand, Jaws seems to only announce aria-selected from the rows, even though the cells are focused.

In future, we might rethink keyboard navigation making rows focusable as well in the grid, but don’t have resources for that currently.

missing aria-colindex if you can hide columns on the client side

Developers can hide columns using the <vaadin-grid-column> API. But by default end-users cannot hide columns as they like.

By spec, when aria-colindex is missing, the column numbers should be calculated automatically. This should result in column numbering in the way as if the hidden columns were removed from the grid rather than hidden. Might be a bit discomforting, but I don’t think its an issue. Column removal is also allowed in the API.

aria-rowindex and aria-rowcount should be recalculated after a filter was applied

Done.

aria-rowindex and aria-rowcount should include the header and body rows but not the footer

Footer rows also have aria-rowindex in this PR, and here is why. The grid users so called “virtual scrolling”, so with hundreds+ of items only a portion of the data set is present in the DOM.

First of all, this is what the spec says: “However, if only a portion of the rows is present in the DOM at a given moment, this attribute is needed to provide an explicit indication of each row's position with respect to the full table.” (Source: aria-rowindex description).

Let’s say, a grid has one row for header, one row for footer, there is 100 items in total, but only 50 of them is actually in the DOM as body rows. The full table is contains 102 rows then. I would expect the footer row to have position of 1 + 100 + 1 = 102 with respect to the full table in this case.

Missing explicit aria-rowindex for the footer row might turn automatic calculations on, which, because of only 50 body rows are present in the DOM, might result in the footer row position of 1 + 50 + 1 = 52, which is incorrect and confusing for a footer row.

Since the footer rows use explicit aria-rowindex values, they are also counted in the total number of rows for aria-rowcount.

@knoobie
Copy link

knoobie commented Oct 4, 2017

@platosha Thank you for your detailed answer!

aria-sort

👍 - I know 'other' is a special use-case. I still hope for an aria-sort-order='Integer' but using it your way is fine as well! It's kinda a philosophical question, who likes what more.

aria-multiselectable / aria-selected

I can understand your concerns and implementation! If it's currently not possible to determine whether the 'user/developer' wants to use single or multiselect, it would be lovely if aria-multiselectable and aria-selected are only added if the developer has any kind of selection allowed. Default behaviour of the grid should be 'unselectable', or?

keyboard navigation

I think I found somewhere a keyboard navigation guide written by w3c how the grid should perform, but idk if this is what your Usability Engineer wants in the end.
(Navigation model for grid made by w3c: https://w3c.github.io/aria-practices/examples/grid/dataGrids.html)

aria-colindex

👍

aria-rowindex and aria-rowcount

I had the same questions in mind some days ago, where I wanted to add this to the vaadin framework. I opened an issue (w3c/aria-practices#465) where this was answered by a staff member from aria-practice. That's why I wanted to include it here as well.

Edit:

Please ping me if you have the latest branch somewhere deployed, I can ask a sightless colleague mid october if he has any problems using the latest version!

@platosha
Copy link
Contributor Author

platosha commented Oct 4, 2017

@knoobie

Default behaviour of the grid should be 'unselectable', or?

My decision is to have selectable default for now. This could confusing for end users, but I believe they still can try selecting items and discover that they can’t trigger selection.

On the other hand, having unselectable default might confuse end users to not even try selection while it is available, so they won’t discover selection feature at all. This is worse than the current default IMO.

I think I found somewhere a keyboard navigation guide written by w3c how the grid should perform, but idk if this is what your Usability Engineer wants in the end.

We have been this WAI-ARIA Authoring Practices for Grid guide back in the day when developing keyboard navigation for grid v2.0. Keyboard navigation in the grid works in the spec way with a few minor exceptions.

aria-rowindex and aria-rowcount

Thanks for explaining your reasoning. I assume that footer rows can be included in a similar way the header rows are. Neither the spec text, nor the referred examples, nor the staff member’s response explicitly disagree with that.

@knoobie
Copy link

knoobie commented Oct 4, 2017

@platosha I agree with your reasoning! I've added the link with the keyboard navigation above in the comment and added an edit below.

I think this is a really good starting point for Grid 4.0! Kudos!

@tomivirkki tomivirkki merged commit 758cd43 into master Oct 4, 2017
@manolo manolo added this to the 4.0.0-alpha4 milestone Oct 10, 2017
hesara pushed a commit to vaadin/framework that referenced this pull request Oct 11, 2017
@web-padawan web-padawan deleted the feature/a11y branch August 15, 2018 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants