From f7c656af6844ba4897f6eff965c487ad6b5df79f Mon Sep 17 00:00:00 2001 From: Shawn Rutledge Date: Wed, 25 Oct 2023 19:13:28 -0700 Subject: [PATCH] Localize Flickable delayed release to scene, not to grabber When pressDelay > 0, if Flickable receives mouse or touch release before it has sent the delayed press, it needs to send the delayed press before allowing the release to be seen, in some sense. Further back in d02131e743597b9bd3070d986c61a1c91ea8317a the continuation of release event delivery after the delayed press seems to have been more limited in scope, to say the least. 8673ae8bb6d4bac01cc54638a7d617072299a808 added localPos = window()->mouseGrabberItem()->mapFromScene(...) QQuickWindowPrivate::cloneMouseEvent(event, localPos) window()->sendEvent(window()->mouseGrabberItem(), mouseEvent) where QQuickWindow::sendEvent() was _not_ the same as sending the event to the whole window (the grabber is the intended recipient, but sendEvent() was taking care of child event filtering back then). But over time we became convinced that sendEvent() should not exist, because it was used for non-standard forms of event delivery, could be hacked over time to do arbitrary things, and bypassed any old-school QObject event filters that users might expect to work. In bfde65a8180e3dbf811e757f6d95f9554f622475 though, it was assumed that since the delayed press got delivered to the whole scene, the release should be too. Now it looks a bit redundant: one can see in the debugger that the release gets an extra nested delivery to the whole scene again, after being partially delivered before the delayed press is sent. But that might be risky to change right now. So at the time of writing d7623d79ef4bc9533fced027bf1d173d68b4eba6, QQuickFlickable::mouseReleaseEvent() used an item-localized position (leftover code from 8673ae8bb6d4bac01cc54638a7d617072299a808) and it seemed reasonable to follow that precedent to handle delivery of a delayed touch press and then the release; but in retrospect, 1) we're sending the event to a window, so we don't expect the grabber-localized position to be retained; 2) in fact, QQuickDeliveryAgentPrivate::translateTouchEvent() treats the local position as scene position. QTBUG-118069 occurred because of this assumption being violated. 3) Even for mouse release, it no longer makes sense to localize to the grabber, now that we are sending the event to the window rather than just to the grabber (for quite a number of years already). It will get relocalized during delivery to each item and handler visited. 4) Maybe it doesn't even matter whether there is a grabber or not: we could resend the release to the whole scene regardless. But this patch is conservative; and now we optimize slightly by using QObject::isQuickItemType() rather than qmlobject_cast. tst_qquickflickable::pressDelay() tests the same old scenarios as before, but now with both mouse and touch, and gets a general revamping while we're at it. Fixes: QTBUG-118069 Change-Id: I0f33d23ac1eae9fd697f2eca315107169619706c Reviewed-by: Richard Moe Gustavsen Reviewed-by: Santhosh Kumar (cherry picked from commit 45d4ccc765b7fae86aca749f7b0aabc9c4671b23) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit ca9e3ee30fe6e1756c219d9383667c26ada97a60) Reviewed-by: Shawn Rutledge --- src/quick/items/qquickflickable.cpp | 23 ++-- .../quick/qquickflickable/data/pressDelay.qml | 12 +-- .../qquickflickable/tst_qquickflickable.cpp | 102 +++++++++++------- 3 files changed, 76 insertions(+), 61 deletions(-) diff --git a/src/quick/items/qquickflickable.cpp b/src/quick/items/qquickflickable.cpp index cda8e62587..59e1f65e7b 100644 --- a/src/quick/items/qquickflickable.cpp +++ b/src/quick/items/qquickflickable.cpp @@ -1521,13 +1521,14 @@ void QQuickFlickable::mouseReleaseEvent(QMouseEvent *event) if (d->delayedPressEvent) { d->replayDelayedPress(); - // Now send the release - if (auto grabber = qmlobject_cast(event->exclusiveGrabber(event->point(0)))) { - // not copying or detaching anything, so make sure we return the original event unchanged - const auto oldPosition = event->point(0).position(); - QMutableEventPoint::setPosition(event->point(0), grabber->mapFromScene(event->scenePosition())); + auto &firstPoint = event->point(0); + if (const auto *grabber = event->exclusiveGrabber(firstPoint); grabber && grabber->isQuickItemType()) { + // Since we sent the delayed press to the window, we need to resend the release to the window too. + // We're not copying or detaching, so restore the original event position afterwards. + const auto oldPosition = firstPoint.position(); + QMutableEventPoint::setPosition(firstPoint, event->scenePosition()); QCoreApplication::sendEvent(window(), event); - QMutableEventPoint::setPosition(event->point(0), oldPosition); + QMutableEventPoint::setPosition(firstPoint, oldPosition); } // And the event has been consumed @@ -1580,11 +1581,11 @@ void QQuickFlickable::touchEvent(QTouchEvent *event) if (d->delayedPressEvent) { d->replayDelayedPress(); - // Now send the release - auto &firstPoint = event->point(0); - if (auto grabber = qmlobject_cast(event->exclusiveGrabber(firstPoint))) { - const auto localPos = grabber->mapFromScene(firstPoint.scenePosition()); - QScopedPointer localizedEvent(QQuickDeliveryAgentPrivate::clonePointerEvent(event, localPos)); + const auto &firstPoint = event->point(0); + if (const auto *grabber = event->exclusiveGrabber(firstPoint); grabber && grabber->isQuickItemType()) { + // Since we sent the delayed press to the window, we need to resend the release to the window too. + QScopedPointer localizedEvent( + QQuickDeliveryAgentPrivate::clonePointerEvent(event, firstPoint.scenePosition())); QCoreApplication::sendEvent(window(), localizedEvent.data()); } diff --git a/tests/auto/quick/qquickflickable/data/pressDelay.qml b/tests/auto/quick/qquickflickable/data/pressDelay.qml index a69c4af6de..97bc6b794f 100644 --- a/tests/auto/quick/qquickflickable/data/pressDelay.qml +++ b/tests/auto/quick/qquickflickable/data/pressDelay.qml @@ -1,4 +1,4 @@ -import QtQuick 2.0 +import QtQuick Flickable { flickableDirection: Flickable.VerticalFlick @@ -15,7 +15,6 @@ Flickable { MouseArea { id: ma - objectName: "mouseArea" y: 100 anchors.horizontalCenter: parent.horizontalCenter width: 240 @@ -32,14 +31,7 @@ Flickable { Rectangle { anchors.fill: parent - color: parent.pressed ? 'blue' : 'green' - - Text { - anchors.fill: parent - verticalAlignment: Text.AlignVCenter - horizontalAlignment: Text.AlignHCenter - text: 'Hello' - } + color: parent.pressed ? 'blue' : 'lightsteelblue' } } } diff --git a/tests/auto/quick/qquickflickable/tst_qquickflickable.cpp b/tests/auto/quick/qquickflickable/tst_qquickflickable.cpp index 37810c967d..a9019683c3 100644 --- a/tests/auto/quick/qquickflickable/tst_qquickflickable.cpp +++ b/tests/auto/quick/qquickflickable/tst_qquickflickable.cpp @@ -165,6 +165,7 @@ private slots: void rebound(); void maximumFlickVelocity(); void flickDeceleration(); + void pressDelay_data(); void pressDelay(); void nestedPressDelay(); void filterReplayedPress(); @@ -229,6 +230,10 @@ private slots: private: void flickWithTouch(QQuickWindow *window, const QPoint &from, const QPoint &to); QPointingDevice *touchDevice = QTest::createTouchDevice(); + const QPointingDevice *mouseDevice = new QPointingDevice( + "test mouse", 1000, QInputDevice::DeviceType::Mouse, QPointingDevice::PointerType::Generic, + QInputDevice::Capability::Position | QInputDevice::Capability::Hover | QInputDevice::Capability::Scroll, + 1, 5, QString(), QPointingDeviceUniqueId(), this); }; void tst_qquickflickable::initTestCase() @@ -238,6 +243,8 @@ void tst_qquickflickable::initTestCase() #endif QQmlDataTest::initTestCase(); qmlRegisterType("Test",1,0,"TouchDragArea"); + touchDevice->setParent(this); // avoid leak + QWindowSystemInterface::registerInputDevice(mouseDevice); } void tst_qquickflickable::cleanup() @@ -523,45 +530,54 @@ void tst_qquickflickable::flickDeceleration() delete flickable; } +void tst_qquickflickable::pressDelay_data() +{ + QTest::addColumn("device"); + const QPointingDevice *constTouchDevice = touchDevice; + + QTest::newRow("mouse") << mouseDevice; + QTest::newRow("touch") << constTouchDevice; +} + void tst_qquickflickable::pressDelay() { - QScopedPointer window(new QQuickView); - window->setSource(testFileUrl("pressDelay.qml")); - QTRY_COMPARE(window->status(), QQuickView::Ready); - QQuickViewTestUtils::centerOnScreen(window.data()); - QQuickViewTestUtils::moveMouseAway(window.data()); - window->show(); - QVERIFY(QTest::qWaitForWindowActive(window.data())); - QVERIFY(window->rootObject() != nullptr); + QFETCH(const QPointingDevice *, device); - QQuickFlickable *flickable = qobject_cast(window->rootObject()); - QSignalSpy spy(flickable, SIGNAL(pressDelayChanged())); + QQuickView window; + QVERIFY(QQuickTest::showView(window, testFileUrl("pressDelay.qml"))); + QQuickFlickable *flickable = qobject_cast(window.rootObject()); QVERIFY(flickable); - QCOMPARE(flickable->pressDelay(), 100); + QQuickMouseArea *mouseArea = flickable->findChild(); + QSignalSpy clickedSpy(mouseArea, &QQuickMouseArea::clicked); + // Test the pressDelay property itself + QSignalSpy pressDelayChangedSpy(flickable, &QQuickFlickable::pressDelayChanged); + QCOMPARE(flickable->pressDelay(), 100); flickable->setPressDelay(200); QCOMPARE(flickable->pressDelay(), 200); - QCOMPARE(spy.size(),1); + QCOMPARE(pressDelayChangedSpy.size(), 1); flickable->setPressDelay(200); - QCOMPARE(spy.size(),1); + QCOMPARE(pressDelayChangedSpy.size(), 1); - QQuickItem *mouseArea = window->rootObject()->findChild("mouseArea"); - QSignalSpy clickedSpy(mouseArea, SIGNAL(clicked(QQuickMouseEvent*))); - moveAndPress(window.data(), QPoint(150, 150)); + // Test the press delay + QPoint p(150, 150); + if (device->type() == QInputDevice::DeviceType::Mouse) + QQuickTest::pointerMove(device, &window, 0, p); + QQuickTest::pointerPress(device, &window, 0, p); // The press should not occur immediately - QVERIFY(!mouseArea->property("pressed").toBool()); + QCOMPARE(mouseArea->pressed(), false); // But, it should occur eventually - QTRY_VERIFY(mouseArea->property("pressed").toBool()); + QTRY_VERIFY(mouseArea->pressed()); - QCOMPARE(clickedSpy.size(),0); + QCOMPARE(clickedSpy.size(), 0); // On release the clicked signal should be emitted - QTest::mouseRelease(window.data(), Qt::LeftButton, Qt::NoModifier, QPoint(150, 150)); - QCOMPARE(clickedSpy.size(),1); + QQuickTest::pointerRelease(device, &window, 0, p); + QCOMPARE(clickedSpy.size(), 1); // Press and release position should match QCOMPARE(flickable->property("pressX").toReal(), flickable->property("releaseX").toReal()); @@ -569,38 +585,44 @@ void tst_qquickflickable::pressDelay() // Test a quick tap within the pressDelay timeout + p = QPoint(180, 180); clickedSpy.clear(); - moveAndPress(window.data(), QPoint(180, 180)); + if (device->type() == QInputDevice::DeviceType::Mouse) + QQuickTest::pointerMove(device, &window, 0, p); + QQuickTest::pointerPress(device, &window, 0, p); // The press should not occur immediately - QVERIFY(!mouseArea->property("pressed").toBool()); + QCOMPARE(mouseArea->pressed(), false); + QCOMPARE(clickedSpy.size(), 0); - QCOMPARE(clickedSpy.size(),0); - - // On release the press, release and clicked signal should be emitted - QTest::mouseRelease(window.data(), Qt::LeftButton, Qt::NoModifier, QPoint(180, 180)); - QCOMPARE(clickedSpy.size(),1); + // On release, the press, release and clicked signal should be emitted + QQuickTest::pointerRelease(device, &window, 0, p); + QCOMPARE(clickedSpy.size(), 1); // Press and release position should match QCOMPARE(flickable->property("pressX").toReal(), flickable->property("releaseX").toReal()); QCOMPARE(flickable->property("pressY").toReal(), flickable->property("releaseY").toReal()); - // QTBUG-31168 - moveAndPress(window.data(), QPoint(150, 110)); + // Test flick after press (QTBUG-31168) + QPoint startPosition(150, 110); + p = QPoint(150, 190); + clickedSpy.clear(); + if (device->type() == QInputDevice::DeviceType::Mouse) + QQuickTest::pointerMove(device, &window, 0, startPosition); + QQuickTest::pointerPress(device, &window, 0, startPosition); // The press should not occur immediately - QVERIFY(!mouseArea->property("pressed").toBool()); + QCOMPARE(mouseArea->pressed(), false); + QQuickTest::pointerMove(device, &window, 0, p); - QTest::mouseMove(window.data(), QPoint(150, 190)); + // Since we moved past the drag threshold, we should never receive the press + QCOMPARE(mouseArea->pressed(), false); + QTRY_COMPARE(mouseArea->pressed(), false); - // As we moved pass the drag threshold, we should never receive the press - QVERIFY(!mouseArea->property("pressed").toBool()); - QTRY_VERIFY(!mouseArea->property("pressed").toBool()); - - // On release the clicked signal should *not* be emitted - QTest::mouseRelease(window.data(), Qt::LeftButton, Qt::NoModifier, QPoint(150, 190)); - QCOMPARE(clickedSpy.size(),1); + // On release, the clicked signal should *not* be emitted + QQuickTest::pointerRelease(device, &window, 0, p); + QCOMPARE(clickedSpy.size(), 0); } // QTBUG-17361 @@ -1951,7 +1973,7 @@ void tst_qquickflickable::nestedStopAtBounds() // drag toward the aligned boundary. Outer flickable dragged. moveAndPress(&view, position); if (waitForPressDelay) { - QVERIFY(innerFiltering); // isPressed will never be true if the mouse area isn't enabled. + QVERIFY(innerFiltering); // pressed will never be true if the mouse area isn't enabled. QTRY_VERIFY(mouseArea->pressed()); }