Skip to content

Try to fix QTreeWidget accessibility bug #3573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ann0see wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Try to fix QTreeWidget accessibility bug #3573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d148bc1
Try to fix QTreeWidget accessibility bug
ann0see Dec 6, 2025
46c45a3
Add bare bones, AI generated accessibility button
ann0see Dec 6, 2025
867e87c
Clean up probably invalid text
ann0see Dec 6, 2025
6101375
Fix bug
ann0see Dec 6, 2025
e6d543a
Revert "Fix bug"
ann0see Dec 6, 2025
1d39c1d
Revert "Clean up probably invalid text"
ann0see Dec 6, 2025
2ba71f7
Simplify text
ann0see Dec 6, 2025
0be5b53
Move around layout for more intuitive screen reader use
ann0see Dec 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
284 changes: 284 additions & 0 deletions src/connectdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,90 @@ CConnectDlg::CConnectDlg ( CClientSettings* pNSetP, const bool bNewShowCompleteR
// per default the root shall not be decorated (to save space)
lvwServers->setRootIsDecorated ( false );

#ifdef USE_ACCESSIBLE_SERVER_LIST
Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd need to track a change in the setting, though.

Copy link
Member Author

@ann0see ann0see Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I was also not sure if it should or should not be. Best case, it's just a button/checkbox.

// Create simplified accessible navigation panel for screen readers
// Design: One info label + 2 navigation buttons (Previous/Next) + Toggle button
Copy link
Member Author

@ann0see ann0see Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add screenshot of macOS version


wAccessibleNavPanel = new QWidget ( this );
wAccessibleNavPanel->setObjectName ( "wAccessibleNavPanel" );

QVBoxLayout* accessibleMainLayout = new QVBoxLayout ( wAccessibleNavPanel );
accessibleMainLayout->setContentsMargins ( 0, 5, 0, 5 );

// Create horizontal layout for navigation buttons (Previous, Next)
QHBoxLayout* navLayout = new QHBoxLayout();

butAccessiblePrevious = new QPushButton ( u8"\u2190 " + tr ( "Previous Server" ), wAccessibleNavPanel );
Copy link
Member Author

@ann0see ann0see Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unicode magic is not nice.

Copy link
Member Author

@ann0see ann0see Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this does not go to the previous server, but rather to the previous entry - so it could also be a musician.

Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ann0see ann0see Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we even need those characters.

butAccessiblePrevious->setObjectName ( "butAccessiblePrevious" );
butAccessiblePrevious->setAccessibleName ( tr ( "Go to previous server" ) );
Copy link
Collaborator

@pljones pljones Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, "Previous Button" and "Next Button" would be better for these, I think.

butAccessiblePrevious->setAccessibleDescription ( tr ( "Navigate to the previous server in the list" ) );
Copy link
Collaborator

@pljones pljones Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

butAccessiblePrevious->setShortcut ( QKeySequence ( Qt::ALT | Qt::Key_Up ) );
butAccessiblePrevious->setToolTip ( tr ( "Previous server (Alt+Up)" ) );
navLayout->addWidget ( butAccessiblePrevious, 1 );

butAccessibleNext = new QPushButton ( tr ( "Next Server" ) + u8" \u2192", wAccessibleNavPanel );
butAccessibleNext->setObjectName ( "butAccessibleNext" );
butAccessibleNext->setAccessibleName ( tr ( "Go to next server" ) );
butAccessibleNext->setAccessibleDescription ( tr ( "Navigate to the next server in the list" ) );
butAccessibleNext->setShortcut ( QKeySequence ( Qt::ALT | Qt::Key_Down ) );
butAccessibleNext->setToolTip ( tr ( "Next server (Alt+Down)" ) );
navLayout->addWidget ( butAccessibleNext, 1 );

accessibleMainLayout->addLayout ( navLayout );

// Create read-only, focusable label showing current server information
// Using QLabel with focus policy so screenreaders can read it
lblAccessibleServerInfo = new QLabel ( tr ( "No server selected" ), wAccessibleNavPanel );
lblAccessibleServerInfo->setObjectName ( "lblAccessibleServerInfo" );
lblAccessibleServerInfo->setWordWrap ( true );
lblAccessibleServerInfo->setFrameStyle ( QFrame::Panel | QFrame::Sunken );
lblAccessibleServerInfo->setTextInteractionFlags ( Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard );
lblAccessibleServerInfo->setMinimumHeight ( 50 );
lblAccessibleServerInfo->setFocusPolicy ( Qt::StrongFocus ); // Make it focusable for screen readers
lblAccessibleServerInfo->setAccessibleName ( tr ( "Current server information" ) );
lblAccessibleServerInfo->setAccessibleDescription ( tr ( "Shows details of the currently selected server. Use Previous/Next buttons or Alt+Up/Down to navigate." ) );
accessibleMainLayout->addWidget ( lblAccessibleServerInfo );

// Create toggle button
butToggleAccessible = new QPushButton ( u8"\u25BC " + tr ( "Hide Accessible Controls" ), this );
butToggleAccessible->setObjectName ( "butToggleAccessible" );
butToggleAccessible->setAccessibleName ( tr ( "Toggle accessible controls" ) );
butToggleAccessible->setAccessibleDescription ( tr ( "Show or hide the accessible navigation panel for screen readers" ) );
butToggleAccessible->setCheckable ( true );
butToggleAccessible->setChecked ( true );
butToggleAccessible->setToolTip ( tr ( "Toggle accessible controls" ) );

// Insert the accessible panel and toggle button into the layout right after the tree widget
QVBoxLayout* mainLayout = qobject_cast<QVBoxLayout*> ( layout() );
if ( mainLayout )
{
// Find the tree widget in the layout
bool inserted = false;
for ( int i = 0; i < mainLayout->count(); ++i )
{
QLayoutItem* item = mainLayout->itemAt ( i );
if ( item && item->widget() == lvwServers )
{
mainLayout->insertWidget ( i + 1, butToggleAccessible );
mainLayout->insertWidget ( i + 2, wAccessibleNavPanel );
inserted = true;
break;
}
}
if ( !inserted )
{
qWarning ( "Accessible navigation panel could not be inserted: tree widget not found in layout." );
}
}
else
{
qWarning ( "Accessible navigation panel could not be inserted: main layout cast failed." );
}

// Initially show the accessible panel
wAccessibleNavPanel->setVisible ( true );
Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the setting, you'd set to false immediate after creating and based on the setting here.

#endif

// make sure the connect button has the focus
butConnect->setFocus();

Expand Down Expand Up @@ -243,6 +327,9 @@ CConnectDlg::CConnectDlg ( CClientSettings* pNSetP, const bool bNewShowCompleteR
// to get default return key behaviour working
QObject::connect ( lvwServers, &QTreeWidget::activated, this, &CConnectDlg::OnConnectClicked );

// connect selection change for accessibility support
QObject::connect ( lvwServers, &QTreeWidget::itemSelectionChanged, this, &CConnectDlg::OnServerListItemSelectionChanged );

// line edit
QObject::connect ( edtFilter, &QLineEdit::textEdited, this, &CConnectDlg::OnFilterTextEdited );

Expand All @@ -266,6 +353,13 @@ CConnectDlg::CConnectDlg ( CClientSettings* pNSetP, const bool bNewShowCompleteR
QObject::connect ( &TimerPing, &QTimer::timeout, this, &CConnectDlg::OnTimerPing );

QObject::connect ( &TimerReRequestServList, &QTimer::timeout, this, &CConnectDlg::OnTimerReRequestServList );

#ifdef USE_ACCESSIBLE_SERVER_LIST
Copy link
Collaborator

@pljones pljones Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the widget exists (even if not shown), these need not be compile-time.

// accessible navigation panel
QObject::connect ( butAccessiblePrevious, &QPushButton::clicked, this, &CConnectDlg::OnAccessiblePreviousClicked );
QObject::connect ( butAccessibleNext, &QPushButton::clicked, this, &CConnectDlg::OnAccessibleNextClicked );
QObject::connect ( butToggleAccessible, &QPushButton::clicked, this, &CConnectDlg::OnToggleAccessibleClicked );
#endif
}

void CConnectDlg::showEvent ( QShowEvent* )
Expand Down Expand Up @@ -631,6 +725,196 @@ void CConnectDlg::OnServerAddrEditTextChanged ( const QString& )
lvwServers->clearSelection();
}

void CConnectDlg::OnServerListItemSelectionChanged()
{
#ifdef USE_ACCESSIBLE_SERVER_LIST
UpdateAccessibleServerInfo();
#endif
}

#ifdef USE_ACCESSIBLE_SERVER_LIST
void CConnectDlg::UpdateAccessibleServerInfo()
Copy link
Collaborator

@pljones pljones Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests showing:

  1. Empty filter, Show all musicians off
Image 2. Empty filter, Show all musicians on Image 3. Occupied servers filter on, Show all musicians off Image 3. Occupied servers filter on, Show all musicians on Image

{
QList<QTreeWidgetItem*> selectedItems = lvwServers->selectedItems();

if ( !selectedItems.isEmpty() && selectedItems.first()->parent() == nullptr )
Copy link
Collaborator

@pljones pljones Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to comment this to explain it has to handle Server entries and Client entries separately.

{
// We have a server item selected (not a musician child item)
QTreeWidgetItem* pItem = selectedItems.first();

// Extract server information
QString serverName = pItem->text ( LVC_NAME );
QString pingTime = pItem->text ( LVC_PING );
QString musicians = pItem->text ( LVC_CLIENTS );
QString location = pItem->text ( LVC_LOCATION );
QString version = pItem->text ( LVC_VERSION );

// Build text for screen readers
QString accessibleText = tr ( "Server: %1" ).arg ( serverName );
if ( !pingTime.isEmpty() )
{
accessibleText += tr ( ", Ping: %1" ).arg ( pingTime );
}
if ( !musicians.isEmpty() )
{
accessibleText += tr ( ", Musicians: %1" ).arg ( musicians );
}
if ( !location.isEmpty() )
{
accessibleText += tr ( ", Location: %1" ).arg ( location );
}
if ( !version.isEmpty() )
{
accessibleText += tr ( ", Version: %1" ).arg ( version );
}

// Update label
lblAccessibleServerInfo->setText ( accessibleText );
lblAccessibleServerInfo->setAccessibleName ( tr ( "Selected server information" ) );
lblAccessibleServerInfo->setAccessibleDescription ( accessibleText );

// Update navigation buttons to show previous/next server names
int currentIndex = lvwServers->indexOfTopLevelItem ( pItem );

// Update "Previous" button with previous server name
if ( currentIndex > 0 )
{
QTreeWidgetItem* prevItem = lvwServers->topLevelItem ( currentIndex - 1 );
QString prevName = prevItem->text ( LVC_NAME );
butAccessiblePrevious->setText ( u8"\u2190 " + prevName );
butAccessiblePrevious->setAccessibleName ( tr ( "Previous server: %1" ).arg ( prevName ) );
butAccessiblePrevious->setAccessibleDescription ( tr ( "Go to previous server: %1" ).arg ( prevName ) );
butAccessiblePrevious->setEnabled ( true );
}
else
{
butAccessiblePrevious->setText ( u8"\u2190 " + tr ( "(first)" ) );
butAccessiblePrevious->setAccessibleName ( tr ( "No previous server - at first server" ) );
butAccessiblePrevious->setAccessibleDescription ( tr ( "Cannot go back, already at first server" ) );
butAccessiblePrevious->setEnabled ( false );
}

// Update "Next" button with next server name
if ( currentIndex < lvwServers->topLevelItemCount() - 1 )
{
QTreeWidgetItem* nextItem = lvwServers->topLevelItem ( currentIndex + 1 );
QString nextName = nextItem->text ( LVC_NAME );
butAccessibleNext->setText ( nextName + u8" \u2192" );
butAccessibleNext->setAccessibleName ( tr ( "Next server: %1" ).arg ( nextName ) );
butAccessibleNext->setAccessibleDescription ( tr ( "Go to next server: %1" ).arg ( nextName ) );
butAccessibleNext->setEnabled ( true );
}
else
{
butAccessibleNext->setText ( tr ( "(last)" ) + u8" \u2192" );
butAccessibleNext->setAccessibleName ( tr ( "No next server - at last server" ) );
butAccessibleNext->setAccessibleDescription ( tr ( "Cannot go forward, already at last server" ) );
butAccessibleNext->setEnabled ( false );
}

// Force VoiceOver to announce the change
QAccessible::updateAccessibility ( new QAccessibleValueChangeEvent ( lblAccessibleServerInfo, accessibleText ) );
}
else
{
// Reset navigation buttons
butAccessiblePrevious->setText ( u8"\u2190 " + tr ( "Previous" ) );
butAccessiblePrevious->setAccessibleName ( tr ( "Navigate to previous server" ) );
butAccessiblePrevious->setEnabled ( lvwServers->topLevelItemCount() > 0 );

butAccessibleNext->setText ( tr ( "Next" ) + u8" \u2192" );
butAccessibleNext->setAccessibleName ( tr ( "Navigate to next server" ) );
butAccessibleNext->setEnabled ( lvwServers->topLevelItemCount() > 0 );

// No server selected or musician child selected
lblAccessibleServerInfo->setText ( tr ( "<i>No server selected or in musician selection</i>" ) );
lblAccessibleServerInfo->setAccessibleName ( tr ( "No server selected or in musician selection" ) );
lblAccessibleServerInfo->setAccessibleDescription ( tr ( "No server selected. Use Previous/Next buttons or Alt+Up/Down to navigate servers." ) );

}
}

void CConnectDlg::OnAccessiblePreviousClicked()
Copy link
Collaborator

@pljones pljones Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these will trigger UpdateAccessibleServerInfo through

QObject::connect ( lvwServers, &QTreeWidget::itemSelectionChanged, this, &CConnectDlg::OnServerListItemSelectionChanged );

Copy link
Collaborator

@pljones pljones Dec 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
// Navigate to previous server
QList<QTreeWidgetItem*> selectedItems = lvwServers->selectedItems();
QTreeWidgetItem* currentItem = selectedItems.isEmpty() ? nullptr : selectedItems.first();

// Get current item or first item if none selected
if ( currentItem == nullptr )
{
// Select first item
if ( lvwServers->topLevelItemCount() > 0 )
{
lvwServers->setCurrentItem ( lvwServers->topLevelItem ( 0 ) );
}
return;
}

// If current item is a musician (child), move to parent
if ( currentItem->parent() != nullptr )
{
lvwServers->setCurrentItem ( currentItem->parent() );
return;
}

// Get previous server item
int currentIndex = lvwServers->indexOfTopLevelItem ( currentItem );
if ( currentIndex > 0 )
{
lvwServers->setCurrentItem ( lvwServers->topLevelItem ( currentIndex - 1 ) );
}
}

void CConnectDlg::OnAccessibleNextClicked()
{
// Navigate to next server
QList<QTreeWidgetItem*> selectedItems = lvwServers->selectedItems();
QTreeWidgetItem* currentItem = selectedItems.isEmpty() ? nullptr : selectedItems.first();

// Get current item or first item if none selected
if ( currentItem == nullptr )
{
// Select first item
if ( lvwServers->topLevelItemCount() > 0 )
{
lvwServers->setCurrentItem ( lvwServers->topLevelItem ( 0 ) );
}
return;
}

// If current item is a musician (child), find next server
if ( currentItem->parent() != nullptr )
{
currentItem = currentItem->parent();
}

// Find next server item (skip musician children)
int currentIndex = lvwServers->indexOfTopLevelItem ( currentItem );
if ( currentIndex >= 0 && currentIndex < lvwServers->topLevelItemCount() - 1 )
{
lvwServers->setCurrentItem ( lvwServers->topLevelItem ( currentIndex + 1 ) );
}
}

void CConnectDlg::OnToggleAccessibleClicked()
{
bool isVisible = wAccessibleNavPanel->isVisible();
wAccessibleNavPanel->setVisible ( !isVisible );

if ( isVisible )
{
butToggleAccessible->setText ( u8"\u25B6 " + tr ( "Show Accessible Controls" ) );
butToggleAccessible->setAccessibleDescription ( tr ( "Show the accessible navigation controls" ) );
}
else
{
butToggleAccessible->setText ( u8"\u25BC " + tr ( "Hide Accessible Controls" ) );
butToggleAccessible->setAccessibleDescription ( tr ( "Hide the accessible navigation controls" ) );
}
}
#endif

void CConnectDlg::OnCustomDirectoriesChanged()
{

Expand Down
28 changes: 28 additions & 0 deletions src/connectdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@

#pragma once

// Enable accessible server list by default (can be disabled with -DDISABLE_ACCESSIBLE_SERVER_LIST)
#ifndef DISABLE_ACCESSIBLE_SERVER_LIST
# define USE_ACCESSIBLE_SERVER_LIST
#endif

#include <QLabel>
#include <QLineEdit>
#include <QPushButton>
Expand All @@ -32,6 +37,9 @@
#include <QLocale>
#include <QtConcurrent>
#include <QRegularExpression>
#include <QAccessible>
#include <QHBoxLayout>
#include <QVBoxLayout>
#include "global.h"
#include "util.h"
#include "settings.h"
Expand Down Expand Up @@ -105,8 +113,21 @@ class CConnectDlg : public CBaseDlg, private Ui_CConnectDlgBase
void EmitCLServerListPingMes ( const CHostAddress& haServerAddress, const bool bNeedVersion );
void UpdateDirectoryComboBox();

#ifdef USE_ACCESSIBLE_SERVER_LIST
void UpdateAccessibleServerInfo();
#endif

CClientSettings* pSettings;

#ifdef USE_ACCESSIBLE_SERVER_LIST
// Accessible navigation panel for screen readers
QWidget* wAccessibleNavPanel; // Container for accessible navigation
QLabel* lblAccessibleServerInfo; // Label showing current server information (read-only)
QPushButton* butAccessiblePrevious; // Button to navigate to previous server
QPushButton* butAccessibleNext; // Button to navigate to next server
QPushButton* butToggleAccessible; // Button to show/hide accessible panel
#endif

QTimer TimerPing;
QTimer TimerReRequestServList;
QTimer TimerInitialSort;
Expand All @@ -132,6 +153,13 @@ public slots:
void OnDeleteServerAddrClicked();
void OnTimerPing();
void OnTimerReRequestServList();
void OnServerListItemSelectionChanged();

#ifdef USE_ACCESSIBLE_SERVER_LIST
void OnAccessiblePreviousClicked();
void OnAccessibleNextClicked();
void OnToggleAccessibleClicked();
#endif

signals:
void ReqServerListQuery ( CHostAddress InetAddr );
Expand Down
Loading