summaryrefslogtreecommitdiffstats
path: root/docs/superpowers/plans
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 /docs/superpowers/plans
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
Diffstat (limited to 'docs/superpowers/plans')
-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
2 files changed, 173 insertions, 137 deletions
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