Fix focus chain with compound widgets if created out of order

When a compound widget is created not directly before its children,
then another widget will be in the focus chain between the compound and
the compound's first child. If one of those children is then made the
focus proxy of the compound, then the widget in between becomes
unreachable by tabbing.

To fix this, detect that we set the focus proxy to be a descendent of
the compound widget, and then move the compound widget directly in front
of its first child in the focus chain. This way we can't have any gaps
in the focus chain.

Augment the test case with a corresponding scenario. As a drive-by, move
the debug helper up in the code so that it can be easier used, and set
object names on relevant widgets.

Pick-to: 6.4 6.2 5.15
Fixes: QTBUG-89156
Change-Id: I17057719a90f59629087afbd1d2ca58c1aa1d8f6
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

2 files changed, 109 insertions, 17 deletions

diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp
index 95fbcdc7df..2be71482e4 100644
--- a/src/widgets/kernel/qwidget.cpp
+++ b/src/widgets/kernel/qwidget.cpp
@@ -6381,6 +6381,33 @@ void QWidget::setFocusProxy(QWidget * w)
     d->createExtra();
     d->extra->focus_proxy = w;
 
+    if (w && isAncestorOf(w)) {
+        // If the focus proxy is a child of this (so this is a compound widget), then
+        // we need to make sure that this widget is immediately in front of its own children
+        // in the focus chain. Otherwise focusNextPrev_helper might jump over unrelated
+        // widgets that are positioned between this compound widget, and its proxy in
+        // the focus chain.
+        const QWidget *parentOfW = w->parentWidget();
+        Q_ASSERT(parentOfW); // can't be nullptr since we are an ancestor of w
+        QWidget *firstChild = nullptr;
+        const auto childList = children();
+        for (QObject *child : childList) {
+            if ((firstChild = qobject_cast<QWidget *>(child)))
+                break;
+        }
+        Q_ASSERT(firstChild); // can't be nullptr since w is a child
+        QWidget *oldNext = d->focus_next;
+        QWidget *oldPrev = d->focus_prev;
+        oldNext->d_func()->focus_prev = oldPrev;
+        oldPrev->d_func()->focus_next = oldNext;
+
+        oldPrev = firstChild->d_func()->focus_prev;
+        d->focus_next = firstChild;
+        d->focus_prev = oldPrev;
+        oldPrev->d_func()->focus_next = this;
+        firstChild->d_func()->focus_prev = this;
+    }
+
     if (moveFocusToProxy)
         setFocus(Qt::OtherFocusReason);
 }
@@ -7072,8 +7099,8 @@ void QWidgetPrivate::reparentFocusWidgets(QWidget * oldtlw)
         n->d_func()->focus_next = topLevel;
     } else {
         //repair the new list
-            n->d_func()->focus_next = q;
-            focus_prev = n;
+        n->d_func()->focus_next = q;
+        focus_prev = n;
     }
 
 }
diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
index 37a395194d..7bfe78cd57 100644
--- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
+++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
@@ -166,6 +166,7 @@ private slots:
     void reverseTabOrder();
     void tabOrderWithProxy();
     void tabOrderWithProxyDisabled();
+    void tabOrderWithProxyOutOfOrder();
     void tabOrderWithCompoundWidgets();
     void tabOrderWithCompoundWidgetsNoFocusPolicy();
     void tabOrderNoChange();
@@ -1899,8 +1900,11 @@ public:
         setObjectName(name);
 
         lineEdit1 = new QLineEdit;
+        lineEdit1->setObjectName(name + "/lineEdit1");
         lineEdit2 = new QLineEdit;
+        lineEdit2->setObjectName(name + "/lineEdit2");
         lineEdit3 = new QLineEdit;
+        lineEdit3->setObjectName(name + "/lineEdit3");
         lineEdit3->setEnabled(false);
 
         QHBoxLayout* hbox = new QHBoxLayout(this);
@@ -2146,6 +2150,24 @@ void tst_QWidget::tabOrderWithProxyDisabled()
              qPrintable(QApplication::focusWidget()->objectName()));
 }
 
+//#define DEBUG_FOCUS_CHAIN
+static void dumpFocusChain(QWidget *start, bool bForward, const char *desc = nullptr)
+{
+#ifdef DEBUG_FOCUS_CHAIN
+    qDebug() << "Dump focus chain, start:" << start << "isForward:" << bForward << desc;
+    QWidget *cur = start;
+    do {
+        qDebug() << "-" << cur;
+        auto widgetPrivate = static_cast<QWidgetPrivate *>(qt_widget_private(cur));
+        cur = bForward ? widgetPrivate->focus_next : widgetPrivate->focus_prev;
+    } while (cur != start);
+#else
+    Q_UNUSED(start);
+    Q_UNUSED(bForward);
+    Q_UNUSED(desc);
+#endif
+}
+
 void tst_QWidget::tabOrderWithCompoundWidgets()
 {
     if (QGuiApplication::platformName().startsWith(QLatin1String("wayland"), Qt::CaseInsensitive))
@@ -2249,22 +2271,65 @@ static QList<QWidget *> getFocusChain(QWidget *start, bool bForward)
     return ret;
 }
 
-//#define DEBUG_FOCUS_CHAIN
-static void dumpFocusChain(QWidget *start, bool bForward, const char *desc = nullptr)
+void tst_QWidget::tabOrderWithProxyOutOfOrder()
 {
-#ifdef DEBUG_FOCUS_CHAIN
-    qDebug() << "Dump focus chain, start:" << start << "isForward:" << bForward << desc;
-    QWidget *cur = start;
-    do {
-        qDebug() << cur;
-        auto widgetPrivate = static_cast<QWidgetPrivate *>(qt_widget_private(cur));
-        cur = bForward ? widgetPrivate->focus_next : widgetPrivate->focus_prev;
-    } while (cur != start);
-#else
-    Q_UNUSED(start);
-    Q_UNUSED(bForward);
-    Q_UNUSED(desc);
-#endif
+    Container container;
+    container.setWindowTitle(QLatin1String(QTest::currentTestFunction()));
+
+    // important to create the widgets with parent so that they are
+    // added to the focus chain already now, and with the buttonBox
+    // before the outsideButton.
+    QWidget buttonBox(&container);
+    buttonBox.setObjectName("buttonBox");
+    QPushButton outsideButton(&container);
+    outsideButton.setObjectName("outsideButton");
+
+    container.box->addWidget(&outsideButton);
+    container.box->addWidget(&buttonBox);
+    QCOMPARE(getFocusChain(&container, true),
+             QList<QWidget*>({&container, &buttonBox, &outsideButton}));
+
+    // this now adds okButon and cancelButton to the focus chain,
+    // after the outsideButton - so the outsideButton is in between
+    // the buttonBox and the children of the buttonBox!
+    QPushButton okButton(&buttonBox);
+    okButton.setObjectName("okButton");
+    QPushButton cancelButton(&buttonBox);
+    cancelButton.setObjectName("cancelButton");
+    QCOMPARE(getFocusChain(&container, true),
+             QList<QWidget*>({&container, &buttonBox, &outsideButton, &okButton, &cancelButton}));
+
+    // by setting the okButton as the focusProxy, the outsideButton becomes
+    // unreachable when navigating the focus chain as the buttonBox is in front
+    // of, and proxies to the okButton behind the outsideButton. setFocusProxy
+    // must fix that by moving the buttonBox in front of the first sibling of
+    // the proxy.
+    buttonBox.setFocusProxy(&okButton);
+    QCOMPARE(getFocusChain(&container, true),
+             QList<QWidget*>({&container, &outsideButton, &buttonBox, &okButton, &cancelButton}));
+
+    container.show();
+    container.activateWindow();
+    QApplicationPrivate::setActiveWindow(&container);
+    if (!QTest::qWaitForWindowActive(&container))
+        QSKIP("Window failed to activate, skipping test");
+
+    QCOMPARE(QApplication::focusWidget(), &outsideButton);
+    container.tab();
+    QCOMPARE(QApplication::focusWidget(), &okButton);
+    container.tab();
+    QCOMPARE(QApplication::focusWidget(), &cancelButton);
+    container.tab();
+    QCOMPARE(QApplication::focusWidget(), &outsideButton);
+
+    container.backTab();
+    QCOMPARE(QApplication::focusWidget(), &cancelButton);
+    container.backTab();
+    QCOMPARE(QApplication::focusWidget(), &okButton);
+    container.backTab();
+    QCOMPARE(QApplication::focusWidget(), &outsideButton);
+    container.backTab();
+    QCOMPARE(QApplication::focusWidget(), &cancelButton);
 }
 
 void tst_QWidget::tabOrderWithCompoundWidgetsNoFocusPolicy()