jamulus icon indicating copy to clipboard operation
jamulus copied to clipboard

Add `--accessible` button to fix voice over bug on server list

Open ann0see opened this issue 1 month ago • 35 comments

Short description of changes Applies the fix by @chigkim to upstream by introducing a CLI argument.

CHANGELOG: Accessability: Add --accessible CLI argument to enable alternative, VoiceOver compatible server list Context: Fixes an issue?

Maybe: #490

Does this change need documentation? What needs to be documented and how?

Yes. This should have an entry somewhere in the docs.

Status of this Pull Request

Ready for testing on macOS. (I did not do it yet).

What is missing until this pull request can be merged?

Testing

Checklist

  • [x] I've verified that this Pull Request follows the general code principles
  • [ ] I tested my code and it does what I want
  • [x] My code follows the style guide
  • [ ] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • [ ] I've filled all the content above

ann0see avatar Nov 09 '25 11:11 ann0see

@Michael1972 could you please test if this build started with --accessible works for you?

ann0see avatar Nov 09 '25 11:11 ann0see

Can we also have a Settings->My Profile "User Interface" checkbox, like there is for "Audio Alerts", please. (It would need to send a signal wire to a slot in the main client, which would issue a signal wired to a slot in the Connect dialog, which would switch over the widget.)

I'd also take care with a generic term like --accessible if it only changes one specific widget in the UI. (I mean, the Audio Alerts are explicitly an accessibility feature and it's not like you need to use --accessible to get that.)

pljones avatar Nov 09 '25 12:11 pljones

The flag is there for a quick first version to get 3.12.0 out. Something in the UI could be added for sure - but I don't know this part of the code well enough anymore.

ann0see avatar Nov 09 '25 12:11 ann0see

I'd also take care with a generic term like --accessible

Agree. My thought was to leave this generic to allow future accessibility features which don't "just work" out of the box.

ann0see avatar Nov 09 '25 12:11 ann0see

I'd also take care with a generic term like --accessible

Agree. My thought was to leave this generic to allow future accessibility features which don't "just work" out of the box.

Unless it's a compile-time option, the code is there to be used. We shouldn't introduce new code that needs a run time option to enable it. It should be configurable through the UI, through RPC (if relevant -- this isn't, I suppose) or through the command line equally. When configured through the UI or RPC, the setting should be persisted.

This is why I don't think this feature is close to ready for release.

pljones avatar Nov 09 '25 12:11 pljones

Ok. Implementing might take some time then.

ann0see avatar Nov 09 '25 20:11 ann0see

Could we not replace the original widget with the QLabel with the text visible on screen and the accessibleName set? That way we'd have one widget doing two things, rather than two widget, one of which is trying to partly hide itself. Or am I missing something?

pljones avatar Nov 13 '25 19:11 pljones

Could we not replace the original widget with the QLabel with the text visible on screen and the accessibleName set?

I tried that today, without success. I will be trying other approaches.

ChatGPT has given me some ideas in a test app that seems to work with VoiceOver. I just need to abstract the technique into the Jamulus code. https://chatgpt.com/share/6916555e-bb84-8006-9993-d049ad4b6eef

softins avatar Nov 13 '25 21:11 softins

OK, I need to get some familiarity with the terminology... The connect dialog contains a QTreeWidget nominally titled the "Server List". This contains a header and then one or more server rows. Each server row may have client rows:

erDiagram
    serverList ||--|| headerRow : has
    headerRow ||--|{ headerColumns : contains
    serverList ||--o{ serverRow : has
    serverRow ||--|{ serverDetailColumns : contains
    serverRow ||--o{ clientRow : has
    clientRow ||--|{ clientDetailColumns : contains

The QTreeWidget is the outermost thing, right -- the serverList? And is everything else a QTreeWidgetItem? And we'd need to implement at least the basic "read the content of the existing QTreeWidgetItem text"?

So the bit from ChatGPT "Here’s a small enhancement for the Qt 5.15 build that makes VoiceOver read both the parent and the currently selected child together" -- this would start by saying "Server List" (if that was in the accessibleText), then ... what? Say "3ms" if you were on the ping time of a nearby server? I don't know how the accessible navigation works for the widget and what context makes sense. It feels like having the header column accessibleText along with the server column accessibleText would probably work better.

(ChatGPT was able to parse the mermaid diagram, by the way.)

pljones avatar Nov 14 '25 18:11 pljones

I've not tried it yet in the Jamulus context. And I only have macOS 10.15 with Qt 5.15.2.

Apparently Qt 6.5+ should just work out of the box, even on Mac, but I don't have anything to try that on. Maybe that's why someone (you?) said it worked fine on Windows, because our Windows build uses a recent Qt6?

The original accessibility problem reports date from 2020 when Volker was still maintainer and we were on earlier Qt5.

softins avatar Nov 14 '25 23:11 softins

Sorry, what I mean is do the suggested solution for 5.15 but pick up the header and field values rather than container and content. That's why I did the diagram - I'm not sure what contains what.

pljones avatar Nov 15 '25 12:11 pljones

No, I'm not sure either. I'll look at it this week.

softins avatar Nov 15 '25 18:11 softins

And the point with using the accessibleText is that it would do the same thing in Qt 6 as Qt 5, then -- if we wanted custom accessibleText, we could set that and have Qt 5 pick it up, I presume, using the fix suggested for Qt 5? Or is accessibleText not even there on the QTreeWidgetItem in Qt 5?

pljones avatar Nov 16 '25 13:11 pljones

Wading through treacle a bit on this, with Qt 5.15.

According to ChatGPT, accessible navigation of a QTreeWidget should just work out of the box for Qt 6.5 onwards.

We should note that @chigkim's original report #490 and suggested fix date from 2020, when we were using Qt5.

Our standard Mac build now uses Qt 6.9, and only the legacy build uses 5.15.

Please could someone verify whether our standard Mac build (main, not this PR) with Qt6 actually does or doesn't provide VoiceOver accessibility for the Connect dialog already? If it does, then this PR is not actually needed, and we would be justified in just saying the legacy Mac build does not provide accessibility.

I can't verify it myself, as I only have an old Mac with macOS 10.15, so can only run the legacy version.

softins avatar Nov 17 '25 17:11 softins

I just built the master branch with qt 6.9.3 and tried on MacOS, and the server list is still not accessible.

chigkim avatar Nov 17 '25 19:11 chigkim

Bad news. Also I tried this pr #3559 with qt 6.9.3, and it doesn't work either. Maybe something broken in QT?

chigkim avatar Nov 17 '25 19:11 chigkim

@chigkim did you start Jamulus with the --accessible flag?

ann0see avatar Nov 17 '25 19:11 ann0see

Yes, and it says Accessible server list enabled.

chigkim avatar Nov 18 '25 03:11 chigkim

Does the code of this PR contain all necessary changes from your build?

ann0see avatar Nov 19 '25 07:11 ann0see

It's the same with my accessible fork. NO longer accessible in qt 6.9.3 on MacOS 26.

chigkim avatar Nov 22 '25 05:11 chigkim

@chigkim This means we might need to check the legacy build. Maybe this is still accessible

ann0see avatar Nov 22 '25 07:11 ann0see

Yes the build on chigkim.github.io/jamulus which was built with older qt is still accessible on MacOS 26.

chigkim avatar Nov 22 '25 19:11 chigkim

I tried to build one of the QT examples, examples/widgets/tools/regularexpression that comes with qt 10.2 and uses QTreeWidget, and it's the same unfortunately.

chigkim avatar Nov 22 '25 23:11 chigkim

OK, so the older Qt 5 patch approach works on current macOS?

We have two variables and, to keep things clear for me, it helps to make sure that we're not changing both at once.

  • [x] Qt 5x without patch -> not expected to work (regardless of macOS version) and doesn't, original bug report
  • [x] Qt 5x plus original patch -> works on current macOS
  • [ ] Qt 6x without patch -> expected to work on current macOS with native Qt widget but doesn't (although "expected" here isn't clear) -- led to Tony's patch
  • [ ] Qt 6x plus original patch -> doesn't work on current macOS

And Tony's patch also did

  • [ ] Qt 5x with a different patch -> didn't work on current macOS
  • [ ] Qt 6x with a different patch -> didn't work on current macOS

(This is given that the Qt 5x build is generally considered "legacy only", so I'm trying to compare like with like.)

Did I get all that correct?

pljones avatar Nov 23 '25 09:11 pljones

I'll test the legacy build with VoiceOver. But I assume that the following is the case:

Qt5 with fix on modern macOS works, everything else does not.

ann0see avatar Dec 06 '25 11:12 ann0see

Actually, I did somehow manage to get into the server list without having enabled the accessible flag on Qt6 and VoiceOver spoke out the content. I'll try to reproduce this as this would show that the bug is actually fixed, but rather somthing weird needs to happen to start VoiceOver to speak out the contents.

Maybe a tabstop is missing in the table?

But it's definitely not really usable.

ann0see avatar Dec 06 '25 11:12 ann0see

But I assume that the following is the case:

Qt5 with fix on modern macOS works

Really, I don't want this to be an assumption, I want someone to confirm it actually holds true.

pljones avatar Dec 06 '25 12:12 pljones

@chigkim @pljones Please have a look at https://github.com/jamulussoftware/jamulus/pull/3573 This is a workaround (not yet finished as it has edge cases) which copilot suggested. I believe this method could work.

ann0see avatar Dec 06 '25 15:12 ann0see

When I try #3573, it says unknown option --accessible. Also it doesn't work.

chigkim avatar Dec 11 '25 09:12 chigkim

Ok, I installed bunch of different qt versions from 6.2.0 to 6.10.1, and found out that the last qt version that works with my hack (included in #3559) is qt6.5.0.

What's interesting is that the TreeWidget actually started to become somewhat accessible even without my hack (but still not usable) between qt6.5.1 and qt6.9.0. However, it regressed in qt6.9.1, and I can't even focus on the element with keyboard.

I also tried #3573 with qt6.5.0, but you can only navigate server name with up/down or tab. Left/right doesn't get to focus on latency, number of users, location, etc.

The best workaround for now might be to incorporate #=3559 and implement a screen reader toggle in the settings.

chigkim avatar Dec 11 '25 18:12 chigkim