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>

1 files changed, 56 insertions, 43 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