From 63428b1f1fac614cd7d372962f6dcb1d61865169 Mon Sep 17 00:00:00 2001 From: Axel Spoerl Date: Sat, 24 Jun 2023 11:52:18 +0200 Subject: [PATCH] QDialogButtonBox - Update focus chain when buttons show or hide Hiding a button in a QDialogButtonBox doesn't remove its default and focus behavior. Hiding the button shown in the first position, breaks the focus chain. Tabbing between the button is no longer possible. This patch implements listening to the buttons' HideToParent and ShowToParent events. Hidden buttons are removed from the button box and kept in a separate hash. That ensures focus chain consistency. When they are shown again, they are added to the button logic and their default/focus behavior is restored. An autotest is added in tst_QDialogButtonBox. Fixes: QTBUG-114377 Change-Id: Id10c4675f43d6007206e41c694688c4f0a34ee52 Reviewed-by: Volker Hilsheimer Reviewed-by: Qt CI Bot (cherry picked from commit bbb71e7e80f292c2e69faef81b1624832981147e) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/widgets/qdialogbuttonbox.cpp | 95 +++++++++++++++++-- src/widgets/widgets/qdialogbuttonbox.h | 2 + src/widgets/widgets/qdialogbuttonbox_p.h | 7 +- .../widgets/qdialogbuttonbox/CMakeLists.txt | 1 + .../qdialogbuttonbox/tst_qdialogbuttonbox.cpp | 50 ++++++++++ 5 files changed, 145 insertions(+), 10 deletions(-) diff --git a/src/widgets/widgets/qdialogbuttonbox.cpp b/src/widgets/widgets/qdialogbuttonbox.cpp index 8d8c82e67be..f319db7e10f 100644 --- a/src/widgets/widgets/qdialogbuttonbox.cpp +++ b/src/widgets/widgets/qdialogbuttonbox.cpp @@ -112,7 +112,6 @@ QT_BEGIN_NAMESPACE \sa QMessageBox, QPushButton, QDialog */ - QDialogButtonBoxPrivate::QDialogButtonBoxPrivate(Qt::Orientation orient) : orientation(orient), buttonLayout(nullptr), center(false) { @@ -172,6 +171,7 @@ void QDialogButtonBoxPrivate::layoutButtons() Q_Q(QDialogButtonBox); const int MacGap = 36 - 8; // 8 is the default gap between a widget and a spacer item + QBoolBlocker blocker(byPassEventFilter); for (int i = buttonLayout->count() - 1; i >= 0; --i) { QLayoutItem *item = buttonLayout->takeAt(i); if (QWidget *widget = item->widget()) @@ -367,11 +367,22 @@ QPushButton *QDialogButtonBoxPrivate::createButton(QDialogButtonBox::StandardBut } void QDialogButtonBoxPrivate::addButton(QAbstractButton *button, QDialogButtonBox::ButtonRole role, - LayoutRule layoutRule) + LayoutRule layoutRule, AddRule addRule) { - QObjectPrivate::connect(button, &QAbstractButton::clicked, this, &QDialogButtonBoxPrivate::handleButtonClicked); - QObjectPrivate::connect(button, &QAbstractButton::destroyed, this, &QDialogButtonBoxPrivate::handleButtonDestroyed); + Q_Q(QDialogButtonBox); buttonLists[role].append(button); + switch (addRule) { + case AddRule::Connect: + QObjectPrivate::connect(button, &QAbstractButton::clicked, + this, &QDialogButtonBoxPrivate::handleButtonClicked); + QObjectPrivate::connect(button, &QAbstractButton::destroyed, + this, &QDialogButtonBoxPrivate::handleButtonDestroyed); + button->installEventFilter(q); + break; + case AddRule::SkipConnect: + break; + } + switch (layoutRule) { case LayoutRule::DoLayout: layoutButtons(); @@ -453,10 +464,13 @@ QDialogButtonBox::QDialogButtonBox(StandardButtons buttons, Qt::Orientation orie */ QDialogButtonBox::~QDialogButtonBox() { + Q_D(QDialogButtonBox); + + d->byPassEventFilter = true; + // QObjectPrivate::connect requires explicit disconnect in destructor // otherwise the connection may kick in on child destruction and reach // the parent's destroyed private object - Q_D(QDialogButtonBox); d->disconnectAll(); } @@ -622,22 +636,32 @@ void QDialogButtonBox::clear() } /*! - Returns a list of all the buttons that have been added to the button box. + Returns a list of all buttons that have been added to the button box. \sa buttonRole(), addButton(), removeButton() */ QList QDialogButtonBox::buttons() const { Q_D(const QDialogButtonBox); + return d->allButtons(); +} + +QList QDialogButtonBoxPrivate::visibleButtons() const +{ QList finalList; - for (int i = 0; i < NRoles; ++i) { - const QList &list = d->buttonLists[i]; + for (int i = 0; i < QDialogButtonBox::NRoles; ++i) { + const QList &list = buttonLists[i]; for (int j = 0; j < list.size(); ++j) finalList.append(list.at(j)); } return finalList; } +QList QDialogButtonBoxPrivate::allButtons() const +{ + return visibleButtons() << hiddenButtons.keys(); +} + /*! Returns the button role for the specified \a button. This function returns \l InvalidRole if \a button is \nullptr or has not been added to the button box. @@ -654,7 +678,7 @@ QDialogButtonBox::ButtonRole QDialogButtonBox::buttonRole(QAbstractButton *butto return ButtonRole(i); } } - return InvalidRole; + return d->hiddenButtons.value(button, InvalidRole); } /*! @@ -670,9 +694,13 @@ void QDialogButtonBox::removeButton(QAbstractButton *button) void QDialogButtonBoxPrivate::removeButton(QAbstractButton *button, RemoveRule rule) { + Q_Q(QDialogButtonBox); if (!button) return; + // Remove it from hidden buttons + hiddenButtons.remove(button); + // Remove it from the standard button hash first and then from the roles standardButtonHash.remove(reinterpret_cast(button)); for (int i = 0; i < QDialogButtonBox::NRoles; ++i) @@ -685,6 +713,7 @@ void QDialogButtonBoxPrivate::removeButton(QAbstractButton *button, RemoveRule r this, &QDialogButtonBoxPrivate::handleButtonClicked); QObjectPrivate::disconnect(button, &QAbstractButton::destroyed, this, &QDialogButtonBoxPrivate::handleButtonDestroyed); + button->removeEventFilter(q); break; case RemoveRule::KeepConnections: break; @@ -841,6 +870,54 @@ void QDialogButtonBoxPrivate::handleButtonDestroyed() removeButton(reinterpret_cast(object), RemoveRule::KeepConnections); } +bool QDialogButtonBox::eventFilter(QObject *object, QEvent *event) +{ + Q_D(QDialogButtonBox); + if (d->byPassEventFilter) + return false; + + QAbstractButton *button = qobject_cast(object); + if (!button) + return false; + + + const QEvent::Type type = event->type(); + if (type == QEvent::HideToParent || type == QEvent::ShowToParent) + return d->handleButtonShowAndHide(button, event); + + return false; +} + +bool QDialogButtonBoxPrivate::handleButtonShowAndHide(QAbstractButton *button, QEvent *event) +{ + Q_Q(QDialogButtonBox); + + const QEvent::Type type = event->type(); + + switch (type) { + case QEvent::HideToParent: { + const QDialogButtonBox::ButtonRole role = q->buttonRole(button); + if (role != QDialogButtonBox::ButtonRole::InvalidRole) { + removeButton(button, RemoveRule::KeepConnections); + hiddenButtons.insert(button, role); + layoutButtons(); + } + break; + } + case QEvent::ShowToParent: + if (hiddenButtons.contains(button)) { + const auto role = hiddenButtons.take(button); + addButton(button, role, LayoutRule::DoLayout, AddRule::SkipConnect); + if (role == QDialogButtonBox::AcceptRole) + ensureFirstAcceptIsDefault(); + } + break; + default: break; + } + + return false; +} + /*! \property QDialogButtonBox::centerButtons \brief whether the buttons in the button box are centered diff --git a/src/widgets/widgets/qdialogbuttonbox.h b/src/widgets/widgets/qdialogbuttonbox.h index d967494d0df..4c116b5bab7 100644 --- a/src/widgets/widgets/qdialogbuttonbox.h +++ b/src/widgets/widgets/qdialogbuttonbox.h @@ -107,6 +107,8 @@ public: void setCenterButtons(bool center); bool centerButtons() const; + bool eventFilter(QObject *object, QEvent *event) override; + Q_SIGNALS: void clicked(QAbstractButton *button); void accepted(); diff --git a/src/widgets/widgets/qdialogbuttonbox_p.h b/src/widgets/widgets/qdialogbuttonbox_p.h index 6e02512b2ba..5911c96a469 100644 --- a/src/widgets/widgets/qdialogbuttonbox_p.h +++ b/src/widgets/widgets/qdialogbuttonbox_p.h @@ -32,6 +32,10 @@ public: DoLayout, SkipLayout, }; + enum class AddRule { + Connect, + SkipConnect, + }; QDialogButtonBoxPrivate(Qt::Orientation orient); @@ -54,7 +58,8 @@ public: QPushButton *createButton(QDialogButtonBox::StandardButton button, LayoutRule layoutRule = LayoutRule::DoLayout); void addButton(QAbstractButton *button, QDialogButtonBox::ButtonRole role, - LayoutRule layoutRule = LayoutRule::DoLayout); + LayoutRule layoutRule = LayoutRule::DoLayout, + AddRule addRule = AddRule::Connect); void handleButtonDestroyed(); void handleButtonClicked(); bool handleButtonShowAndHide(QAbstractButton *button, QEvent *event); diff --git a/tests/auto/widgets/widgets/qdialogbuttonbox/CMakeLists.txt b/tests/auto/widgets/widgets/qdialogbuttonbox/CMakeLists.txt index a7eb85b1403..62d0998510a 100644 --- a/tests/auto/widgets/widgets/qdialogbuttonbox/CMakeLists.txt +++ b/tests/auto/widgets/widgets/qdialogbuttonbox/CMakeLists.txt @@ -11,4 +11,5 @@ qt_internal_add_test(tst_qdialogbuttonbox LIBRARIES Qt::Gui Qt::Widgets + Qt::WidgetsPrivate ) diff --git a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp index df26b4c5da1..c3f8fb0e30d 100644 --- a/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp +++ b/tests/auto/widgets/widgets/qdialogbuttonbox/tst_qdialogbuttonbox.cpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include Q_DECLARE_METATYPE(QDialogButtonBox::ButtonRole) @@ -49,6 +51,9 @@ private slots: void clear(); void removeButton_data(); void removeButton(); +#ifdef QT_BUILD_INTERNAL + void hideAndShowButton(); +#endif void buttonRole_data(); void buttonRole(); void setStandardButtons_data(); @@ -372,6 +377,51 @@ void tst_QDialogButtonBox::removeButton() delete button; } +#ifdef QT_BUILD_INTERNAL +void tst_QDialogButtonBox::hideAndShowButton() +{ + QDialogButtonBox buttonBox; + QDialogButtonBoxPrivate *d = static_cast(QObjectPrivate::get(&buttonBox)); + auto *apply = buttonBox.addButton( "Apply", QDialogButtonBox::ApplyRole ); + auto *accept = buttonBox.addButton( "Accept", QDialogButtonBox::AcceptRole ); + buttonBox.addButton( "Reject", QDialogButtonBox::RejectRole ); + auto *widget = new QWidget(); + auto *layout = new QHBoxLayout(widget); + layout->addWidget(&buttonBox); + + // apply button shows first on macOS. accept button on all other OSes. + QAbstractButton *hideButton; +#ifdef Q_OS_MACOS + hideButton = apply; + Q_UNUSED(accept); +#else + hideButton = accept; + Q_UNUSED(apply); +#endif + + hideButton->hide(); + widget->show(); + QVERIFY(QTest::qWaitForWindowExposed(widget)); + QVERIFY(buttonBox.focusWidget()); // QTBUG-114377: Without fixing, focusWidget() == nullptr + QCOMPARE(d->visibleButtons().count(), 2); + QCOMPARE(d->hiddenButtons.count(), 1); + QVERIFY(d->hiddenButtons.contains(hideButton)); + QCOMPARE(buttonBox.buttons().count(), 3); + QSignalSpy spy(qApp, &QApplication::focusChanged); + QTest::sendKeyEvent(QTest::KeyAction::Click, &buttonBox, Qt::Key_Tab, QString(), Qt::KeyboardModifiers()); + QCOMPARE(spy.count(), 1); // QTBUG-114377: Without fixing, tabbing wouldn't work. + hideButton->show(); + QCOMPARE_GE(spy.count(), 1); + QTRY_COMPARE(QApplication::focusWidget(), hideButton); + QCOMPARE(d->visibleButtons().count(), 3); + QCOMPARE(d->hiddenButtons.count(), 0); + QCOMPARE(buttonBox.buttons().count(), 3); + spy.clear(); + QTest::sendKeyEvent(QTest::KeyAction::Click, &buttonBox, Qt::Key_Backtab, QString(), Qt::KeyboardModifiers()); + QCOMPARE(spy.count(), 1); +} +#endif + void tst_QDialogButtonBox::testDelete() { QDialogButtonBox buttonBox;