wxGrid: implement basic accessibility / screen reader support #24368
Conversation
|
Some background information: As of now, wxGrid does not support screen readers. The reason is that it is a generic widget, i.e. it is drawn by wxWidgets itself. It is not using an underlying native Windows widget. The communication between screen readers and wxWidgets is done via A cell or label can have one of these roles:
The accessibility information that a screen reader will display is retrieved via several methods:
There is no specification what these methods should return for a grid. With this PR |
DietmarSchwertberger
changed the title
|
OK, finally got feedback from a screen reader user and tested again with JAWS: |
|
@vadz : Is there a way to re-start the checks? They seem to have stalled. |
|
Sorry, I have no idea about what happened to the checks here, I can't even cancel them... Do you plan to push any more commits here or is this ready for reviewing and merging? |
|
This was the last commit, except if there is a real problem with the checks. |
|
Would you mind also checking my other PRs? |
There was a problem hiding this comment.
Thanks a lot, this is clearly very useful and I don't see any big problems with this (but then I just read the code and didn't actually test anything), however it would be nice to improve this a bit if possible by (in priority order):
- Moving private classes declarations to private headers.
- Replacing raw pointer with
std::unique_ptr<>to make sure we don't leak memory anywhere. - Using
wxGridCellCoordsinstead of pairs of row/col to simplify the code.
Please let me know if you plan to do this (even if not right now) or if you think that this shouldn't be done.
Thanks again!
|
Thanks. The modifications are in the repository now. |
|
Oops, I thought you had finished and started doing my own changes. Could you please wait a bit before I push them before changing anything else? |
Remove some redundant initializations. Remove dtor which is not needed any more. Make the member variables that never change const.
Also add some comments.
|
I've rebased my changes on yours and pushed them now. Please let me know if you have any objections, if not I'll (squash) merge this soon. Thanks again! |
|
Yes, sure. Thanks. |
| if ( grid->GetRowLabelSize() ) | ||
| numCols++; | ||
| int row = (childId - 1) / numCols; | ||
| int col = (childId - 1) % numCols; |
There was a problem hiding this comment.
Are we sure that numCols can't be 0 here? It doesn't seem totally obvious...
There was a problem hiding this comment.
GetChildCount should return the correct value in this case, i.e. 0 if there is also no header.
But yes, I don't know the timing and whether the accessibility framework's data could become out of date.
There was a problem hiding this comment.
Just to be clear: what I meant was that we must ensure we don't divide by 0 here as this would crash the program. If we're not sure that this code can't be called in the case when it's 0, we must check for it here explicitly.
There was a problem hiding this comment.
Yes, that's how I understood your comment. Is there a good way to return a NULL value instead of the wxGridCellCoords instance?
When thinking about this, it might not be the worst idea to validate the coordinates at some other positions, e.g. at wxGridCellAccessible::GetName. I'm not sure how the accessibility framework is actually working. If it retrieves the information and caches it, things should be fine. If a client application like NVDA could trigger a late call itself, then a check would be good.
I think I will implement this.
There was a problem hiding this comment.
No, we can't return null object. We could change the function to return unique_ptr<wxGridCellCoords> which could be null, but this looks like an overkill — it looks like we could just return invalid wxGridCellCoords (i.e. -1,-1) for a completely empty grid, couldn't we?
But we really, really shouldn't crash with a division by 0.
There was a problem hiding this comment.
The cleanest approach for all would be to destroy the current wxGridCellAccessible instance on each change of table dimension or label sizes, but I don't see a mechanism in wxGrid for this, especially in the general case with a separate data table.
Returning (-1,-1) for numCols==0 is probably good enough.
The correct but clumsy approach would be for wxGridCellAccessible to cache the number of rows, cols and sizes of row and col header at creation. This could then be compared to the current dimensions and in case of mismatch error values would be returned. This seems a bit overengineered, though. I've modeled this after the implemenation in DataViewCtrl and I don't remember any checks there.
There was a problem hiding this comment.
Should I push this modification?
// Get coordinates corresponding to the given child ID (see the converse
// function above).
wxGridCellCoords ConvertChildIdToCoords(wxGrid* grid, int childId)
{
wxGridCellCoords coords;
if ( childId != 1 || !grid->GetColLabelSize() || !grid->GetRowLabelSize() )
{
int numCols = grid->GetNumberCols();
if ( grid->GetRowLabelSize() )
numCols++;
if ( numCols ) {
// actually, numCols == 0 should not happen
int row = (childId - 1) / numCols;
int col = (childId - 1) % numCols;
if ( grid->GetRowLabelSize() )
{
if ( col == 0 )
col = -1;
else
col--;
}
if ( grid->GetColLabelSize() )
{
if ( row == 0 )
row = -1;
else
row--;
}
coords.SetCol(col);
coords.SetRow(row);
}
}
return coords;
}There was a problem hiding this comment.
If it really shouldn't happen, I'd just add wxCHECK_MSG( numCols, coords, "shouldn't be called" ); to make it clear. But if it can happen, i.e. if this function can be called because the user uses screen reader on an empty grid, then I'd indeed do this but change the comment.
There was a problem hiding this comment.
An empty grid is fine.
HitTest and GetChildCount are responsible for returning only valid child ids. E.g. in case of a grid with zero columns but row labels, the child count would be the number of rows plus 1 for the corner.
The only way to have numCols==0 here would be if a call would be done on an outdated child id due to a grid resize. I don't think that the accessibility mechanism does that, but I would not bet my life on it. So, the wxCHECK_MSG seems a good containment for the hypothetical case.
Btw. we would be in good company.
I found that Visual Studio crashes a lot when using a screen reader:
https://developercommunity.visualstudio.com/t/Many-crashes-when-accessibility-tools-ar/10606336
src/generic/grid.cpp
Outdated
| // this needs to be returned such that a child below is recognized as focused | ||
| // (check for cursorRow and cursorCol being != -1?) | ||
| if ( grid->HasFocus() ) | ||
| st |= wxACC_STATE_SYSTEM_FOCUSED; |
There was a problem hiding this comment.
Totally nitpickish but there is an extra space here:
| st |= wxACC_STATE_SYSTEM_FOCUSED; | |
| st |= wxACC_STATE_SYSTEM_FOCUSED; |
This should have been part of the grandparent commit.
|
Your modifications look good for me. I have just built and tested with NVDA. Thanks. |
|
Thanks again for your work on this, finally merging this! |
This does add basic support only.
GetNameis returning row/col information andGetValueis returning the string value of a cell.Maybe the other way round would be better. The documenation is not clear about this and I don't have feedback from actual screen reader users yet.
GetDescriptionis not implemented.For non-string values like checkboxes, a more sophisticated implementation would be nice, but I think this PR is already a huge step forward from the current situation.
I'm wondering whether there is some method already to convert a position to cell and label coordinate information.
This could replace many lines inside
wxGridAccessible::HitTest.