QComboBox: scroll the popup to the top before positioning it

If the style changes while a popup is open (or about to close),
the popup will change style as well before it's hidden. And
this can result in the popup window briefly ending up smaller
than what it needs to be, in order to fit all the menu items.
In that case, it will show 'up' and 'down' widgets in the
menu that auto scrolls it when hovered. And all this can
happen for a split second while the menu is about to close
(as a result of the user clicking on a menu item).

A bug happens because of this if you click on the last menu
item in the list, and this causes the style to change. In
that case, the 'down' widget will end up directly underneath
the mouse for a split second, which will trigger an auto-scroll
timer to start. This timer will trigger a bit later, after
the popup has been hidden, and scroll the list view a bit down.
The result is that the next time you open the popup, it ends up
at the wrong place on the screen in a failed attempt to center
the current index on top of the combobox.

This patch will make sure that we always scroll the list view
to the top before we start calculating where the popup should
be placed on the screen. Otherwise the geometry ends up wrong
since the popup will anyway be resized (if possible) to fit
all the menu items before it's shown and should therefore not
take scrolling into account.

Fixes: QTBUG-113765
Change-Id: I61b5b832904de471c2303fc67325feec322b1449
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>
(cherry picked from commit 8393922e7071221a9c6c0811eb714f20bf4ed02b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

2 files changed, 61 insertions, 1 deletions

diff --git a/src/widgets/widgets/qcombobox.cpp b/src/widgets/widgets/qcombobox.cpp
index db957014c6..67c7452627 100644
--- a/src/widgets/widgets/qcombobox.cpp
+++ b/src/widgets/widgets/qcombobox.cpp
@@ -2683,7 +2683,13 @@ void QComboBox::showPopup()
         listRect.moveLeft(above.x());
 
         // Position vertically so the currently selected item lines up
-        // with the combo box.
+        // with the combo box. In order to do that, make sure that the item
+        // view is scrolled to the top first, otherwise calls to view()->visualRect()
+        // will return the geometry the selected item had the last time the popup
+        // was visible (and perhaps scrolled). And this will not match the geometry
+        // it will actually have when we resize the container to fit all the items
+        // further down in this function.
+        view()->scrollToTop();
         const QRect currentItemRect = view()->visualRect(view()->currentIndex());
         const int offset = listRect.top() - currentItemRect.top();
         listRect.moveTop(above.y() + offset - listRect.top());
diff --git a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
index f632844002..cb4a99e30a 100644
--- a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
+++ b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
@@ -42,6 +42,7 @@
 #include <qstandarditemmodel.h>
 #include <qproxystyle.h>
 #include <qfont.h>
+#include <qstylehints.h>
 
 #include "../../../shared/platforminputcontext.h"
 #include <private/qinputmethod_p.h>
@@ -109,6 +110,7 @@ private slots:
 #ifndef QT_NO_STYLE_FUSION
     void task190351_layout();
     void task191329_size();
+    void popupPositionAfterStyleChange();
 #endif
     void task166349_setEditableOnReturn();
     void task190205_setModelAdjustToContents();
@@ -3388,6 +3390,58 @@ void tst_QComboBox::task_QTBUG_56693_itemFontFromModel()
     box.hidePopup();
 }
 
+void tst_QComboBox::popupPositionAfterStyleChange()
+{
+    // Check that the popup opens up centered on top of the current
+    // index if the style has changed since the last time it was
+    // opened (QTBUG-113765).
+    QComboBox box;
+    QStyleOptionComboBox opt;
+    const bool usePopup = qApp->style()->styleHint(QStyle::SH_ComboBox_Popup, &opt, &box);
+    if (!usePopup)
+        QSKIP("This test is only relevant for styles that centers the popup on top of the combo!");
+
+    box.addItems({"first", "middle", "last"});
+    box.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&box));
+    box.showPopup();
+
+    QFrame *container = box.findChild<QComboBoxPrivateContainer *>();
+    QVERIFY(container);
+    QVERIFY(QTest::qWaitForWindowExposed(container));
+
+    // Select the last menu item, which will close the popup. This item is then expected
+    // to be centered on top of the combobox the next time the popup opens.
+    const QRect lastItemRect = box.view()->visualRect(box.view()->model()->index(2, 0));
+    QTest::mouseClick(box.view(), Qt::LeftButton, Qt::NoModifier, lastItemRect.center());
+
+    // Change style. This can make the popup smaller, which will result in up-and-down
+    // scroll widgets showing in the menu, directly underneath the mouse before the popup
+    // ends up hidden. This again will trigger the item view to scroll, which seems to be
+    // the root cause of QTBUG-113765.
+    qApp->setStyle(QStringLiteral("Fusion"));
+
+    // Click on the combobox again to reopen it. But since both QComboBox
+    // (QComboBoxPrivateScroller) is using its own internal timer to do scrolling, we
+    // need to wait a bit until the scrolling is done before we can reopen it (since
+    // the scrolling is the sore spot that we want to test).
+    // But note, we expect, but don't require, the popup to scroll. And for that
+    // reason, we don't see it as a failure if the scrolling doesn't happen.
+    (void) QTest::qWaitFor([&box]{ return box.view()->verticalScrollBar()->value() > 0; }, 1000);
+
+    // Verify that the popup is hidden before we click the button
+    QTRY_VERIFY(!container->isVisible());
+    QTest::mouseClick(&box, Qt::LeftButton);
+
+    // Click on item under mouse. But wait a bit, to avoid a double click
+    QTest::qWait(qApp->styleHints()->mouseDoubleClickInterval());
+    QTest::mouseClick(&box, Qt::LeftButton);
+
+    // Ensure that the item that was centered on top of the combobox, and which
+    // we therefore clicked, was the same item we clicked on the first time.
+    QCOMPARE(box.currentText(), QStringLiteral("last"));
+}
+
 void tst_QComboBox::inputMethodUpdate()
 {
     if (QGuiApplication::platformName().startsWith(QLatin1String("wayland"), Qt::CaseInsensitive))