Skip to content

Commit

Permalink
Fix shrinking issue when binding a Dialogs padding to SafeArea margins
Browse files Browse the repository at this point in the history
When running the following code:
```
Window {
  visible: true
  width: 600
  height: 600
  flags: Qt.ExpandedClientAreaHint | Qt.NoTitleBarBackgroundHint

  Dialog {
    id: popup
    popupType: Popup.Window
    topPadding: SafeArea.margins.top
    width: 300
    height: 300
    modal: true
    Component.onCompleted: popup.open()
  }
}
```
The Dialog would end up with a height of 6 pixels, effectively ignoring
the binding on height, which should have caused the dialog's height to
be 300.

This happened, because the popup item was moved to the popup window
after the popup window's size was set, but before showing the popup
window. When the popup item was moved to the popup window, it would
cause its attached SafeArea object to update, which would cause bindings
that are using safe area margins, to be reevaluated. When binding on a
dialogs padding, the dialogs implicitWidth/implicitHeight would also
need to be reevaluated, due to the default binding that each style is
using. The implicitWidth/implicitHeight should add both the dialogs padding,
and the contentItem's size, in its calculation, however, due to deferred
execution, the contentItem's size is 0 at this point in time, which
causes the binding to evaluate to a really small value.

The popup window listens to changes in the popup's implicitWidth and
implicitHeight, and resizes itself automatically to fit the contentItem's
new geometry. This means the window's initial resize of 300x300, got
overwritten by a much lower value, during all the binding reevaluations
mentioned above, which is catalysted by changing the popup item's parent
to be the popup window's root item.

The issue can be easily fixed by doing the reparenting first, before resizing
the popup window to be popup item's size + insets.

Pick-to: 6.9
Change-Id: I0e2ae69c84c69a49681c4b648790539c5d6da88e
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>
  • Loading branch information
Oliver Eftevaag committed Dec 27, 2024
1 parent 7ed50b4 commit 422601b
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/quicktemplates/qquickpopup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1104,8 +1104,12 @@ void QQuickPopupPrivate::adjustPopupItemParentAndWindow()
if (visible) {
if (!popupWindow) {
popupWindow = new QQuickPopupWindow(q, window);
popupWindow->setWidth(popupItem->width() + windowInsets().left() + windowInsets().right());
popupWindow->setHeight(popupItem->height() + windowInsets().top() + windowInsets().bottom());
// Changing the visual parent can cause the popup item's implicit width/height to change.
// We store the initial size here first, to ensure that we're resizing the window to the correct size.
const qreal initialWidth = popupItem->width() + windowInsets().left() + windowInsets().right();
const qreal initialHeight = popupItem->height() + windowInsets().top() + windowInsets().bottom();
popupItem->setParentItem(popupWindow->contentItem());
popupWindow->resize(initialWidth, initialHeight);
popupWindow->setModality(modal ? Qt::ApplicationModal : Qt::NonModal);
popupItem->resetTitle();
popupWindow->setTitle(m_title);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import QtQuick
import QtQuick.Controls

Window {
width: 600
height: 600
flags: Qt.ExpandedClientAreaHint | Qt.NoTitleBarBackgroundHint

SafeArea.additionalMargins.top: 50
Dialog {
id: popup
popupType: Popup.Window
topPadding: SafeArea.margins.top
width: 300
height: 300
modal: true
}
}
37 changes: 37 additions & 0 deletions tests/auto/quickcontrols/qquickpopup/tst_qquickpopup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ private slots:
void resetHoveredStateForItemsWithinPopup();
void noInfiniteRecursionOnParentWindowDestruction();
void popupWindowDestructedBeforeQQuickPopup();
void popupWindowWithPaddingFromSafeArea();

private:
QScopedPointer<QPointingDevice> touchScreen = QScopedPointer<QPointingDevice>(QTest::createTouchDevice());
Expand Down Expand Up @@ -3212,6 +3213,42 @@ void tst_QQuickPopup::popupWindowDestructedBeforeQQuickPopup()
// Doesn't crash on destruction
}

void tst_QQuickPopup::popupWindowWithPaddingFromSafeArea()
{
if (!popupWindowsSupported)
QSKIP("The platform doesn't support popup windows. Skipping test.");

QQuickControlsApplicationHelper helper(this, "DialogWithPaddingFromSafeArea.qml");
QVERIFY2(helper.ready, helper.failureMessage());
QQuickWindow *window = helper.window;
QVERIFY(window);
window->show();
QVERIFY(QTest::qWaitForWindowExposed(window));

auto *dialog = helper.window->contentItem()->findChild<QQuickDialog *>();
QVERIFY(dialog);

const qreal dialogWidth = dialog->width();
const qreal dialogHeight = dialog->height();

dialog->open();
QTRY_VERIFY(dialog->isOpened());

QQuickPopupPrivate* popupPrivate = QQuickPopupPrivate::get(dialog);

QTRY_VERIFY(popupPrivate->popupWindow);
auto *popupWindow = popupPrivate->popupWindow;
QVERIFY(QTest::qWaitForWindowExposed(popupWindow));
QQuickTest::qWaitForPolish(popupWindow);

// Verify that the binding on topPadding didn't cause
// a change in geometry after the popup window was shown.
QCOMPARE(popupWindow->width(), dialogWidth);
QCOMPARE(popupWindow->height(), dialogHeight);
QCOMPARE(dialog->width(), dialogWidth);
QCOMPARE(dialog->height(), dialogHeight);
}

QTEST_QUICKCONTROLS_MAIN(tst_QQuickPopup)

#include "tst_qquickpopup.moc"

0 comments on commit 422601b

Please sign in to comment.