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.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) (limited to 'src/messageview.h') 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