QComboBox: Don't dereference potential nullptr, simplify

Amends a874087504cf5af8bb4171d4137f23f100b7063b, which tested whether
d->container is nullptr to decide whether to hide the popup, and then
dereferences d->container later without checking again. This raised a
correct static analyzer warning.

Simplify that logic. hidePopup() does nothing if there is no visible
container, and we don't want to accept() the cancel key if there isn't.
So the closeOnCancel logic isn't actually needed, we only need to accept
the ShortcutOverride to make sure that QComboBox sees the Cancel key
even if there is a shortcut registered, and then we can handle and
accept the cancel key to call hidePopup() only if the popup is visible.

Add test to verify that this interaction works as expected.

Pick-to: 6.4
Task-number: QTBUG-108908
Change-Id: I60d92b068f0f5139d629cf4a58e225512170df77
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

3 files changed, 54 insertions, 6 deletions

diff --git a/src/widgets/widgets/qcombobox.cpp b/src/widgets/widgets/qcombobox.cpp
index 5889545b1c..8b02f7c494 100644
--- a/src/widgets/widgets/qcombobox.cpp
+++ b/src/widgets/widgets/qcombobox.cpp
@@ -727,8 +727,7 @@ bool QComboBoxPrivateContainer::eventFilter(QObject *o, QEvent *e)
             return true;
         default:
 #if QT_CONFIG(shortcut)
-            if (keyEvent->matches(QKeySequence::Cancel)) {
-                closeOnCancel = true;
+            if (keyEvent->matches(QKeySequence::Cancel) && isVisible()) {
                 keyEvent->accept();
                 return true;
             }
@@ -776,7 +775,6 @@ bool QComboBoxPrivateContainer::eventFilter(QObject *o, QEvent *e)
 
 void QComboBoxPrivateContainer::showEvent(QShowEvent *)
 {
-    closeOnCancel = true;
     combo->update();
 }
 
@@ -3218,10 +3216,9 @@ void QComboBox::keyPressEvent(QKeyEvent *e)
         break;
 #endif
     default:
-        if (e->matches(QKeySequence::Cancel) && (!d->container || d->container->closeOnCancel)) {
+        if (d->container && d->container->isVisible() && e->matches(QKeySequence::Cancel)) {
             hidePopup();
             e->accept();
-            d->container->closeOnCancel = false;
         }
 
         if (!d->lineEdit) {
diff --git a/src/widgets/widgets/qcombobox_p.h b/src/widgets/widgets/qcombobox_p.h
index 2a694d27a4..8e686ac79a 100644
--- a/src/widgets/widgets/qcombobox_p.h
+++ b/src/widgets/widgets/qcombobox_p.h
@@ -221,7 +221,6 @@ private:
     QComboBoxPrivateScroller *bottom = nullptr;
     QElapsedTimer popupTimer;
     bool maybeIgnoreMouseButtonRelease = false;
-    bool closeOnCancel = false;
 
     friend class QComboBox;
     friend class QComboBoxPrivate;
diff --git a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
index 5b9f8f4f43..4bbf8f7b5c 100644
--- a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
+++ b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
@@ -24,6 +24,7 @@
 #include <qtablewidget.h>
 #include <qscrollbar.h>
 #include <qboxlayout.h>
+#include <qshortcut.h>
 #include <qstackedwidget.h>
 
 #include <qstandarditemmodel.h>
@@ -149,6 +150,7 @@ private slots:
     void propagateStyleChanges();
     void buttonPressKeys();
     void clearModel();
+    void cancelClosesPopupNotDialog();
 
 private:
     PlatformInputContext m_platformInputContext;
@@ -3635,5 +3637,55 @@ void tst_QComboBox::clearModel()
     QCOMPARE(combo.currentText(), QString());
 }
 
+void tst_QComboBox::cancelClosesPopupNotDialog()
+{
+    if (QGuiApplication::platformName() == "offscreen")
+        QSKIP("The offscreen platform plugin doesn't activate popups.");
+
+    QDialog dialog;
+    QComboBox combobox;
+    combobox.addItems({"A", "B", "C"});
+
+    std::unique_ptr<QShortcut> shortcut(new QShortcut(QKeySequence::Cancel, &dialog));
+    bool shortcutTriggered = false;
+    connect(shortcut.get(), &QShortcut::activated, [&shortcutTriggered]{
+        shortcutTriggered = true;
+    });
+
+    QVBoxLayout vbox;
+    vbox.addWidget(&combobox);
+    dialog.setLayout(&vbox);
+
+    dialog.show();
+    QVERIFY(QTest::qWaitForWindowActive(&dialog));
+
+    // while the combobox is closed, escape key triggers the shortcut
+    QTest::keyClick(dialog.window()->windowHandle(), Qt::Key_Escape);
+    QVERIFY(shortcutTriggered);
+    shortcutTriggered = false;
+
+    combobox.showPopup();
+    QTRY_VERIFY(combobox.view()->isVisible());
+
+    // an open combobox overrides and accepts the escape key to close
+    QTest::keyClick(dialog.window()->windowHandle(), Qt::Key_Escape);
+    QVERIFY(!shortcutTriggered);
+    shortcutTriggered = false;
+    QTRY_VERIFY(!combobox.view()->isVisible());
+    QVERIFY(dialog.isVisible());
+
+    // once closed, escape key triggers the shortcut again
+    QTest::keyClick(dialog.window()->windowHandle(), Qt::Key_Escape);
+    QVERIFY(shortcutTriggered);
+    shortcutTriggered = false;
+    QVERIFY(dialog.isVisible());
+
+    shortcut.reset();
+
+    // without shortcut, escape key propagates to the parent
+    QTest::keyClick(dialog.window()->windowHandle(), Qt::Key_Escape);
+    QVERIFY(!dialog.isVisible());
+}
+
 QTEST_MAIN(tst_QComboBox)
 #include "tst_qcombobox.moc"