QQuickMenu: fix key navigation

Skip non-focusable separators, and use a key focus reason
(Qt::TabFocusReason & Qt::BacktabFocusReason) to give the
items visual focus.

[ChangeLog][Controls][Menu] Fixed key navigation to skip separators.

Change-Id: I99affabc50703c7363ab8146e5ced9b45111de00
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

3 files changed, 104 insertions, 12 deletions

diff --git a/src/quicktemplates2/qquickmenu.cpp b/src/quicktemplates2/qquickmenu.cpp
index 9e8fabe0..c40c49e1 100644
--- a/src/quicktemplates2/qquickmenu.cpp
+++ b/src/quicktemplates2/qquickmenu.cpp
@@ -254,6 +254,31 @@ void QQuickMenuPrivate::setCurrentIndex(int index)
     contentItem->setProperty("currentIndex", index);
 }
 
+void QQuickMenuPrivate::activateNextItem()
+{
+    int index = currentIndex();
+    int count = contentModel->count();
+    while (++index < count) {
+        QQuickItem *item = itemAt(index);
+        if (!item || !item->activeFocusOnTab())
+            continue;
+        item->forceActiveFocus(Qt::TabFocusReason);
+        break;
+    }
+}
+
+void QQuickMenuPrivate::activatePreviousItem()
+{
+    int index = currentIndex();
+    while (--index >= 0) {
+        QQuickItem *item = itemAt(index);
+        if (!item || !item->activeFocusOnTab())
+            continue;
+        item->forceActiveFocus(Qt::BacktabFocusReason);
+        break;
+    }
+}
+
 void QQuickMenuPrivate::contentData_append(QQmlListProperty<QObject> *prop, QObject *obj)
 {
     QQuickMenuPrivate *p = static_cast<QQuickMenuPrivate *>(prop->data);
@@ -511,23 +536,16 @@ void QQuickMenu::keyReleaseEvent(QKeyEvent *event)
     // shown at once.
     switch (event->key()) {
     case Qt::Key_Up:
-        if (d->contentItem->metaObject()->indexOfMethod("decrementCurrentIndex()") != -1)
-            QMetaObject::invokeMethod(d->contentItem, "decrementCurrentIndex");
+        d->activatePreviousItem();
         break;
 
     case Qt::Key_Down:
-        if (d->contentItem->metaObject()->indexOfMethod("incrementCurrentIndex()") != -1)
-            QMetaObject::invokeMethod(d->contentItem, "incrementCurrentIndex");
+        d->activateNextItem();
         break;
 
     default:
         break;
     }
-
-    int index = d->currentIndex();
-    QQuickItem *item = itemAt(index);
-    if (item)
-        item->forceActiveFocus();
 }
 
 QFont QQuickMenu::defaultFont() const
diff --git a/src/quicktemplates2/qquickmenu_p_p.h b/src/quicktemplates2/qquickmenu_p_p.h
index 504bc74d..583bb41d 100644
--- a/src/quicktemplates2/qquickmenu_p_p.h
+++ b/src/quicktemplates2/qquickmenu_p_p.h
@@ -84,6 +84,9 @@ public:
     int currentIndex() const;
     void setCurrentIndex(int index);
 
+    void activateNextItem();
+    void activatePreviousItem();
+
     static void contentData_append(QQmlListProperty<QObject> *prop, QObject *obj);
     static int contentData_count(QQmlListProperty<QObject> *prop);
     static QObject *contentData_at(QQmlListProperty<QObject> *prop, int index);
diff --git a/tests/auto/menu/tst_menu.cpp b/tests/auto/menu/tst_menu.cpp
index 1e6eaceb..19a4006b 100644
--- a/tests/auto/menu/tst_menu.cpp
+++ b/tests/auto/menu/tst_menu.cpp
@@ -165,7 +165,8 @@ void tst_menu::contextMenuKeyboard()
     QQuickMenu *menu = window->property("menu").value<QQuickMenu*>();
     QCOMPARE(menu->contentItem()->property("currentIndex"), QVariant(-1));
 
-    QQuickItem *firstItem = menu->itemAt(0);
+    QQuickMenuItem *firstItem = qobject_cast<QQuickMenuItem *>(menu->itemAt(0));
+    QVERIFY(firstItem);
     QSignalSpy visibleSpy(menu, SIGNAL(visibleChanged()));
 
     menu->setFocus(true);
@@ -178,12 +179,18 @@ void tst_menu::contextMenuKeyboard()
 
     QTest::keyClick(window, Qt::Key_Tab);
     QVERIFY(firstItem->hasActiveFocus());
+    QVERIFY(firstItem->hasVisualFocus());
+    QCOMPARE(firstItem->focusReason(), Qt::TabFocusReason);
     QCOMPARE(menu->contentItem()->property("currentIndex"), QVariant(0));
 
-    QQuickItem *secondItem = menu->itemAt(1);
+    QQuickMenuItem *secondItem = qobject_cast<QQuickMenuItem *>(menu->itemAt(1));
+    QVERIFY(secondItem);
     QTest::keyClick(window, Qt::Key_Tab);
     QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
     QVERIFY(secondItem->hasActiveFocus());
+    QVERIFY(secondItem->hasVisualFocus());
+    QCOMPARE(secondItem->focusReason(), Qt::TabFocusReason);
     QCOMPARE(menu->contentItem()->property("currentIndex"), QVariant(1));
 
     QSignalSpy secondTriggeredSpy(secondItem, SIGNAL(triggered()));
@@ -193,7 +200,9 @@ void tst_menu::contextMenuKeyboard()
     QVERIFY(!menu->isVisible());
     QVERIFY(!window->overlay()->childItems().contains(menu->contentItem()));
     QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
     QVERIFY(!secondItem->hasActiveFocus());
+    QVERIFY(!secondItem->hasVisualFocus());
     QCOMPARE(menu->contentItem()->property("currentIndex"), QVariant(-1));
 
     menu->open();
@@ -201,26 +210,59 @@ void tst_menu::contextMenuKeyboard()
     QVERIFY(menu->isVisible());
     QVERIFY(window->overlay()->childItems().contains(menu->contentItem()->parentItem()));
     QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
     QVERIFY(!secondItem->hasActiveFocus());
+    QVERIFY(!secondItem->hasVisualFocus());
     QCOMPARE(menu->contentItem()->property("currentIndex"), QVariant(-1));
 
     QTest::keyClick(window, Qt::Key_Down);
     QVERIFY(firstItem->hasActiveFocus());
+    QVERIFY(firstItem->hasVisualFocus());
+    QCOMPARE(firstItem->focusReason(), Qt::TabFocusReason);
 
     QTest::keyClick(window, Qt::Key_Down);
     QVERIFY(secondItem->hasActiveFocus());
+    QVERIFY(secondItem->hasVisualFocus());
+    QCOMPARE(secondItem->focusReason(), Qt::TabFocusReason);
 
     QTest::keyClick(window, Qt::Key_Down);
-    QQuickItem *thirdItem = menu->itemAt(2);
+    QQuickMenuItem *thirdItem = qobject_cast<QQuickMenuItem *>(menu->itemAt(2));
+    QVERIFY(thirdItem);
     QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
     QVERIFY(!secondItem->hasActiveFocus());
+    QVERIFY(!secondItem->hasVisualFocus());
     QVERIFY(thirdItem->hasActiveFocus());
+    QVERIFY(thirdItem->hasVisualFocus());
+    QCOMPARE(thirdItem->focusReason(), Qt::TabFocusReason);
 
     // Key navigation shouldn't wrap by default.
     QTest::keyClick(window, Qt::Key_Down);
     QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
     QVERIFY(!secondItem->hasActiveFocus());
+    QVERIFY(!secondItem->hasVisualFocus());
     QVERIFY(thirdItem->hasActiveFocus());
+    QVERIFY(thirdItem->hasVisualFocus());
+    QCOMPARE(thirdItem->focusReason(), Qt::TabFocusReason);
+
+    QTest::keyClick(window, Qt::Key_Up);
+    QVERIFY(!firstItem->hasActiveFocus());
+    QVERIFY(!firstItem->hasVisualFocus());
+    QVERIFY(secondItem->hasActiveFocus());
+    QVERIFY(secondItem->hasVisualFocus());
+    QCOMPARE(secondItem->focusReason(), Qt::BacktabFocusReason);
+    QVERIFY(!thirdItem->hasActiveFocus());
+    QVERIFY(!thirdItem->hasVisualFocus());
+
+    QTest::keyClick(window, Qt::Key_Backtab);
+    QVERIFY(firstItem->hasActiveFocus());
+    QVERIFY(firstItem->hasVisualFocus());
+    QCOMPARE(firstItem->focusReason(), Qt::BacktabFocusReason);
+    QVERIFY(!secondItem->hasActiveFocus());
+    QVERIFY(!secondItem->hasVisualFocus());
+    QVERIFY(!thirdItem->hasActiveFocus());
+    QVERIFY(!thirdItem->hasVisualFocus());
 
     QTest::keyClick(window, Qt::Key_Escape);
     QCOMPARE(visibleSpy.count(), 4);
@@ -317,6 +359,35 @@ void tst_menu::menuSeparator()
     QTest::mouseClick(window, Qt::LeftButton, Qt::NoModifier,
         saveMenuItem->mapToScene(QPointF(saveMenuItem->width() / 2, saveMenuItem->height() / 2)).toPoint());
     QTRY_VERIFY(!menu->isVisible());
+
+    menu->open();
+    QVERIFY(menu->isVisible());
+
+    // Key navigation skips separators
+    QTest::keyClick(window, Qt::Key_Down);
+    QVERIFY(newMenuItem->hasActiveFocus());
+    QVERIFY(newMenuItem->hasVisualFocus());
+    QCOMPARE(newMenuItem->focusReason(), Qt::TabFocusReason);
+
+    QTest::keyClick(window, Qt::Key_Down);
+    QVERIFY(saveMenuItem->hasActiveFocus());
+    QVERIFY(saveMenuItem->hasVisualFocus());
+    QCOMPARE(saveMenuItem->focusReason(), Qt::TabFocusReason);
+
+    QTest::keyClick(window, Qt::Key_Down);
+    QVERIFY(saveMenuItem->hasActiveFocus());
+    QVERIFY(saveMenuItem->hasVisualFocus());
+    QCOMPARE(saveMenuItem->focusReason(), Qt::TabFocusReason);
+
+    QTest::keyClick(window, Qt::Key_Up);
+    QVERIFY(newMenuItem->hasActiveFocus());
+    QVERIFY(newMenuItem->hasVisualFocus());
+    QCOMPARE(newMenuItem->focusReason(), Qt::BacktabFocusReason);
+
+    QTest::keyClick(window, Qt::Key_Up);
+    QVERIFY(newMenuItem->hasActiveFocus());
+    QVERIFY(newMenuItem->hasVisualFocus());
+    QCOMPARE(newMenuItem->focusReason(), Qt::BacktabFocusReason);
 }
 
 void tst_menu::repeater()