From 96d03be3e06649f9c9ed277622709e7a732bbcf9 Mon Sep 17 00:00:00 2001 From: Romain Pokrzywka Date: Fri, 23 Aug 2019 11:26:17 -0500 Subject: Fix crash when handling QEvent::TouchCancel TouchCancel events have an empty touchPoints() list, which first trips when accessing touchPoints[0], and later on crashes Chromium if we pass the empty list to m_touchSelectionController. Rework handleTouchEvent() to route TouchCancel events like other touch events, and make sure we pass a non-empty touchpoints list to Chromium. Task-number: QTBUG-80893 Change-Id: Ie8396a1191f72b5bbb2b047f131794b37cfded48 Reviewed-by: Peter Varga --- src/core/render_widget_host_view_qt.cpp | 47 +++--- .../widgets/qwebengineview/tst_qwebengineview.cpp | 172 +++++++++++++++++++++ 2 files changed, 201 insertions(+), 18 deletions(-) diff --git a/src/core/render_widget_host_view_qt.cpp b/src/core/render_widget_host_view_qt.cpp index 0adf8091f..901cbf0bd 100644 --- a/src/core/render_widget_host_view_qt.cpp +++ b/src/core/render_widget_host_view_qt.cpp @@ -1575,7 +1575,12 @@ void RenderWidgetHostViewQt::handleTouchEvent(QTouchEvent *ev) eventTimestamp += m_eventsToNowDelta; QList touchPoints = mapTouchPointIds(ev->touchPoints()); - { + // Make sure that ACTION_POINTER_DOWN is delivered before ACTION_MOVE, + // and ACTION_MOVE before ACTION_POINTER_UP. + std::sort(touchPoints.begin(), touchPoints.end(), compareTouchPoints); + + // Check first if the touch event should be routed to the selectionController + if (!touchPoints.isEmpty()) { ui::MotionEvent::Action action; switch (touchPoints[0].state()) { case Qt::TouchPointPressed: @@ -1594,6 +1599,23 @@ void RenderWidgetHostViewQt::handleTouchEvent(QTouchEvent *ev) MotionEventQt motionEvent(touchPoints, eventTimestamp, action, ev->modifiers(), 0); if (m_touchSelectionController->WillHandleTouchEvent(motionEvent)) { + m_previousTouchPoints = touchPoints; + ev->accept(); + return; + } + } else { + // An empty touchPoints always corresponds to a TouchCancel event. + // We can't forward touch cancellations without a previously processed touch event, + // as Chromium expects the previous touchPoints for Action::CANCEL. + // If both are empty that means the TouchCancel was sent without an ongoing touch, + // so there's nothing to cancel anyway. + touchPoints = m_previousTouchPoints; + if (touchPoints.isEmpty()) + return; + + MotionEventQt cancelEvent(touchPoints, eventTimestamp, ui::MotionEvent::Action::CANCEL, ev->modifiers()); + if (m_touchSelectionController->WillHandleTouchEvent(cancelEvent)) { + m_previousTouchPoints.clear(); ev->accept(); return; } @@ -1610,21 +1632,13 @@ void RenderWidgetHostViewQt::handleTouchEvent(QTouchEvent *ev) break; case QEvent::TouchCancel: { - // Don't process a TouchCancel event if no motion was started beforehand, or if there are - // no touch points in the current event or in the previously processed event. - if (!m_touchMotionStarted || (touchPoints.isEmpty() && m_previousTouchPoints.isEmpty())) { - clearPreviousTouchMotionState(); - return; + // Only process TouchCancel events received following a TouchBegin or TouchUpdate event + if (m_touchMotionStarted) { + MotionEventQt cancelEvent(touchPoints, eventTimestamp, ui::MotionEvent::Action::CANCEL, ev->modifiers()); + processMotionEvent(cancelEvent); } - // Use last saved touch points for the cancel event, to get rid of a QList assert, - // because Chromium expects a MotionEvent::ACTION_CANCEL instance to contain at least - // one touch point, whereas a QTouchCancel may not contain any touch points at all. - if (touchPoints.isEmpty()) - touchPoints = m_previousTouchPoints; clearPreviousTouchMotionState(); - MotionEventQt cancelEvent(touchPoints, eventTimestamp, ui::MotionEvent::Action::CANCEL, ev->modifiers()); - processMotionEvent(cancelEvent); return; } case QEvent::TouchEnd: @@ -1648,11 +1662,6 @@ void RenderWidgetHostViewQt::handleTouchEvent(QTouchEvent *ev) #endif } - // Make sure that ACTION_POINTER_DOWN is delivered before ACTION_MOVE, - // and ACTION_MOVE before ACTION_POINTER_UP. - std::sort(touchPoints.begin(), touchPoints.end(), compareTouchPoints); - - m_previousTouchPoints = touchPoints; for (int i = 0; i < touchPoints.size(); ++i) { ui::MotionEvent::Action action; switch (touchPoints[i].state()) { @@ -1679,6 +1688,8 @@ void RenderWidgetHostViewQt::handleTouchEvent(QTouchEvent *ev) MotionEventQt motionEvent(touchPoints, eventTimestamp, action, ev->modifiers(), i); processMotionEvent(motionEvent); } + + m_previousTouchPoints = touchPoints; } #if QT_CONFIG(tabletevent) diff --git a/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp b/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp index 044fac9d7..0b11be355 100644 --- a/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp +++ b/tests/auto/widgets/qwebengineview/tst_qwebengineview.cpp @@ -60,6 +60,8 @@ do { \ QCOMPARE((__expr), __expected); \ } while (0) +static QTouchDevice* s_touchDevice = nullptr; + static QPoint elementCenter(QWebEnginePage *page, const QString &id) { const QString jsCode( @@ -165,6 +167,9 @@ private Q_SLOTS: void keyboardEvents(); void keyboardFocusAfterPopup(); void mouseClick(); + void touchTap(); + void touchTapAndHold(); + void touchTapAndHoldCancelled(); void postData(); void inputFieldOverridesShortcuts(); @@ -210,6 +215,7 @@ private Q_SLOTS: // It is only called once. void tst_QWebEngineView::initTestCase() { + s_touchDevice = QTest::createTouchDevice(); } // This will be called after the last test function is executed. @@ -1513,6 +1519,172 @@ void tst_QWebEngineView::mouseClick() QVERIFY(view.focusProxy()->inputMethodQuery(Qt::ImCurrentSelection).toString().isEmpty()); } +void tst_QWebEngineView::touchTap() +{ +#if defined(Q_OS_MACOS) + QSKIP("Synthetic touch events are not supported on macOS"); +#endif + + QWebEngineView view; + view.show(); + view.resize(200, 200); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + + QSignalSpy loadFinishedSpy(&view, &QWebEngineView::loadFinished); + + view.settings()->setAttribute(QWebEngineSettings::FocusOnNavigationEnabled, false); + view.setHtml("" + "

The Qt Company

" + "
" + "
" + ""); + QVERIFY(loadFinishedSpy.wait()); + QVERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + + auto singleTap = [](QWidget* target, const QPoint& tapCoords) -> void { + QTest::touchEvent(target, s_touchDevice).press(1, tapCoords, target); + QTest::touchEvent(target, s_touchDevice).stationary(1); + QTest::touchEvent(target, s_touchDevice).release(1, tapCoords, target); + }; + + // Single tap on text doesn't trigger a selection + singleTap(view.focusProxy(), elementCenter(view.page(), "text")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + QTRY_VERIFY(!view.hasSelection()); + + // Single tap inside the input field focuses it without selecting the text + singleTap(view.focusProxy(), elementCenter(view.page(), "input")); + QTRY_COMPARE(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString(), QStringLiteral("input")); + QTRY_VERIFY(!view.hasSelection()); + + // Single tap on the div clears the input field focus + singleTap(view.focusProxy(), elementCenter(view.page(), "notext")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + + // Double tap on text still doesn't trigger a selection + singleTap(view.focusProxy(), elementCenter(view.page(), "text")); + singleTap(view.focusProxy(), elementCenter(view.page(), "text")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + QTRY_VERIFY(!view.hasSelection()); + + // Double tap inside the input field focuses it and selects the word under it + singleTap(view.focusProxy(), elementCenter(view.page(), "input")); + singleTap(view.focusProxy(), elementCenter(view.page(), "input")); + QTRY_COMPARE(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString(), QStringLiteral("input")); + QTRY_COMPARE(view.selectedText(), QStringLiteral("Company2")); + + // Double tap outside the input field behaves like a single tap: clears its focus and selection + singleTap(view.focusProxy(), elementCenter(view.page(), "notext")); + singleTap(view.focusProxy(), elementCenter(view.page(), "notext")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + QTRY_VERIFY(!view.hasSelection()); +} + +void tst_QWebEngineView::touchTapAndHold() +{ +#if defined(Q_OS_MACOS) + QSKIP("Synthetic touch events are not supported on macOS"); +#endif + + QWebEngineView view; + view.show(); + view.resize(200, 200); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + + QSignalSpy loadFinishedSpy(&view, &QWebEngineView::loadFinished); + + view.settings()->setAttribute(QWebEngineSettings::FocusOnNavigationEnabled, false); + view.setHtml("" + "

The Qt Company

" + "
" + "
" + ""); + QVERIFY(loadFinishedSpy.wait()); + QVERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + + auto tapAndHold = [](QWidget* target, const QPoint& tapCoords) -> void { + QTest::touchEvent(target, s_touchDevice).press(1, tapCoords, target); + QTest::touchEvent(target, s_touchDevice).stationary(1); + QTest::qWait(1000); + QTest::touchEvent(target, s_touchDevice).release(1, tapCoords, target); + }; + + // Tap-and-hold on text selects the word under it + tapAndHold(view.focusProxy(), elementCenter(view.page(), "text")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + QTRY_COMPARE(view.selectedText(), QStringLiteral("Company")); + + // Tap-and-hold inside the input field focuses it and selects the word under it + tapAndHold(view.focusProxy(), elementCenter(view.page(), "input")); + QTRY_COMPARE(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString(), QStringLiteral("input")); + QTRY_COMPARE(view.selectedText(), QStringLiteral("Company2")); + + // Only test the page context menu on Windows, as Linux doesn't handle context menus consistently + // and other non-desktop platforms like Android may not even support context menus at all +#if defined(Q_OS_WIN) + // Tap-and-hold clears the text selection and shows the page's context menu + QVERIFY(QApplication::activePopupWidget() == nullptr); + tapAndHold(view.focusProxy(), elementCenter(view.page(), "notext")); + QTRY_VERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + QTRY_VERIFY(!view.hasSelection()); + QTRY_VERIFY(QApplication::activePopupWidget() != nullptr); + + QApplication::activePopupWidget()->close(); + QVERIFY(QApplication::activePopupWidget() == nullptr); +#endif +} + +void tst_QWebEngineView::touchTapAndHoldCancelled() +{ +#if defined(Q_OS_MACOS) + QSKIP("Synthetic touch events are not supported on macOS"); +#endif + + QWebEngineView view; + view.show(); + view.resize(200, 200); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + + QSignalSpy loadFinishedSpy(&view, &QWebEngineView::loadFinished); + + view.settings()->setAttribute(QWebEngineSettings::FocusOnNavigationEnabled, false); + view.setHtml("" + "

The Qt Company

" + "
" + "
" + ""); + QVERIFY(loadFinishedSpy.wait()); + QVERIFY(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty()); + + auto cancelledTapAndHold = [](QWidget* target, const QPoint& tapCoords) -> void { + QTest::touchEvent(target, s_touchDevice).press(1, tapCoords, target); + QTest::touchEvent(target, s_touchDevice).stationary(1); + QTest::qWait(1000); + QWindowSystemInterface::handleTouchCancelEvent(target->windowHandle(), s_touchDevice); + }; + + // A cancelled tap-and-hold should cancel text selection, but currently doesn't + cancelledTapAndHold(view.focusProxy(), elementCenter(view.page(), "text")); + QEXPECT_FAIL("", "Incorrect Chromium selection behavior when cancelling tap-and-hold on text", Continue); + QTRY_VERIFY_WITH_TIMEOUT(!view.hasSelection(), 100); + + // A cancelled tap-and-hold should cancel input field focusing and selection, but currently doesn't + cancelledTapAndHold(view.focusProxy(), elementCenter(view.page(), "input")); + QEXPECT_FAIL("", "Incorrect Chromium selection behavior when cancelling tap-and-hold on input field", Continue); + QTRY_VERIFY_WITH_TIMEOUT(evaluateJavaScriptSync(view.page(), "document.activeElement.id").toString().isEmpty(), 100); + QEXPECT_FAIL("", "Incorrect Chromium focus behavior when cancelling tap-and-hold on input field", Continue); + QTRY_VERIFY_WITH_TIMEOUT(!view.hasSelection(), 100); + + // Only test the page context menu on Windows, as Linux doesn't handle context menus consistently + // and other non-desktop platforms like Android may not even support context menus at all +#if defined(Q_OS_WIN) + // A cancelled tap-and-hold cancels the context menu + QVERIFY(QApplication::activePopupWidget() == nullptr); + cancelledTapAndHold(view.focusProxy(), elementCenter(view.page(), "notext")); + QVERIFY(QApplication::activePopupWidget() == nullptr); +#endif +} + void tst_QWebEngineView::postData() { QMap postData; -- cgit v1.2.3