Try to fix QTreeWidget accessibility bug #3573
Conversation
Co-authored-by: GitHub Copilot
Co-authored-by: Copilot
|
|
||
| #ifdef USE_ACCESSIBLE_SERVER_LIST | ||
| // Create simplified accessible navigation panel for screen readers | ||
| // Design: One info label + 2 navigation buttons (Previous/Next) + Toggle button |
There was a problem hiding this comment.
Will add screenshot of macOS version
| // Create horizontal layout for navigation buttons (Previous, Next) | ||
| QHBoxLayout* navLayout = new QHBoxLayout(); | ||
|
|
||
| butAccessiblePrevious = new QPushButton ( u8"\u2190 " + tr ( "Previous Server" ), wAccessibleNavPanel ); |
There was a problem hiding this comment.
Unicode magic is not nice.
There was a problem hiding this comment.
Also, this does not go to the previous server, but rather to the previous entry - so it could also be a musician.
There was a problem hiding this comment.
As a minimum, the unicode characters want putting into constants, named so someone reading the code gets some idea of what they represent when rendered.
There was a problem hiding this comment.
Not sure if we even need those characters.
| // per default the root shall not be decorated (to save space) | ||
| lvwServers->setRootIsDecorated ( false ); | ||
|
|
||
| #ifdef USE_ACCESSIBLE_SERVER_LIST |
There was a problem hiding this comment.
So this will be an additional build. Will it be included in the macOS bundle so it all gets signed once in the Apple Store?
There was a problem hiding this comment.
Actually, does this need to be a compile time option? Just have the accessibility panel hidden unless turned on in Settings->User Profile->User Interface.
There was a problem hiding this comment.
You'd need to track a change in the setting, though.
There was a problem hiding this comment.
Agree. I was also not sure if it should or should not be. Best case, it's just a button/checkbox.
| } | ||
|
|
||
| // Initially show the accessible panel | ||
| wAccessibleNavPanel->setVisible ( true ); |
There was a problem hiding this comment.
Using the setting, you'd set to false immediate after creating and based on the setting here.
|
|
||
| QObject::connect ( &TimerReRequestServList, &QTimer::timeout, this, &CConnectDlg::OnTimerReRequestServList ); | ||
|
|
||
| #ifdef USE_ACCESSIBLE_SERVER_LIST |
There was a problem hiding this comment.
If the widget exists (even if not shown), these need not be compile-time.
|
|
||
| butAccessiblePrevious = new QPushButton ( u8"\u2190 " + tr ( "Previous Server" ), wAccessibleNavPanel ); | ||
| butAccessiblePrevious->setObjectName ( "butAccessiblePrevious" ); | ||
| butAccessiblePrevious->setAccessibleName ( tr ( "Go to previous server" ) ); |
There was a problem hiding this comment.
Yeah, "Previous Button" and "Next Button" would be better for these, I think.
| butAccessiblePrevious = new QPushButton ( u8"\u2190 " + tr ( "Previous Server" ), wAccessibleNavPanel ); | ||
| butAccessiblePrevious->setObjectName ( "butAccessiblePrevious" ); | ||
| butAccessiblePrevious->setAccessibleName ( tr ( "Go to previous server" ) ); | ||
| butAccessiblePrevious->setAccessibleDescription ( tr ( "Navigate to the previous server in the list" ) ); |
There was a problem hiding this comment.
Then these can be the more vague but more accurate "Moves to the previous server list entry" and "Moves to the next server list entry"
| { | ||
| QList<QTreeWidgetItem*> selectedItems = lvwServers->selectedItems(); | ||
|
|
||
| if ( !selectedItems.isEmpty() && selectedItems.first()->parent() == nullptr ) |
There was a problem hiding this comment.
Need to comment this to explain it has to handle Server entries and Client entries separately.
| } | ||
| } | ||
|
|
||
| void CConnectDlg::OnAccessiblePreviousClicked() |
There was a problem hiding this comment.
So these will trigger UpdateAccessibleServerInfo through
QObject::connect ( lvwServers, &QTreeWidget::itemSelectionChanged, this, &CConnectDlg::OnServerListItemSelectionChanged );
There was a problem hiding this comment.
I don't think they should skip Clients -- it needs to step through all entries in the list, otherwise the list isn't fully accessible.
| } | ||
|
|
||
| #ifdef USE_ACCESSIBLE_SERVER_LIST | ||
| void CConnectDlg::UpdateAccessibleServerInfo() |
There was a problem hiding this comment.
Needs tests showing:
- Empty filter, Show all musicians off
2. Empty filter, Show all musicians on
3. Occupied servers filter on, Show all musicians off
3. Occupied servers filter on, Show all musicians on
2 participants
Co-authored-by: GitHub Copilot
This might contain a lot of low quality code for now.
How it looks like:
There is a "Show/Hide accessible controls button" at the top which shows/hides the previous/next buttons + the current server info label.
Short description of changes
Adds forward/backward buttons and label to connect dialog for accessability
Copilot's suggestion to fix: #3559 (together with some tweaking)
CHANGELOG: Accessability: Add buttons and label to go through server list
Context: Fixes an issue?
Yes: #490 (workaround)
Does this change need documentation? What needs to be documented and how?
Yes
Status of this Pull Request
Proof of concept. The code is probably extremely convoluted.
What is missing until this pull request can be merged?
Testing, review and simplification.
Checklist
AUTOBUILD: Please build all targets