aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-17 13:49:08 +0200
committerDanilo M. <danix@danix.xyz>2026-08-17 13:49:08 +0200
commit435a41c8476a4e94f28ad7730351498465194454 (patch)
tree0acd8c175caa24a021c57f701394680e21c4e3dd
parentabf56abd167109f50e0ddb06e1baf92192174cd7 (diff)
downloadqtmaildir-435a41c8476a4e94f28ad7730351498465194454.tar.gz
qtmaildir-435a41c8476a4e94f28ad7730351498465194454.zip
fix(ui): drop the browser's own actions from the message pane menu
The pane's context menu started from QWebEngineView::createStandardContextMenu() and kept it whole, so it offered Back, Forward, Reload and Save page. None of them can apply: every message is rendered with setHtml() from memory, so there is no history to go back to and nothing to reload, and the request interceptor blocks everything by default. They were inert as well as meaningless. removeBrowserActions() matches on the QAction pointer returned by page->action(), never on the entry's text, which is translated: a text match would work in English and fail in every other locale, which is a defect no test written in English would catch. Removing entries also strands separators at the edges or doubles them up, which reads as a menu that lost something, so the filter sweeps them; Qt offers nothing for this. View source is deliberately NOT filtered. It was removed with the other four at first, which was an overreach: the user asked for four and view-source has a real document and a real use. Chromium's own entry cannot work here either, since it navigates to view-source:<url> and MessagePage refuses that, so backlog item 113 implements it as our own plain-text dialog. The test builds a menu by hand, which is right for testing the filter and proves nothing about what Chromium's real menu contains. That limit is stated at the test, and is why it does not assert on SelectAll: the real menu has never offered it, verified by hand against a build with this filter reverted (backlog item 117). Backlog item 100.
-rw-r--r--src/messageview.cpp52
-rw-r--r--src/messageview.h21
-rw-r--r--tests/test_messageview.cpp80
3 files changed, 153 insertions, 0 deletions
diff --git a/src/messageview.cpp b/src/messageview.cpp
index bdf1b13..68821d8 100644
--- a/src/messageview.cpp
+++ b/src/messageview.cpp
@@ -612,6 +612,54 @@ SearchOffer MessageView::selectionSearchOffer(const QString &selectedText) const
query };
}
+void MessageView::removeBrowserActions(QMenu *menu, QWebEnginePage *page)
+{
+ if (!menu || !page)
+ return;
+
+ // Item 100. Every one of these needs a history, a network or a file, and
+ // this pane has none of the three.
+ //
+ // ViewSource is NOT in this list, and that is deliberate. It was removed
+ // here first, on the reasoning that it was the same kind of thing; it is
+ // not. The four below have nothing to act on, while view-source has a real
+ // document and a real use. Chromium's own entry cannot work here either
+ // (it navigates to view-source:<url>, which MessagePage refuses), so item
+ // 113 implements it as our own plain-text dialog. Removing it in the
+ // meantime would delete the gesture the user reaches for.
+ static constexpr QWebEnginePage::WebAction kUnwanted[] = {
+ QWebEnginePage::Back,
+ QWebEnginePage::Forward,
+ QWebEnginePage::Reload,
+ QWebEnginePage::SavePage,
+ };
+
+ for (const QWebEnginePage::WebAction which : kUnwanted) {
+ // pageAction() is the same QAction instance the standard menu holds,
+ // so the pointer identifies it whatever language it is displayed in.
+ if (QAction *action = page->action(which))
+ menu->removeAction(action);
+ }
+
+ // Removing entries can leave a separator at an edge or two in a row, which
+ // reads as a menu that lost something. Qt has no "tidy separators", so
+ // this walks what is left.
+ const QList<QAction *> remaining = menu->actions();
+ bool previousWasSeparator = true; // leading separators are unwanted too
+ for (QAction *action : remaining) {
+ if (!action->isSeparator()) {
+ previousWasSeparator = false;
+ continue;
+ }
+ if (previousWasSeparator)
+ menu->removeAction(action);
+ else
+ previousWasSeparator = true;
+ }
+ if (!menu->actions().isEmpty() && menu->actions().constLast()->isSeparator())
+ menu->removeAction(menu->actions().constLast());
+}
+
void MessageView::showBodyContextMenu(const QPoint &pos)
{
// The page's own menu first: copy, select all and the rest stay exactly as
@@ -621,6 +669,10 @@ void MessageView::showBodyContextMenu(const QPoint &pos)
menu = new QMenu(this);
menu->setAttribute(Qt::WA_DeleteOnClose);
+ // ...minus the browser's own navigation and page actions, which cannot
+ // apply here. Item 100.
+ removeBrowserActions(menu, m_view->page());
+
// selectedText() reads the selection out of the render process with no
// script injection. JavaScript is disabled in this profile and stays so.
const SearchOffer offer = selectionSearchOffer(m_view->page()->selectedText());
diff --git a/src/messageview.h b/src/messageview.h
index 56925b2..df20e40 100644
--- a/src/messageview.h
+++ b/src/messageview.h
@@ -29,6 +29,7 @@
class QLabel;
class QMenu;
+class QWebEnginePage;
class QPushButton;
class QWebEngineView;
class QWebEngineProfile;
@@ -149,6 +150,26 @@ public:
/// of which notmuch reports as an error.
SearchOffer selectionSearchOffer(const QString &selectedText) const;
+ /// Strips the browser actions out of Chromium's standard context menu.
+ ///
+ /// Item 100. The pane is not a browser: every document arrives through
+ /// setHtml() with a fixed base URL, so Back, Forward, Reload and Save page
+ /// have nothing to act on and the interceptor blocks everything by default
+ /// anyway. Copy and Select all are the reason the standard menu is used at
+ /// all, so the menu is filtered, not rebuilt.
+ ///
+ /// View source is NOT filtered, though it was at first. It has a real
+ /// document and a real use; item 113 implements it as our own dialog,
+ /// since Chromium's entry navigates to view-source:<url> and MessagePage
+ /// refuses that.
+ ///
+ /// Matches on the page's own QAction POINTERS, never on text, which is
+ /// translated and would make the filter fail in every locale but one.
+ ///
+ /// Static and taking the menu so a test can build one and check it
+ /// without a rendered document or a shown popup.
+ static void removeBrowserActions(QMenu *menu, QWebEnginePage *page);
+
/// Tells the pane whether the query bar currently holds anything.
///
/// The menus need it to grey out "Exclude from search": excluding from an
diff --git a/tests/test_messageview.cpp b/tests/test_messageview.cpp
index c97a216..87234aa 100644
--- a/tests/test_messageview.cpp
+++ b/tests/test_messageview.cpp
@@ -17,8 +17,10 @@
*/
#include <QLabel>
+#include <QMenu>
#include <QPushButton>
#include <QSignalSpy>
+#include <QWebEnginePage>
#include <QWebEngineUrlScheme>
#include <QWebEngineView>
#include <QtTest>
@@ -54,6 +56,7 @@ private slots:
void headerOffersNoSenderForARealThread();
void headerOffersNothingForAnAbsentField();
void bodySelectionBecomesAQuotedSearch();
+ void theBodyMenuDropsTheBrowsersOwnActions();
void aSearchFromTheDetailsDialogClosesIt();
private:
@@ -699,6 +702,83 @@ void TestMessageView::bodySelectionBecomesAQuotedSearch()
.label.isEmpty());
}
+void TestMessageView::theBodyMenuDropsTheBrowsersOwnActions()
+{
+ // Item 100. The pane starts from Chromium's standard context menu, which
+ // is built for a browser: Back, Forward, Reload, Save page and View source
+ // all arrive with it and none of them can apply, since every document
+ // comes through setHtml() with a fixed base URL and the interceptor blocks
+ // everything by default.
+ //
+ // Asserted on the ACTION POINTERS, which is also how the production code
+ // matches them. Matching on text would pass here and fail in every locale
+ // but English, and an untranslated match is exactly the defect this could
+ // reintroduce without any test noticing.
+ 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::Back, QWebEnginePage::Forward,
+ QWebEnginePage::Reload, QWebEnginePage::SavePage,
+ };
+ // Kept, and the reason the standard menu is used at all rather than being
+ // rebuilt from scratch.
+ //
+ // ViewSource is in this list deliberately. It was removed with the four
+ // above at first, which was an overreach: it has a real document and a
+ // real use, and item 113 implements it properly. A test asserting it is
+ // GONE would lock in the overreach, so it asserts it survives.
+ //
+ // SelectAll is deliberately NOT here, and the reason is a limit of this
+ // test worth stating. This menu is built BY HAND, so "SelectAll survives"
+ // would only prove the filter does not remove it, and prove nothing about
+ // whether Chromium's real menu ever offers it. Measured by hand on
+ // 2026-08-17, with a selection active: the real menu holds Copy and the
+ // search entries and no Select all, both before and after this filter
+ // existed. Asserting on it here would read as a guarantee the code does
+ // not make. See item 117.
+ const QList<QWebEnginePage::WebAction> wanted = {
+ QWebEnginePage::Copy,
+ QWebEnginePage::ViewSource,
+ };
+
+ 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: the menu really does hold what the assertions below are about,
+ // so a filter that removed everything, or a page that offered nothing,
+ // 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 browser action survived the filter: %1")
+ .arg(page->action(which)->text())));
+ }
+ for (const QWebEnginePage::WebAction which : wanted) {
+ QVERIFY2(left.contains(page->action(which)),
+ qPrintable(QStringLiteral(
+ "the filter removed an action the pane needs: "
+ "%1")
+ .arg(page->action(which)->text())));
+ }
+
+ // No separator left stranded at either edge by the removals, which reads
+ // as a menu that lost something.
+ QVERIFY(!left.isEmpty());
+ QVERIFY(!left.constFirst()->isSeparator());
+ QVERIFY(!left.constLast()->isSeparator());
+}
+
void TestMessageView::aSearchFromTheDetailsDialogClosesIt()
{
// The dialog is modal. Without closing it, the query runs and the thread