QMenu: don't trigger a menu item if the mouse didn't move

If you right-click to open a context menu, followed by another
click to close it again, without moving the mouse, it can
happen that the menu item under the mouse will trigger.
This is especially prone to happen if Qt has a large scale
factor and the number of menu items is high, since then the
menu has to move a bit up to make room for as menu items as
possible on the screen.

This patch will reuse the already existing hasMouseMoved()
function to avoid triggering a menu item if the mouse hasn't
moved. This will make sure that the user won't trigger a menu
item by accident when instead trying to close it.

Fixes: QTBUG-128359
Pick-to: 6.7 6.5 6.2
Change-Id: Ie4d73ee2031d1159dd6c003124869004b4928d18
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>
(cherry picked from commit dc1d3abd7e852829b9bc81c1ba9565f40147402d)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/widgets/widgets/qmenu.cpp

@@ -2459,7 +2459,7 @@ void QMenuPrivate::popup(const QPoint &p, QAction *atAction, PositionFunction po
         }
     }
 
-    QPoint mouse = QCursor::pos();
+    const QPoint mouse = QGuiApplicationPrivate::lastCursorPosition.toPoint();
     mousePopupPos = mouse;
     const bool snapToMouse = !causedPopup.widget && (QRect(p.x() - 3, p.y() - 3, 6, 6).contains(mouse));
 
@@ -2892,7 +2892,9 @@ void QMenu::mousePressEvent(QMouseEvent *e)
     // and mouse clicks on second screen, e->pos() is QPoint(0,0) and the menu doesn't hide. This trick makes
     // possible to hide the menu when mouse clicks on another screen (e->screenPos() returns correct value).
     // Only when mouse clicks in QPoint(0,0) on second screen, the menu doesn't hide.
-    if ((e->position().toPoint().isNull() && !e->globalPosition().isNull()) || !rect().contains(e->position().toPoint())) {
+    if ((e->position().toPoint().isNull() && !e->globalPosition().isNull())
+        || !rect().contains(e->position().toPoint())
+        || !d->hasMouseMoved(e->globalPosition().toPoint())) {
          if (d->noReplayFor
              && QRect(d->noReplayFor->mapToGlobal(QPoint()), d->noReplayFor->size()).contains(e->globalPosition().toPoint()))
              setAttribute(Qt::WA_NoMouseReplay);
@@ -2923,8 +2925,15 @@ void QMenu::mouseReleaseEvent(QMouseEvent *e)
 
     QMenuPrivate::mouseDown = nullptr;
     d->setSyncAction();
-    QAction *action = d->actionAt(e->position().toPoint());
 
+    if (!d->hasMouseMoved(e->globalPosition().toPoint())) {
+        // We don't want to trigger a menu item if the mouse hasn't moved
+        // since the popup was opened. Instead we want to close the menu.
+        d->hideUpToMenuBar();
+        return;
+    }
+
+    QAction *action = d->actionAt(e->position().toPoint());
     if (action && action == d->currentAction) {
         if (!action->menu()) {
 #if defined(Q_OS_WIN)
@@ -2933,7 +2942,7 @@ void QMenu::mouseReleaseEvent(QMouseEvent *e)
 #endif
                 d->activateAction(action, QAction::Trigger);
         }
-    } else if ((!action || action->isEnabled()) && d->hasMouseMoved(e->globalPosition().toPoint())) {
+    } else if (!action || action->isEnabled()) {
         d->hideUpToMenuBar();
     }
 }

tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp

@@ -116,6 +116,7 @@ private slots:
     void deleteWhenTriggered();
 
     void nestedTearOffDetached();
+    void closeMenuOnClickIfMouseHasntMoved();
 
 protected slots:
     void onActivated(QAction*);
@@ -2113,5 +2114,36 @@ void tst_QMenu::nestedTearOffDetached()
     QTest::mouseClick(subSubMenu, Qt::LeftButton, {}, QPoint(subSubMenu->width() / 2, tearOffOffset));
 }
 
+/*!
+    Test that a menu will close if you do a mouse click on top of
+    it without having moved the mouse.
+    (QTBUG-128359).
+*/
+void tst_QMenu::closeMenuOnClickIfMouseHasntMoved()
+{
+    QWidget w;
+    w.resize(100, 100);
+    w.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&w));
+
+    QMenu contextMenu;
+    for (int i = 0; i < 5; ++i) {
+        QAction *action = contextMenu.addAction(QStringLiteral("action"));
+        connect(action, &QAction::triggered, []{ QFAIL("No menu item should trigger"); });
+    }
+
+    const QPoint pos = w.rect().center();
+    const QPoint globalPos = w.mapToGlobal(pos);
+    // Move the mouse inside the window
+    QTest::mouseMove(&w, pos);
+    // Move the menu a bit up, so that a menu item falls underneath the
+    // mouse (similar to the code attached to the bug report: QTBUG-128359).
+    contextMenu.popup(globalPos - QPoint(0, 20));
+    QVERIFY(QTest::qWaitForWindowExposed(&contextMenu));
+    // Do a mouse click without having moved the cursor. This
+    // should close the menu, even if it's underneath the mouse.
+    QTest::mouseClick(&contextMenu, Qt::RightButton, {}, contextMenu.mapFromGlobal(pos));
+}
+
 QTEST_MAIN(tst_QMenu)
 #include "tst_qmenu.moc"

tests/auto/widgets/widgets/qtoolbutton/tst_qtoolbutton.cpp

@@ -130,6 +130,11 @@ void tst_QToolButton::triggered()
 
     m_menu = menu.data();
 
+    // QMenu uses QGuiApplicationPrivate::lastCursorPosition to detect pointer
+    // movement. And GuiApplication needs at least one mouse move to properly
+    // initialize it. So we send a mouse move now, before we open the menu.
+    QTest::mouseMove(mainWidget.windowHandle(), mainWidget.mapFromGlobal(QPoint(0, 0)));
+
     QTimer *timer = new QTimer(this);
     timer->setInterval(50);
     connect(timer, SIGNAL(timeout()), this, SLOT(sendMouseClick()));