Widgets: change QWidget::setTabOrder to understand compound widgets

A "compound widget" is a widget that has a focus proxy set to an inner
child. This is normal for complex black-box components where focus handling
is delegated to the children. Since the compound can have several
children, a local tab order might exist between them.

The current implementation of setTabOrder had no idea about
compound widgets. As such, when connecting two compounds in the
tab chain, it would just break up their inner tab order and
cause tabbing to ignore children other than the proxy.

The new implementation recognizes compound widgets, and add some
extra code to figure out the correct tab targets. This way, the
local tab order between the children will be preserved.

This implementation was inspired by the patches of Marek Wieckowski posted
in the linked bug report, and later modified by Nikita Krupenko.

[ChangeLog][Widgets] QWidget::setTabOrder() will now preserve the local
tab order inside a widget if it has a focus proxy set to an inner child.

Task-number: QTBUG-10907
Change-Id: I0673d39d70ec8c6bf64af30bf978d67c651b2f3c
Reviewed-by: Andy Shaw <andy.shaw@qt.io>
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@qt.io>

2 files changed, 149 insertions, 44 deletions

diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp
index 8e6b44c370..be9287d39b 100644
--- a/src/widgets/kernel/qwidget.cpp
+++ b/src/widgets/kernel/qwidget.cpp
@@ -6945,6 +6945,9 @@ bool QWidget::isActiveWindow() const
 /*!
     Puts the \a second widget after the \a first widget in the focus order.
 
+    It effectively removes the \a second widget from its focus chain and
+    inserts it after the \a first widget.
+
     Note that since the tab order of the \a second widget is changed, you
     should order a chain like this:
 
@@ -6957,11 +6960,19 @@ bool QWidget::isActiveWindow() const
     If \a first or \a second has a focus proxy, setTabOrder()
     correctly substitutes the proxy.
 
+    \note Since Qt 5.10: A widget that has a child as focus proxy is understood as
+    a compound widget. When setting a tab order between one or two compound widgets, the
+    local tab order inside each will be preserved. This means that if both widgets are
+    compound widgets, the resulting tab order will be from the last child inside
+    \a first, to the first child inside \a second.
+
     \sa setFocusPolicy(), setFocusProxy(), {Keyboard Focus in Widgets}
 */
 void QWidget::setTabOrder(QWidget* first, QWidget *second)
 {
-    if (!first || !second || first->focusPolicy() == Qt::NoFocus || second->focusPolicy() == Qt::NoFocus)
+    if (!first || !second || first == second
+            || first->focusPolicy() == Qt::NoFocus
+            || second->focusPolicy() == Qt::NoFocus)
         return;
 
     if (Q_UNLIKELY(first->window() != second->window())) {
@@ -6969,54 +6980,56 @@ void QWidget::setTabOrder(QWidget* first, QWidget *second)
         return;
     }
 
-    QWidget *fp = first->focusProxy();
-    if (fp) {
-        // If first is redirected, set first to the last child of first
-        // that can take keyboard focus so that second is inserted after
-        // that last child, and the focus order within first is (more
-        // likely to be) preserved.
-        QList<QWidget *> l = first->findChildren<QWidget *>();
-        for (int i = l.size()-1; i >= 0; --i) {
-            QWidget * next = l.at(i);
-            if (next->window() == fp->window()) {
-                fp = next;
-                if (fp->focusPolicy() != Qt::NoFocus)
-                    break;
-            }
-        }
-        first = fp;
-    }
+    auto determineLastFocusChild = [](QWidget *target, QWidget *&lastFocusChild)
+    {
+        // Since we need to repeat the same logic for both 'first' and 'second', we add a function that
+        // determines the last focus child for a widget, taking proxies and compound widgets into account.
+        // If the target is not a compound widget (it doesn't have a focus proxy that points to a child),
+        // 'lastFocusChild' will be set to the target itself.
+        lastFocusChild = target;
+
+        QWidget *focusProxy = target->d_func()->deepestFocusProxy();
+        if (!focusProxy || !target->isAncestorOf(focusProxy))
+            return;
 
-    if (fp == second)
-        return;
+        lastFocusChild = focusProxy;
 
-    if (QWidget *sp = second->focusProxy())
-        second = sp;
+        for (QWidget *focusNext = lastFocusChild->d_func()->focus_next;
+             focusNext != focusProxy && target->isAncestorOf(focusNext) && focusNext->window() == focusProxy->window();
+             focusNext = focusNext->d_func()->focus_next) {
+            if (focusNext->focusPolicy() != Qt::NoFocus)
+                lastFocusChild = focusNext;
+        }
+    };
 
-//    QWidget *fp = first->d_func()->focus_prev;
-    QWidget *fn = first->d_func()->focus_next;
+    QWidget *lastFocusChildOfFirst, *lastFocusChildOfSecond;
+    determineLastFocusChild(first, lastFocusChildOfFirst);
+    determineLastFocusChild(second, lastFocusChildOfSecond);
 
-    if (fn == second || first == second)
+    // If the tab order is already correct, exit early
+    if (lastFocusChildOfFirst->d_func()->focus_next == second)
         return;
 
-    QWidget *sp = second->d_func()->focus_prev;
-    QWidget *sn = second->d_func()->focus_next;
-
-    fn->d_func()->focus_prev = second;
-    first->d_func()->focus_next = second;
-
-    second->d_func()->focus_next = fn;
-    second->d_func()->focus_prev = first;
-
-    sp->d_func()->focus_next = sn;
-    sn->d_func()->focus_prev = sp;
-
-
-    Q_ASSERT(first->d_func()->focus_next->d_func()->focus_prev == first);
-    Q_ASSERT(first->d_func()->focus_prev->d_func()->focus_next == first);
-
-    Q_ASSERT(second->d_func()->focus_next->d_func()->focus_prev == second);
-    Q_ASSERT(second->d_func()->focus_prev->d_func()->focus_next == second);
+    // Note that we need to handle two different sections in the tab chain; The section
+    // that 'first' belongs to (firstSection), where we are about to insert 'second', and
+    // the section that 'second' used be a part of (secondSection). When we pull 'second'
+    // out of the second section and insert it into the first, we also need to ensure
+    // that we leave the second section in a connected state.
+    QWidget *firstChainOldSecond = lastFocusChildOfFirst->d_func()->focus_next;
+    QWidget *secondChainNewFirst = second->d_func()->focus_prev;
+    QWidget *secondChainNewSecond = lastFocusChildOfSecond->d_func()->focus_next;
+
+    // Insert 'second' after 'first'
+    lastFocusChildOfFirst->d_func()->focus_next = second;
+    second->d_func()->focus_prev = lastFocusChildOfFirst;
+
+    // The widget that used to be 'second' in the first section, should now become 'third'
+    lastFocusChildOfSecond->d_func()->focus_next = firstChainOldSecond;
+    firstChainOldSecond->d_func()->focus_prev = lastFocusChildOfSecond;
+
+    // Repair the second section after we pulled 'second' out of it
+    secondChainNewFirst->d_func()->focus_next = secondChainNewSecond;
+    secondChainNewSecond->d_func()->focus_prev = secondChainNewFirst;
 }
 
 /*!\internal
diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
index 36258d8196..c328de37ca 100644
--- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
+++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
@@ -199,6 +199,7 @@ private slots:
     void defaultTabOrder();
     void reverseTabOrder();
     void tabOrderWithProxy();
+    void tabOrderWithCompoundWidgets();
 #ifdef Q_OS_WIN
     void activation();
 #endif
@@ -1665,22 +1666,26 @@ public:
 class Composite : public QFrame
 {
 public:
-    Composite(QWidget* parent = 0, const char* name = 0)
+    Composite(QWidget* parent = 0, const QString &name = 0)
         : QFrame(parent)
     {
         setObjectName(name);
 
         lineEdit1 = new QLineEdit;
         lineEdit2 = new QLineEdit;
+        lineEdit3 = new QLineEdit;
+        lineEdit3->setEnabled(false);
 
         QHBoxLayout* hbox = new QHBoxLayout(this);
         hbox->addWidget(lineEdit1);
         hbox->addWidget(lineEdit2);
+        hbox->addWidget(lineEdit3);
     }
 
 public:
     QLineEdit *lineEdit1;
     QLineEdit *lineEdit2;
+    QLineEdit *lineEdit3;
 };
 
 void tst_QWidget::defaultTabOrder()
@@ -1851,6 +1856,93 @@ void tst_QWidget::tabOrderWithProxy()
     QVERIFY(firstEdit->hasFocus());
 }
 
+void tst_QWidget::tabOrderWithCompoundWidgets()
+{
+    const int compositeCount = 4;
+    Container container;
+    Composite *composite[compositeCount];
+
+    QLineEdit *firstEdit = new QLineEdit();
+    container.box->addWidget(firstEdit);
+
+    for (int i = 0; i < compositeCount; i++) {
+        composite[i] = new Composite(0, QStringLiteral("Composite: ") + QString::number(i));
+        container.box->addWidget(composite[i]);
+
+        // Let the composite handle focus, and set a child as focus proxy (use the second child, just
+        // to ensure that we don't just tab to the first child by coinsidence). This will make the
+        // composite "compound". Also enable the last line edit to have a bit more data to check when
+        // tabbing forwards.
+        composite[i]->setFocusPolicy(Qt::StrongFocus);
+        composite[i]->setFocusProxy(composite[i]->lineEdit2);
+        composite[i]->lineEdit3->setEnabled(true);
+    }
+
+    QLineEdit *lastEdit = new QLineEdit();
+    container.box->addWidget(lastEdit);
+
+    // Reverse tab order between each composite
+    // (but not inside them), including first and last line edit.
+    // The result should not affect local tab order inside each
+    // composite, only between them.
+    QWidget::setTabOrder(lastEdit, composite[compositeCount - 1]);
+    for (int i = compositeCount - 1; i >= 1; --i)
+        QWidget::setTabOrder(composite[i], composite[i-1]);
+    QWidget::setTabOrder(composite[0], firstEdit);
+
+    container.show();
+    container.activateWindow();
+    qApp->setActiveWindow(&container);
+    QVERIFY(QTest::qWaitForWindowActive(&container));
+
+    lastEdit->setFocus();
+    QTRY_VERIFY(lastEdit->hasFocus());
+
+    // Check that focus moves between the line edits in the normal
+    // order when tabbing inside each compound, but in the reverse
+    // order when tabbing between them. Since the composites have
+    // lineEdit2 as focus proxy, lineEdit2 will be the first with focus
+    // when the compound gets focus, and lineEdit1 will therefore be skipped.
+    for (int i = compositeCount - 1; i >= 0; --i) {
+        container.tab();
+        Composite *c = composite[i];
+        QVERIFY(!c->lineEdit1->hasFocus());
+        QVERIFY(c->lineEdit2->hasFocus());
+        QVERIFY(!c->lineEdit3->hasFocus());
+        container.tab();
+        QVERIFY(!c->lineEdit1->hasFocus());
+        QVERIFY(!c->lineEdit2->hasFocus());
+        QVERIFY(c->lineEdit3->hasFocus());
+    }
+
+    container.tab();
+    QVERIFY(firstEdit->hasFocus());
+
+    // Check that focus moves in reverse order when backTab inside the composites, but
+    // in the 'correct' order when backTab between them (since the composites are in reverse tab
+    // order from before, which cancels it out). Note that when we backtab into a compound, we start
+    // at lineEdit3 rather than the focus proxy, since that is the reverse of what happens when we tab
+    // forward. And this time we will also backtab to lineEdit1, since there is no focus proxy that interferes.
+    for (int i = 0; i < compositeCount; ++i) {
+        container.backTab();
+        Composite *c = composite[i];
+        QVERIFY(!c->lineEdit1->hasFocus());
+        QVERIFY(!c->lineEdit2->hasFocus());
+        QVERIFY(c->lineEdit3->hasFocus());
+        container.backTab();
+        QVERIFY(!c->lineEdit1->hasFocus());
+        QVERIFY(c->lineEdit2->hasFocus());
+        QVERIFY(!c->lineEdit3->hasFocus());
+        container.backTab();
+        QVERIFY(c->lineEdit1->hasFocus());
+        QVERIFY(!c->lineEdit2->hasFocus());
+        QVERIFY(!c->lineEdit3->hasFocus());
+    }
+
+    container.backTab();
+    QVERIFY(lastEdit->hasFocus());
+}
+
 #ifdef Q_OS_WIN
 void tst_QWidget::activation()
 {