Treat ARIA treegrids as tables in browse mode#11699
Conversation
… as soon as possible, so that it presents exactly like a table in brwose mode and quick nav to the table also works.
…poken at all, should at least have expanded / collapsed / level announced.
See test results for failed build of commit 558c71aafc |
|
This would be a change that offends the Core Accessibility API Mappings. Of course what you pointed out in the initial description justifies this, but I wonder whether it could be considered to fix it on the IA2 implementation level. Would that be something @jcsteh could anser? Also, according to the mappings, UIA uses the data grid role for tree grids. We have an internal NVDA role of ROLE_DATAGRID. Have you considered using that instead? I think it more closely resembles what a tree grid does. It probably requires more changes to treat a data grid as a table, though. |
Not really. The Core AAM just states how ARIA maps to a11y APIs. It doesn't state how an AT behaves. A treegrid has the IAccessibleTable interfaces, whereas a tree doesn't. This differentiates the two from a mapping perspective, thus justifying differentiated AT behaviour.
I tend to agree this probably deserves a different role in an ideal world. However, treegrids have existed for years now (since ARIA 1.0). Changing this now would break backwards compatibility and risk breaking things for other AT. I don't think it's worth the churn or risk at this point. |
feerrenrut
left a comment
There was a problem hiding this comment.
What are your thoughts on adding a system test for this?
| # Speak expanded / collapsed / level for treeview items (in ARIA treegrids) | ||
| if role == controlTypes.ROLE_TREEVIEWITEM: | ||
| if controlTypes.STATE_EXPANDED in states: | ||
| out.extend( | ||
| getPropertiesSpeech(reason=reason, states={controlTypes.STATE_EXPANDED}, _role=role) | ||
| ) | ||
| elif controlTypes.STATE_COLLAPSED in states: | ||
| out.extend( | ||
| getPropertiesSpeech(reason=reason, states={controlTypes.STATE_COLLAPSED}, _role=role) | ||
| ) | ||
| if levelSequence: | ||
| out.extend(levelSequence) |
There was a problem hiding this comment.
Given the complexity (and suppressed warning) for getControlFieldSpeech can this be simplified?
There was a problem hiding this comment.
I can't think of a way currently... and that code I added looks pretty simple to me... unless I'm missing your point?
There was a problem hiding this comment.
We use mccabe to check for code complexity. It is a quantitative measure of the number of linearly independent paths through the source code. The code complexity check essentially adds points for branching and loops.
You can read more about it here: https://en.wikipedia.org/wiki/Cyclomatic_complexity
I think to reduce the complexity of this system, we'll need to work towards reducing the responsibilities of functions like getPropertiesSpeech, splitting them up into more specific pieces.
As an interim, working towards this goal, this block might be replaced with something like:
| # Speak expanded / collapsed / level for treeview items (in ARIA treegrids) | |
| if role == controlTypes.ROLE_TREEVIEWITEM: | |
| if controlTypes.STATE_EXPANDED in states: | |
| out.extend( | |
| getPropertiesSpeech(reason=reason, states={controlTypes.STATE_EXPANDED}, _role=role) | |
| ) | |
| elif controlTypes.STATE_COLLAPSED in states: | |
| out.extend( | |
| getPropertiesSpeech(reason=reason, states={controlTypes.STATE_COLLAPSED}, _role=role) | |
| ) | |
| if levelSequence: | |
| out.extend(levelSequence) | |
| # Speak expanded / collapsed / level for treeview items (in ARIA treegrids) | |
| if role == controlTypes.ROLE_TREEVIEWITEM: | |
| out.extend( | |
| getExpandedCollapsedLevelForTreeViewItems(reason, states, role) | |
| ) |
There was a problem hiding this comment.
And what about level? levelSequence would also need to be passed to that function.
I do agree that getcontrolFieldSpeech really does need to be completely redesigned at some point, but not sure doing it little bit by little bit is necessarily the best way to go myself. I personally find that more confusing than less, but that might be just me.
There was a problem hiding this comment.
And what about level?
Yep, I forgot to add it.
getcontrolFieldSpeech really does need to be completely redesigned at some point,
The problem with this is that it's always too much work to ever get started.
I personally find that more confusing than less, but that might be just me.
It might be in some ways, like having to navigate to the function to know what it does. But in other ways you know it's state and logic is limited to it's return value and params. Knowing this makes refactoring easier, it becomes easier to untangle the dependencies and separate parts of code that aren't really dependent on each other.
|
|
||
|
|
||
| class Treegrid(Ia2Web): | ||
| role = controlTypes.ROLE_TABLE |
There was a problem hiding this comment.
Probably missed my earlier comment, but have you considered using ROLE_DATAGRID for these?
There was a problem hiding this comment.
Perhaps we could use that role, but a lot of other checks would need to change within the code I think. Users are used to the concept of a "table" in browse mode, and also normal ARIA grids (tables with focusable cells but not collapsable rows) also get exposed as tables at the moment.
…MapOfIA2AttributesFromPacc, and returns a map.
|
A system test for this would be different from other tests we have so far as an ARIA treegrid requires a lot of web authoring code (including javascript). Much more complex than a small html code snippet, But happy to hear thoughts on this. |
We could use the Treegrid Email Inbox Example from wai-aria-practices right? |
See test results for failed build of commit b0078c296b |
See test results for failed build of commit 49197a7482 |
feerrenrut
left a comment
There was a problem hiding this comment.
Generally looks good.
It looks like these examples come from the following repo:
https://github.com/w3c/aria-practices/tree/master/examples/treegrid
To comply with the license
- A copy of the license needs to live with the source
- A copyright notice needs to be added to the files.
Unrelated to this change, we need to revisit the system test mechanisms to investigate this intermittent issue.
| test_pr11606 | ||
| ARIA treegrid | ||
| [Documentation] Ensure that ARIA treegrids are accessible as a standard table in browse mode. | ||
| test_ariaTreeGrid_browseMode No newline at end of file |
There was a problem hiding this comment.
Please add a trailing newline
| </table> | ||
| <p>After treegrid</p> | ||
| </body> | ||
| </html> No newline at end of file |
See test results for failed build of commit b1320ba9d8 |
…ARIA practices repository, rather than a local copy, as it is very likely we will want to use more of these test cases in future.
9d1195c to
f1f8190
Compare
See test results for failed build of commit e766317b57 |
…quence) is stripped of whitespace at its beginning and end. Previously whitespace was only stripped from the ends of the entire multiline string.
See test results for failed build of commit 129dd0ba91 |
…o the treegrid so as to not have to deal with a possible focus event on the iframe document when switching to focus mode in the treegrid later.
…inish after sending a key.
|
The system test now uses the w3c ARIA practices repository directly which solves any issues with copyright / licencing, and no doubt we will use many more of these test cases in our system tests in the future. |
feerrenrut
left a comment
There was a problem hiding this comment.
looks good to me. Thanks @michaelDCurran
See test results for failed build of commit f4e739cfde |
Link to issue number:
Fixes #9715
Summary of the issue:
A web author can indicate with ARIA that an element is a treegrid. A treegrid is a table with focusable rows and cells, where the rows can be expanded and collapsed.
Currently both Firefox and Chrome expose treegrids as a treeview control via accessibility. Thus, in browse mode, NVDA reports it as a treeview, and only exposes the top and or currently selected row. It also currently does not seem to be possible to switch to focus mode when pressing enter on it.
Feeling is that treegrids in browse mode should really be exposed as a table, so that all visible rows are included in the virtual document, and that NvDA's table navigation commands will work. Pressing t / shift+t should also find it as a table. Finally, pressing enter or NVDA+space when on one of the cells should switch to focus mode and move focus to that cell / row.
Description of how this pull request fixes the issue:
These changes mean that in Firefox and Chrome:
Testing performed:
Tested all above scendarios in Firefox and Chrome using the ARIA treegrid testcase: https://www.w3.org/TR/wai-aria-practices/examples/treegrid/treegrid-1.html
Known issues with pull request:
Perhaps we could set the role of treeview items in treegrids to table row, but this would be costly to do as we only know it is a treegrid from the root of the element, which may mean ascending multiple parents. I don't think it is worth it for something the majority of users would not notice.
Change log entry:
Bug fixes: