From 6e1d70ae12baae4610356ec7b69635ad75a97b4e Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 22 Jan 2020 13:51:01 +0100 Subject: [PATCH] QPushButton: only trigger button when click occurs within the bevel rect On the mac, the push button's bevel doesn't cover the entire widget rectangle, but is smaller to leave space for focus frame, shadow, and in general to meet style guidelines. Without this change, a click anywhere inside the widget would activate the button. QAbstractButton::hitButton can be reimplemented to limit the area in which the button is triggered. However, getting the rectangle also requires an addition to QStyle, so that we can query QStyle::subElementRect for the actual area the button's bevel covers. As a side effect, tests that use QPushButton and assume that it responds to clicks at position 0,0 have to be fixed so that they don't fail on mac. Change-Id: I01b60a763bccf39090aee5b2369af300f922d226 Fixes: QTBUG-81452 Reviewed-by: Richard Moe Gustavsen --- src/plugins/styles/mac/qmacstyle_mac.mm | 18 +++++++-- src/widgets/styles/qcommonstyle.cpp | 6 +++ src/widgets/styles/qstyle.cpp | 2 + src/widgets/styles/qstyle.h | 2 + src/widgets/widgets/qpushbutton.cpp | 11 ++++++ src/widgets/widgets/qpushbutton.h | 1 + .../tst_qmouseevent_modal.cpp | 17 ++++++++- .../widgets/qbuttongroup/tst_qbuttongroup.cpp | 37 ++++++++++++++----- .../qmdisubwindow/tst_qmdisubwindow.cpp | 28 +++++++++++--- 9 files changed, 101 insertions(+), 21 deletions(-) diff --git a/src/plugins/styles/mac/qmacstyle_mac.mm b/src/plugins/styles/mac/qmacstyle_mac.mm index 37b658dd76..b34a304ec0 100644 --- a/src/plugins/styles/mac/qmacstyle_mac.mm +++ b/src/plugins/styles/mac/qmacstyle_mac.mm @@ -4613,6 +4613,7 @@ QRect QMacStyle::subElementRect(SubElement sr, const QStyleOption *opt, case SE_ToolBoxTabContents: rect = QCommonStyle::subElementRect(sr, opt, widget); break; + case SE_PushButtonBevel: case SE_PushButtonContents: if (const QStyleOptionButton *btn = qstyleoption_cast(opt)) { // Comment from the old HITheme days: @@ -4626,9 +4627,20 @@ QRect QMacStyle::subElementRect(SubElement sr, const QStyleOption *opt, const auto ct = cocoaControlType(btn, widget); const auto cs = d->effectiveAquaSizeConstrain(btn, widget); const auto cw = QMacStylePrivate::CocoaControl(ct, cs); - const auto frameRect = cw.adjustedControlFrame(btn->rect); - const auto titleMargins = cw.titleMargins(); - rect = (frameRect - titleMargins).toRect(); + auto frameRect = cw.adjustedControlFrame(btn->rect); + if (sr == SE_PushButtonContents) { + frameRect -= cw.titleMargins(); + } else { + auto *pb = static_cast(d->cocoaControl(cw)); + if (cw.type != QMacStylePrivate::Button_SquareButton) { + frameRect = QRectF::fromCGRect([pb alignmentRectForFrame:pb.frame]); + if (cw.type == QMacStylePrivate::Button_PushButton) + frameRect -= pushButtonShadowMargins[cw.size]; + else if (cw.type == QMacStylePrivate::Button_PullDown) + frameRect -= pullDownButtonShadowMargins[cw.size]; + } + } + rect = frameRect.toRect(); } break; case SE_HeaderLabel: { diff --git a/src/widgets/styles/qcommonstyle.cpp b/src/widgets/styles/qcommonstyle.cpp index 1e5830d216..eb5399bd4a 100644 --- a/src/widgets/styles/qcommonstyle.cpp +++ b/src/widgets/styles/qcommonstyle.cpp @@ -2465,6 +2465,12 @@ QRect QCommonStyle::subElementRect(SubElement sr, const QStyleOption *opt, r = visualRect(opt->direction, opt->rect, r); } break; + case SE_PushButtonBevel: + { + r = opt->rect; + r = visualRect(opt->direction, opt->rect, r); + } + break; case SE_CheckBoxIndicator: { int h = proxy()->pixelMetric(PM_IndicatorHeight, opt, widget); diff --git a/src/widgets/styles/qstyle.cpp b/src/widgets/styles/qstyle.cpp index 72e40d657d..987465efae 100644 --- a/src/widgets/styles/qstyle.cpp +++ b/src/widgets/styles/qstyle.cpp @@ -1012,6 +1012,7 @@ void QStyle::drawItemPixmap(QPainter *painter, const QRect &rect, int alignment, \value SE_PushButtonFocusRect Area for the focus rect (usually larger than the contents rect). \value SE_PushButtonLayoutItem Area that counts for the parent layout. + \value SE_PushButtonBevel Area used for the bevel of the button. \value SE_CheckBoxIndicator Area for the state indicator (e.g., check mark). \value SE_CheckBoxContents Area for the label (text or pixmap). @@ -1120,6 +1121,7 @@ void QStyle::drawItemPixmap(QPainter *painter, const QRect &rect, int alignment, \header \li Sub Element \li QStyleOption Subclass \row \li \l SE_PushButtonContents \li \l QStyleOptionButton \row \li \l SE_PushButtonFocusRect \li \l QStyleOptionButton + \row \li \l SE_PushButtonBevel \li \l QStyleOptionButton \row \li \l SE_CheckBoxIndicator \li \l QStyleOptionButton \row \li \l SE_CheckBoxContents \li \l QStyleOptionButton \row \li \l SE_CheckBoxFocusRect \li \l QStyleOptionButton diff --git a/src/widgets/styles/qstyle.h b/src/widgets/styles/qstyle.h index ee234457f5..04628e6ef9 100644 --- a/src/widgets/styles/qstyle.h +++ b/src/widgets/styles/qstyle.h @@ -361,6 +361,8 @@ public: SE_TabBarScrollRightButton, SE_TabBarTearIndicatorRight, + SE_PushButtonBevel, + // do not add any values below/greater than this SE_CustomBase = 0xf0000000 }; diff --git a/src/widgets/widgets/qpushbutton.cpp b/src/widgets/widgets/qpushbutton.cpp index b0d3ba51f9..3d075bf92f 100644 --- a/src/widgets/widgets/qpushbutton.cpp +++ b/src/widgets/widgets/qpushbutton.cpp @@ -509,6 +509,17 @@ void QPushButton::focusOutEvent(QFocusEvent *e) #endif } +/*! + \reimp +*/ +bool QPushButton::hitButton(const QPoint &pos) const +{ + QStyleOptionButton option; + initStyleOption(&option); + const QRect bevel = style()->subElementRect(QStyle::SE_PushButtonBevel, &option, this); + return bevel.contains(pos); +} + #if QT_CONFIG(menu) /*! Associates the popup menu \a menu with this push button. This diff --git a/src/widgets/widgets/qpushbutton.h b/src/widgets/widgets/qpushbutton.h index b02ba63d07..283dfe2c61 100644 --- a/src/widgets/widgets/qpushbutton.h +++ b/src/widgets/widgets/qpushbutton.h @@ -94,6 +94,7 @@ protected: void focusInEvent(QFocusEvent *) override; void focusOutEvent(QFocusEvent *) override; void initStyleOption(QStyleOptionButton *option) const; + bool hitButton(const QPoint &pos) const override; QPushButton(QPushButtonPrivate &dd, QWidget* parent = nullptr); public: diff --git a/tests/auto/gui/kernel/qmouseevent_modal/tst_qmouseevent_modal.cpp b/tests/auto/gui/kernel/qmouseevent_modal/tst_qmouseevent_modal.cpp index 8e6ca486d5..758aeb42b9 100644 --- a/tests/auto/gui/kernel/qmouseevent_modal/tst_qmouseevent_modal.cpp +++ b/tests/auto/gui/kernel/qmouseevent_modal/tst_qmouseevent_modal.cpp @@ -42,6 +42,19 @@ class TstWidget; class TstDialog; QT_FORWARD_DECLARE_CLASS(QPushButton) +class TestButton : public QPushButton +{ +public: + TestButton(const QString &title, QWidget *parent = nullptr) + : QPushButton(title, parent) + {} +protected: + bool hitButton(const QPoint &pos) const override + { + return rect().contains(pos); + } +}; + class tst_qmouseevent_modal : public QObject { Q_OBJECT @@ -63,7 +76,7 @@ public: public slots: void buttonPressed(); public: - QPushButton *pb; + TestButton *pb; TstDialog *d; }; @@ -135,7 +148,7 @@ void tst_qmouseevent_modal::mousePressRelease() TstWidget::TstWidget() { - pb = new QPushButton( "Press me", this ); + pb = new TestButton( "Press me", this ); pb->setObjectName("testbutton"); QSize s = pb->sizeHint(); pb->setGeometry( 5, 5, s.width(), s.height() ); diff --git a/tests/auto/widgets/widgets/qbuttongroup/tst_qbuttongroup.cpp b/tests/auto/widgets/widgets/qbuttongroup/tst_qbuttongroup.cpp index 47dfc86a69..279fe49e3a 100644 --- a/tests/auto/widgets/widgets/qbuttongroup/tst_qbuttongroup.cpp +++ b/tests/auto/widgets/widgets/qbuttongroup/tst_qbuttongroup.cpp @@ -58,6 +58,23 @@ protected: } }; +class TestPushButton : public QPushButton +{ +public: + TestPushButton(QWidget *parent = nullptr) + : QPushButton(parent) + {} + TestPushButton(const QString &title, QWidget *parent = nullptr) + : QPushButton(title, parent) + {} + +protected: + bool hitButton(const QPoint &pos) const override + { + return rect().contains(pos); + } +}; + #include class tst_QButtonGroup : public QObject @@ -97,7 +114,7 @@ void tst_QButtonGroup::arrowKeyNavigation() QGroupBox g1("1", &dlg); QHBoxLayout g1layout(&g1); QRadioButton bt1("Radio1", &g1); - QPushButton pb("PB", &g1); + TestPushButton pb("PB", &g1); QLineEdit le(&g1); QRadioButton bt2("Radio2", &g1); g1layout.addWidget(&bt1); @@ -231,9 +248,9 @@ void tst_QButtonGroup::exclusive() { QDialog dlg(0); QHBoxLayout layout(&dlg); - QPushButton *pushButton1 = new QPushButton(&dlg); - QPushButton *pushButton2 = new QPushButton(&dlg); - QPushButton *pushButton3 = new QPushButton(&dlg); + TestPushButton *pushButton1 = new TestPushButton(&dlg); + TestPushButton *pushButton2 = new TestPushButton(&dlg); + TestPushButton *pushButton3 = new TestPushButton(&dlg); pushButton1->setCheckable(true); pushButton2->setCheckable(true); pushButton3->setCheckable(true); @@ -271,9 +288,9 @@ void tst_QButtonGroup::exclusive() void tst_QButtonGroup::testSignals() { QButtonGroup buttons; - QPushButton pb1; - QPushButton pb2; - QPushButton pb3; + TestPushButton pb1; + TestPushButton pb2; + TestPushButton pb3; buttons.addButton(&pb1); buttons.addButton(&pb2, 23); buttons.addButton(&pb3); @@ -390,9 +407,9 @@ void tst_QButtonGroup::checkedButton() { QButtonGroup buttons; buttons.setExclusive(false); - QPushButton pb1; + TestPushButton pb1; pb1.setCheckable(true); - QPushButton pb2; + TestPushButton pb2; pb2.setCheckable(true); buttons.addButton(&pb1); buttons.addButton(&pb2, 23); @@ -456,7 +473,7 @@ void tst_QButtonGroup::task209485_removeFromGroupInEventHandler() QFETCH(int, signalCount); qRegisterMetaType("QAbstractButton *"); - QPushButton *button = new QPushButton; + TestPushButton *button = new TestPushButton; QButtonGroup group; group.addButton(button); diff --git a/tests/auto/widgets/widgets/qmdisubwindow/tst_qmdisubwindow.cpp b/tests/auto/widgets/widgets/qmdisubwindow/tst_qmdisubwindow.cpp index 2c5cb88ed6..1c19bf96d6 100644 --- a/tests/auto/widgets/widgets/qmdisubwindow/tst_qmdisubwindow.cpp +++ b/tests/auto/widgets/widgets/qmdisubwindow/tst_qmdisubwindow.cpp @@ -152,6 +152,22 @@ Q_DECLARE_METATYPE(Qt::WindowType); Q_DECLARE_METATYPE(Qt::WindowFlags); Q_DECLARE_METATYPE(QMdiSubWindow*); +class TestPushButton : public QPushButton +{ +public: + TestPushButton(const QString &title, QWidget *parent = nullptr) + : QPushButton(title, parent) + { + } + +protected: + // don't rely on style-specific button behavior in test + bool hitButton(const QPoint &point) const override + { + return rect().contains(point); + } +}; + class tst_QMdiSubWindow : public QObject { Q_OBJECT @@ -390,7 +406,7 @@ void tst_QMdiSubWindow::mainWindowSupport() // the maximized subwindow's title is imposed onto the main window's titlebar. if (!nativeMenuBar) { QCOMPARE(mainWindow.windowTitle(), QString()); - QMdiSubWindow *window = workspace->addSubWindow(new QPushButton(QLatin1String("Test"))); + QMdiSubWindow *window = workspace->addSubWindow(new TestPushButton(QLatin1String("Test"))); QString expectedTitle = QLatin1String("MainWindow's title is empty"); window->setWindowTitle(expectedTitle); QCOMPARE(window->windowTitle(), expectedTitle); @@ -1298,7 +1314,7 @@ void tst_QMdiSubWindow::changeFocusWithTab() QTest::keyPress(widget, Qt::Key_Backtab); QCOMPARE(QApplication::focusWidget(), static_cast(firstLineEdit)); - QMdiSubWindow *window = mdiArea.addSubWindow(new QPushButton); + QMdiSubWindow *window = mdiArea.addSubWindow(new TestPushButton(QLatin1String("test"))); window->show(); QCOMPARE(mdiArea.activeSubWindow(), window); @@ -1907,7 +1923,7 @@ void tst_QMdiSubWindow::setFont() QSKIP("This test function is unstable in CI, please see QTBUG-22544"); QMdiArea mdiArea; mdiArea.setWindowTitle(QLatin1String(QTest::currentTestFunction())); - QMdiSubWindow *subWindow = mdiArea.addSubWindow(new QPushButton(QLatin1String("test"))); + QMdiSubWindow *subWindow = mdiArea.addSubWindow(new TestPushButton(QLatin1String("test"))); subWindow->resize(300, 100); subWindow->setWindowTitle(QLatin1String("Window title")); mdiArea.show(); @@ -2057,19 +2073,19 @@ void tst_QMdiSubWindow::task_233197() QMenuBar *menuBar = mainWindow->menuBar(); // force creation of a menubar Q_UNUSED(menuBar); - QPushButton *focus1 = new QPushButton(QLatin1String("Focus 1"), mainWindow); + QPushButton *focus1 = new TestPushButton(QLatin1String("Focus 1"), mainWindow); QObject::connect(focus1, &QAbstractButton::clicked, subWindow1, QOverload<>::of(&QWidget::setFocus)); focus1->move(5, 30); focus1->show(); - QPushButton *focus2 = new QPushButton(QLatin1String("Focus 2"), mainWindow); + QPushButton *focus2 = new TestPushButton(QLatin1String("Focus 2"), mainWindow); QObject::connect(focus2, &QAbstractButton::clicked, subWindow2, QOverload<>::of(&QWidget::setFocus)); focus2->move(5, 60); focus2->show(); - QPushButton *close = new QPushButton(QLatin1String("Close"), mainWindow); + QPushButton *close = new TestPushButton(QLatin1String("Close"), mainWindow); QObject::connect(close, &QAbstractButton::clicked, mainWindow, &QWidget::close); close->move(5, 90); close->show();