Widgets: be able to tab to a widget that has a focus proxy

When tabbing/searching for the next focus widget, the current
code would check if the next widget in the focus chain
had a focus proxy, and if so, ignore it. The exact reason
for this behavior is not clearly understood, but some widgets
(e.g QSpinBox) has children (a QLineEdit) that sets the parent
as focus proxy. If we didn't ignore children with focus proxy, tabbing
from a QSpinBox would lead us to find the inner QLineEdit, which
(because of its proxy), would lead us back to the QSpinBox. And
therefore not be able to tab out.

But ignoring the focus proxy has other problems. Normally a focus
proxy is the next sibling to the widget it acts as a proxy for, and
tabbing to the widget will therefore appear correct. But if the
focus proxy is not the next sibling, the logic will fail, since
the tab would anyway give focus to the next sibling. This becomes very
apparent if the focus proxy is a child of the widget, since then
its likely that the focus proxy is not the _first_ child among all
the children. So tabbing to the parent would not give focus to
the proxy.

This patch will change this logic so that you are allowed to tab to a
widget with a focus proxy. But we check that if you do so, you actually
end up moving focus in the right direction. If not, we ignore it like
before. This will ensure that we tab correctly when dealing with focus
proxies, and especially when focus proxies are used to construct
compound widgets.

[ChangeLog][Widgets] When tabbing to a widget with focus proxy, focus
will now be given to the proxy rather than just being ignored.

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

2 files changed, 162 insertions, 42 deletions

diff --git a/src/widgets/kernel/qapplication.cpp b/src/widgets/kernel/qapplication.cpp
index ee61a25b09..943aeaa2d9 100644
--- a/src/widgets/kernel/qapplication.cpp
+++ b/src/widgets/kernel/qapplication.cpp
@@ -2228,8 +2228,15 @@ QWidget *QApplicationPrivate::focusNextPrevChild_helper(QWidget *toplevel, bool
         if (test->isWindow())
             seenWindow = true;
 
+        // If the next focus widget has a focus proxy, we need to check to ensure
+        // that the proxy is in the correct parent-child direction (according to
+        // \a next). This is to ensure that we can tab in and out of compound widgets
+        // without getting stuck in a tab-loop between parent and child.
+        QWidget *focusProxy = test->d_func()->deepestFocusProxy();
+
         if ((test->focusPolicy() & focus_flag) == focus_flag
-            && !(test->d_func()->extra && test->d_func()->extra->focus_proxy)
+            && !(next && focusProxy && focusProxy->isAncestorOf(test))
+            && !(!next && focusProxy && test->isAncestorOf(focusProxy))
             && test->isVisibleTo(toplevel) && test->isEnabled()
             && !(w->windowType() == Qt::SubWindow && !w->isAncestorOf(test))
             && (toplevel->windowType() != Qt::SubWindow || toplevel->isAncestorOf(test))) {
diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
index e284d92f72..36258d8196 100644
--- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
+++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp
@@ -196,7 +196,9 @@ private slots:
     void mapFromAndTo();
     void focusChainOnHide();
     void focusChainOnReparent();
-    void setTabOrder();
+    void defaultTabOrder();
+    void reverseTabOrder();
+    void tabOrderWithProxy();
 #ifdef Q_OS_WIN
     void activation();
 #endif
@@ -1667,74 +1669,185 @@ public:
         : QFrame(parent)
     {
         setObjectName(name);
-        //QHBoxLayout* hbox = new QHBoxLayout(this, 2, 0);
-        //hbox->setAutoAdd(true);
+
+        lineEdit1 = new QLineEdit;
+        lineEdit2 = new QLineEdit;
+
         QHBoxLayout* hbox = new QHBoxLayout(this);
+        hbox->addWidget(lineEdit1);
+        hbox->addWidget(lineEdit2);
+    }
 
-        lineEdit = new QLineEdit(this);
-        hbox->addWidget(lineEdit);
+public:
+    QLineEdit *lineEdit1;
+    QLineEdit *lineEdit2;
+};
 
-        button = new QPushButton(this);
-        hbox->addWidget(button);
-        button->setFocusPolicy( Qt::NoFocus );
+void tst_QWidget::defaultTabOrder()
+{
+    const int compositeCount = 2;
+    Container container;
+    Composite *composite[compositeCount];
 
-        setFocusProxy( lineEdit );
-        setFocusPolicy( Qt::StrongFocus );
+    QLineEdit *firstEdit = new QLineEdit;
+    container.box->addWidget(firstEdit);
 
-        setTabOrder(lineEdit, button);
+    for (int i = 0; i < compositeCount; i++) {
+        composite[i] = new Composite();
+        container.box->addWidget(composite[i]);
     }
 
-private:
-    QLineEdit* lineEdit;
-    QPushButton* button;
-};
+    QLineEdit *lastEdit = new QLineEdit();
+    container.box->addWidget(lastEdit);
 
-#define NUM_WIDGETS 4
+    container.show();
+    container.activateWindow();
+    qApp->setActiveWindow(&container);
+    QVERIFY(QTest::qWaitForWindowActive(&container));
 
-void tst_QWidget::setTabOrder()
-{
-    QTest::qWait(100);
+    QTRY_VERIFY(firstEdit->hasFocus());
 
-    Container container;
-    container.setObjectName("setTabOrder");
-    container.setWindowTitle(container.objectName());
+    // Check that focus moves between the line edits when we tab forward
+    for (int i = 0; i < compositeCount; ++i) {
+        container.tab();
+        QVERIFY(composite[i]->lineEdit1->hasFocus());
+        QVERIFY(!composite[i]->lineEdit2->hasFocus());
+        container.tab();
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
+    }
+
+    container.tab();
+    QVERIFY(lastEdit->hasFocus());
 
-    Composite* comp[NUM_WIDGETS];
+    // Check that focus moves between the line edits in reverse
+    // order when we tab backwards
+    for (int i = compositeCount - 1; i >= 0; --i) {
+        container.backTab();
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
 
-    QLineEdit *firstEdit = new QLineEdit(&container);
+        container.backTab();
+        QVERIFY(composite[i]->lineEdit1->hasFocus());
+        QVERIFY(!composite[i]->lineEdit2->hasFocus());
+    }
+
+    container.backTab();
+    QVERIFY(firstEdit->hasFocus());
+}
+
+void tst_QWidget::reverseTabOrder()
+{
+    const int compositeCount = 2;
+    Container container;
+    Composite* composite[compositeCount];
+
+    QLineEdit *firstEdit = new QLineEdit();
     container.box->addWidget(firstEdit);
 
-    int i = 0;
-    for(i = 0; i < NUM_WIDGETS; i++) {
-        comp[i] = new Composite(&container);
-        container.box->addWidget(comp[i]);
+    for (int i = 0; i < compositeCount; i++) {
+        composite[i] = new Composite();
+        container.box->addWidget(composite[i]);
     }
 
-    QLineEdit *lastEdit = new QLineEdit(&container);
+    QLineEdit *lastEdit = new QLineEdit();
     container.box->addWidget(lastEdit);
 
-    container.setTabOrder(lastEdit, comp[NUM_WIDGETS-1]);
-    for(i = NUM_WIDGETS-1; i > 0; i--) {
-        container.setTabOrder(comp[i], comp[i-1]);
+    // Reverse tab order inside each composite
+    for (int i = 0; i < compositeCount; ++i)
+        QWidget::setTabOrder(composite[i]->lineEdit2, composite[i]->lineEdit1);
+
+    container.show();
+    container.activateWindow();
+    qApp->setActiveWindow(&container);
+    QVERIFY(QTest::qWaitForWindowActive(&container));
+
+    QTRY_VERIFY(firstEdit->hasFocus());
+
+    // Check that focus moves in reverse order when tabbing inside the composites
+    // (but in the correct order when tabbing between them)
+    for (int i = 0; i < compositeCount; ++i) {
+        container.tab();
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
+        container.tab();
+        QVERIFY(composite[i]->lineEdit1->hasFocus());
+        QVERIFY(!composite[i]->lineEdit2->hasFocus());
+    }
+
+    container.tab();
+    QVERIFY(lastEdit->hasFocus());
+
+    // Check that focus moves in "normal" order when tabbing backwards inside the
+    // composites (since backwards of reversed order cancels each other out),
+    // but in the reverse order when tabbing between them.
+    for (int i = compositeCount - 1; i >= 0; --i) {
+        container.backTab();
+        QVERIFY(composite[i]->lineEdit1->hasFocus());
+        QVERIFY(!composite[i]->lineEdit2->hasFocus());
+        container.backTab();
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
+    }
+
+    container.backTab();
+    QVERIFY(firstEdit->hasFocus());
+}
+
+void tst_QWidget::tabOrderWithProxy()
+{
+    const int compositeCount = 2;
+    Container container;
+    Composite* composite[compositeCount];
+
+    QLineEdit *firstEdit = new QLineEdit();
+    container.box->addWidget(firstEdit);
+
+    for (int i = 0; i < compositeCount; i++) {
+        composite[i] = new Composite();
+        container.box->addWidget(composite[i]);
+
+        // Set second child as focus proxy
+        composite[i]->setFocusPolicy(Qt::StrongFocus);
+        composite[i]->setFocusProxy(composite[i]->lineEdit2);
     }
-    container.setTabOrder(comp[0], firstEdit);
 
-    int current = NUM_WIDGETS-1;
-    lastEdit->setFocus();
+    QLineEdit *lastEdit = new QLineEdit();
+    container.box->addWidget(lastEdit);
 
     container.show();
     container.activateWindow();
     qApp->setActiveWindow(&container);
     QVERIFY(QTest::qWaitForWindowActive(&container));
 
-    QTRY_VERIFY(lastEdit->hasFocus());
-    container.tab();
-    do {
-        QVERIFY(comp[current]->focusProxy()->hasFocus());
+    QTRY_VERIFY(firstEdit->hasFocus());
+
+    // Check that focus moves between the second line edits
+    // (the focus proxies) when we tab forward
+    for (int i = 0; i < compositeCount; ++i) {
         container.tab();
-        current--;
-    } while (current >= 0);
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
+    }
 
+    container.tab();
+    QVERIFY(lastEdit->hasFocus());
+
+    // Check that focus moves between the line edits
+    // in reverse order when we tab backwards.
+    // Note that in this case, the focus proxies should not
+    // be taken into consideration, since they only take
+    // effect when tabbing forward
+    for (int i = compositeCount - 1; i >= 0; --i) {
+        container.backTab();
+        QVERIFY(!composite[i]->lineEdit1->hasFocus());
+        QVERIFY(composite[i]->lineEdit2->hasFocus());
+        container.backTab();
+        QVERIFY(composite[i]->lineEdit1->hasFocus());
+        QVERIFY(!composite[i]->lineEdit2->hasFocus());
+    }
+
+    container.backTab();
     QVERIFY(firstEdit->hasFocus());
 }