wxGrid: implement basic accessibility support (PR #24368)
Dietmar Schwertberger
This does add basic support only.
GetName is returning row/col information and GetValue is 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.
GetDescription is 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.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24368
Commit Summary
- b5d2845 implement basic accessibility support for wxGrid
File Changes
(3 files)
- M include/wx/generic/grid.h (91)
- M src/generic/grid.cpp (651)
- M src/generic/gridsel.cpp (4)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24368.patch
- https://github.com/wxWidgets/wxWidgets/pull/24368.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
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 other notable generic widget is wxDataView, which has accessibililty support in wxWidgets already.
The communication between screen readers and wxWidgets is done via Microsoft Active Accessiblilty or IAccessible.
The MSAA documenation at https://learn.microsoft.com/en-us/previous-versions/windows/desktop/dnacc/exposing-data-tables-through-microsoft-active-accessibility describes how to implement a hierarchy of grid -> rows -> cells.
Unfortunately, via MSAA the screen readers NVDA and JAWS only support a hierarchy of grid -> cells.
A cell or label can have one of these roles:
- ROLE_SYSTEM_COLUMNHEADER
- ROLE_SYSTEM_ROWHEADER
- ROLE_SYSTEM_CELL
The accessibility information that a screen reader will display is retrieved via several methods:
- get_accRole
- get_accName
- get_accValue
- get_accDescription
- get_accHelp
There is no specification what these methods should return for a grid. With this PR Description is not used.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 4887f28 remove white space
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- d5e0f2b remove debug message
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 61b47ac remove additional qualification
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 6ea38f3 use nullptr
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 3965a95 declare unused argument
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 24e053c refactor, return different roles for headers
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
OK, finally got feedback from a screen reader user and tested again with JAWS:
JAWS seems to take into account only the Name property, not the Value.
(NVDA behaves better.)
So, I'm working on this again. (Even though it could be merged already.)
I'm also working to get some things more user friendly (e.g. don't repeat row or column information when moving on the same row or column.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- c182baa combine coordinate and value into GetName
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@vadz : Is there a way to re-start the checks? They seem to have stalled.
Feature-wise the PR is now finalized.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
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?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
This was the last commit, except if there is a real problem with the checks.
It was difficult to get feedback from a screen reader user, but I think the current version is a huge step forward compared to now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
Would you mind also checking my other PRs?
PR #24392 requires feedback about how to implement, but the others are ready for review and merge.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
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!
> @@ -3467,5 +3484,63 @@ extern const int wxEVT_GRID_CHANGE_SEL_LABEL; #endif +#if wxUSE_ACCESSIBILITY
Just curious: does this class have to be public? It doesn't seem to be documented, so perhaps it's not supposed to be used by the code outside the library? And in this case, could it please be moved to include/wx/generic/private/grid.h or even some new include/wx/generic/private/gridaccessible.h to reduce physical coupling?
> @@ -3467,5 +3484,63 @@ extern const int wxEVT_GRID_CHANGE_SEL_LABEL;
#endif
+#if wxUSE_ACCESSIBILITY
+//-----------------------------------------------------------------------------
+// accessibility support for whole grid and per cell
+//-----------------------------------------------------------------------------
+
+class WXDLLIMPEXP_CORE wxGridAccessible: public wxWindowAccessible
+{
+public:
+ wxGridAccessible(wxGrid* win);
Minor, but:
⬇️ Suggested change- wxGridAccessible(wxGrid* win); + explicit wxGridAccessible(wxGrid* win);
> @@ -11019,6 +11044,44 @@ void wxGrid::SetRowSizes(const wxGridSizesInfo& sizeInfo)
DoSetSizes(sizeInfo, wxGridRowOperations());
}
+#if wxUSE_ACCESSIBILITY
+
+bool wxGrid::Show(bool show)
This implementation seems pretty generic, why don't we do this at wxWindow level?
> @@ -11019,6 +11044,44 @@ void wxGrid::SetRowSizes(const wxGridSizesInfo& sizeInfo)
DoSetSizes(sizeInfo, wxGridRowOperations());
}
+#if wxUSE_ACCESSIBILITY
+
+bool wxGrid::Show(bool show)
+{
+ bool changed = wxWindow::Show(show);
+ if ( changed )
+ {
+ wxAccessible::NotifyEvent(show ? wxACC_EVENT_OBJECT_SHOW : wxACC_EVENT_OBJECT_HIDE,
+ this, wxOBJID_CLIENT, wxACC_SELF);
+ }
+
+ return changed;
+}
+
+void wxGrid::SetName(const wxString &name)
And actually this one and Reparent() don't seem to be wxGrid-specific neither...
> @@ -11337,4 +11400,640 @@ wxGetContentRect(wxSize contentSize,
return contentRect;
}
+
+#if wxUSE_ACCESSIBILITY
+
+//-----------------------------------------------------------------------------
+// helpers for wxGridAccessible and wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+// helper to convert row / col into child object id
+// error handling is not required here
+static int GetChildId(wxGrid* grid, int row, int col,
+ bool isRowHeader, bool isColHeader)
I strongly dislike having (especially multiple) boolean parameters in the functions and it looks like we could avoid them her just by treating row == -1 as column header and col == -1 as row header, couldn't we?
IMHO this would make the code shorter and more readable.
> @@ -11337,4 +11400,640 @@ wxGetContentRect(wxSize contentSize,
return contentRect;
}
+
+#if wxUSE_ACCESSIBILITY
+
+//-----------------------------------------------------------------------------
+// helpers for wxGridAccessible and wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+// helper to convert row / col into child object id
+// error handling is not required here
+static int GetChildId(wxGrid* grid, int row, int col,
It might be worth renaming this to ConvertRowColToChildId() and the function below to ConvertChildIdToRowCol() to emphasize that they're symmetric to each other.
> +static void GetRowCol(wxGrid* grid, int childId, int* row, int* col, + bool* isRowHeader, bool* isColHeader)
So I'd also like to get rid of isXXXHeader output parameters here and just set row and/or col to -1.
I also think that it could be nicer to return wxGridCellCoords from this function instead of using output parameters.
> + }
+ else
+ (*row)--;
+ }
+ }
+}
+
+
+//-----------------------------------------------------------------------------
+// wxGridAccessible
+//-----------------------------------------------------------------------------
+
+wxGridAccessible::wxGridAccessible(wxGrid* win)
+ : wxWindowAccessible(win)
+{
+ m_gridCellAccessible = nullptr;
Minor, but it is preferable to initialize members in their declarations in the new code.
> + if ( m_gridCellAccessible )
+ {
+ delete m_gridCellAccessible;
+ m_gridCellAccessible = nullptr;
+ }
All this could be replaced by just delete m_gridCellAccessible.
Even better would be to replace the raw pointer with std::unique_ptr<>, then it wouldn't need to be neither initialized nor deleted, as it would happen automatically.
> +// Can return either a child object, or an integer
+// representing the child element, starting from 1.
+wxAccStatus wxGridAccessible::HitTest(const wxPoint& pt, int* childId, wxAccessible** childObject)
+{
+ wxGrid* grid = wxDynamicCast(GetWindow(), wxGrid);
+ wxCHECK(grid, wxACC_FAIL);
+
+ wxPoint pos = grid->ScreenToClient(pt);
+
+ if ( grid->HitTest(pos) != wxHT_WINDOW_INSIDE )
+ {
+ *childId = wxACC_SELF;
+ return wxACC_OK;
+ }
+
+ // this one is reduced by the sizes of the labels
Maybe s/reduced/offset/? Or do I misunderstand what this comment says?
> + }
+ else if ( m_gridCellAccessible->m_childId != childId )
+ {
+ // pass on the information whether the cell is on the same row or
+ // column than the previous one to avoid redundant coordinate info
+ int previousRow = m_gridCellAccessible->m_row;
+ int previousCol = m_gridCellAccessible->m_col;
+ int previousRowHeader = m_gridCellAccessible->m_isRowHeader;
+ int previousColHeader = m_gridCellAccessible->m_isColHeader;
+ delete m_gridCellAccessible;
+ m_gridCellAccessible = new wxGridCellAccessible(grid, childId);
+ if ( m_gridCellAccessible->m_row == previousRow &&
+ m_gridCellAccessible->m_isColHeader == previousColHeader )
+ {
+ m_gridCellAccessible->m_isSameRow = true;
+ printf("same row\n");
Should be removed.
> + int previousCol = m_gridCellAccessible->m_col;
+ int previousRowHeader = m_gridCellAccessible->m_isRowHeader;
+ int previousColHeader = m_gridCellAccessible->m_isColHeader;
+ delete m_gridCellAccessible;
+ m_gridCellAccessible = new wxGridCellAccessible(grid, childId);
+ if ( m_gridCellAccessible->m_row == previousRow &&
+ m_gridCellAccessible->m_isColHeader == previousColHeader )
+ {
+ m_gridCellAccessible->m_isSameRow = true;
+ printf("same row\n");
+ }
+ if ( m_gridCellAccessible->m_col == previousCol &&
+ m_gridCellAccessible->m_isRowHeader == previousRowHeader )
+ {
+ m_gridCellAccessible->m_isSameCol = true;
+ printf("same column\n");
And this too.
> + }
+
+ return wxACC_OK;
+}
+
+
+//-----------------------------------------------------------------------------
+// wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+wxGridCellAccessible::wxGridCellAccessible(wxGrid* grid, int childId)
+ : wxAccessible(grid)
+{
+ m_childId = childId;
+ GetRowCol(grid, childId, &m_row, &m_col, &m_isRowHeader, &m_isColHeader);
+ m_isSameRow = m_isSameCol = false;
Those should be also initialized in their declarations.
> +// If *child is nullptr and return value is wxACC_OK, this means that the child +// is a simple element and not an accessible object.
This doesn't seem to be relevant here?
> + const int cursorRow = grid->GetGridCursorRow(); + const int cursorCol = grid->GetGridCursorCol(); + + if ( !m_isColHeader && !m_isRowHeader && cursorRow == m_row && cursorCol == m_col )
Comparisons like this would be simpler to write if we used wxGridCellCoords, e.g. replacing m_row and m_col with a single m_coords would allow to reduce this to just
- const int cursorRow = grid->GetGridCursorRow(); - const int cursorCol = grid->GetGridCursorCol(); - - if ( !m_isColHeader && !m_isRowHeader && cursorRow == m_row && cursorCol == m_col ) + if ( GetGridCursorCoords() == m_coords )
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 3 commits.
- 46c906d move wxGridAccessible to private header
- c558212 replace m_row/m_col etc. with m_coords
- 5ad258f cleanup after review
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> @@ -3467,5 +3484,63 @@ extern const int wxEVT_GRID_CHANGE_SEL_LABEL; #endif +#if wxUSE_ACCESSIBILITY
Done; it's in .../generic/private/grid.h now
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> @@ -3467,5 +3484,63 @@ extern const int wxEVT_GRID_CHANGE_SEL_LABEL;
#endif
+#if wxUSE_ACCESSIBILITY
+//-----------------------------------------------------------------------------
+// accessibility support for whole grid and per cell
+//-----------------------------------------------------------------------------
+
+class WXDLLIMPEXP_CORE wxGridAccessible: public wxWindowAccessible
+{
+public:
+ wxGridAccessible(wxGrid* win);
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> @@ -11019,6 +11044,44 @@ void wxGrid::SetRowSizes(const wxGridSizesInfo& sizeInfo)
DoSetSizes(sizeInfo, wxGridRowOperations());
}
+#if wxUSE_ACCESSIBILITY
+
+bool wxGrid::Show(bool show)
At the moment wxGrid and wxDataViewCtrl are the only generic controls which have these. The native controls probably have their Show and SetName support in the OS. I would not want to make experiments about this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> @@ -11337,4 +11400,640 @@ wxGetContentRect(wxSize contentSize,
return contentRect;
}
+
+#if wxUSE_ACCESSIBILITY
+
+//-----------------------------------------------------------------------------
+// helpers for wxGridAccessible and wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+// helper to convert row / col into child object id
+// error handling is not required here
+static int GetChildId(wxGrid* grid, int row, int col,
+ bool isRowHeader, bool isColHeader)
Yes, you're right. This originated from HitTest, where row and col can also be -1 for being outside the cell area, but for the child objects that can't happen.
This now uses m_coords, as suggested below.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> + }
+ else if ( m_gridCellAccessible->m_childId != childId )
+ {
+ // pass on the information whether the cell is on the same row or
+ // column than the previous one to avoid redundant coordinate info
+ int previousRow = m_gridCellAccessible->m_row;
+ int previousCol = m_gridCellAccessible->m_col;
+ int previousRowHeader = m_gridCellAccessible->m_isRowHeader;
+ int previousColHeader = m_gridCellAccessible->m_isColHeader;
+ delete m_gridCellAccessible;
+ m_gridCellAccessible = new wxGridCellAccessible(grid, childId);
+ if ( m_gridCellAccessible->m_row == previousRow &&
+ m_gridCellAccessible->m_isColHeader == previousColHeader )
+ {
+ m_gridCellAccessible->m_isSameRow = true;
+ printf("same row\n");
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> + int previousCol = m_gridCellAccessible->m_col;
+ int previousRowHeader = m_gridCellAccessible->m_isRowHeader;
+ int previousColHeader = m_gridCellAccessible->m_isColHeader;
+ delete m_gridCellAccessible;
+ m_gridCellAccessible = new wxGridCellAccessible(grid, childId);
+ if ( m_gridCellAccessible->m_row == previousRow &&
+ m_gridCellAccessible->m_isColHeader == previousColHeader )
+ {
+ m_gridCellAccessible->m_isSameRow = true;
+ printf("same row\n");
+ }
+ if ( m_gridCellAccessible->m_col == previousCol &&
+ m_gridCellAccessible->m_isRowHeader == previousRowHeader )
+ {
+ m_gridCellAccessible->m_isSameCol = true;
+ printf("same column\n");
done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> + }
+
+ return wxACC_OK;
+}
+
+
+//-----------------------------------------------------------------------------
+// wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+wxGridCellAccessible::wxGridCellAccessible(wxGrid* grid, int childId)
+ : wxAccessible(grid)
+{
+ m_childId = childId;
+ GetRowCol(grid, childId, &m_row, &m_col, &m_isRowHeader, &m_isColHeader);
+ m_isSameRow = m_isSameCol = false;
Done. Not sure whether the new m_coords should be initialized there somehow:
wxGridCellCoords m_coords;
bool m_isSameRow = false;
bool m_isSameCol = false;
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> +// If *child is nullptr and return value is wxACC_OK, this means that the child +// is a simple element and not an accessible object.
Right. Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> + const int cursorRow = grid->GetGridCursorRow(); + const int cursorCol = grid->GetGridCursorCol(); + + if ( !m_isColHeader && !m_isRowHeader && cursorRow == m_row && cursorCol == m_col )
Thanks. Done. This required several changes.
I hope I did not introduce bugs.
I hope this is correct to retain the previous coordinates for initialization of a new wxGridCellAccessible instance:
wxGridCellCoords previous_coords = m_gridCellAccessible->m_coords;
delete m_gridCellAccessible;
m_gridCellAccessible = new wxGridCellAccessible(grid, childId);
if ( m_gridCellAccessible->m_coords.GetRow() == previous_coords.GetRow() )
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- c0208b9 fix bug for row header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
Thanks. The modifications are in the repository now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -11019,6 +11044,44 @@ void wxGrid::SetRowSizes(const wxGridSizesInfo& sizeInfo)
DoSetSizes(sizeInfo, wxGridRowOperations());
}
+#if wxUSE_ACCESSIBILITY
+
+bool wxGrid::Show(bool show)
I'd still prefer to avoid duplicating this code (I'm pretty sure that the next person to implement a11y for something else will just copy it again), so I think we should move it to the common base class but somehow make it opt-in, but we can postpone it for later...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> + }
+
+ return wxACC_OK;
+}
+
+
+//-----------------------------------------------------------------------------
+// wxGridCellAccessible
+//-----------------------------------------------------------------------------
+
+wxGridCellAccessible::wxGridCellAccessible(wxGrid* grid, int childId)
+ : wxAccessible(grid)
+{
+ m_childId = childId;
+ GetRowCol(grid, childId, &m_row, &m_col, &m_isRowHeader, &m_isColHeader);
+ m_isSameRow = m_isSameCol = false;
No, this class has a (default) ctor which already initializes it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 60c735d use std::unique_ptr for reference to wxGridCellAccessible
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
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?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
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!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
Yes, sure. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
Are we sure that numCols can't be 0 here? It doesn't seem totally obvious...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> + wxGrid* grid = wxDynamicCast(GetWindow(), wxGrid);
+ wxCHECK( grid, wxACC_FAIL );
+
+ if ( childId == wxACC_SELF )
+ {
+ *role = wxROLE_SYSTEM_TABLE;
+ }
+ else
+ {
+ // children can be cells or headers
+ wxGridCellCoords coords = ConvertChildIdToCoords(grid, childId);
+ if ( coords.GetRow() == -1 )
+ *role = wxROLE_SYSTEM_COLUMNHEADER;
+ else if ( coords.GetCol() == -1 )
+ *role = wxROLE_SYSTEM_ROWHEADER;
+ else
Shouldn't we check for (-1,-1) here, which is the corner and not a cell?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> + if ( childId > 0 ) + return wxACC_NOT_IMPLEMENTED; + + long st = 0; + if ( !grid->IsEnabled() ) + st |= wxACC_STATE_SYSTEM_UNAVAILABLE; + if ( !grid->IsShown() ) + st |= wxACC_STATE_SYSTEM_INVISIBLE; + + if( grid->IsFocusable() ) + st |= wxACC_STATE_SYSTEM_FOCUSABLE; + + // 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;
Totally nitpickish but there is an extra space here:
⬇️ Suggested change- st |= wxACC_STATE_SYSTEM_FOCUSED; + st |= wxACC_STATE_SYSTEM_FOCUSED;
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 5d7e768 Remove leftover wxGridAccessible dtor declaration
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
> + wxGrid* grid = wxDynamicCast(GetWindow(), wxGrid);
+ wxCHECK( grid, wxACC_FAIL );
+
+ if ( childId == wxACC_SELF )
+ {
+ *role = wxROLE_SYSTEM_TABLE;
+ }
+ else
+ {
+ // children can be cells or headers
+ wxGridCellCoords coords = ConvertChildIdToCoords(grid, childId);
+ if ( coords.GetRow() == -1 )
+ *role = wxROLE_SYSTEM_COLUMNHEADER;
+ else if ( coords.GetCol() == -1 )
+ *role = wxROLE_SYSTEM_ROWHEADER;
+ else
No, that's fine. It's for all the left labels except the corner.
There is no separate role for the corner and therefore the check before will return wxROLE_SYSTEM_COLUMNHEADER also for the corner.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
Your modifications look good for me. I have just built and tested with NVDA. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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;
}—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger pushed 1 commit.
- 14644c2 insert check
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dietmar Schwertberger
@DietmarSchwertberger commented on this pull request.
> + return childId;
+}
+
+// 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++;
+ int row = (childId - 1) / numCols;
+ int col = (childId - 1) % numCols;
An empty grid is fine.
HitTest and GetChildCount are responsible for returning only valid child ids. In case of a grid with zero columns, 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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks again for your work on this, finally merging this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.