QTreeView: fix keyboard navigation when first or last item is disabled

The keyboard navigation did not consider the disabled state when trying
to find the new index under all circumstances. This lead to a
non-working PageUp/Down/Home/End navigation when the first or last item
was disabled or hidden.
Fix it by explicitly checking if the calculated item is hidden/enabled
and skip it in this case.

Fixes: QTBUG-44746
Fixes: QTBUG-34832
Change-Id: Ifa3b64a405e67b792db5db9d186d426fcfe183fb
Reviewed-by: David Faure <david.faure@kdab.com>

src/widgets/itemviews/qtreeview.cpp

@@ -2301,9 +2301,9 @@ QModelIndex QTreeView::moveCursor(CursorAction cursorAction, Qt::KeyboardModifie
     case MovePageDown:
         return d->modelIndex(d->pageDown(vi), current.column());
     case MoveHome:
-        return d->model->index(0, current.column(), d->root);
+        return d->modelIndex(d->itemForKeyHome(), current.column());
     case MoveEnd:
-        return d->modelIndex(d->viewItems.count() - 1, current.column());
+        return d->modelIndex(d->itemForKeyEnd(), current.column());
     }
     return current;
 }
@@ -3422,7 +3422,11 @@ int QTreeViewPrivate::pageUp(int i) const
     int index = itemAtCoordinate(coordinateForItem(i) - viewport->height());
     while (isItemHiddenOrDisabled(index))
         index--;
-    return index == -1 ? 0 : index;
+    if (index == -1)
+        index = 0;
+    while (isItemHiddenOrDisabled(index))
+        index++;
+    return index >= viewItems.count() ? 0 : index;
 }
 
 int QTreeViewPrivate::pageDown(int i) const
@@ -3430,6 +3434,26 @@ int QTreeViewPrivate::pageDown(int i) const
     int index = itemAtCoordinate(coordinateForItem(i) + viewport->height());
     while (isItemHiddenOrDisabled(index))
         index++;
+    if (index == -1 || index >= viewItems.count())
+        index = viewItems.count() - 1;
+    while (isItemHiddenOrDisabled(index))
+        index--;
+    return index == -1 ? viewItems.count() - 1 : index;
+}
+
+int QTreeViewPrivate::itemForKeyHome() const
+{
+    int index = 0;
+    while (isItemHiddenOrDisabled(index))
+        index++;
+    return index >= viewItems.count() ? 0 : index;
+}
+
+int QTreeViewPrivate::itemForKeyEnd() const
+{
+    int index = viewItems.count() - 1;
+    while (isItemHiddenOrDisabled(index))
+        index--;
     return index == -1 ? viewItems.count() - 1 : index;
 }
 

src/widgets/itemviews/qtreeview_p.h

@@ -140,6 +140,8 @@ public:
 
     int pageUp(int item) const;
     int pageDown(int item) const;
+    int itemForKeyHome() const;
+    int itemForKeyEnd() const;
 
     int itemHeight(int item) const;
     int indentationForItem(int item) const;

tests/auto/widgets/itemviews/qtreeview/tst_qtreeview.cpp

@@ -4135,6 +4135,30 @@ void tst_QTreeView::keyboardNavigationWithDisabled()
     QCOMPARE(view.currentIndex(), model.index(12, 0));
     QTest::keyClick(view.viewport(), Qt::Key_Up);
     QCOMPARE(view.currentIndex(), model.index(6, 0));
+    // QTBUG-44746 - when first/last item is disabled,
+    // Key_PageUp/Down/Home/End will not work as expected.
+    model.item(0)->setEnabled(false);
+    model.item(1)->setEnabled(true);
+    model.item(2)->setEnabled(true);
+    model.item(model.rowCount() - 1)->setEnabled(false);
+    model.item(model.rowCount() - 2)->setEnabled(true);
+    model.item(model.rowCount() - 3)->setEnabled(true);
+    // PageUp
+    view.setCurrentIndex(model.index(2, 0));
+    QCOMPARE(view.currentIndex(), model.index(2, 0));
+    QTest::keyClick(view.viewport(), Qt::Key_PageUp);
+    QCOMPARE(view.currentIndex(), model.index(1, 0));
+    // PageDown
+    view.setCurrentIndex(model.index(model.rowCount() - 3, 0));
+    QCOMPARE(view.currentIndex(), model.index(model.rowCount() - 3, 0));
+    QTest::keyClick(view.viewport(), Qt::Key_PageDown);
+    QCOMPARE(view.currentIndex(), model.index(model.rowCount() - 2, 0));
+    // Key_Home
+    QTest::keyClick(view.viewport(), Qt::Key_Home);
+    QCOMPARE(view.currentIndex(), model.index(1, 0));
+    // Key_End
+    QTest::keyClick(view.viewport(), Qt::Key_End);
+    QCOMPARE(view.currentIndex(), model.index(model.rowCount() - 2, 0));
 }
 
 class RemoveColumnOne : public QSortFilterProxyModel

tests/auto/widgets/itemviews/qtreewidget/tst_qtreewidget.cpp

@@ -99,6 +99,7 @@ private slots:
     void insertTopLevelItems_data();
     void insertTopLevelItems();
     void keyboardNavigation();
+    void keyboardNavigationWithHidden();
     void scrollToItem();
     void setSortingEnabled();
     void match();
@@ -1575,6 +1576,33 @@ void tst_QTreeWidget::keyboardNavigation()
     }
 }
 
+void tst_QTreeWidget::keyboardNavigationWithHidden()
+{
+    QTreeWidget tw;
+    for (int i = 0; i < 1000; ++i)
+        tw.addTopLevelItem(new QTreeWidgetItem({QString::number(i), QStringLiteral("second col")}));
+    // QTBUG-34832 - when first/last item is hidden,
+    // Key_PageUp/Down/Home/End will not work as expected.
+    tw.topLevelItem(0)->setHidden(true);
+    tw.topLevelItem(tw.model()->rowCount() - 1)->setHidden(true);
+    // PageUp
+    tw.setCurrentIndex(tw.model()->index(2, 0));
+    QCOMPARE(tw.currentIndex(), tw.model()->index(2, 0));
+    QTest::keyClick(tw.viewport(), Qt::Key_PageUp);
+    QCOMPARE(tw.currentIndex(), tw.model()->index(1, 0));
+    // PageDown
+    tw.setCurrentIndex(tw.model()->index(tw.model()->rowCount() - 3, 0));
+    QCOMPARE(tw.currentIndex(), tw.model()->index(tw.model()->rowCount() - 3, 0));
+    QTest::keyClick(tw.viewport(), Qt::Key_PageDown);
+    QCOMPARE(tw.currentIndex(), tw.model()->index(tw.model()->rowCount() - 2, 0));
+    // Key_Home
+    QTest::keyClick(tw.viewport(), Qt::Key_Home);
+    QCOMPARE(tw.currentIndex(), tw.model()->index(1, 0));
+    // Key_End
+    QTest::keyClick(tw.viewport(), Qt::Key_End);
+    QCOMPARE(tw.currentIndex(), tw.model()->index(tw.model()->rowCount() - 2, 0));
+}
+
 void tst_QTreeWidget::scrollToItem()
 {
     // Check if all parent nodes of the item found are expanded.