From c4ccc39aac41a99bd62efd29aedbb52f4ecc4861 Mon Sep 17 00:00:00 2001 From: Storm Dragon Date: Tue, 15 Sep 2026 14:33:20 -0400 Subject: [PATCH] Accessibility improvements. --- distro-packages/ArchLinux/navipy-git/PKGBUILD | 2 +- src/accessibility/accessible_tree.py | 182 +++++------------- src/main_window.py | 10 +- src/widgets/search_dialog.py | 1 - tests/test_accessible_tree.py | 130 +++++++++++++ 5 files changed, 186 insertions(+), 139 deletions(-) create mode 100644 tests/test_accessible_tree.py diff --git a/distro-packages/ArchLinux/navipy-git/PKGBUILD b/distro-packages/ArchLinux/navipy-git/PKGBUILD index 3c1d632..5f26c84 100644 --- a/distro-packages/ArchLinux/navipy-git/PKGBUILD +++ b/distro-packages/ArchLinux/navipy-git/PKGBUILD @@ -1,6 +1,6 @@ # Maintainer: Storm Dragon pkgname=navipy-git -pkgver=r8.gd5545da +pkgver=r9.gd2e68bd pkgrel=1 pkgdesc='Accessible Subsonic desktop client tested with Navidrome' arch=('any') diff --git a/src/accessibility/accessible_tree.py b/src/accessibility/accessible_tree.py index 7c1618d..fdb9611 100644 --- a/src/accessibility/accessible_tree.py +++ b/src/accessibility/accessible_tree.py @@ -3,7 +3,6 @@ Accessible tree widget for hierarchical navigation Based on the implementation from Bifrost """ -import re from PySide6.QtWidgets import QTreeWidget, QTreeWidgetItem from PySide6.QtCore import Qt, Signal from PySide6.QtGui import QKeyEvent @@ -63,12 +62,12 @@ class AccessibleTreeWidget(QTreeWidget): if current.childCount() > 0: if not current.isExpanded(): self.expandItem(current) - self.updateChildAccessibility(current, True) return # If already expanded and no shift, move to first child elif not hasShift: - firstChild = current.child(0) - if firstChild and self.isItemNavigable(firstChild): + firstChild = next((current.child(i) for i in range(current.childCount()) + if self.isItemNavigable(current.child(i))), None) + if firstChild: self.setCurrentItem(firstChild) self.scrollToItem(firstChild) return @@ -85,9 +84,6 @@ class AccessibleTreeWidget(QTreeWidget): # Plain Left: Collapse current item if expanded, otherwise move to parent if current.childCount() > 0 and current.isExpanded(): self.collapseItem(current) - self.updateChildAccessibility(current, False) - self.repaint() - self.update() return elif current.parent(): parentItem = current.parent() @@ -139,7 +135,7 @@ class AccessibleTreeWidget(QTreeWidget): elif key == Qt.Key_Home: # Go to first item - firstItem = self.topLevelItem(0) + firstItem = self.getFirstVisibleItem() if firstItem: self.setCurrentItem(firstItem) self.scrollToItem(firstItem) @@ -159,135 +155,54 @@ class AccessibleTreeWidget(QTreeWidget): super().keyPressEvent(event) def getNextVisibleItem(self, item: QTreeWidgetItem) -> QTreeWidgetItem: - """Get the next visible item in the tree""" - if not item: - return None - - # If item has children, check if first child is navigable - if item.childCount() > 0: - firstChild = item.child(0) - if firstChild and self.isItemNavigable(firstChild): - return firstChild - - # Otherwise, find next sibling or ancestor's sibling - current = item - while current: - parent = current.parent() - if parent: - # Find next sibling - index = parent.indexOfChild(current) - if index + 1 < parent.childCount(): - nextSibling = parent.child(index + 1) - if nextSibling and self.isItemNavigable(nextSibling): - return nextSibling - # No more siblings, go up to parent - current = parent - else: - # Top-level item, find next top-level item - index = self.indexOfTopLevelItem(current) - if index + 1 < self.topLevelItemCount(): - nextItem = self.topLevelItem(index + 1) - if nextItem and self.isItemNavigable(nextItem): - return nextItem - # No more items - return None - return None + """Follow Qt's visible row order, skipping unavailable items.""" + candidate = self.itemBelow(item) if item else None + while candidate and not self.isItemNavigable(candidate): + candidate = self.itemBelow(candidate) + return candidate def getPreviousVisibleItem(self, item: QTreeWidgetItem) -> QTreeWidgetItem: - """Get the previous visible item in the tree""" - if not item: - return None + """Follow Qt's visible row order backwards.""" + candidate = self.itemAbove(item) if item else None + while candidate and not self.isItemNavigable(candidate): + candidate = self.itemAbove(candidate) + return candidate - parent = item.parent() - - if parent: - # Find previous sibling - index = parent.indexOfChild(item) - if index > 0: - # Get previous sibling and its last visible descendant - prevSibling = parent.child(index - 1) - if prevSibling and self.isItemNavigable(prevSibling): - return self.getLastVisibleDescendant(prevSibling) - else: - # No previous sibling, go to parent - if self.isItemNavigable(parent): - return parent - else: - # Top-level item - index = self.indexOfTopLevelItem(item) - if index > 0: - # Get previous top-level item and its last visible descendant - prevItem = self.topLevelItem(index - 1) - if prevItem and self.isItemNavigable(prevItem): - return self.getLastVisibleDescendant(prevItem) + def getFirstVisibleItem(self) -> QTreeWidgetItem: + for i in range(self.topLevelItemCount()): + item = self.topLevelItem(i) + if not item.isHidden(): + return item if self.isItemNavigable(item) else self.getNextVisibleItem(item) return None - def getLastVisibleDescendant(self, item: QTreeWidgetItem) -> QTreeWidgetItem: - """Get the last visible descendant of an item""" - if not item or not self.isItemNavigable(item): - return None - - # Check if any children are navigable - if item.childCount() > 0: - # Get the last child and check if it's navigable - lastChild = item.child(item.childCount() - 1) - if lastChild and self.isItemNavigable(lastChild): - result = self.getLastVisibleDescendant(lastChild) - return result if result else item + def getLastVisibleItem(self) -> QTreeWidgetItem: + item = self.getFirstVisibleItem() + while item: + following = self.getNextVisibleItem(item) + if following is None: + break + item = following return item - def getLastVisibleItem(self) -> QTreeWidgetItem: - """Get the last visible item in the tree""" - if self.topLevelItemCount() == 0: - return None - lastTopLevel = self.topLevelItem(self.topLevelItemCount() - 1) - return self.getLastVisibleDescendant(lastTopLevel) - def onItemExpanded(self, item: QTreeWidgetItem): - """Handle item expansion""" - self.announceItemState(item, "expanded") - # Make child items accessible - self.updateChildAccessibility(item, True) + # Qt exposes expansion state and accessibility events itself. Changing + # the name here loses custom labels and leaves stale lazy-load counts. + self.itemStateChanged.emit(item, "expanded") def onItemCollapsed(self, item: QTreeWidgetItem): - """Handle item collapse""" - self.announceItemState(item, "collapsed") - # Hide child items from screen readers - self.updateChildAccessibility(item, False) - - def updateChildAccessibility(self, item: QTreeWidgetItem, visible: bool): - """Update accessibility properties of child items""" - for i in range(item.childCount()): - child = item.child(i) - if visible: - # Make child accessible and ensure proper selection - child.setFlags(child.flags() | Qt.ItemIsEnabled | Qt.ItemIsSelectable) - child.setData(0, Qt.AccessibleDescriptionRole, "") # Clear hidden marker - # Recursively handle nested children - self.updateChildAccessibility(child, child.isExpanded()) - else: - # Hide from screen readers and navigation - child.setData(0, Qt.AccessibleDescriptionRole, "hidden") - # Remove selectable flag - collapsed items should not be navigable - child.setFlags((child.flags() | Qt.ItemIsEnabled) & ~Qt.ItemIsSelectable) - # Hide all nested children too - self.updateChildAccessibility(child, False) - - def announceItemState(self, item: QTreeWidgetItem, state: str): - """Announce item state change for screen readers""" - # Update accessible text to include state - if item.childCount() > 0: - originalText = item.text(0) - childCount = item.childCount() - - # Remove any existing state information to prevent duplication - cleanText = re.sub(r' \(\d+ items?, (?:collapsed|expanded)\)$', '', originalText) - - accessibleText = f"{cleanText} ({childCount} items, {state})" - item.setData(0, Qt.AccessibleTextRole, accessibleText) - - # Emit signal for potential sound feedback - self.itemStateChanged.emit(item, state) + """Keep the current item visible when an ancestor is collapsed.""" + current = self.currentItem() + ancestor = current.parent() if current else None + while ancestor: + if ancestor is item: + if self.isItemNavigable(item): + self.setCurrentItem(item) + else: + self.setCurrentItem(self.getNextVisibleItem(item) + or self.getPreviousVisibleItem(item)) + break + ancestor = ancestor.parent() + self.itemStateChanged.emit(item, "collapsed") def getItemDescription(self, item: QTreeWidgetItem) -> str: """Get a full description of an item for screen readers""" @@ -332,9 +247,12 @@ class AccessibleTreeWidget(QTreeWidget): """Check if an item is safe to navigate to""" if not item: return False - # Check if item is enabled and selectable, and not hidden + if item.treeWidget() is not self or item.isHidden(): + return False + ancestor = item.parent() + while ancestor: + if ancestor.isHidden() or not ancestor.isExpanded(): + return False + ancestor = ancestor.parent() flags = item.flags() - isHidden = item.data(0, Qt.AccessibleDescriptionRole) == "hidden" - isEnabled = bool(flags & Qt.ItemIsEnabled) - isSelectable = bool(flags & Qt.ItemIsSelectable) - return isEnabled and isSelectable and not isHidden + return bool(flags & Qt.ItemIsEnabled) and bool(flags & Qt.ItemIsSelectable) diff --git a/src/main_window.py b/src/main_window.py index a8c60e7..8c88ae8 100644 --- a/src/main_window.py +++ b/src/main_window.py @@ -735,30 +735,30 @@ class MainWindow(QMainWindow): # Root sections self.artistsRoot = QTreeWidgetItem(["Artists"]) self.artistsRoot.setData(0, Qt.UserRole, {"type": "artists_root", "loaded": True}) - self.artistsRoot.setExpanded(True) self.libraryTree.addTopLevelItem(self.artistsRoot) + self.artistsRoot.setExpanded(True) self.favoritesRoot = QTreeWidgetItem(["Favorites"]) self.favoritesRoot.setData(0, Qt.UserRole, {"type": "favorites_root", "loaded": True}) - self.favoritesRoot.setExpanded(True) self.libraryTree.addTopLevelItem(self.favoritesRoot) + self.favoritesRoot.setExpanded(True) self._populateFavorites(data.get("favorites", {})) self.discoverRoot = QTreeWidgetItem(["Discover"]) self.discoverRoot.setData(0, Qt.UserRole, {"type": "discover_root", "loaded": True}) - self.discoverRoot.setExpanded(True) self.libraryTree.addTopLevelItem(self.discoverRoot) + self.discoverRoot.setExpanded(True) self.loadDiscoverSections() self.playlistsRoot = QTreeWidgetItem(["Playlists"]) self.playlistsRoot.setData(0, Qt.UserRole, {"type": "playlists_root", "loaded": True}) - self.playlistsRoot.setExpanded(True) self.libraryTree.addTopLevelItem(self.playlistsRoot) + self.playlistsRoot.setExpanded(True) self.genresRoot = QTreeWidgetItem(["Genres"]) self.genresRoot.setData(0, Qt.UserRole, {"type": "genres_root", "loaded": True}) - self.genresRoot.setExpanded(True) self.libraryTree.addTopLevelItem(self.genresRoot) + self.genresRoot.setExpanded(True) self._populateArtists(data.get("artists", [])) self._populatePlaylists(data.get("playlists", [])) diff --git a/src/widgets/search_dialog.py b/src/widgets/search_dialog.py index 3a2a478..5bffedc 100644 --- a/src/widgets/search_dialog.py +++ b/src/widgets/search_dialog.py @@ -301,7 +301,6 @@ class SearchDialog(QDialog): item.addChild(child) data.update(loaded=True, loading=False) item.setData(0, Qt.UserRole, data) - self.resultsTree.updateChildAccessibility(item, item.isExpanded()) def failed(error): data["loading"] = False diff --git a/tests/test_accessible_tree.py b/tests/test_accessible_tree.py new file mode 100644 index 0000000..431460f --- /dev/null +++ b/tests/test_accessible_tree.py @@ -0,0 +1,130 @@ +"""Tree navigation and accessibility regressions, without a server or audio.""" +import pytest +from PySide6.QtCore import Qt +from PySide6.QtGui import QAccessible +from PySide6.QtTest import QTest +from PySide6.QtWidgets import QApplication, QTreeWidgetItem + +from src.accessibility.accessible_tree import AccessibleTreeWidget + + +@pytest.fixture +def tree(): + app = QApplication.instance() or QApplication([]) + widget = AccessibleTreeWidget() + widget.setHeaderHidden(True) + widget.show() + app.processEvents() + yield widget + widget.close() + + +def add(parent, text): + item = QTreeWidgetItem([text]) + if isinstance(parent, AccessibleTreeWidget): + parent.addTopLevelItem(item) + else: + parent.addChild(item) + return item + + +def press(tree, key, expected): + QTest.keyClick(tree, key) + assert tree.currentItem() is expected + + +def test_collapsed_children_and_late_results_are_not_navigable(tree): + root = add(tree, 'Albums') + child = add(root, 'Album') + last = add(tree, 'Playlists') + tree.setCurrentItem(root) + press(tree, Qt.Key_Down, last) + press(tree, Qt.Key_Up, root) + press(tree, Qt.Key_Right, root) + press(tree, Qt.Key_Right, child) + root.setExpanded(False) + assert tree.currentItem() is root + late = add(root, 'Loaded after collapse') + assert not tree.isItemNavigable(late) + press(tree, Qt.Key_Down, last) + root.setExpanded(True) + assert tree.isItemNavigable(late) + + +def test_hidden_rows_are_skipped_in_both_directions_and_at_edges(tree): + hidden_first = add(tree, 'Hidden first') + hidden_first.setHidden(True) + root = add(tree, 'Root') + hidden_child = add(root, 'Hidden child') + hidden_child.setHidden(True) + child = add(root, 'Visible child') + disabled = add(root, 'Disabled') + disabled.setDisabled(True) + other = add(root, 'Other child') + hidden_last = add(tree, 'Hidden last') + hidden_last.setHidden(True) + root.setExpanded(True) + tree.setCurrentItem(root) + press(tree, Qt.Key_Right, child) + press(tree, Qt.Key_Down, other) + press(tree, Qt.Key_Up, child) + press(tree, Qt.Key_Home, root) + press(tree, Qt.Key_End, other) + press(tree, Qt.Key_PageUp, root) + press(tree, Qt.Key_PageDown, other) + + +def test_expansion_preserves_names_descriptions_and_flags(tree): + root = add(tree, 'Artist') + root.setData(0, Qt.AccessibleTextRole, 'Artist, 12 albums') + child = add(root, 'Album') + child.setData(0, Qt.AccessibleDescriptionRole, 'Released in 2024') + child.setFlags(child.flags() & ~Qt.ItemIsSelectable) + disabled = add(root, 'Unavailable') + disabled.setDisabled(True) + original_flags = [child.flags(), disabled.flags()] + states = [] + tree.itemStateChanged.connect(lambda item, state: states.append(state)) + for _ in range(2): + root.setExpanded(True) + root.setExpanded(False) + assert root.data(0, Qt.AccessibleTextRole) == 'Artist, 12 albums' + assert child.data(0, Qt.AccessibleDescriptionRole) == 'Released in 2024' + assert [child.flags(), disabled.flags()] == original_flags + assert states == ['expanded', 'collapsed'] * 2 + + +def test_nested_collapsed_branch_stays_closed_on_parent_reopen(tree): + root = add(tree, 'Root') + branch = add(root, 'Branch') + leaf = add(branch, 'Leaf') + root.setExpanded(True) + branch.setExpanded(True) + tree.setCurrentItem(leaf) + branch.setExpanded(False) + assert tree.currentItem() is branch + root.setExpanded(False) + root.setExpanded(True) + assert not tree.isItemNavigable(leaf) + tree.setCurrentItem(branch) + press(tree, Qt.Key_End, branch) + + +def test_accessibility_exposes_only_expanded_rows(tree): + root = add(tree, 'Albums') + add(root, 'Song') + tree.setCurrentItem(root) + QApplication.processEvents() + interface = QAccessible.queryAccessibleInterface(tree) + def spoken_rows(): + return [interface.child(i).text(QAccessible.Name) + for i in range(interface.childCount()) + if interface.child(i).role() == QAccessible.TreeItem] + + assert spoken_rows() == ['Albums'] + root.setExpanded(True) + QApplication.processEvents() + assert spoken_rows() == ['Albums', 'Song'] + root.setExpanded(False) + QApplication.processEvents() + assert spoken_rows() == ['Albums']