Skip to content

Improve macOS login tree accessibility#244

Merged
zarvox32 merged 3 commits intoXGDevGroup:mainfrom
vlad-a-c:main
Apr 30, 2026
Merged

Improve macOS login tree accessibility#244
zarvox32 merged 3 commits intoXGDevGroup:mainfrom
vlad-a-c:main

Conversation

@vlad-a-c
Copy link
Copy Markdown
Contributor

the items in the treeview are also readable on mac now again in.

@zarvox32
Copy link
Copy Markdown
Contributor

Review by Claude (Opus 4.7), driven by the PR author's collaborator. Posted on their behalf.

What it does

Provides a macOS-only ManagedTreeCtrl backed by wx.dataview.TreeListCtrl (better VoiceOver support than wx.TreeCtrl). Re-implements the wx.TreeCtrl API surface used by login_dialog.py on top of TreeListCtrl. Also adds name="Servers and accounts" to the tree control for accessibility.

The non-darwin path is untouched.

Test results

  • Module imports cleanly on both real Windows and a forced-darwin code path.
  • Windows MRO unchanged: ManagedTreeCtrl → TreeSelectionManagerMixin → TreeCtrl → Control
  • Forced-darwin MRO: ManagedTreeCtrl → TreeListCtrl → Window → WindowBase — note: no mixin.
  • API audit of login_dialog.py against the shim: every call site is covered (either explicitly shimmed or inherited from TreeListCtrl with compatible semantics).
  • Couldn't run the actual GUI test on macOS from a Windows machine. The author needs to confirm VoiceOver behavior in person.

Issues

1. API parity gap — select_after_delete() missing on macOS

The non-macOS class inherits select_after_delete() from TreeSelectionManagerMixin; the macOS class doesn't. login_dialog.py doesn't currently call it, but any future caller hits AttributeError on macOS only — a silent platform regression.

Worse: the macOS __init__ accepts and stores focus_after_delete but the method that would use it doesn't exist. Either port select_after_delete() onto the macOS class, or remove the unused parameter.

2. Class redefinition pattern triggers linter warnings

class ManagedTreeCtrl(TreeSelectionManagerMixin, wx.TreeCtrl):
    pass

if sys.platform == "darwin":
    class ManagedTreeCtrl(dv.TreeListCtrl):
        ...

Most linters (flake8 F811, pylint redefined-outer-name, mypy) will flag this. Cleaner pattern:

if sys.platform == "darwin":
    class _MacManagedTreeCtrl(dv.TreeListCtrl): ...
    ManagedTreeCtrl = _MacManagedTreeCtrl
else:
    class ManagedTreeCtrl(TreeSelectionManagerMixin, wx.TreeCtrl): pass

3. style parameter silently dropped on macOS

def __init__(self, parent, ..., style: int = 0, ...) -> None:
    super().__init__(parent, id=id, pos=pos, size=size, **kwargs)  # style not passed

login_dialog.py passes style=wx.TR_HAS_BUTTONS | wx.TR_SINGLE | wx.TR_HIDE_ROOT. The TR_* flags are TreeCtrl-specific and don't apply to TreeListCtrl, so the silent drop happens to be safe, but it's invisible to readers. Add a comment explaining tree style flags are intentionally ignored, or accept-and-warn on unknown flags.

4. Bind override only translates two events

if event == wx.EVT_TREE_SEL_CHANGED: return dv.EVT_TREELIST_SELECTION_CHANGED
if event == wx.EVT_TREE_ITEM_ACTIVATED: return dv.EVT_TREELIST_ITEM_ACTIVATED
return event

login_dialog.py only uses these two, so it works today. If anyone later binds EVT_TREE_KEY_DOWN, EVT_TREE_ITEM_EXPANDED, etc., the bind silently passes through to TreeListCtrl which doesn't fire those events — a future invisible breakage. Either expand the table or assert/warn on unknown wx.EVT_TREE_* events.

5. No tests

GUI code is hard to unit-test, but the shim's bookkeeping (_item_key, GetFirstChild/GetNextChild traversal, GetChildrenCount, DeleteAllItems reset) is pure Python and could be exercised against a stubbed TreeListCtrl. The PR is 144 lines with zero tests.

6. Type hints use Any everywhere

parent: Any, item: Any, event: Any, data: Any. The proper types exist (dv.TreeListItem, wx.PyEventBinder, etc.) and would catch the kind of platform-asymmetry bugs called out above. Minor.

Recommendation

The PR's core idea — switch macOS to TreeListCtrl for VoiceOver — is right. The shim is a pragmatic way to avoid touching every caller. Suggested fixes before merge:

  1. Add select_after_delete() to the macOS class (or factor the mixin to be callable on either base) so the API matches across platforms.
  2. Restructure as class _MacManagedTreeCtrl + alias to silence linter warnings and make platform routing explicit.

Optional but worthwhile:
3. Make the Bind translation table exhaustive (or fail loud on unknown EVT_TREE_*).
4. Tighten type hints.

@zarvox32
Copy link
Copy Markdown
Contributor

Tested locally on Windows. The new tests catch a real cross-platform issue:

test_managed_tree_ctrl_delete_all_items_resets_bookkeeping fails on Windows because it exercises the macOS shim's bookkeeping (_item_data dict reset, persistent compat-root) on the wx.TreeCtrl path. After DeleteAllItems(), wx.TreeCtrl.GetRootItem() returns an invalid item, so GetFirstChild(root) raises wxAssertionError: "item.IsOk()" failed, and GetItemData(server_a) on a stale TreeItemId is undefined behavior on the native control.

The other three tests pass / skip cleanly on Windows, so this is a test-only issue — the production shim and login-dialog changes are fine to merge.

A follow-up branch with the one-line fix is ready: zarvox/fix-tree-selection-test-gate (commit). It just adds @pytest.mark.skipif(sys.platform != "darwin", ...) to that test, mirroring the existing gate on test_managed_tree_ctrl_rejects_unsupported_tree_event_binders. I'll open a follow-up PR after this one merges.

@zarvox32 zarvox32 merged commit aa02678 into XGDevGroup:main Apr 30, 2026
1 check failed
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.

2 participants