Fix wxDataViewCustomRenderer cells reading as raw object refs on macOS (PR #26809)

[Lorenzo Salami]

## Summary

Fixes #26808: on macOS, cells drawn by a `wxDataViewCustomRenderer` (or subclass) had no accessible text at all. VoiceOver and tooltips both announced/showed the raw debug representation of the private Cocoa value holder (`<wxCustomRendererObject: 0x...>`) instead of the cell's actual content.

Root cause: Cocoa's `wxDataViewCtrl` implementation relies entirely on native `NSAccessibility`, not the generic `wxAccessible`/`wxUSE_ACCESSIBILITY` layer (that layer is unused on this port). There was no code anywhere describing a custom-rendered cell's value to the accessibility bridge, so both VoiceOver and NSCell's own tooltip/stringification machinery fell back to `NSObject`'s default `-description`.

## Changes

- `wxDataViewCustomRendererBase::GetAccessibleText()` (new, `include/wx/dvrenderers.h` / `src/common/datavcmn.cpp`): an always-on virtual, not gated by `wxUSE_ACCESSIBILITY`, that stringifies the renderer's current value. Explicitly handles `wxDataViewIconText` (the common "icon + text" custom renderer shape), since `wxVariant::MakeString()` doesn't know that type. Renderers whose value has no sensible plain-text form (e.g. a progress bar) can override it to return something more useful, or leave it as the default (falls back to the debug representation, same as before this change, so nothing regresses).
- `src/osx/cocoa/dataview.mm`: override `-description` on the private `wxCustomRendererObject` value holder to call the above. Both `-[NSCell _formatObjectValue:invalid:]` (used by tooltips and by the legacy accessibility bridge, `NSAccessibilityGetObjectForAttributeUsingLegacyAPI`) and `NSObject`'s own debug printing consult `-description` for a non-string `objectValue`, so this one override fixes both VoiceOver and ordinary tooltips

[Lorenzo Salami]

LSalami left a comment (wxWidgets/wxWidgets#26809)

The three failing checks (MSW/MSVC wxQt 6.10, wxMSW vs2022 Debug Win32, wxMSW vs2026 DLL Release x64) are not related to this change: they're a pre-existing MSVC compile error on master itself, introduced by the spaces-colour-names merge (tests/asserthelper.cpp(16,58): error C2445: result type of conditional expression is ambiguous: types 'wxString' and 'const char [8]', in operator<<(std::ostream&, const wxColour&)). Same error, same two jobs, fails identically on master's own CI run for that merge (e.g. run 31326861331) -- nothing in this PR touches wxColour or asserthelper.cpp. Every other platform/config (GTK, Qt, X11, DFB, macOS, iOS, the two non-MSVC MSW jobs) is green.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/26809/c5233568347@github.com>

[VZ]

vadz left a comment (wxWidgets/wxWidgets#26809)

Thanks for the PR, but I have to say it looks rather weird to add a separate GetAccessibleText() when we already have a perfectly cromulent GetAccessibleDescription() which appears to be doing broadly (exactly?) the same thing. If the only problem is that wxUSE_ACCESSIBILITY is currently always 0 in wxOSX builds, then I think that either of the following solutions would be better:

1. Set wxUSE_ACCESSIBILITY to 1 under Mac by default, even if it will be only used for this so far.
2. If the above is impossible for whatever reason, remove wxUSE_ACCESSIBILITY checks around the existing function (and explain in a comment that this is done to allow using it in wxOSX where wxUSE_ACCESSIBILITY can't be turned on).

Could you please consider doing the above, ideally (1)?

The three failing checks (MSW/MSVC wxQt 6.10, wxMSW vs2022 Debug Win32, wxMSW vs2026 DLL Release x64) are not related to this change: they're a pre-existing MSVC compile error on master itself

Yes, sorry about that.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/26809/c5233629137@github.com>

[Lorenzo Salami]

LSalami left a comment (wxWidgets/wxWidgets#26809)

Thanks for taking a look, and no worries about the CI noise.

I looked into both options before replying, and I don't think (1) actually
changes anything here:

• wxUSE_ACCESSIBILITY is hardcoded off on every non-MSW port
  (include/wx/osx/setup.h: #ifdef __WXMSW__ ... 1 ... #else ... 0), not a
  user-toggleable default — flipping it for OSX specifically would need a
  chkconf.h/setup.h change of its own.
• The only native wxAccessible bridge in the tree is
  src/msw/ole/access.cpp; there's no OSX equivalent.
• More to the point: GetAccessibleDescription() is only ever called from
  src/generic/datavgen.cpp (the generic dataview backend's own
  accessibility tree-walk). src/osx/cocoa/dataview.mm -- the native Cocoa
  backend this PR touches -- never calls it anywhere. So even with
  wxUSE_ACCESSIBILITY compiled in on Mac, nothing on the native dataview
  path would pick it up; it'd only affect the generic backend, which isn't
  what's normally instantiated on this port.

So (2) is the one that actually does something, and I'm happy to switch to
it -- it does remove the duplication, you're right that GetAccessibleText()
and GetAccessibleDescription() are doing the same thing. One wrinkle:
the base declaration is currently pure virtual
(dvrenderers.h:153, virtual wxString GetAccessibleDescription() const = 0;
inside the #if wxUSE_ACCESSIBILITY block). Unguarding it as-is would make
it a mandatory override for every wxDataViewRenderer subclass on every
port, in-tree and downstream -- a bigger compat break than this PR should
be causing.

Proposed shape instead: unguard the declarations/definitions but give the
base a non-pure default implementation (the body I'd written for
GetAccessibleText() -- stringifies GetValue(), with the existing
bool/wxDataViewIconText special-casing). Every in-tree renderer already
overrides it, so this is source-compatible; anyone who doesn't override it
now gets a sensible default instead of a link error. I'll fold
GetAccessibleText()'s body into GetAccessibleDescription() and have
dataview.mm call that instead, drop the new method entirely, and keep the
comment on the (now-unguarded) declaration explaining why it's called
unconditionally on wxOSX.

Let me know if that matches what you had in mind and I'll push the revision.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/26809/c5233706029@github.com>

[VZ]

vadz left a comment (wxWidgets/wxWidgets#26809)

If (1) requires too many changes, let's do (2) but in an easy to generalize later way: instead of simply removing all #if wxUSE_ACCESSIBILITY in include/wx/dvrenderers.h let's replace them with something like #if wxUSE_DATAVIEW_A11Y and define this symbol as wxUSE_ACCESSIBILITY || defined(__WXOSX__) (with the appropriate comment explaining why do we do this).

You would, of course, keep the macOS-specific code added by this PR and just call GetAccessibleDescription() from it.

It's not clear to me why it can't remain pure virtual: it already is under MSW, so it should be already implemented in all the standard renderer classes. And wxDataViewCustomRenderer implements it too. So what is the problem with leaving it pure virtual, exactly?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/26809/c5234123777@github.com>

[Lorenzo Salami]

LSalami left a comment (wxWidgets/wxWidgets#26809)

wxUSE_DATAVIEW_A11Y sounds good, will do.

On staying pure virtual: it's fine for the renderer classes wx itself ships (wxDataViewTextRenderer, wxDataViewSpinRenderer, etc. all already implement it, guarded or not), but it's not fine for wxDataViewCustomRenderer on OSX specifically. Traced the actual chain:

• wxDataViewRendererBase declares GetAccessibleDescription() = 0.
• wxDataViewCustomRendererBase (dvrenderers.h, shared across ports) derives from it and does not implement it -- it only has GetValue()/Render()/etc., which are renderer-specific, not this.
• wx/osx/dvrenderers.h's wxDataViewCustomRenderer (the OSX-specific concrete base every OSX custom renderer derives from) doesn't implement it either -- I checked, there's no GetAccessibleDescription() anywhere under src/osx/.

So today, on OSX, the pure virtual is inherited all the way down unimplemented. It's currently harmless only because wxUSE_ACCESSIBILITY is always 0 there, so the #if compiles it out entirely -- no OSX app has ever had to implement it. The moment wxUSE_DATAVIEW_A11Y makes that block compile on OSX, every existing concrete wxDataViewCustomRenderer subclass on that port that doesn't already override it stops compiling. That's not hypothetical: it's aMule's own CMuleBarRenderer today (a wxDataViewCustomRenderer subclass with no GetAccessibleDescription()/GetAccessibleText() override, since neither was ever needed there before this PR).

Given wx doesn't know what a third-party custom renderer draws, I don't think there's a way to keep it pure virtual on OSX without breaking every existing out-of-tree custom renderer on that port the moment this lands. A default implementation on wxDataViewCustomRendererBase -- stringify GetValue(), same body GetAccessibleText() has now -- keeps it source-compatible: apps that care can still override it for a better answer, apps that don't get a reasonable one for free instead of a build break.

I'll fold GetAccessibleText()'s body into that default GetAccessibleDescription(), drop the separate method, switch dataview.mm to call GetAccessibleDescription(), and gate the whole thing on wxUSE_DATAVIEW_A11Y as you suggested. Shout if you'd still rather it be pure and accept the OSX compat break.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <wxWidgets/wxWidgets/pull/26809/c5236033062@github.com>