Accessibility improvements.

distro-packages/ArchLinux/navipy-git/PKGBUILD

@@ -1,6 +1,6 @@
 # Maintainer: Storm Dragon <storm_dragon@stormux.org>
 pkgname=navipy-git
-pkgver=r8.gd5545da
+pkgver=r9.gd2e68bd
 pkgrel=1
 pkgdesc='Accessible Subsonic desktop client tested with Navidrome'
 arch=('any')

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:
+        """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
+
+    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
 
-        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)
-        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
-        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)
+        item = self.getFirstVisibleItem()
+        while item:
+            following = self.getNextVisibleItem(item)
+            if following is None:
+                break
+            item = following
+        return item
 
     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())
+        """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:
-                # 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)
+                    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)

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", []))

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

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']