From df5d831bae99662fab43ed2628187113c18aac2c Mon Sep 17 00:00:00 2001 From: Kirill Burtsev Date: Wed, 12 Feb 2020 16:15:34 +0100 Subject: Clear previous page text selection on new navigation unconditionally Remove code duplication on triggering new url load, and use direct code to clear SelectedText instead of CollapseSelection as it assumes focused frame and might be ignored. Fixes: QTBUG-81574 Change-Id: I01cf02967e118f407c8a3997e176d5b258478a5a Reviewed-by: Peter Varga --- src/core/web_contents_adapter.cpp | 46 +++++++++++++++------- src/core/web_contents_adapter.h | 1 + .../widgets/qwebenginepage/tst_qwebenginepage.cpp | 46 +++++++++++++--------- 3 files changed, 61 insertions(+), 32 deletions(-) diff --git a/src/core/web_contents_adapter.cpp b/src/core/web_contents_adapter.cpp index 8cc8179cf..a7579f916 100644 --- a/src/core/web_contents_adapter.cpp +++ b/src/core/web_contents_adapter.cpp @@ -67,6 +67,7 @@ #include "base/task/sequence_manager/thread_controller_with_message_pump_impl.h" #include "base/values.h" #include "content/browser/renderer_host/render_view_host_impl.h" +#include "content/browser/renderer_host/text_input_manager.h" #include "content/browser/web_contents/web_contents_impl.h" #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/child_process_security_policy.h" @@ -369,6 +370,23 @@ static void deserializeNavigationHistory(QDataStream &input, int *currentIndex, } } +static void Navigate(WebContentsAdapter *adapter, const content::NavigationController::LoadURLParams ¶ms) +{ + Q_ASSERT(adapter); + adapter->webContents()->GetController().LoadURLWithParams(params); + adapter->focusIfNecessary(); + adapter->resetSelection(); +} + +static void NavigateTask(QWeakPointer weakAdapter, const content::NavigationController::LoadURLParams ¶ms) +{ + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + const auto adapter = weakAdapter.toStrongRef(); + if (!adapter) + return; + Navigate(adapter.get(), params); +} + namespace { static QList recursive_guard_loading_adapters; @@ -705,21 +723,12 @@ void WebContentsAdapter::load(const QWebEngineHttpRequest &request) } } - auto navigate = [](QWeakPointer weakAdapter, const content::NavigationController::LoadURLParams ¶ms) { - const auto adapter = weakAdapter.toStrongRef(); - if (!adapter) - return; - adapter->webContents()->GetController().LoadURLWithParams(params); - adapter->focusIfNecessary(); - }; - - QWeakPointer weakThis(sharedFromThis()); if (resizeNeeded) { // Schedule navigation on the event loop. base::PostTaskWithTraits(FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(navigate, std::move(weakThis), std::move(params))); + base::BindOnce(&NavigateTask, sharedFromThis().toWeakRef(), std::move(params))); } else { - navigate(std::move(weakThis), params); + Navigate(this, params); } } @@ -752,9 +761,7 @@ void WebContentsAdapter::setContent(const QByteArray &data, const QString &mimeT params.can_load_local_resources = true; params.transition_type = ui::PageTransitionFromInt(ui::PAGE_TRANSITION_TYPED | ui::PAGE_TRANSITION_FROM_API); params.override_user_agent = content::NavigationController::UA_OVERRIDE_TRUE; - m_webContents->GetController().LoadURLWithParams(params); - focusIfNecessary(); - m_webContents->CollapseSelection(); + Navigate(this, params); } void WebContentsAdapter::save(const QString &filePath, int savePageFormat) @@ -1676,6 +1683,17 @@ bool WebContentsAdapter::hasFocusedFrame() const return m_webContents->GetFocusedFrame() != nullptr; } +void WebContentsAdapter::resetSelection() +{ + CHECK_INITIALIZED(); + // unconditionally clears the selection in contrast to CollapseSelection, which checks focus state first + if (auto rwhv = static_cast(m_webContents->GetRenderWidgetHostView())) { + if (auto mgr = rwhv->GetTextInputManager()) + if (auto selection = const_cast(mgr->GetTextSelection(rwhv))) + selection->SetSelection(base::string16(), 0, gfx::Range(), false); + } +} + WebContentsAdapterClient::RenderProcessTerminationStatus WebContentsAdapterClient::renderProcessExitStatus(int terminationStatus) { auto status = WebContentsAdapterClient::RenderProcessTerminationStatus(-1); diff --git a/src/core/web_contents_adapter.h b/src/core/web_contents_adapter.h index 11f8f9cb1..f8f147f9a 100644 --- a/src/core/web_contents_adapter.h +++ b/src/core/web_contents_adapter.h @@ -229,6 +229,7 @@ public: void focusIfNecessary(); bool isFindTextInProgress() const; bool hasFocusedFrame() const; + void resetSelection(); // meant to be used within WebEngineCore only void initialize(content::SiteInstance *site); diff --git a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp index 8cdcc9f46..94b3f16c1 100644 --- a/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp +++ b/tests/auto/widgets/qwebenginepage/tst_qwebenginepage.cpp @@ -700,7 +700,7 @@ public: CursorTrackedPage(QWidget *parent = 0): QWebEnginePage(parent) { } - QString selectedText() { + QString jsSelectedText() { return evaluateJavaScriptSync(this, "window.getSelection().toString()").toString(); } @@ -716,42 +716,52 @@ public: int isSelectionCollapsed() { return evaluateJavaScriptSync(this, "window.getSelection().getRangeAt(0).collapsed").toBool(); } - bool hasSelection() - { - return !selectedText().isEmpty(); - } }; void tst_QWebEnginePage::textSelection() { - QWebEngineView view; - CursorTrackedPage *page = new CursorTrackedPage(&view); - QString content("

The quick brown fox

" \ + CursorTrackedPage page; + + QString textToSelect("The quick brown fox"); + QString content = QString("

%1

" \ "

jumps over the lazy dog

" \ - "

May the source
be with you!

"); - page->setView(&view); - QSignalSpy loadSpy(&view, SIGNAL(loadFinished(bool))); - page->setHtml(content); + "

May the source
be with you!

").arg(textToSelect); + + QSignalSpy loadSpy(&page, SIGNAL(loadFinished(bool))); + page.setHtml(content); QTRY_COMPARE_WITH_TIMEOUT(loadSpy.count(), 1, 20000); // these actions must exist - QVERIFY(page->action(QWebEnginePage::SelectAll) != 0); + QVERIFY(page.action(QWebEnginePage::SelectAll) != 0); // ..but SelectAll is disabled because the page has no focus due to disabled FocusOnNavigationEnabled. - QCOMPARE(page->action(QWebEnginePage::SelectAll)->isEnabled(), false); + QCOMPARE(page.action(QWebEnginePage::SelectAll)->isEnabled(), false); // Verify hasSelection returns false since there is no selection yet... - QCOMPARE(page->hasSelection(), false); + QVERIFY(!page.hasSelection()); + QVERIFY(page.jsSelectedText().isEmpty()); // this will select the first paragraph QString selectScript = "var range = document.createRange(); " \ "var node = document.getElementById(\"one\"); " \ "range.selectNode(node); " \ "getSelection().addRange(range);"; - evaluateJavaScriptSync(page, selectScript); - QCOMPARE(page->selectedText().trimmed(), QString::fromLatin1("The quick brown fox")); + evaluateJavaScriptSync(&page, selectScript); + // Make sure hasSelection returns true, since there is selected text now... - QCOMPARE(page->hasSelection(), true); + QTRY_VERIFY(page.hasSelection()); + QCOMPARE(page.selectedText().trimmed(), textToSelect); + + QCOMPARE(page.jsSelectedText().trimmed(), textToSelect); + + // navigate away and check that selection is cleared + page.load(QUrl("about:blank")); + QTRY_COMPARE(loadSpy.count(), 2); + + QVERIFY(!page.hasSelection()); + QVERIFY(page.selectedText().isEmpty()); + + QVERIFY(page.jsSelectedText().isEmpty()); } -- cgit v1.2.3