From 15317f352e8181ac2efd152eea6cc0d0b3e02e3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tor=20Arne=20Vestb=C3=B8?= Date: Tue, 24 Oct 2023 00:14:47 +0200 Subject: [PATCH] Explicitly focus automatic default button in QDialogButtonBox inside dialog QDialogButtonBox has logic to automatically resolve a default button, in case one is not already set via QPushButton directly, or via e.g. QMessageBox::setDefaultButton(). Unfortunately, this default button can in some cases be overridden by the first button in the dialog button box. The reason this happens is that the first button is the focus proxy of the button box, so when the button box gains focus, it will transfer it to the first button. And since QPushButtons inside a QDialog have their autoDefault property set to true by default, this focus transfer will also reset the default button to the focused button. This arbitrarily happens for any role that happens to be earlier in the QDialogButtonBox::ButtonLayout than the AcceptRole, and can be very confusing for the user, so we try to avoid the situation by explicitly setting the automatic default button as the focus widget of the button box, unless one is set already. This is also what QMessageBox::setDefaultButton() does internally. The only case we're not covering is when the user sets a push button as default via QPushButton::setDefault(), but we conservatively assume that the user then also calls setFocus() on the button. Pick-to: 6.5 Change-Id: Ibdac0a51ba9439321d4fd7a1dac1ed695e11d693 Reviewed-by: Volker Hilsheimer (cherry picked from commit d44413d526ec12ed83acd7343c2005782178c7ad) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/widgets/qdialogbuttonbox.cpp | 14 +++++- .../qdialogbuttonbox/tst_qdialogbuttonbox.cpp | 49 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/widgets/widgets/qdialogbuttonbox.cpp b/src/widgets/widgets/qdialogbuttonbox.cpp index c35f8c8edcf..91d846d9a0b 100644 --- a/src/widgets/widgets/qdialogbuttonbox.cpp +++ b/src/widgets/widgets/qdialogbuttonbox.cpp @@ -1013,8 +1013,20 @@ void QDialogButtonBoxPrivate::ensureFirstAcceptIsDefault() break; } } - if (!hasDefault && firstAcceptButton) + if (!hasDefault && firstAcceptButton) { firstAcceptButton->setDefault(true); + // When the QDialogButtonBox is focused, and it doesn't have an + // explicit focus widget, it will transfer focus to its focus + // proxy, which is the first button in the layout. This behavior, + // combined with the behavior that QPushButtons in a QDialog will + // by default have their autoDefault set to true, results in the + // focus proxy/first button stealing the default button status + // immediately when the button box is focused, which is not what + // we want. Account for this by explicitly making the firstAcceptButton + // focused as well, unless an explicit focus widget has been set. + if (dialog && !dialog->focusWidget()) + firstAcceptButton->setFocus(); + } } void QDialogButtonBoxPrivate::disconnectAll() diff --git a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp index 3f5ffa687ba..9f405a8bebe 100644 --- a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp +++ b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp @@ -81,6 +81,7 @@ private slots: void task191642_default(); void testDeletedStandardButton(); + void automaticDefaultButton(); private: qint64 timeStamp; @@ -909,5 +910,53 @@ void tst_QDialogButtonBox::testDeletedStandardButton() QVERIFY(!buttonC); } +void tst_QDialogButtonBox::automaticDefaultButton() +{ + // Having a QDialogButtonBox inside a QDialog triggers Qt to + // enable autoDefault for QPushButtons inside the button box. + // Check that the logic for resolving a default button based + // on the Accept role is not overridden by the first button + // in the dialog (the focus proxy) taking focus, and hence + // stealing the default button state. + + { + QDialog dialog; + QDialogButtonBox *bb = new QDialogButtonBox(&dialog); + // Force horizontal orientation, where we know the order between + // Reset and Accept roles are always the same for all layouts. + bb->setOrientation(Qt::Horizontal); + auto *okButton = bb->addButton(QDialogButtonBox::Ok); + auto *resetButton = bb->addButton(QDialogButtonBox::Reset); + // Double check our assumption about Reset being first + QCOMPARE(bb->layout()->itemAt(0)->widget(), resetButton); + + dialog.show(); + QVERIFY(QTest::qWaitForWindowActive(&dialog)); + + QVERIFY(okButton->isDefault()); + QSignalSpy buttonClicked(okButton, &QPushButton::clicked); + QTest::keyPress(&dialog, Qt::Key_Enter); + QCOMPARE(buttonClicked.count(), 1); + } + + // However, if an explicit button has been focused, we respect that. + + { + QDialog dialog; + QDialogButtonBox *bb = new QDialogButtonBox(&dialog); + bb->setOrientation(Qt::Horizontal); + bb->addButton(QDialogButtonBox::Ok); + auto *resetButton = bb->addButton(QDialogButtonBox::Reset); + resetButton->setFocus(); + dialog.show(); + QVERIFY(QTest::qWaitForWindowActive(&dialog)); + + QVERIFY(resetButton->isDefault()); + QSignalSpy buttonClicked(resetButton, &QPushButton::clicked); + QTest::keyPress(&dialog, Qt::Key_Enter); + QCOMPARE(buttonClicked.count(), 1); + } +} + QTEST_MAIN(tst_QDialogButtonBox) #include "tst_qdialogbuttonbox.moc"