From 5c1d96b70a3de9a08a473ffcbcace8d6a7c5fa3b Mon Sep 17 00:00:00 2001 From: Sami Shalayel Date: Wed, 14 Dec 2022 14:15:50 +0100 Subject: [PATCH] qquickpopuppositioner: fix popup mirroring Fixed a bug where QQuickPopupPositioner::reposition() was confusing coordinates obtained by mapToSource() and mapToItem() during mirroring of the popup. This leads to popups at the wrong places, e.g. when a rotated combobox has not enough space to unroll its popup. Translate the coordinates using mapToItem instead of mapToSource after computing the alternative position of the popup (e.g. when it can not be unrolled correctly). Add a test to check if the combobox popup appears at the right places. Skip native styles as they might be pushing the popup around, same for Android as they might have too small screens (which involves more positioning logic then just the mirroring). Fixes: QTBUG-105148 Pick-to: 5.15 6.4 6.2 6.5 Change-Id: I0033509a8824e3a71698f91ef832b94527d8e2c8 Reviewed-by: Oliver Eftevaag --- src/quicktemplates/qquickpopuppositioner.cpp | 8 +- .../qquickpopup/data/mirroredCombobox.qml | 26 +++ .../qquickpopup/data/rotatedCombobox.qml | 26 +++ .../qquickpopup/tst_qquickpopup.cpp | 153 ++++++++++++++++++ 4 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 tests/auto/quickcontrols/qquickpopup/data/mirroredCombobox.qml create mode 100644 tests/auto/quickcontrols/qquickpopup/data/rotatedCombobox.qml diff --git a/src/quicktemplates/qquickpopuppositioner.cpp b/src/quicktemplates/qquickpopuppositioner.cpp index 0e05e189c92..aecbc7373c8 100644 --- a/src/quicktemplates/qquickpopuppositioner.cpp +++ b/src/quicktemplates/qquickpopuppositioner.cpp @@ -129,14 +129,18 @@ void QQuickPopupPositioner::reposition() // if the popup doesn't fit horizontally inside the window, try flipping it around (left <-> right) if (p->allowHorizontalFlip && (rect.left() < bounds.left() || rect.right() > bounds.right())) { - const QRectF flipped(m_parentItem->mapToScene(QPointF(m_parentItem->width() - p->x - rect.width(), p->y)), rect.size()); + const QPointF newTopLeft(m_parentItem->width() - p->x - rect.width(), p->y); + const QRectF flipped(m_parentItem->mapToItem(popupItem->parentItem(), newTopLeft), + rect.size()); if (flipped.intersected(bounds).width() > rect.intersected(bounds).width()) rect.moveLeft(flipped.left()); } // if the popup doesn't fit vertically inside the window, try flipping it around (above <-> below) if (p->allowVerticalFlip && (rect.top() < bounds.top() || rect.bottom() > bounds.bottom())) { - const QRectF flipped(m_parentItem->mapToScene(QPointF(p->x, m_parentItem->height() - p->y - rect.height())), rect.size()); + const QPointF newTopLeft(p->x, m_parentItem->height() - p->y - rect.height()); + const QRectF flipped(m_parentItem->mapToItem(popupItem->parentItem(), newTopLeft), + rect.size()); if (flipped.intersected(bounds).height() > rect.intersected(bounds).height()) rect.moveTop(flipped.top()); } diff --git a/tests/auto/quickcontrols/qquickpopup/data/mirroredCombobox.qml b/tests/auto/quickcontrols/qquickpopup/data/mirroredCombobox.qml new file mode 100644 index 00000000000..ed48179bced --- /dev/null +++ b/tests/auto/quickcontrols/qquickpopup/data/mirroredCombobox.qml @@ -0,0 +1,26 @@ +// Copyright (C) 2022 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause + +import QtQuick +import QtQuick.Controls + +Window { + width: 400 + height: 400 + + contentItem.rotation: 180 + + ComboBox { + objectName: "first" + x: 100 + y: 300 // is missing space, needs to unroll in the "mirrored" direction + model: ["First", "Second", "Third", "Fourth", "Fifth"] + } + + ComboBox { + objectName: "second" + x: 200 + y: 100 // has enough space to unroll + model: ["A", "B", "C"] + } +} diff --git a/tests/auto/quickcontrols/qquickpopup/data/rotatedCombobox.qml b/tests/auto/quickcontrols/qquickpopup/data/rotatedCombobox.qml new file mode 100644 index 00000000000..df217be4b76 --- /dev/null +++ b/tests/auto/quickcontrols/qquickpopup/data/rotatedCombobox.qml @@ -0,0 +1,26 @@ +// Copyright (C) 2022 The Qt Company Ltd. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR BSD-3-Clause + +import QtQuick +import QtQuick.Controls + +Window { + width: 400 + height: 400 + + contentItem.rotation: 90 + + ComboBox { + objectName: "first" + x: 100 + y: 320 // is missing space, needs to unroll in the "mirrored" direction + model: ["First", "Second", "Third", "Fourth", "Fifth"] + } + + ComboBox { + objectName: "second" + x: 200 + y: 100 // has enough space to unroll + model: ["A", "B", "C"] + } +} diff --git a/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp b/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp index 607603d28f6..ecd5479931b 100644 --- a/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp +++ b/tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp @@ -89,12 +89,16 @@ private slots: void shrinkPopupThatWasLargerThanWindow_data(); void shrinkPopupThatWasLargerThanWindow(); void relativeZOrder(); + void mirroredCombobox(); + void rotatedCombobox(); private: static bool hasWindowActivation(); QScopedPointer touchScreen = QScopedPointer(QTest::createTouchDevice()); }; +using namespace Qt::StringLiterals; + tst_QQuickPopup::tst_QQuickPopup() : QQmlDataTest(QT_QMLTEST_DATADIR) { @@ -1958,6 +1962,155 @@ void tst_QQuickPopup::relativeZOrder() QCOMPARE(overlayPrivate->paintOrderChildItems().last(), subDialog->popupItem()); } +void tst_QQuickPopup::mirroredCombobox() +{ +#ifdef Q_OS_ANDROID + // Android screens might be pretty small, such that additional + // repositioning (apart from the mirroring) will happen to the + // popups and mess up the expected positions below. + QSKIP("Skipping test for Android."); +#endif + QStringList nativeStyles; + nativeStyles.append(u"macOS"_s); + nativeStyles.append(u"iOS"_s); + nativeStyles.append(u"Windows"_s); + if (nativeStyles.contains(QQuickStyle::name())) + QSKIP("Skipping test for native styles: they might rearrange their combobox the way they " + "want."); + + QQuickControlsApplicationHelper helper(this, "mirroredCombobox.qml"); + QVERIFY2(helper.ready, helper.failureMessage()); + + QQuickWindow *window = helper.window; + window->show(); + QVERIFY(QTest::qWaitForWindowExposed(window)); + + { + QQuickComboBox *comboBox = window->findChild("first"); + QVERIFY(comboBox); + QQuickPopup *popup = comboBox->popup(); + QVERIFY(popup); + popup->open(); + QTRY_COMPARE(popup->isVisible(), true); + const QPointF popupPos(popup->contentItem()->mapToItem(comboBox->parentItem(), + popup->contentItem()->position())); + const QSizeF popupSize(popup->contentItem()->size()); + + // ignore popup.{top,bottom}Padding() as not included in popup->contentItem()->size() + // some styles prefer to draw the popup "over" (in z-axis direction) the combobox to hide + // the combobox + const bool styleDrawsPopupOverCombobox = + comboBox->position().y() - popupSize.height() + comboBox->size().height() + == popupPos.y(); + // some styles prefer to draw the popup below (in y-axis direction) the combobox + const bool styleDrawsPopupBelowCombobox = + comboBox->position().y() - popupSize.height() + comboBox->topPadding() + == popupPos.y(); + + QVERIFY(styleDrawsPopupOverCombobox || styleDrawsPopupBelowCombobox); + + popup->close(); + } + + { + QQuickComboBox *comboBox = window->findChild("second"); + QVERIFY(comboBox); + QQuickPopup *popup = comboBox->popup(); + QVERIFY(popup); + popup->open(); + QTRY_COMPARE(popup->isVisible(), true); + const QPointF popupPos(popup->contentItem()->mapToItem(comboBox->parentItem(), + popup->contentItem()->position())); + + // some styles prefer to draw the popup "over" (in z-axis direction) the combobox to hide + // the combobox + const bool styleDrawsPopupOverCombobox = comboBox->position().y() + comboBox->topPadding() + + popup->topPadding() + popup->bottomPadding() + == popupPos.y(); + // some styles prefer to draw the popup above (in y-axis direction) the combobox + const bool styleDrawsPopupAboveCombobox = + comboBox->position().y() + comboBox->height() - comboBox->topPadding() + == popupPos.y(); + + QVERIFY(styleDrawsPopupOverCombobox || styleDrawsPopupAboveCombobox); + + popup->close(); + } +} + +void tst_QQuickPopup::rotatedCombobox() +{ +#ifdef Q_OS_ANDROID + // Android screens might be pretty small, such that additional + // repositioning (apart from the rotating) will happen to the + // popups and mess up the expected positions below. + QSKIP("Skipping test for Android."); +#endif + QStringList nativeStyles; + nativeStyles.append(u"macOS"_s); + nativeStyles.append(u"iOS"_s); + nativeStyles.append(u"Windows"_s); + if (nativeStyles.contains(QQuickStyle::name())) + QSKIP("Skipping test for native styles: they might rearrange their combobox the way they " + "want."); + + QQuickControlsApplicationHelper helper(this, "rotatedCombobox.qml"); + QVERIFY2(helper.ready, helper.failureMessage()); + + QQuickWindow *window = helper.window; + window->show(); + QVERIFY(QTest::qWaitForWindowExposed(window)); + + { + QQuickComboBox *comboBox = window->findChild("first"); + QVERIFY(comboBox); + QQuickPopup *popup = comboBox->popup(); + QVERIFY(popup); + popup->open(); + QTRY_COMPARE(popup->isVisible(), true); + const QPointF popupPos(popup->contentItem()->mapToItem(comboBox->parentItem(), + popup->contentItem()->position())); + const QSizeF popupSize(popup->contentItem()->size()); + + // ignore popup.{left,right}Padding() as not included in popup->contentItem()->size() + // some styles prefer to draw the popup "over" (in z-axis direction) the combobox to hide + // the combobox + const bool styleDrawsPopupOverCombobox = + comboBox->position().x() - popupSize.width() + comboBox->width() == popupPos.x(); + // some styles prefer to draw the popup right (in x-axis direction) of the combobox + const bool styleDrawsPopupBelowCombobox = + comboBox->position().x() - popupSize.width() - comboBox->leftPadding() + == popupPos.x(); + + QVERIFY(styleDrawsPopupOverCombobox || styleDrawsPopupBelowCombobox); + } + + { + QQuickComboBox *comboBox = window->findChild("second"); + QVERIFY(comboBox); + QQuickPopup *popup = comboBox->popup(); + QVERIFY(popup); + popup->open(); + QTRY_COMPARE(popup->isVisible(), true); + const QPointF popupPos(popup->contentItem()->mapToItem(comboBox->parentItem(), + popup->contentItem()->position())); + + // some styles prefer to draw the popup "over" (in z-axis direction) the combobox to hide + // the combobox + const bool styleDrawsPopupOverCombobox = comboBox->position().x() + comboBox->leftPadding() + + popup->leftPadding() + popup->rightPadding() + == popupPos.x(); + // some styles prefer to draw the popup left (in y-axis direction) of the combobox + const bool styleDrawsPopupAboveCombobox = + comboBox->position().x() + comboBox->width() - comboBox->leftPadding() + == popupPos.x(); + + QVERIFY(styleDrawsPopupOverCombobox || styleDrawsPopupAboveCombobox); + + popup->close(); + } +} + QTEST_QUICKCONTROLS_MAIN(tst_QQuickPopup) #include "tst_qquickpopup.moc"