diff options
| -rw-r--r-- | CHANGELOG.md | 13 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 171 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 139 | ||||
| -rw-r--r-- | src/messageview.cpp | 136 | ||||
| -rw-r--r-- | src/messageview.h | 42 | ||||
| -rw-r--r-- | tests/test_messageview.cpp | 124 |
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" |