From 3f256acda192fbe2b9fad28d09cd74dca9a69418 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 10:45:33 +0200 Subject: fix(pane): open a target="_blank" link, and drop the dead link actions Items 126 and 127, in one sitting because the second is only safe after the first. 126: an anchor carrying target="_blank" did nothing when clicked, with no error and nothing on screen. Chromium routes such a click to QWebEnginePage::createWindow() rather than to acceptNavigationRequest, and MessagePage did not override it, so the base implementation returned nullptr and the URL was discarded before any of our code saw it. Plain anchors were unaffected and already worked, which is why this presented as "HTML mail is broken" while a text mail's links opened: marketing HTML sets _blank on practically every anchor. createWindow() receives a WebWindowType and no URL, so an override cannot simply read the target: it arrives afterwards as a navigation on whatever page is returned. LinkRelayPage is that page. It has no view, hands the URL to the same handler the plain-link path uses, refuses the navigation, and deletes itself. Nothing is ever fetched and no second QWebEngineView is created. 127: OpenLinkInNewTab, OpenLinkInNewWindow and OpenLinkInThisWindow join removeBrowserActions()'s list. Item 100's list is the PAGE actions and was tested by right-clicking the page; these appear only over a link, so it never saw them. CopyLinkToClipboard stays, being the fallback for any link that will not open. The order matters: 126 gives the page a working createWindow(), so those entries would have stopped being dead and started opening links into a tab that does not exist. Testing needed two seams. The click cannot be synthesised, since JavaScript is off in this profile (measured: runJavaScript returns an invalid QVariant) and a synthetic press would depend on the anchor's rect and the desktop's fonts; setUrl() is no substitute because it arrives as NavigationTypeTyped. clickLinkForTest() and relayBlankTargetForTest() drive the real overrides on the real page, and setLinkOpener() substitutes a recorder for QDesktopServices::openUrl. Both routes are asserted rather than only the broken one, since they share a handler now. Three mutations checked and caught, including the filter also removing CopyLinkToClipboard, which a later sweep of "dead link actions" would otherwise take silently. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM --- src/messageview.cpp | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++- src/messageview.h | 42 ++++++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/messageview.cpp b/src/messageview.cpp index a7d340a..3dbd0f1 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -58,6 +58,29 @@ namespace { +} // namespace + +MessageView::LinkOpener &linkOpenerRef() +{ + static MessageView::LinkOpener opener; + return opener; +} + +void MessageView::setLinkOpener(LinkOpener opener) +{ + linkOpenerRef() = std::move(opener); +} + +void MessageView::openExternally(const QUrl &url) +{ + if (const LinkOpener &opener = linkOpenerRef()) + opener(url); + else + QDesktopServices::openUrl(url); +} + +namespace { + /// Intercepts link clicks so a message can never navigate the pane. class MessagePage : public QWebEnginePage { @@ -105,7 +128,7 @@ protected: return false; } - QDesktopServices::openUrl(url); + MessageView::openExternally(url); return false; } @@ -114,12 +137,103 @@ protected: return !isMainFrame; } + /// Item 126. An anchor carrying target="_blank" never reaches + /// acceptNavigationRequest: Chromium asks for a new window instead, and + /// the base implementation returns nullptr, so the click is discarded with + /// no error and nothing on screen. Marketing HTML sets _blank on + /// practically every link, which is what made "HTML mail" look broken + /// while a plain-text mail's links worked. + /// + /// The obvious override cannot work: createWindow() is handed a + /// WebWindowType and NO url. The target arrives afterwards, as a + /// navigation on whatever page is returned, so returning nullptr throws it + /// away before it can be read. + /// + /// So return a page whose only job is to receive that navigation. It + /// reuses the same handler the plain-link path uses rather than sourcing + /// the URL a second way, which is what keeps the two kinds of link from + /// drifting apart. No view is ever created and nothing is ever fetched: + /// the page refuses the navigation, and deleteLater() disposes of it once + /// the URL has been handed on. + QWebEnginePage *createWindow(WebWindowType type) override + { + return makeRelay(type); + } + +public: + /// The same call Chromium makes, reachable from a test. See + /// MessageView::relayBlankTargetForTest(). + QWebEnginePage *createWindowForTest(WebWindowType type) + { + return makeRelay(type); + } + + /// The same call Chromium makes for a clicked anchor. See + /// MessageView::clickLinkForTest(). + bool clickLinkForTest(const QUrl &url) + { + return acceptNavigationRequest(url, NavigationTypeLinkClicked, true); + } + +protected: + private: + QWebEnginePage *makeRelay(WebWindowType) + { + return new LinkRelayPage(profile(), this); + } + + /// Receives the navigation createWindow() could not see, hands the URL to + /// the external browser, and refuses. Never shown, never given a view. + class LinkRelayPage : public QWebEnginePage + { + public: + LinkRelayPage(QWebEngineProfile *profile, QObject *parent) + : QWebEnginePage(profile, parent) {} + + protected: + bool acceptNavigationRequest(const QUrl &url, NavigationType, + bool) override + { + // Whatever the type, this page exists for exactly one URL and is + // finished the moment it has it. + if (url.isValid() && !url.scheme().isEmpty()) + MessageView::openExternally(url); + deleteLater(); + return false; + } + }; + QueryHandler m_onQuery; }; } // namespace +bool MessageView::clickLinkForTest(const QUrl &url) +{ + auto *page = static_cast(m_view->page()); + if (!page) + return false; + return page->clickLinkForTest(url); +} + +bool MessageView::relayBlankTargetForTest(const QUrl &url) +{ + // static_cast, not qobject_cast: MessagePage carries no Q_OBJECT, and the + // page is one this class constructed itself, so the type is not in doubt. + auto *page = static_cast(m_view->page()); + if (!page) + return false; + // Chromium's own sequence: ask for the window, then navigate it. The type + // is what a target="_blank" anchor produces. + QWebEnginePage *relay = page->createWindowForTest( + QWebEnginePage::WebBrowserTab); + if (!relay) + return false; + relay->setUrl(url); + return true; +} + MessageView::MessageView(QWidget *parent) : QWidget(parent) { @@ -691,6 +805,26 @@ void MessageView::removeBrowserActions(QMenu *menu, QWebEnginePage *page) QWebEnginePage::Forward, QWebEnginePage::Reload, QWebEnginePage::SavePage, + // Item 127. Chromium adds these only when the menu is raised over a + // LINK, so the four above, which are page actions, were the whole list + // until now and this was tested by right-clicking the page. + // + // None of the three can be honoured. There are no tabs, and a window + // means a second QWebEngineView, which the pane deliberately never + // creates: one view per message is one Chromium render process per + // message. "In this window" would navigate the pane away from the + // message, which no message may do. + // + // They became MORE dangerous with item 126, not less: that fix gives + // the page a real createWindow(), so an entry that used to be merely + // dead would now do something, and what it would do is open a link the + // user asked to open in a tab that does not exist. + // + // CopyLinkToClipboard is deliberately NOT here. It works, and it is + // the fallback for any link that still will not open. + QWebEnginePage::OpenLinkInNewTab, + QWebEnginePage::OpenLinkInNewWindow, + QWebEnginePage::OpenLinkInThisWindow, }; for (const QWebEnginePage::WebAction which : kUnwanted) { diff --git a/src/messageview.h b/src/messageview.h index 09fc905..3cc1604 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -20,6 +20,8 @@ #include #include + +#include #include #include @@ -57,6 +59,46 @@ public: /// interceptor fails closed and the pane renders nothing at all. static QUrl documentUrl() { return QUrl(QStringLiteral("qtmaildir://message")); } + /// How a clicked link reaches the outside world. + /// + /// A seam, because the alternative is untestable: the call sits inside + /// MessagePage, ends in QDesktopServices::openUrl(), and a passing test + /// would have to launch a real browser. Item 126's regression is about + /// WHICH clicks arrive here, not about what openUrl does, so a test + /// substitutes a recorder and asserts on the URLs it collects. + /// + /// Production never sets this; the default opens the system browser. + using LinkOpener = std::function; + static void setLinkOpener(LinkOpener opener); + static void openExternally(const QUrl &url); + + /// Asks the pane's page for the window a target="_blank" click wants, and + /// drives the returned page with `url` exactly as Chromium would. + /// + /// A test hook, and it exists because the alternative proves nothing. + /// MessagePage lives in an anonymous namespace so createWindow() cannot be + /// called directly, and the click itself cannot be synthesised: JavaScript + /// is off in this profile (verified, runJavaScript returns an invalid + /// QVariant), so `element.click()` does nothing, and a synthetic mouse + /// press would have to land on the anchor's rect, which depends on the + /// desktop's fonts. This drives the real override on the real page. + /// + /// Returns false when the page declined to provide one at all, which is + /// the pre-item-126 behaviour and the regression worth catching. + bool relayBlankTargetForTest(const QUrl &url); + + /// Drives the pane's page with a link click, as + /// acceptNavigationRequest() sees one. + /// + /// The same reasoning as relayBlankTargetForTest(): the click cannot be + /// synthesised. setUrl() is no substitute, because it arrives as + /// NavigationTypeTyped and takes the branch that accepts our own document + /// load, never the link branch. + /// + /// Returns what the page decided: false means the navigation was refused, + /// which is what a link click must always produce here. + bool clickLinkForTest(const QUrl &url); + /// Renders a whole thread, oldest first. Items whose expanded flag is /// false collapse to a one-line stub. void showThread(const QList &items); -- cgit v1.2.3