Add `--accessible` button to fix voice over bug on server list
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
@Michael1972 could you please test if this build started with --accessible works for you?
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.)
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.
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.
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.
Ok. Implementing might take some time then.
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?
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
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.)
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.
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.
No, I'm not sure either. I'll look at it this week.
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?
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.
I just built the master branch with qt 6.9.3 and tried on MacOS, and the server list is still not accessible.
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 did you start Jamulus with the --accessible flag?
Yes, and it says Accessible server list enabled.
Does the code of this PR contain all necessary changes from your build?
It's the same with my accessible fork. NO longer accessible in qt 6.9.3 on MacOS 26.
@chigkim This means we might need to check the legacy build. Maybe this is still accessible
Yes the build on chigkim.github.io/jamulus which was built with older qt is still accessible on MacOS 26.
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.
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?
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.
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.
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.
@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.
When I try #3573, it says unknown option --accessible. Also it doesn't work.
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.