Avoid dock widget separators stealing mouse events from expanded toolbar

The way QMainWindow handles dock widget separators is a bit evil. They
are real QWidgets, added as direct children of the QMainWindow, but
painting and mouse interaction is handled by a hook into the QMainWindow
events, via QMainWindowLayoutSeparatorHelper::windowEvent(), which
is called at the start of QMainWindow::event().

For reasons unknown, we also raise() these separator widgets, which
means they are always at the top of the child window stack of the main
window. This is problematic for the situation when a QToolBar has an
expanded overflow-menu, which is also raised(), but where we end up
raising the separators on top of the toolbar again. As a result the
toolbar gets leave events when we hover one of the separators, even
if the toolbar menu is overlaid the dock area, resulting in the menu
closing.

We don't see this visually, as the separators are drawn via the hook
mentioned above, which happens before QMainWindow draws any of its
child widgets, like the toolbar.

Fixing this requires us to selectively place the separators below
any expanded toolbar each time we raise a separator, so that we
respect the request of the toolbar to be on top, while still
ensuring that the separators are above any of the dock widgets.

Unfortunately the QMainWindowLayoutSeparatorHelper hook also
processes mouse move and click events, resulting in the cursor
changing shape when hovering the separator, even if the toolbar
menu is over it.

To fix this we amend the logic of finding a separator widget
based on the mouse event position to also querying the QWidget
childAt() function, and if we are over a tool bar, we bail out
instead of looking for a separator on that position in the dock
area layout.

As the special separator logic in QMainWindow is conditioned on
the style returning 1 for QStyle::PM_DockWidgetSeparatorExtent
the test checks for this, and skips if another style is in use.
So far only the macOS style is affected by this. See the original
change in fba33a6bbc09bb278df75829d8d705863d40a6a3.

Fixes: QTBUG-124733
Pick-to: 6.5 6.2
Change-Id: Ifc741336bd9725ef0fea9f0474e27b7481f3a183
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit 3d99f90b50ab22ef5ab4433c57f9ee584a0a7cae)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/widgets/widgets/qdockarealayout.cpp

@@ -19,6 +19,11 @@
 #include "qdockwidget_p.h"
 #include <private/qlayoutengine_p.h>
 
+#if QT_CONFIG(toolbar)
+#include "qtoolbar.h"
+#include "qtoolbarlayout_p.h"
+#endif
+
 #include <qpainter.h>
 #include <qstyleoption.h>
 
@@ -2036,6 +2041,30 @@ bool QDockAreaLayoutInfo::restoreState(QDataStream &stream, QList<QDockWidget*>
 }
 
 #if QT_CONFIG(tabbar)
+
+static void raiseSeparatorWidget(QWidget *separatorWidget)
+{
+    Q_ASSERT(separatorWidget);
+
+#if QT_CONFIG(toolbar)
+    // Raise the separator widget, but make sure it doesn't go above
+    // an expanded toolbar, as that would break mouse event hit testing.
+    Q_ASSERT(separatorWidget->parent());
+    const auto toolBars = separatorWidget->parent()->findChildren<QToolBar*>(Qt::FindDirectChildrenOnly);
+    for (auto *toolBar : toolBars) {
+        if (auto *toolBarLayout = qobject_cast<QToolBarLayout*>(toolBar->layout())) {
+            if (toolBarLayout->expanded) {
+                separatorWidget->stackUnder(toolBar);
+                return;
+            }
+        }
+    }
+#endif
+
+    separatorWidget->raise();
+}
+
+
 void QDockAreaLayoutInfo::updateSeparatorWidgets() const
 {
     if (tabbed) {
@@ -2077,7 +2106,7 @@ void QDockAreaLayoutInfo::updateSeparatorWidgets() const
         j++;
 
         Q_ASSERT(sepWidget);
-        sepWidget->raise();
+        raiseSeparatorWidget(sepWidget);
 
         QRect sepRect = separatorRect(i).adjusted(-2, -2, 2, 2);
         sepWidget->setGeometry(sepRect);
@@ -3362,7 +3391,7 @@ void QDockAreaLayout::updateSeparatorWidgets() const
         j++;
 
         Q_ASSERT(sepWidget);
-        sepWidget->raise();
+        raiseSeparatorWidget(sepWidget);
 
         QRect sepRect = separatorRect(i).adjusted(-2, -2, 2, 2);
         sepWidget->setGeometry(sepRect);

src/widgets/widgets/qmainwindowlayout.cpp

@@ -274,6 +274,14 @@ public:
 
     QDockAreaLayoutInfo *dockAreaLayoutInfo() { return &layoutState; }
 
+#if QT_CONFIG(toolbar)
+    QToolBarAreaLayout *toolBarAreaLayout()
+    {
+        auto *mainWindow = static_cast<QMainWindow*>(parentWidget());
+        return qt_mainwindow_layout(mainWindow)->toolBarAreaLayout();
+    }
+#endif
+
     bool nativeWindowDeco() const
     {
         return groupWindow()->hasNativeDecos();

src/widgets/widgets/qmainwindowlayout_p.h

@@ -43,6 +43,7 @@ struct QDockWidgetPrivate {
 #endif
 #if QT_CONFIG(toolbar)
 #include "qtoolbararealayout_p.h"
+#include "qtoolbar.h"
 #endif
 
 #include <QtCore/qloggingcategory.h>
@@ -92,6 +93,9 @@ public:
     bool endSeparatorMove(const QPoint &pos);
     bool windowEvent(QEvent *e);
 
+private:
+    QList<int> findSeparator(const QPoint &pos) const;
+
 #endif // QT_CONFIG(dockwidget)
 
 };
@@ -142,7 +146,7 @@ void QMainWindowLayoutSeparatorHelper<Layout>::adjustCursor(const QPoint &pos)
                 w->unsetCursor();
         }
     } else if (movingSeparator.isEmpty()) { // Don't change cursor when moving separator
-        QList<int> pathToSeparator = layout()->dockAreaLayoutInfo()->findSeparator(pos);
+        QList<int> pathToSeparator = findSeparator(pos);
 
         if (pathToSeparator != hoverSeparator) {
             if (!hoverSeparator.isEmpty())
@@ -279,10 +283,35 @@ bool QMainWindowLayoutSeparatorHelper<Layout>::windowEvent(QEvent *event)
     return false;
 }
 
+template <typename Layout>
+QList<int> QMainWindowLayoutSeparatorHelper<Layout>::findSeparator(const QPoint &pos) const
+{
+    Layout *layout = const_cast<Layout*>(this->layout());
+#if QT_CONFIG(toolbar)
+    QToolBarAreaLayout *toolBarAreaLayout = layout->toolBarAreaLayout();
+    if (!toolBarAreaLayout->isEmpty()) {
+        // We might have a toolbar that is currently expanded, covering
+        // parts of the dock area, in which case we don't want the dock
+        // area layout to treat mouse events for the expanded toolbar as
+        // hitting a separator.
+        const QWidget *widget = layout->window();
+        QWidget *childWidget = widget->childAt(pos);
+        while (childWidget && childWidget != widget) {
+            if (auto *toolBar = qobject_cast<QToolBar*>(childWidget)) {
+                if (!toolBarAreaLayout->indexOf(toolBar).isEmpty())
+                    return {};
+            }
+            childWidget = childWidget->parentWidget();
+        }
+    }
+#endif
+    return layout->dockAreaLayoutInfo()->findSeparator(pos);
+}
+
 template <typename Layout>
 bool QMainWindowLayoutSeparatorHelper<Layout>::startSeparatorMove(const QPoint &pos)
 {
-    movingSeparator = layout()->dockAreaLayoutInfo()->findSeparator(pos);
+    movingSeparator = findSeparator(pos);
 
     if (movingSeparator.isEmpty())
         return false;
@@ -493,6 +522,7 @@ public:
     void removeToolBar(QToolBar *toolbar);
     void toggleToolBarsVisible();
     void moveToolBar(QToolBar *toolbar, int pos);
+    QToolBarAreaLayout *toolBarAreaLayout() { return &layoutState.toolBarAreaLayout; }
 #endif
 
     // dock widgets

src/widgets/widgets/qtoolbarlayout_p.h

@@ -38,7 +38,7 @@ public:
     bool customWidget;
 };
 
-class QToolBarLayout : public QLayout
+class Q_AUTOTEST_EXPORT QToolBarLayout : public QLayout
 {
     Q_OBJECT
 

tests/auto/widgets/widgets/qmainwindow/tst_qmainwindow.cpp

@@ -22,6 +22,8 @@
 #include <qscreen.h>
 #include <private/qmainwindowlayout_p.h>
 #include <private/qdockarealayout_p.h>
+#include <private/qtoolbarlayout_p.h>
+#include <private/qtoolbarextension_p.h>
 
 #if QT_CONFIG(tabbar)
 #include <qtabbar.h>
@@ -133,6 +135,9 @@ private slots:
 #if QT_CONFIG(dockwidget) && QT_CONFIG(tabbar)
     void QTBUG52175_tabifiedDockWidgetActivated();
 #endif
+#ifdef QT_BUILD_INTERNAL
+    void expandedToolBarHitTesting();
+#endif
 };
 
 
@@ -2255,5 +2260,58 @@ void tst_QMainWindow::QTBUG52175_tabifiedDockWidgetActivated()
 }
 #endif
 
+#ifdef QT_BUILD_INTERNAL
+void tst_QMainWindow::expandedToolBarHitTesting()
+{
+    QMainWindow mainWindow;
+    if (mainWindow.style()->pixelMetric(
+        QStyle::PM_DockWidgetSeparatorExtent, nullptr, &mainWindow) != 1) {
+        QSKIP("Style does not trigger the use of qt_qmainwindow_extended_splitter");
+    }
+
+    mainWindow.setAnimated(false);
+
+    auto *dockWidget = new QDockWidget(&mainWindow);
+    dockWidget->setWidget(new QWidget(dockWidget));
+    mainWindow.addDockWidget(Qt::RightDockWidgetArea, dockWidget);
+
+    auto *toolBar = new QToolBar(&mainWindow);
+    for (int i = 0; i < 10; ++i)
+        toolBar->addWidget(new QLabel(QString("Label %1").arg(i)));
+    mainWindow.addToolBar(toolBar);
+
+    auto *centralWidget = new QWidget(&mainWindow);
+    centralWidget->setMinimumSize(QSize(100, 100));
+    mainWindow.setCentralWidget(centralWidget);
+
+    mainWindow.resize(centralWidget->minimumSize());
+    mainWindow.show();
+    QVERIFY(QTest::qWaitForWindowActive(&mainWindow));
+
+    auto *toolBarExtension = toolBar->findChild<QToolBarExtension*>();
+    QVERIFY(toolBarExtension);
+    QPoint buttonCenter = toolBarExtension->parentWidget()->mapTo(&mainWindow, toolBarExtension->geometry().center());
+    QTest::mouseMove(mainWindow.windowHandle(), buttonCenter);
+    QTest::mouseClick(mainWindow.windowHandle(), Qt::LeftButton, Qt::NoModifier, buttonCenter);
+
+    auto *toolBarLayout = static_cast<QToolBarLayout*>(toolBar->layout());
+    QVERIFY(toolBarLayout);
+    QTRY_COMPARE(toolBarLayout->expanded, true);
+
+    auto *splitter = mainWindow.findChild<QWidget*>("qt_qmainwindow_extended_splitter");
+    QVERIFY(splitter);
+    QCOMPARE(splitter->parentWidget(), &mainWindow);
+
+    // Moving the mouse over the splitter when it's covered by the toolbar
+    // extension area should not trigger a closing of the extension area.
+    QTest::mouseMove(mainWindow.windowHandle(), splitter->geometry().center());
+    QCOMPARE(toolBarLayout->expanded, true);
+
+    // Nor should it result in a split cursor shape, indicating we can move
+    // the splitter.
+    QCOMPARE(mainWindow.cursor().shape(), Qt::ArrowCursor);
+}
+#endif // QT_BUILD_INTERNAL
+
 QTEST_MAIN(tst_QMainWindow)
 #include "tst_qmainwindow.moc"