wxGrid: implement basic accessibility support (PR #24368)

61 views
Skip to first unread message

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:11:25 PMMar 2
to wx-...@googlegroups.com, Subscribed

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)

Patch Links:


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:24:48 PMMar 2
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c1974913199@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:32:00 PMMar 2
to wx-...@googlegroups.com, Push

@DietmarSchwertberger pushed 1 commit.


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17359204862@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:36:52 PMMar 2
to wx-...@googlegroups.com, Push

@DietmarSchwertberger pushed 1 commit.


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17359230358@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:49:08 PMMar 2
to wx-...@googlegroups.com, Push

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17359289136@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:51:33 PMMar 2
to wx-...@googlegroups.com, Push

@DietmarSchwertberger pushed 1 commit.


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17359300899@github.com>

Dietmar Schwertberger

unread,
Mar 2, 2024, 4:58:57 PMMar 2
to wx-...@googlegroups.com, Push

@DietmarSchwertberger pushed 1 commit.


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17359335463@github.com>

Dietmar Schwertberger

unread,
Mar 3, 2024, 7:44:38 AMMar 3
to wx-...@googlegroups.com, Push

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/push/17363935371@github.com>

Dietmar Schwertberger

unread,
Apr 14, 2024, 2:56:19 PMApr 14
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2054154109@github.com>

Dietmar Schwertberger

unread,
May 13, 2024, 3:08:34 PMMay 13
to wx-...@googlegroups.com, Push

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/push/18400317832@github.com>

Dietmar Schwertberger

unread,
May 18, 2024, 10:45:44 AMMay 18
to wx-...@googlegroups.com, Subscribed

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2118845896@github.com>

VZ

unread,
May 19, 2024, 2:55:46 PMMay 19
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2119328628@github.com>

Dietmar Schwertberger

unread,
May 19, 2024, 3:02:17 PMMay 19
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2119330139@github.com>

Dietmar Schwertberger

unread,
May 19, 2024, 3:05:24 PMMay 19
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2119331086@github.com>

VZ

unread,
May 19, 2024, 6:40:56 PMMay 19
to wx-...@googlegroups.com, Subscribed

@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):

  1. Moving private classes declarations to private headers.
  2. Replacing raw pointer with std::unique_ptr<> to make sure we don't leak memory anywhere.
  3. Using wxGridCellCoords instead 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!


In include/wx/generic/grid.h:

> @@ -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?


In include/wx/generic/grid.h:

> @@ -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);

In src/generic/grid.cpp:

> @@ -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?


In src/generic/grid.cpp:

> @@ -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...


In src/generic/grid.cpp:

> @@ -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.


In src/generic/grid.cpp:

> @@ -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.


In src/generic/grid.cpp:

> +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.


In src/generic/grid.cpp:

> +            }
+            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.


In src/generic/grid.cpp:

> +    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.


In src/generic/grid.cpp:

> +// 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?


In src/generic/grid.cpp:

> +        }
+        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.


In src/generic/grid.cpp:

> +            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.


In src/generic/grid.cpp:

> +    }
+
+    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.


In src/generic/grid.cpp:

> +// 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?


In src/generic/grid.cpp:

> +    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

⬇️ Suggested change
-    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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2065152448@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:00:19 AMMay 20
to wx-...@googlegroups.com, Push

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/push/18501904105@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:00:40 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In include/wx/generic/grid.h:

> @@ -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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066171641@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:00:48 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In include/wx/generic/grid.h:

> @@ -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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066171934@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:00:54 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> @@ -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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172075@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:03 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> @@ -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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172323@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:06 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> +        }
+        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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172507@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:10 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> +            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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172577@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:15 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> +    }
+
+    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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172686@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:19 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> +// 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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066172889@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:01:28 AMMay 20
to wx-...@googlegroups.com, Subscribed

@DietmarSchwertberger commented on this pull request.


In src/generic/grid.cpp:

> +    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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066173127@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:06:02 AMMay 20
to wx-...@googlegroups.com, Push

@DietmarSchwertberger pushed 1 commit.


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/18501996850@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 9:07:35 AMMay 20
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2120427186@github.com>

VZ

unread,
May 20, 2024, 9:40:03 AMMay 20
to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.


In src/generic/grid.cpp:

> @@ -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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066249155@github.com>

VZ

unread,
May 20, 2024, 9:40:31 AMMay 20
to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.


In src/generic/grid.cpp:

> +    }
+
+    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.Message ID: <wxWidgets/wxWidgets/pull/24368/review/2066250177@github.com>

Dietmar Schwertberger

unread,
May 20, 2024, 10:01:37 AMMay 20
to wx-...@googlegroups.com, Push

@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.Message ID: <wxWidgets/wxWidgets/pull/24368/push/18502865063@github.com>

VZ

unread,
May 20, 2024, 10:03:25 AMMay 20
to wx-...@googlegroups.com, Subscribed

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.Message ID: <wxWidgets/wxWidgets/pull/24368/c2120528542@github.com>

VZ

unread,
May 20, 2024, 10:05:03 AMMay 20
to wx-...@googlegroups.com, Push

@vadz pushed 2 commits.

  • 61101da Some minor cleanup
  • d73a7ee Rename child id-related functions to have more symmetric names


View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/24368/push/18502918991@github.com>

VZ

unread,
May 20, 2024, 10:05:46 AMMay 20