QQuickMenuBar: fix menu not opening

When a Menu is declared within a MenuBar, a MenuBarItem has to be created for
it. Creation takes the following steps:

- 1 Begin creation of the item
  - 1.1 Set the parent of the MenuBarItem to the MenuBar
- 2 Set the menu on the item
- 3 Complete creation of the item
- 4 Add the item

When setting the parent of the MenuBarItem, the following call stack can be
observed:

1   QQuickContainer::itemChange              qquickcontainer.cpp       757  0x7fff6e5f4544
2   QQuickItemPrivate::itemChange            qquickitem.cpp            6213 0x7fff6aa226f7
3   QQuickItemPrivate::addChild              qquickitem.cpp            2964 0x7fff6aa1e663
4   QQuickItem::setParentItem                qquickitem.cpp            2753 0x7fff6aa0f57c
5   QQuickMenuBarPrivate::beginCreateItem    qquickmenubar.cpp         100  0x7fff6e627c08
6   QQuickMenuBarPrivate::createItem         qquickmenubar.cpp         115  0x7fff6e627c98
7   QQuickMenuBarPrivate::contentData_append qquickmenubar.cpp         263  0x7fff6e6285d9

In particular, the following function is called:

    void QQuickContainer::itemChange(ItemChange change, const ItemChangeData &data)
    {
        Q_D(QQuickContainer);
        QQuickControl::itemChange(change, data);
        if (change == QQuickItem::ItemChildAddedChange && isComponentComplete() && data.item != d->background && data.item != d->contentItem) {
            if (!QQuickItemPrivate::get(data.item)->isTransparentForPositioner() && d->contentModel->indexOf(data.item, nullptr) == -1)
                addItem(data.item);
        }
    }

This check is for items that are added after component completion of the
control (QQuickMenuBar), as there is a isComponentComplete() check.

Before d923dd46, QQuickMenuBarItems were constructed the moment their
QQuickMenus were appended to QQuickMenuBar's contentData, which was before
component completion. This meant that the isComponentComplete() check above
failed as expected and the item was instead added after its creation process
was completed (step #4 in the list above):

    void QQuickMenuBarPrivate::contentData_append(QQmlListProperty<QObject> *prop, QObject *obj)
    {
        QQuickMenuBar *menuBar = static_cast<QQuickMenuBar *>(prop->object);
        if (QQuickMenu *menu = qobject_cast<QQuickMenu *>(obj))
            obj = QQuickMenuBarPrivate::get(menuBar)->createItem(menu);
        QQuickContainerPrivate::contentData_append(prop, obj); // leads to addItem() being called
    }

Part of the process of an item being added to QQuickMenuBar involves connecting
to the signals of its corresponding QQuickMenu (if it has a menu). Quoting the
code from QQuickMenuBar::itemAdded():

    if (QQuickMenu *menu = menuBarItem->menu())
        QObjectPrivate::connect(menu, &QQuickPopup::aboutToHide, d, &QQuickMenuBarPrivate::onMenuAboutToHide);

After d923dd46, QQuickMenuBarItems are now constructed *after* component
completion to ensure that delegates declared outside of the menu bar have
been completed. This means that the isComponentComplete() check in
QQuickContainer::itemChange() no longer fails and the item is added
before its QQuickMenu is set on it (step #2). As a result, it never
connects to the QQuickPopup::aboutToHide() signal and hence it stays
activated/highlighted even after the menu has been dismissed, which
results in having to click twice on the QQuickMenuBarItem to open
the menu the next time.

This patch fixes the issue by simply setting the menu on the item before
setting its parent:

- 1 Begin creation of the item
  - 1.1 Set the menu on the item
  - 1.2 Set the parent of the MenuBarItem to the MenuBar
- 2 Complete creation of the item
- 3 Add the item

This ensures that the item has a menu and the connection is made.

Change-Id: I93edf7e5a8616a851595ce28ed43f0348078f0b5
Fixes: QTBUG-71583
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

1 files changed, 60 insertions, 0 deletions

diff --git a/tests/auto/qquickmenubar/tst_qquickmenubar.cpp b/tests/auto/qquickmenubar/tst_qquickmenubar.cpp
index cfcdee5e..70d487a9 100644
--- a/tests/auto/qquickmenubar/tst_qquickmenubar.cpp
+++ b/tests/auto/qquickmenubar/tst_qquickmenubar.cpp
@@ -44,6 +44,7 @@
 
 #include <QtQuickTemplates2/private/qquickapplicationwindow_p.h>
 #include <QtQuickTemplates2/private/qquickmenu_p.h>
+#include <QtQuickTemplates2/private/qquickmenu_p_p.h>
 #include <QtQuickTemplates2/private/qquickmenubar_p.h>
 #include <QtQuickTemplates2/private/qquickmenubar_p_p.h>
 #include <QtQuickTemplates2/private/qquickmenubaritem_p.h>
@@ -64,6 +65,7 @@ private slots:
     void mnemonics();
     void addRemove();
     void delegateFromSeparateComponent();
+    void openTwice();
 };
 
 void tst_qquickmenubar::delegate()
@@ -605,6 +607,64 @@ void tst_qquickmenubar::delegateFromSeparateComponent()
     QCOMPARE(menuBarItemBg->property("color").value<QColor>(), green);
 }
 
+void tst_qquickmenubar::openTwice()
+{
+    if ((QGuiApplication::platformName() == QLatin1String("offscreen"))
+        || (QGuiApplication::platformName() == QLatin1String("minimal")))
+        QSKIP("Mouse highlight not functional on offscreen/minimal platforms");
+
+    QQmlApplicationEngine engine(testFileUrl("menubar.qml"));
+
+    QScopedPointer<QQuickApplicationWindow> window(qobject_cast<QQuickApplicationWindow *>(engine.rootObjects().value(0)));
+    QVERIFY(window);
+    QVERIFY(QTest::qWaitForWindowActive(window.data()));
+
+    centerOnScreen(window.data());
+
+    QQuickMenuBar *menuBar = window->property("header").value<QQuickMenuBar *>();
+    QVERIFY(menuBar);
+
+    QQuickMenu *fileMenuBarMenu = menuBar->menuAt(0);
+    QVERIFY(fileMenuBarMenu);
+
+    QQuickMenuBarItem *fileMenuBarItem = qobject_cast<QQuickMenuBarItem *>(fileMenuBarMenu->parentItem());
+    QVERIFY(fileMenuBarItem);
+    QQuickMenuBarPrivate *menuBarPrivate = QQuickMenuBarPrivate::get(menuBar);
+    menuBar->polish();
+    QVERIFY(menuBarPrivate->polishScheduled);
+    QTRY_VERIFY(!menuBarPrivate->polishScheduled);
+
+    // Open a menu.
+    QTest::mouseClick(window.data(), Qt::LeftButton, Qt::NoModifier,
+        fileMenuBarItem->mapToScene(QPointF(fileMenuBarItem->width() / 2, fileMenuBarItem->height() / 2)).toPoint());
+    QVERIFY(fileMenuBarItem->isHighlighted());
+    QVERIFY(fileMenuBarMenu->isVisible());
+    QTRY_VERIFY(fileMenuBarMenu->isOpened());
+    waitForMenuListViewPolish(fileMenuBarMenu);
+
+    // Click on an item to close the menu.
+    QQuickMenuItem *openMenuItem = qobject_cast<QQuickMenuItem *>(fileMenuBarMenu->itemAt(0));
+    QVERIFY(openMenuItem);
+    QTest::mouseClick(window.data(), Qt::LeftButton, Qt::NoModifier,
+        openMenuItem->mapToScene(QPointF(openMenuItem->width() / 2, openMenuItem->height() / 2)).toPoint());
+    QVERIFY(!fileMenuBarItem->isHighlighted());
+    QTRY_VERIFY(!fileMenuBarMenu->isVisible());
+
+    // Re-open the menu. It should open with one click of the QQuickMenuBarItem.
+    QTest::mouseClick(window.data(), Qt::LeftButton, Qt::NoModifier,
+        fileMenuBarItem->mapToScene(QPointF(fileMenuBarItem->width() / 2, fileMenuBarItem->height() / 2)).toPoint());
+    QVERIFY(fileMenuBarItem->isHighlighted());
+    QVERIFY(fileMenuBarMenu->isVisible());
+    QTRY_VERIFY(fileMenuBarMenu->isOpened());
+    waitForMenuListViewPolish(fileMenuBarMenu);
+
+    // Click on an item to close the menu.
+    QTest::mouseClick(window.data(), Qt::LeftButton, Qt::NoModifier,
+        openMenuItem->mapToScene(QPointF(openMenuItem->width() / 2, openMenuItem->height() / 2)).toPoint());
+    QVERIFY(!fileMenuBarItem->isHighlighted());
+    QTRY_VERIFY(!fileMenuBarMenu->isVisible());
+}
+
 QTEST_QUICKCONTROLS_MAIN(tst_qquickmenubar)
 
 #include "tst_qquickmenubar.moc"