Skip to content

Commit

Permalink
qquickpopuppositioner: fix popup mirroring
Browse files Browse the repository at this point in the history
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 <oliver.eftevaag@qt.io>
  • Loading branch information
samishalayel committed Dec 20, 2022
1 parent e09b72f commit 5c1d96b
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/quicktemplates/qquickpopuppositioner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
26 changes: 26 additions & 0 deletions tests/auto/quickcontrols/qquickpopup/data/mirroredCombobox.qml
Original file line number Diff line number Diff line change
@@ -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"]
}
}
26 changes: 26 additions & 0 deletions tests/auto/quickcontrols/qquickpopup/data/rotatedCombobox.qml
Original file line number Diff line number Diff line change
@@ -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"]
}
}
153 changes: 153 additions & 0 deletions tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,16 @@ private slots:
void shrinkPopupThatWasLargerThanWindow_data();
void shrinkPopupThatWasLargerThanWindow();
void relativeZOrder();
void mirroredCombobox();
void rotatedCombobox();

private:
static bool hasWindowActivation();
QScopedPointer<QPointingDevice> touchScreen = QScopedPointer<QPointingDevice>(QTest::createTouchDevice());
};

using namespace Qt::StringLiterals;

tst_QQuickPopup::tst_QQuickPopup()
: QQmlDataTest(QT_QMLTEST_DATADIR)
{
Expand Down Expand Up @@ -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<QQuickComboBox *>("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<QQuickComboBox *>("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<QQuickComboBox *>("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<QQuickComboBox *>("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"

0 comments on commit 5c1d96b

Please sign in to comment.