Add --accessible button to fix voice over bug on server list
#3559
Conversation
ann0see
added
the
needs documentation
|
@Michael1972 could you please test if this build started with |
ann0see
force-pushed
the
a11y/serverList
branch
from
de29d84 to
b26794b
Compare
| //### TODO: BEGIN ###// | ||
| // is this still required??? | ||
| // immediately update status bar | ||
| // ### TODO: BEGIN ###// |
There was a problem hiding this comment.
Clang format did this automatically...
| ClientSettingsDlg ( pNCliP, pNSetP, parent ), | ||
| ChatDlg ( parent ), | ||
| ConnectDlg ( pNSetP, bNewShowComplRegConnList, parent ), | ||
| ConnectDlg ( pNSetP, bNewShowComplRegConnList, bNEnableIPv6, bNEnableAccessiblePushButtonUi, parent ), |
There was a problem hiding this comment.
the ipv6 toggle wasn't passed correctly.
There was a problem hiding this comment.
Wow, so that bug has been hiding for a long time! The parent argument was being converted to a bool, and therefore was always true? Although I can't think what negative effect that would have had in practice.
There was a problem hiding this comment.
It's another indicator of the overall design problem.
Passing parameters is just the wrong way to do it. This should be signals on CClient with slots wired inside CConnectDlg to get notifications of updates to the values, and getters on CClient to supply the current state. Only CClient should provide this information. Even things like the IPv6 toggle should be controllable through the UI -- so you'd need the CConnectDlg to track changes in that state. CClientDlg would then get passed a thread-safe reference to CClient.
(All for r4.0.0, naturally.)
ann0see
force-pushed
the
a11y/serverList
branch
from
b26794b to
df95777
Compare
|
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 |
|
The flag is there for a quick first version to get 3.12.0 out. |
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. |
There was a problem hiding this comment.
I have tried this out. The QPushButton widgets don't look good in the server list.
I read the links mentioned by @ann0see here and following a suggestion in the second one, I changed the item widgets QPushButton to QLabel. It was necessary to create the QLabel with empty text, otherwise it showed up as well as the original text, and then instead of setText() on the widget, I used setAccessibleName(). It now looks fine, but how do I test it? I have a Mac (still running 10.15 Catalina), but don't know anything about screen readers.
EDIT: ok, it seems VoiceOver is built in to macOS. I'll try it out.
|
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? |
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 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 ( 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.
And Tony's patch also did
(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? |
4 participants
Short description of changes
Applies the fix by @chigkim to upstream by introducing a CLI argument.
CHANGELOG: Accessability: Add
--accessibleCLI argument to enable alternative, VoiceOver compatible server listContext: 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