aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-20 10:45:33 +0200
committerDanilo M. <danix@danix.xyz>2026-08-20 10:45:33 +0200
commit3f256acda192fbe2b9fad28d09cd74dca9a69418 (patch)
treee77e97c9f23425f2fc767361562ab3f1dd3730ca
parent5a71cbbbc2326586aa4ef326a4ffa5dc7f5e285a (diff)
downloadqtmaildir-3f256acda192fbe2b9fad28d09cd74dca9a69418.tar.gz
qtmaildir-3f256acda192fbe2b9fad28d09cd74dca9a69418.zip
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDq53rMd3AQp7QmcZzpuBM
-rw-r--r--CHANGELOG.md13
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md171
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md139
-rw-r--r--src/messageview.cpp136
-rw-r--r--src/messageview.h42
-rw-r--r--tests/test_messageview.cpp124
6 files changed, 487 insertions, 138 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 74a7e90..9131c25 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,19 @@ point at which they are stable.
## [Unreleased]
+### Fixed
+
+- Clicking a link in a message opens it in the system browser. Links carrying
+ `target="_blank"`, which is most links in HTML mail, did nothing at all: no
+ error, nothing on screen. Chromium routes those to a new-window request
+ rather than to the navigation handler, and nothing answered it, so the click
+ was discarded. Plain links, as in most text mail, were unaffected and already
+ worked, which is what made this look like "HTML mail is broken".
+- The context menu on a link no longer offers Open in new tab, Open in new
+ window or Open in this window. None can work: the pane has no tabs and must
+ never open a window or navigate away from the message. Copy link address is
+ kept.
+
## [0.26.1] - 2026-08-19
A bugfix release for one defect in 0.26.0, reported the day it shipped: moving
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
index 4a4b1ff..4f361bf 100644
--- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
+++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md
@@ -5140,6 +5140,177 @@ set explicitly. And the procedure's own first step, holding `/tmp/mbsync.lock`,
conflicts with hand-testing Delete, which auto-syncs; that surfaced item 125.
The procedure lives outside this repo, in the user's own documents.
+## 126. A link with `target="_blank"` does nothing when clicked
+
+**Observed (user, 2026-08-20):** "if I click on a link in a mail in the message
+pane, nothing happens." Then, on checking: a plain-text mail (a GitHub PR reply)
+opens its links correctly, while an HTML mail showing the link as a button does
+not.
+
+**That second observation overturned the first diagnosis and is the whole
+item.** This entry originally blamed `RequestInterceptor` blocking `https`.
+That was wrong: the interceptor does deny `https`, but only for resources the
+document FETCHES. A clicked link never becomes a request, because
+`acceptNavigationRequest` calls `QDesktopServices::openUrl` and returns false
+before anything is issued. If the interceptor were the cause, the GitHub link
+could not work either, and it does.
+
+**Cause (verified against the two messages the user named).** The difference is
+`target="_blank"`:
+
+| message | `target` | route taken | result |
+|---|---|---|---|
+| GitHub PR reply | none on any anchor | `acceptNavigationRequest` | opens correctly |
+| HTML newsletter | `_blank` on every anchor | `createWindow()` | silently dropped |
+
+An anchor with no target navigates the main frame, so it reaches
+`MessagePage::acceptNavigationRequest` as `NavigationTypeLinkClicked` and is
+handed to the browser. An anchor asking for a new window does NOT: Chromium
+routes it to `QWebEnginePage::createWindow()`, which `MessagePage` does not
+override, so the base implementation returns `nullptr` and the click is
+discarded. `acceptNavigationRequest` is never consulted, which is why no
+existing code sees it and nothing at all happens.
+
+Both messages render HTML, so this is not a text-versus-HTML distinction: the
+GitHub mail is `multipart/alternative` and its HTML part is what the pane shows.
+Marketing HTML uses `target="_blank"` almost universally, which is why it reads
+as "HTML mail is broken".
+
+**Approach.** Override `createWindow()` in `MessagePage` to hand the URL to the
+same path a plain link takes and return `nullptr`, so no window is ever created.
+
+**One trap, and it decides the shape.** `createWindow()` is called WITHOUT the
+target URL in Qt: the signature carries only a `WebWindowType`. The URL arrives
+afterwards, as a navigation request on the page the override is expected to
+return. Returning `nullptr` therefore discards the URL before it can be seen, so
+the override cannot simply read it. Two known ways around it, and this needs
+measuring before choosing:
+
+- Keep a `linkHovered` cache and use the last hovered URL. Cheap, and wrong if
+ the click arrives without a hover (keyboard activation, synthetic click).
+- Return a throwaway `QWebEnginePage` whose `acceptNavigationRequest` hands the
+ URL to `openUrl` and refuses, then deletes itself. Correct by construction,
+ since the URL arrives through the normal path, at the cost of a short-lived
+ page object.
+
+The second is the one to verify first: it reuses the code that already works for
+plain links rather than adding a second, differently-sourced route to the same
+action.
+
+**Constraints.**
+
+- **No second `QWebEngineView` may be created**, whatever shape this takes. The
+ pane renders a list into one view precisely to avoid one Chromium render
+ process per message.
+- **The pane must never navigate.** Whatever handles the URL must still refuse
+ the navigation, exactly as the plain-link path does.
+- **`m_allowRemote` stays false.** Nothing here needs the interceptor relaxed:
+ the URL goes to an external browser and the pane fetches nothing. The earlier
+ draft of this item proposed loosening the interceptor and would have weakened
+ the remote-content protection for no reason.
+- **A `mailto:` link is still item 123's question**, and marketing HTML carries
+ those with `target="_blank"` too.
+
+**Verification.** A test can assert that a `target="_blank"` anchor reaches
+whatever handler is chosen; it cannot assert the browser opened. The regression
+that matters is the routing, and it is invisible today because nothing observes
+`createWindow()` at all.
+
+**Size: S.**
+
+## 127. A link's context menu offers four browser actions that cannot work
+
+**Observed (user, 2026-08-20):** right-clicking a link still offers "Open in new
+tab", "open in new window", "save link", "copy link" and "select all". The user
+identified them as probable survivors of an earlier removal, which is exactly
+what they are.
+
+**Cause (verified).** Item 100 removed the page-level browser actions, and its
+list is explicit (`src/messageview.cpp:689-694`): `Back`, `Forward`, `Reload`,
+`SavePage`. Those four are what a standard menu offers on the PAGE. The
+link-specific actions are different `WebAction` values entirely
+(`QWebEnginePage::OpenLinkInNewTab`, `OpenLinkInNewWindow`, `DownloadLinkToDisk`,
+`CopyLinkToClipboard`), and Chromium adds them only when the menu is raised over
+a link. Item 100 was tested by right-clicking the page, so they were never in
+the menu it was filtering and were never considered.
+
+**Two of them are dead and two are not**, which is why this is not a single
+sweep:
+
+- `OpenLinkInNewTab` and `OpenLinkInNewWindow` cannot work at all. There are no
+ tabs, and a new window means a second `QWebEngineView`, which the pane
+ deliberately does not create (one Chromium render process per message is the
+ reason the pane renders a list into one view). Both are dead UI today.
+- `DownloadLinkToDisk` needs a `downloadRequested` handler, which item 114
+ records does not exist anywhere. It is dead for the same reason Save image is,
+ and should be decided WITH item 114 rather than separately.
+- `CopyLinkToClipboard` works and is useful. It is the user's entire workaround
+ for item 126 on the messages that fail, by their own description.
+
+**Approach.** Remove `OpenLinkInNewTab` and `OpenLinkInNewWindow` by pointer,
+the way `removeBrowserActions()` already does, so the removal survives
+translation. Keep `CopyLinkToClipboard`. Leave `DownloadLinkToDisk` to item 114.
+
+**This is downstream of item 126 and should follow it.** Note that
+`OpenLinkInNewTab` and `OpenLinkInNewWindow` fail through the SAME missing
+`createWindow()` that 126 is about, so fixing 126 may well make both of them
+start working. That would be worse rather than better: the pane must not open
+windows, and two menu entries silently doing what one click should do is not the
+design. Decide their fate after 126 lands, when it is known what they do rather
+than what they fail to do. Removing them first and adding one back afterwards is
+two changes to the same menu.
+
+**Constraints.**
+
+- **Filter by `pageAction()` POINTER, never by text.** Item 100 established
+ this and the reason is translation: the menu is Italian under `LANG=it_IT`
+ and a text match would silently stop matching.
+- **Stranded separators must still be swept**, which `removeBrowserActions()`
+ already handles; removing two adjacent entries is exactly the case that
+ leaves one behind.
+- **The call site cannot be tested**, per item 117: `createStandardContextMenu()`
+ returns nothing outside a real context-menu event. Assert on the filter
+ function against a menu built by hand, and state the gap rather than faking
+ coverage.
+
+**Size: XS**, and smaller still if done in the same sitting as 126.
+
+**Closed 2026-08-20, unreleased, both items in one sitting.**
+
+126: `MessagePage::createWindow()` returns a `LinkRelayPage`, a page with no
+view whose `acceptNavigationRequest` hands the URL to
+`MessageView::openExternally()` and refuses, then deletes itself. The relay
+exists because `createWindow()` receives only a `WebWindowType`: the URL
+arrives afterwards as a navigation on the returned page, so an override
+returning `nullptr` discards it before it can be read. Routing it through the
+same handler the plain-link path uses is what keeps the two kinds of link from
+drifting apart.
+
+127: three actions added to `removeBrowserActions()`'s list,
+`CopyLinkToClipboard` deliberately left. The order mattered: 126 gives the page
+a real `createWindow()`, so those entries would have stopped being merely dead
+and started opening links into a tab that does not exist.
+
+**Testing needed two seams, and the reason is worth keeping.** The click cannot
+be synthesised: JavaScript is off in this profile, so `element.click()` does
+nothing (measured, `runJavaScript` returns an invalid `QVariant`), and a
+synthetic mouse press would have to land on the anchor's rect, which depends on
+the desktop's fonts. `setUrl()` is no substitute either, since it arrives as
+`NavigationTypeTyped` and takes the branch that accepts our own document load.
+So `clickLinkForTest()` and `relayBlankTargetForTest()` drive the real overrides
+on the real page, and `setLinkOpener()` substitutes a recorder for
+`QDesktopServices::openUrl`, which would otherwise launch a browser.
+
+Both routes are asserted, not just the broken one: they end at the same handler
+now, so breaking the working one while fixing the other was the plausible
+regression. Three mutations checked, all caught: `createWindow` returning
+`nullptr` fails the `_blank` test with the message naming the real defect, the
+filter losing its link actions fails one direction, and the filter also removing
+`CopyLinkToClipboard` fails the other. That last one matters because Copy link
+is the user's fallback for any link that will not open, and a future sweep of
+"dead link actions" would otherwise take it silently.
+
+
## 90. A saved-query button clears the account selection
**Observed (user, notes):** "select an account and hit the 'unread' button, the
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md
index f56f037..3848a6d 100644
--- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md
+++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md
@@ -195,8 +195,8 @@ taking that too literally.
| 125 | A skipped sync leaves the spinner running for ever | defect | S | open, 2026-08-20, found by hand. `mailsync.sh` exits 75 (EX_TEMPFAIL) when another run holds the lock; the indicator never clears, and a held edit waits for a completion that never comes |
-| 126 | A link with `target="_blank"` does nothing when clicked | defect | S | open, 2026-08-20. `createWindow()` is not overridden, so Chromium drops the click before `acceptNavigationRequest` sees it. Plain links work; marketing HTML uses `_blank` almost universally |
-| 127 | A link's context menu offers four browser actions that cannot work | defect | XS | open, 2026-08-20. Item 100 removed the page-level actions and never saw these: they appear only on a link. Follows 126, which decides which of them should survive |
+| 126 | A link with `target="_blank"` does nothing when clicked | defect | S | **done** 2026-08-20, unreleased. `createWindow()` returns a relay page that receives the navigation, hands the URL to the browser and refuses. The URL cannot be read in `createWindow()` itself, which is why a relay rather than a lookup |
+| 127 | A link's context menu offers four browser actions that cannot work | defect | XS | **done** 2026-08-20, unreleased. Three Open-in actions removed, `CopyLinkToClipboard` kept. Item 126 made them more dangerous rather than less: with a real `createWindow()` they would have started working |
Sizes are rough: XS under an hour, S a sitting, M a session.
@@ -1056,141 +1056,6 @@ Then Delete a message. Verified by hand on 2026-08-20; this is how it was found.
**Size: S.**
-## 126. A link with `target="_blank"` does nothing when clicked
-
-**Observed (user, 2026-08-20):** "if I click on a link in a mail in the message
-pane, nothing happens." Then, on checking: a plain-text mail (a GitHub PR reply)
-opens its links correctly, while an HTML mail showing the link as a button does
-not.
-
-**That second observation overturned the first diagnosis and is the whole
-item.** This entry originally blamed `RequestInterceptor` blocking `https`.
-That was wrong: the interceptor does deny `https`, but only for resources the
-document FETCHES. A clicked link never becomes a request, because
-`acceptNavigationRequest` calls `QDesktopServices::openUrl` and returns false
-before anything is issued. If the interceptor were the cause, the GitHub link
-could not work either, and it does.
-
-**Cause (verified against the two messages the user named).** The difference is
-`target="_blank"`:
-
-| message | `target` | route taken | result |
-|---|---|---|---|
-| GitHub PR reply | none on any anchor | `acceptNavigationRequest` | opens correctly |
-| HTML newsletter | `_blank` on every anchor | `createWindow()` | silently dropped |
-
-An anchor with no target navigates the main frame, so it reaches
-`MessagePage::acceptNavigationRequest` as `NavigationTypeLinkClicked` and is
-handed to the browser. An anchor asking for a new window does NOT: Chromium
-routes it to `QWebEnginePage::createWindow()`, which `MessagePage` does not
-override, so the base implementation returns `nullptr` and the click is
-discarded. `acceptNavigationRequest` is never consulted, which is why no
-existing code sees it and nothing at all happens.
-
-Both messages render HTML, so this is not a text-versus-HTML distinction: the
-GitHub mail is `multipart/alternative` and its HTML part is what the pane shows.
-Marketing HTML uses `target="_blank"` almost universally, which is why it reads
-as "HTML mail is broken".
-
-**Approach.** Override `createWindow()` in `MessagePage` to hand the URL to the
-same path a plain link takes and return `nullptr`, so no window is ever created.
-
-**One trap, and it decides the shape.** `createWindow()` is called WITHOUT the
-target URL in Qt: the signature carries only a `WebWindowType`. The URL arrives
-afterwards, as a navigation request on the page the override is expected to
-return. Returning `nullptr` therefore discards the URL before it can be seen, so
-the override cannot simply read it. Two known ways around it, and this needs
-measuring before choosing:
-
-- Keep a `linkHovered` cache and use the last hovered URL. Cheap, and wrong if
- the click arrives without a hover (keyboard activation, synthetic click).
-- Return a throwaway `QWebEnginePage` whose `acceptNavigationRequest` hands the
- URL to `openUrl` and refuses, then deletes itself. Correct by construction,
- since the URL arrives through the normal path, at the cost of a short-lived
- page object.
-
-The second is the one to verify first: it reuses the code that already works for
-plain links rather than adding a second, differently-sourced route to the same
-action.
-
-**Constraints.**
-
-- **No second `QWebEngineView` may be created**, whatever shape this takes. The
- pane renders a list into one view precisely to avoid one Chromium render
- process per message.
-- **The pane must never navigate.** Whatever handles the URL must still refuse
- the navigation, exactly as the plain-link path does.
-- **`m_allowRemote` stays false.** Nothing here needs the interceptor relaxed:
- the URL goes to an external browser and the pane fetches nothing. The earlier
- draft of this item proposed loosening the interceptor and would have weakened
- the remote-content protection for no reason.
-- **A `mailto:` link is still item 123's question**, and marketing HTML carries
- those with `target="_blank"` too.
-
-**Verification.** A test can assert that a `target="_blank"` anchor reaches
-whatever handler is chosen; it cannot assert the browser opened. The regression
-that matters is the routing, and it is invisible today because nothing observes
-`createWindow()` at all.
-
-**Size: S.**
-
-## 127. A link's context menu offers four browser actions that cannot work
-
-**Observed (user, 2026-08-20):** right-clicking a link still offers "Open in new
-tab", "open in new window", "save link", "copy link" and "select all". The user
-identified them as probable survivors of an earlier removal, which is exactly
-what they are.
-
-**Cause (verified).** Item 100 removed the page-level browser actions, and its
-list is explicit (`src/messageview.cpp:689-694`): `Back`, `Forward`, `Reload`,
-`SavePage`. Those four are what a standard menu offers on the PAGE. The
-link-specific actions are different `WebAction` values entirely
-(`QWebEnginePage::OpenLinkInNewTab`, `OpenLinkInNewWindow`, `DownloadLinkToDisk`,
-`CopyLinkToClipboard`), and Chromium adds them only when the menu is raised over
-a link. Item 100 was tested by right-clicking the page, so they were never in
-the menu it was filtering and were never considered.
-
-**Two of them are dead and two are not**, which is why this is not a single
-sweep:
-
-- `OpenLinkInNewTab` and `OpenLinkInNewWindow` cannot work at all. There are no
- tabs, and a new window means a second `QWebEngineView`, which the pane
- deliberately does not create (one Chromium render process per message is the
- reason the pane renders a list into one view). Both are dead UI today.
-- `DownloadLinkToDisk` needs a `downloadRequested` handler, which item 114
- records does not exist anywhere. It is dead for the same reason Save image is,
- and should be decided WITH item 114 rather than separately.
-- `CopyLinkToClipboard` works and is useful. It is the user's entire workaround
- for item 126 on the messages that fail, by their own description.
-
-**Approach.** Remove `OpenLinkInNewTab` and `OpenLinkInNewWindow` by pointer,
-the way `removeBrowserActions()` already does, so the removal survives
-translation. Keep `CopyLinkToClipboard`. Leave `DownloadLinkToDisk` to item 114.
-
-**This is downstream of item 126 and should follow it.** Note that
-`OpenLinkInNewTab` and `OpenLinkInNewWindow` fail through the SAME missing
-`createWindow()` that 126 is about, so fixing 126 may well make both of them
-start working. That would be worse rather than better: the pane must not open
-windows, and two menu entries silently doing what one click should do is not the
-design. Decide their fate after 126 lands, when it is known what they do rather
-than what they fail to do. Removing them first and adding one back afterwards is
-two changes to the same menu.
-
-**Constraints.**
-
-- **Filter by `pageAction()` POINTER, never by text.** Item 100 established
- this and the reason is translation: the menu is Italian under `LANG=it_IT`
- and a text match would silently stop matching.
-- **Stranded separators must still be swept**, which `removeBrowserActions()`
- already handles; removing two adjacent entries is exactly the case that
- leaves one behind.
-- **The call site cannot be tested**, per item 117: `createStandardContextMenu()`
- returns nothing outside a real context-menu event. Assert on the filter
- function against a menu built by hand, and state the gap rather than faking
- coverage.
-
-**Size: XS**, and smaller still if done in the same sitting as 126.
-
## Deferred, unsized, or split out
Items noted while triaging but not part of the original list. Same numbering
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<MessagePage *>(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<MessagePage *>(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 <QList>
#include <QUrl>
+
+#include <functional>
#include <QTimer>
#include <QWidget>
@@ -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<void(const QUrl &)>;
+ 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<ThreadRenderItem> &items);
diff --git a/tests/test_messageview.cpp b/tests/test_messageview.cpp
index 3d21536..c3e21ed 100644
--- a/tests/test_messageview.cpp
+++ b/tests/test_messageview.cpp
@@ -63,6 +63,9 @@ private slots:
void theCopyToastAppearsOverThePaneAndFades();
void theCopyToastStaysAnchoredWhenThePaneResizes();
void aSearchFromTheDetailsDialogClosesIt();
+ void aPlainLinkOpensExternally();
+ void aTargetBlankLinkOpensExternally();
+ void theLinkMenuDropsTheOpenInWindowActions();
private:
QWebEngineView *webViewOf(MessageView *view) const
@@ -1035,5 +1038,126 @@ void TestMessageView::aSearchFromTheDetailsDialogClosesIt()
|| !view.findChild<MessageDetailsDialog *>()->isVisible());
}
+
+// Item 126. A clicked link must leave the pane, and the two kinds of anchor
+// reach the outside world by DIFFERENT routes through Qt. Both are asserted,
+// because the working one is what disproved the first diagnosis: a plain-text
+// mail's links already opened while an HTML newsletter's did nothing, so a
+// test covering one route says nothing about the other.
+//
+// MessageView::setLinkOpener() is the seam. The real call ends in
+// QDesktopServices::openUrl(), which would launch a browser; what is under
+// test is WHICH clicks arrive there, never what openUrl does with them.
+
+void TestMessageView::aPlainLinkOpensExternally()
+{
+ // The route that already worked. Asserted so that fixing the other one
+ // cannot quietly break it, which is the plausible regression: both end at
+ // the same handler now.
+ QList<QUrl> opened;
+ MessageView::setLinkOpener([&opened](const QUrl &u) { opened.append(u); });
+
+ MessageView view;
+
+ // A link click as acceptNavigationRequest sees it. Driven through the page
+ // rather than synthesised: JavaScript is off in this profile, so
+ // element.click() does nothing (verified, runJavaScript returns an invalid
+ // QVariant), and a synthetic mouse press would have to land on the
+ // anchor's rect, which depends on the desktop's fonts.
+ const QUrl target(QStringLiteral("https://example.org/plain"));
+ QVERIFY2(!view.clickLinkForTest(target),
+ "a link click must be REFUSED as a navigation: the pane may "
+ "never follow a link");
+
+ QTRY_VERIFY_WITH_TIMEOUT(!opened.isEmpty(), 5000);
+ QCOMPARE(opened.size(), 1);
+ QCOMPARE(opened.first(), target);
+
+ MessageView::setLinkOpener({});
+}
+
+void TestMessageView::aTargetBlankLinkOpensExternally()
+{
+ // The defect. An anchor carrying target="_blank" never reaches
+ // acceptNavigationRequest: Chromium asks for a new window instead, and the
+ // base createWindow() returns nullptr, so the click was discarded with
+ // nothing on screen and no error anywhere. Marketing HTML sets _blank on
+ // practically every anchor, which is what made "HTML mail" look broken
+ // while a plain-text mail's links worked.
+ QList<QUrl> opened;
+ MessageView::setLinkOpener([&opened](const QUrl &u) { opened.append(u); });
+
+ MessageView view;
+ const QUrl target(QStringLiteral("https://example.org/blank"));
+
+ // Drives the real createWindow() override on the real page, then navigates
+ // what it returns, which is Chromium's own sequence. Before item 126 the
+ // page returned nothing and this is false.
+ QVERIFY2(view.relayBlankTargetForTest(target),
+ "the page provided no window for a target=\"_blank\" click, so "
+ "the URL was discarded");
+
+ QTRY_VERIFY_WITH_TIMEOUT(!opened.isEmpty(), 5000);
+ QCOMPARE(opened.size(), 1);
+ QCOMPARE(opened.first(), target);
+
+ // No second view may exist for it. The pane renders a list into ONE
+ // QWebEngineView deliberately: a view per message is a Chromium render
+ // process per message.
+ QCOMPARE(view.findChildren<QWebEngineView *>().size(), 1);
+
+ MessageView::setLinkOpener({});
+}
+
+void TestMessageView::theLinkMenuDropsTheOpenInWindowActions()
+{
+ // Item 127. Chromium adds these only when the menu is raised over a LINK,
+ // so item 100's filter never saw them: its list is the page actions, and
+ // it was tested by right-clicking the page.
+ //
+ // They are not uniform, which is why this asserts in both directions.
+ // Open in new tab and Open in new window cannot be honoured: there are no
+ // tabs and the pane must never open a window. Copy link works, and is the
+ // whole workaround a user has for any link that will not open, so removing
+ // it would take away the fallback.
+ MessageView view;
+ auto *page = view.findChild<QWebEnginePage *>();
+ QVERIFY2(page, "no page, so this test would assert nothing");
+
+ QMenu menu;
+ const QList<QWebEnginePage::WebAction> unwanted = {
+ QWebEnginePage::OpenLinkInNewTab,
+ QWebEnginePage::OpenLinkInNewWindow,
+ QWebEnginePage::OpenLinkInThisWindow,
+ };
+ const QList<QWebEnginePage::WebAction> wanted = {
+ QWebEnginePage::CopyLinkToClipboard,
+ };
+
+ for (const QWebEnginePage::WebAction which : unwanted)
+ menu.addAction(page->action(which));
+ menu.addSeparator();
+ for (const QWebEnginePage::WebAction which : wanted)
+ menu.addAction(page->action(which));
+
+ // The guard: prove the menu holds what the assertions are about, so a
+ // filter that removed everything cannot pass by accident.
+ QCOMPARE(menu.actions().size(), unwanted.size() + wanted.size() + 1);
+
+ MessageView::removeBrowserActions(&menu, page);
+
+ const QList<QAction *> left = menu.actions();
+ for (const QWebEnginePage::WebAction which : unwanted) {
+ QVERIFY2(!left.contains(page->action(which)),
+ qPrintable(QStringLiteral("a link action survived: %1")
+ .arg(page->action(which)->text())));
+ }
+ for (const QWebEnginePage::WebAction which : wanted) {
+ QVERIFY2(left.contains(page->action(which)),
+ qPrintable(QStringLiteral("a wanted action was removed: %1")
+ .arg(page->action(which)->text())));
+ }
+}
+
QTEST_MAIN(TestMessageView)
#include "test_messageview.moc"