diff options
Diffstat (limited to 'docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md')
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 140 |
1 files changed, 80 insertions, 60 deletions
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 2227cc6..f56f037 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,7 +195,7 @@ 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 | Clicking a link in a message does nothing | defect | S | open, 2026-08-20. The interceptor blocks the request before `acceptNavigationRequest` can hand it to the browser, so the `QDesktopServices::openUrl` already in the code is never reached | +| 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1056,64 +1056,81 @@ Then Delete a message. Verified by hand on 2026-08-20; this is how it was found. **Size: S.** -## 126. Clicking a link in a message does nothing +## 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. The only way now is to copy that link and paste it in a -browser." - -**Cause (verified in the code, and it is not the missing handler it looks -like).** `MessagePage::acceptNavigationRequest` (`src/messageview.cpp:89-109`) -already handles this correctly: a `NavigationTypeLinkClicked` is passed to -`QDesktopServices::openUrl(url)` and then refused as a navigation, so the pane -cannot follow it. That code is right and has presumably never run. - -**`RequestInterceptor` blocks the request first.** `shouldAllow()` -(`src/requestinterceptor.cpp:73-77`) denies `http` and `https` whenever -`m_allowRemote` is false, which is the default for every message and is reset -per message by `resetForNewMessage()`. The interceptor runs on the REQUEST, -before the page is asked whether to accept the navigation, so the click is -dropped at the network layer and `acceptNavigationRequest` is never consulted. -The user sees nothing at all: no error, no navigation, no browser. - -**This is the remote-content protection working as designed**, and the fix must -not weaken it. Blocking `https` is what stops a tracking pixel or a remote -stylesheet phoning home when a message is merely displayed. The bug is that a -deliberate CLICK is indistinguishable, at that layer, from a resource the -document fetched on its own. - -**Approach.** The distinction the interceptor cannot make is available one level -up: a click arrives as `ResourceTypeMainFrame` with a `NavigationTypeLinkClicked`, -while a tracking pixel arrives as `ResourceTypeImage` and never as a navigation -at all. Two shapes are worth considering, and the choice is the user's: - -- Let the main-frame navigation through the interceptor so - `acceptNavigationRequest` can see it, hand it to `QDesktopServices::openUrl`, - and still refuse the navigation. Nothing is ever fetched by the pane; the URL - only reaches the external browser. -- Or intercept the click before it becomes a request at all, which avoids - relaxing anything in the interceptor but needs the link target from the - context-menu data or a `linkHovered` cache. +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.** -- **`m_allowRemote` must stay false by default**, and this must not become a - reason to loosen it. Opening a link in an external browser is the user's - explicit act; fetching a resource into the pane is not. -- **The pane must still never navigate.** `acceptNavigationRequest` returning - false for a link click is what protects that, and it stays whatever else - changes. -- **A `mailto:` link is a separate question** and probably belongs to item 123, - since composing is v2. Until then it should behave like any other scheme - rather than being special-cased into silence. -- **`qtmaildir-query:` links must keep their existing path.** They are the - placeholder's own helper lines and are handled before the `openUrl` call; - a message body carrying one is already refused by the view. - -**Verification.** Not testable through the offscreen platform end to end, since -it ends in an external browser. The reachable assertion is that a link click -reaches `acceptNavigationRequest` at all, which is what fails today; the -`openUrl` call itself is one line beyond that and is better confirmed by hand. +- **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.** @@ -1143,17 +1160,20 @@ sweep: - `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 currently the user's entire - workaround for item 126, by their own description. +- `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.** If 126 makes a click -open the system browser, then "Open link" becomes a meaningful entry and the -right menu is one entry that does what the click does, rather than two Chromium -entries that do nothing. Removing them first and adding one back afterwards is +**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.** |