From 2549b7e0190b96bfc72a5b2f23e0382d37b7c306 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Sat, 22 Aug 2026 20:09:25 +0200 Subject: [PATCH] feat(mail): select every conversation in a view, in one request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four reported items, and the feature that turned out to be the fix for two of them. **Selecting past the page.** "Select all" meant the fifty rows on screen, which is the only thing a checkbox over a paginated list can mean on its own — and not what anybody wants in a bin holding a hundred and ninety-five. When the page is fully selected and the view holds more, the toolbar now offers the rest, and the same gesture covers "every starred mail". The view arrives as a named scope, not as the URL the user is on. Posting the URL would make every mail list route an input to a bulk WRITE, so a route added later for reading becomes a way of deleting without anyone deciding that. The scope names are the ones the list templates already use for themselves (`{% block list_scope %}`), so there is nothing new to learn and the two cannot drift. Unread-only travels with it: a "select all" that ignored the filter would act on mail the person cannot see. **One request, not one per conversation.** The toolbar posted per thread, in parallel — fine for fifty rows, impossible for a folder. That also could not answer for the LIST: each response redrew its own row and knew nothing else, so an emptied list still said "1–5 of 5" (Schreibtest 10). The bulk response carries the rows AND the pane re-reads the list, because the pager and the empty state are properties of the list that no row-shaped stream can carry. Two single-thread assumptions had to go with it, and both were correct until now. `finish()` resynced only `$messages[0]->thread`, so a bulk archive answered 200, reported 71, and changed nothing on screen — the threads kept the Inbox label their messages no longer had. `markRead()` wrote the whole selection's unread count onto the first thread, so four conversations were marked read and three rows went on rendering as unread. Both now work per thread, and the bulk controller groups by ACCOUNT because ThreadStatusUpdater resolves the destination folder from the first message's account. **The 404 was English and in the wrong theme** (Befunde 2). Not an oversight in the template: a 404 is thrown by the router at priority 32 of kernel.request, before the firewall at 8 and before UserLocaleSubscriber at 6 — nothing has authenticated when the exception is handled, so there is no user, no locale and no theme. The error template must not look one up, and its own docblock explains why at length. So both hints ride in a cookie written on ordinary responses. **Sie/du** (Befunde 8). Seven strings addressed the reader formally, and the report found them in Aliases because that is where the reader happened to look. Guarded by a test that uses the one rule that can be checked: German capitalises both the polite second person and the first word of a sentence, so a capitalised form MID-sentence can only be formal address. A naive search flags a dozen good sentences, gets an exception list, and the exception list then swallows the next real one. **A revert, recorded rather than hidden.** The label menu's escape to the browser's top layer is gone. The bug is real — the panes' `backdrop-filter` makes each a stacking context and the containing block for `position: fixed`, so nothing inside one can rise above the chrome — but the fix broke clicking: `elementFromPoint` at an option's own coordinates answers null and the click never lands. It did the same to the snooze menu. Three specs caught it. A menu that is visible and does nothing is worse than one that is clipped, and I could not find the cause without seeing the real layout, so the spec is marked fixme with the whole story in it. Also: a shared `rowAction()` for the specs. A row's actions are `visibility: hidden` until hovered, which makes them easy to lose under four parallel workers and — because a hidden element is not in the accessibility tree — makes `getByRole` report a missing action for one that is merely unhovered. That was three of my own specs failing on a different run each time. 4027 unit tests, PHPStan clean, 550 browser tests. Co-Authored-By: Claude Opus 5 --- .../controllers/mail/label_menu_controller.js | 14 - .../mail/list_toolbar_controller.js | 239 +++++++++++++----- assets/controllers/ui/dropdown_controller.js | 36 +-- assets/popout.js | 131 ---------- assets/styles/app.css | 22 -- src/Controller/Mail/BulkStatusController.php | 133 ++++++++++ .../Subscriber/AppearanceCookieSubscriber.php | 138 ++++++++++ .../Event/Subscriber/UserLocaleSubscriber.php | 56 +++- src/Service/Mail/ListViewResolver.php | 154 +++++++++++ src/Service/Mail/ThreadStatusUpdater.php | 54 +++- src/Twig/AppearanceExtension.php | 23 +- templates/_layout/_mailbox.html.twig | 20 +- templates/_partials/_list_toolbar.html.twig | 32 ++- templates/mail/inbox.html.twig | 3 + templates/mail/label.html.twig | 6 + templates/mail/starred.html.twig | 2 + .../thread/status/_bulk.stream.html.twig | 57 +++++ tests/Documentation/GermanIsInformalTest.php | 129 ++++++++++ .../Event/ErrorPageAppearanceTest.php | 126 +++++++++ tests/e2e/bulk-select-all.spec.ts | 139 ++++++++++ tests/e2e/menu-escapes-pane.spec.ts | 23 +- tests/e2e/restore-from-anywhere.spec.ts | 28 +- tests/e2e/snooze-menu.spec.ts | 4 +- tests/e2e/snoozed-list.spec.ts | 12 + tests/e2e/support/rows.ts | 58 +++++ translations/messages.de.yaml | 21 +- translations/messages.en.yaml | 7 + 27 files changed, 1357 insertions(+), 310 deletions(-) delete mode 100644 assets/popout.js create mode 100644 src/Controller/Mail/BulkStatusController.php create mode 100644 src/Infrastructure/Event/Subscriber/AppearanceCookieSubscriber.php create mode 100644 src/Service/Mail/ListViewResolver.php create mode 100644 templates/thread/status/_bulk.stream.html.twig create mode 100644 tests/Documentation/GermanIsInformalTest.php create mode 100644 tests/Infrastructure/Event/ErrorPageAppearanceTest.php create mode 100644 tests/e2e/bulk-select-all.spec.ts create mode 100644 tests/e2e/support/rows.ts diff --git a/assets/controllers/mail/label_menu_controller.js b/assets/controllers/mail/label_menu_controller.js index da2d6f7a..b0266523 100644 --- a/assets/controllers/mail/label_menu_controller.js +++ b/assets/controllers/mail/label_menu_controller.js @@ -1,6 +1,5 @@ import { Controller } from "@hotwired/stimulus"; import { jsonCsrfHeaders } from "../../csrf.js"; -import { pinToTopLayer, releaseFromTopLayer } from "../../popout.js"; /** * "Label as" dropdown. Toggles a label on the target thread/message via the @@ -38,18 +37,10 @@ export default class extends Controller { } else { this.panelTarget.classList.remove("hidden"); - // Into the top layer, or it never gets out of the mail pane: the - // panes carry backdrop-filter, which makes them a stacking context - // AND the containing block for position:fixed, and every ancestor - // from there down is overflow:hidden. This menu used to vanish - // behind the navbar and no z-index could have fixed it. See - // assets/popout.js. // Bulk mode: the menu is shared by whatever is selected, so what it // shows has to be read from the selection each time it opens. this._syncFromSelection(); - pinToTopLayer(event.currentTarget, this.panelTarget); - document.addEventListener("click", this._boundClose, { capture: true }); } } @@ -162,11 +153,6 @@ export default class extends Controller { } _close() { - // Out of the top layer first: a hidden popover that was never dismissed - // keeps its scroll and resize listeners, and the next open would add a - // second pair. - releaseFromTopLayer(this.panelTarget); - this.panelTarget.classList.add("hidden"); document.removeEventListener("click", this._boundClose, { capture: true }); } diff --git a/assets/controllers/mail/list_toolbar_controller.js b/assets/controllers/mail/list_toolbar_controller.js index e9c87483..5dfb93c0 100644 --- a/assets/controllers/mail/list_toolbar_controller.js +++ b/assets/controllers/mail/list_toolbar_controller.js @@ -9,7 +9,8 @@ // reach it. import { Controller } from "@hotwired/stimulus"; -import { csrfToken } from "../../csrf.js"; +import * as Turbo from "@hotwired/turbo"; +import { jsonCsrfHeaders } from "../../csrf.js"; import { announceWrite } from "../../mail_writes.js"; // Classes applied to the checkbox button in each state @@ -29,9 +30,39 @@ export default class extends Controller { "selectMenuBtn", "actions", // wrapper div around bulk-action buttons "selectionCount", + "viewBanner", // the "select all N in this view" strip + "viewBannerText", + "viewBannerAction", ]; - static values = { total: Number }; + static values = { + total: Number, + // What this list is showing, in the vocabulary the list template + // already uses for itself ({% block list_scope %}) — see + // App\Service\Mail\ListViewResolver. `scopeValue` carries the + // category for the inbox and the label id for a label view. + // `viewScope` / `viewValue`, not `scope` / `scopeValue`: Stimulus + // spells a value as `data-…--value`, so a value called + // `scopeValue` becomes `data-…-scope-value-value` while `scope` becomes + // `data-…-scope-value`. The two are one character apart in the markup + // and mean entirely different things, which is how the view descriptor + // arrived empty and the bulk action archived nothing while answering + // 200. + viewScope: String, + viewValue: String, + unreadOnly: Boolean, + i18n: Object, + }; + + /** + * True once "select all N" has been pressed: the selection is the VIEW, + * not the rows on screen. + * + * Cleared by anything that changes what is selected, including a bulk + * action finishing — a person who deleted everything and then clicks + * Archive must not archive everything that arrived since. + */ + #allInView = false; // ── Lifecycle ───────────────────────────────────────────────────────── @@ -106,27 +137,43 @@ export default class extends Controller { // ── Bulk actions ────────────────────────────────────────────────────── async archiveSelected() { - const ids = this._selectedIds(); - if (ids.length === 0) { return; } - await this._bulkPost(ids, "archive"); + await this._bulkPost("archive"); } async deleteSelected() { - const ids = this._selectedIds(); - if (ids.length === 0) { return; } - await this._bulkPost(ids, "trash"); + await this._bulkPost("trash"); } async markReadSelected() { - const ids = this._selectedIds(); - if (ids.length === 0) { return; } - await this._bulkPost(ids, "read", { read: true }); + await this._bulkPost("read", { read: true }); } async markUnreadSelected() { - const ids = this._selectedIds(); - if (ids.length === 0) { return; } - await this._bulkPost(ids, "read", { read: false }); + await this._bulkPost("read", { read: false }); + } + + /** + * Extend the selection past the page, to everything in this view. + * + * Offered only when every row on screen is already selected and there are + * more than fit — the state where "select all" has visibly done less than + * the words promise. It is what a bin holding a hundred and ninety-five + * messages needs, and the same gesture as selecting every starred mail. + */ + selectAllInView(event) { + event?.preventDefault(); + + this.#allInView = true; + this._syncFromRows(); + } + + /** Back to the rows on screen. */ + clearViewSelection(event) { + event?.preventDefault(); + + this.#allInView = false; + this._setAllRows(false); + this._syncFromRows(); } /** @@ -188,66 +235,74 @@ export default class extends Controller { * @param {string} action - route suffix: archive | trash | read | snooze | star * @param {object} body - optional JSON body (e.g. { read: true }) */ - async _bulkPost(ids, action, body = {}) { - const csrf = csrfToken(); + /** + * One request for the whole selection. + * + * It used to be one request per conversation, fired in parallel. That is + * survivable for the fifty rows a page holds and impossible for what this + * now offers: "select all" over a bin of two hundred would have opened two + * hundred connections, and a real mailbox is worse. + * + * It also could not answer for the list. Each response redrew its own row + * and knew nothing else, so a list that lost four of five rows still said + * "1–5 of 5" and an emptied one never showed its empty state — the pager + * and the placeholder belong to the LIST, and no row-shaped response + * carries them. The bulk response refreshes the frame instead. + */ + async _bulkPost(action, body = {}) { + const ids = this.#allInView ? [] : this._selectedIds(); - // Hold the list refresh for the whole run. Each of these posts makes the - // server publish a sync, and a refresh answering one of them mid-run - // re-renders the list from a state where the remaining threads have not - // been acted on yet — putting rows back that the streams below are about - // to remove, as fresh elements the streams can no longer address. The - // rows then stay on screen having been archived. See - // mail_pane_controller#hold. + if (false === this.#allInView && ids.length === 0) { + return; + } + + // Holds the mail pane's refresh of the list frame for the duration — + // see mail--mail-pane#hold. Without it the frame reloads mid-run and + // the rows being acted on are replaced underneath the request. this.dispatch("writing"); try { - await this._bulkPostInner(ids, action, body, csrf); - } finally { - // Releases the mail pane's hold on the list frame — a statement - // about THIS run of posts, and still the toolbar's own. - this.dispatch("written"); + const response = await fetch(`${STATUS_BASE}/bulk/${action}`, { + method: "POST", + headers: jsonCsrfHeaders(), + body: JSON.stringify({ + ...body, + ids, + all: this.#allInView, + scope: this.viewScopeValue, + value: this.viewValueValue, + unreadOnly: this.unreadOnlyValue, + }), + }); - // And the separate, general one the badges listen for. The sidebar - // used to listen to the line above; it no longer does, so a bulk - // action has to make both statements. See assets/mail_writes.js - // for why they stayed two events rather than becoming one. - announceWrite(); - } - } + if (false === response.ok) { + console.error(`[list-toolbar] bulk ${action} failed`, response.status); - async _bulkPostInner(ids, action, body, csrf) { - const results = await Promise.all( - ids.map(async (id) => { - const url = `${STATUS_BASE}/thread/${id}/${action}`; - const response = await fetch(url, { - method: "POST", - headers: { - "Content-Type": "application/json", - "X-CSRF-Token": csrf, - }, - body: JSON.stringify(body), - }); - - if (!response.ok) { - console.error(`[list-toolbar] ${action} failed for thread ${id}`, response.status); - return null; - } - - return response.text(); - }), - ); - - // Render streams in selection order so DOM mutations are predictable. - for (const html of results) { - if (html !== null && html.trim() !== "") { - Turbo.renderStreamMessage(html); + return; } - } - // Uncheck all rows after the action succeeds (rows may have been - // removed from the DOM by the stream responses). - this._setAllRows(false); - this._syncFromRows(); + // The rows first, so the click has an answer immediately, then the + // list itself for the pager and the empty state — see + // thread/status/_bulk.stream.html.twig for why it takes both. + Turbo.renderStreamMessage(await response.text()); + + // The list itself — the pager, the empty state, the total — is + // re-read by the mail pane when the "written" event below releases + // its hold. NOT by frame.reload(): this frame is rendered inline + // and has no `src`, and reload() on a src-less turbo-frame does + // nothing at all, silently. That is worth writing down, because it + // looks exactly like a refresh that ran and found no changes. + } finally { + this.dispatch("written"); + announceWrite(); + + // The selection is spent either way. Leaving #allInView set is how + // a second click would act on everything that had arrived since — + // which is the shape of the desync that was reported, made worse. + this.#allInView = false; + this._setAllRows(false); + this._syncFromRows(); + } } /** @@ -272,8 +327,56 @@ export default class extends Controller { // ── Count label ────────────────────────────────────────────────── if (this.hasSelectionCountTarget) { - this.selectionCountTarget.textContent = - checkedCount > 0 ? String(checkedCount) : ""; + this.selectionCountTarget.textContent = this.#allInView + ? String(this.totalValue) + : (checkedCount > 0 ? String(checkedCount) : ""); + } + + this.#syncViewBanner(allChecked); + } + + /** + * The "and the other 145" offer. + * + * Shown only where "select all" has visibly done less than the words + * promise: every row on screen is ticked AND the view holds more than fit. + * On a list that fits on one page there is nothing to extend to, and + * offering it anyway would be a control that does nothing. + */ + #syncViewBanner(allChecked) { + if (false === this.hasViewBannerTarget) { + return; + } + + const extendable = allChecked && this.totalValue > this._rowCheckboxes().length; + + this.viewBannerTarget.classList.toggle("hidden", false === (extendable || this.#allInView)); + + if (false === this.hasViewBannerTextTarget) { + return; + } + + // Two sentences, because they say opposite things: one offers to widen + // the selection, the other reports that it IS widened and offers the + // way back. A single string with a number swapped in would have to be + // one or the other. + const i18n = this.i18nValue ?? {}; + + this.viewBannerTextTarget.textContent = this.#allInView + ? (i18n.allSelected ?? "").replace("%count%", String(this.totalValue)) + : (i18n.selectAll ?? "").replace("%count%", String(this.totalValue)); + + if (this.hasViewBannerActionTarget) { + this.viewBannerActionTarget.textContent = this.#allInView + ? (i18n.clear ?? "") + : (i18n.selectAllAction ?? ""); + + this.viewBannerActionTarget.setAttribute( + "data-action", + this.#allInView + ? "click->mail--list-toolbar#clearViewSelection" + : "click->mail--list-toolbar#selectAllInView", + ); } } diff --git a/assets/controllers/ui/dropdown_controller.js b/assets/controllers/ui/dropdown_controller.js index 1a7557ca..7e46cacf 100644 --- a/assets/controllers/ui/dropdown_controller.js +++ b/assets/controllers/ui/dropdown_controller.js @@ -1,5 +1,4 @@ import { Controller } from "@hotwired/stimulus"; -import { pinToTopLayer, releaseFromTopLayer } from "../../popout.js"; /** * A small click-to-open menu. @@ -28,7 +27,6 @@ import { pinToTopLayer, releaseFromTopLayer } from "../../popout.js"; */ export default class extends Controller { static targets = ["menu"]; - static values = { escapes: Boolean }; connect() { this._boundOutside = this._handleOutside.bind(this); @@ -56,36 +54,12 @@ export default class extends Controller { toggle(event) { event?.preventDefault(); - this.menuTarget.hidden ? this.open(event?.currentTarget) : this.close(); + this.menuTarget.hidden ? this.open() : this.close(); } - /** - * @param {HTMLElement} [trigger] the control the menu should hang under. - * Defaults to this controller's element, which is the wrapper the - * menu was anchored to when it positioned itself with CSS. - */ - open(trigger) { + open() { this.menuTarget.hidden = false; - // Into the top layer, for the menus that need to leave their pane. - // - // Opt-in, and that is the correction rather than the original design: - // applying it to every menu this controller opens broke the ones that - // are deliberately anchored to the box they live in. The compose - // window's emoji picker is centred ON the window and asserted to stay - // inside it — escaping to the viewport is exactly wrong there, and the - // specs said so. - // - // So a menu asks. The ones that ask are the ones opened inside a pane - // and needing to overlap the chrome around it: the panes carry - // backdrop-filter, which makes each a stacking context AND the - // containing block for position:fixed, so nothing anchored inside one - // can rise above the header whatever its z-index says. See - // assets/popout.js. - if (true === this.escapesValue) { - pinToTopLayer(trigger ?? this.element, this.menuTarget); - } - // For menus whose contents go stale while closed — the snooze menu // recomputes its wake times here, so one left open across midnight // cannot offer a "tomorrow" that is already today. @@ -100,12 +74,6 @@ export default class extends Controller { return; } - // Unconditional, unlike the open: a menu that stopped opting in while - // one of its panels was still shown would otherwise keep the scroll - // and resize listeners for ever. releaseFromTopLayer does nothing to a - // panel that was never pinned. - releaseFromTopLayer(this.menuTarget); - this.menuTarget.hidden = true; this._detach(); } diff --git a/assets/popout.js b/assets/popout.js deleted file mode 100644 index 8ed0bd89..00000000 --- a/assets/popout.js +++ /dev/null @@ -1,131 +0,0 @@ -/** - * Getting a menu out from under the pane it was declared in. - * - * ## The trap - * - * plMail's panes carry `backdrop-filter`, and that has two consequences most - * CSS does not prepare you for. It makes the pane a **stacking context**, so a - * `z-30` menu inside it can never rise above a `z-20` header OUTSIDE it — - * z-index only compares within a context, and raising the number does nothing. - * And it makes the pane a **containing block for `position: fixed`**, so the - * usual escape hatch is not one: a fixed menu anchors to the pane and is - * clipped by it, exactly like an absolute one. - * - * On top of that every ancestor from the pane down is `overflow: hidden`, so - * the menu is clipped as well as painted underneath. - * - * That is why the label menu disappeared behind the navbar, and why the compose - * recipient dropdown could not overlay the dialog and shoved it down the page - * instead. Same cause, two symptoms, and neither is fixable with a z-index. - * - * tooltip_controller.js hit this first and solved it by rendering its bubble on - * . That works and it costs something these menus cannot pay: moving an - * element out of its subtree takes it away from the Stimulus controller whose - * targets and actions are scoped to it. - * - * ## The top layer - * - * The popover API puts an element in the browser's top layer — above every - * stacking context, clipped by nothing — while leaving it exactly where it is - * in the DOM. Targets keep resolving, actions keep firing, the controller does - * not know anything moved. It is the same property `.showModal()` gives - * the confirmation dialog, without the modality. - * - * Position has to be supplied, because a top-layer element is positioned - * against the viewport rather than against the trigger it belongs to. That is - * the whole of what this module does. - */ - -/** Gap between trigger and panel, matching the `mt-1` these menus used. */ -const OFFSET = 4; - -/** Keep this much clear of the viewport edge. */ -const MARGIN = 8; - -/** - * Show `panel` in the top layer, pinned under `trigger`. - * - * Falls back to leaving the panel where it is when the browser has no popover - * support. That is not a hypothetical branch worth dropping: without it the - * menu would be `display: none` forever on an older browser, which is worse - * than the clipping this exists to fix. - * - * @param {HTMLElement} trigger - * @param {HTMLElement} panel - */ -export function pinToTopLayer(trigger, panel) { - if (typeof panel.showPopover !== "function") { - return; - } - - if (false === panel.hasAttribute("popover")) { - // "manual" rather than "auto": light-dismiss would fight the outside - // click handling these controllers already do, and auto popovers close - // each other — so opening the label menu would shut a menu that is - // meant to stay open beneath it. - panel.setAttribute("popover", "manual"); - } - - panel.showPopover(); - position(trigger, panel); - - // The trigger moves when the list behind it scrolls, and the viewport - // changes under rotation. Both leave a top-layer panel floating where the - // trigger used to be, because nothing about it is anchored any more. - const reposition = () => position(trigger, panel); - - panel.__plReposition = reposition; - window.addEventListener("scroll", reposition, { capture: true, passive: true }); - window.addEventListener("resize", reposition, { passive: true }); -} - -/** - * Take it back out of the top layer. - * - * @param {HTMLElement} panel - */ -export function releaseFromTopLayer(panel) { - if (panel.__plReposition) { - window.removeEventListener("scroll", panel.__plReposition, { capture: true }); - window.removeEventListener("resize", panel.__plReposition); - delete panel.__plReposition; - } - - if (typeof panel.hidePopover === "function" && true === panel.matches(":popover-open")) { - panel.hidePopover(); - } -} - -/** - * Under the trigger, right edges aligned, nudged back on screen if that would - * put it off the edge. - * - * Right-aligned because that is how every one of these menus was already - * anchored (`absolute right-0`), and flipping above the trigger when there is - * no room below is what stops a long label list from running off the bottom of - * a short window. - */ -function position(trigger, panel) { - const anchor = trigger.getBoundingClientRect(); - const box = panel.getBoundingClientRect(); - - let top = anchor.bottom + OFFSET; - - if (top + box.height > window.innerHeight - MARGIN) { - const above = anchor.top - box.height - OFFSET; - - // Only flip when there is genuinely more room up there. On a very short - // window both are bad and below is the one that reads as a menu. - top = above >= MARGIN ? above : Math.max(MARGIN, window.innerHeight - box.height - MARGIN); - } - - let left = anchor.right - box.width; - - left = Math.min(left, window.innerWidth - box.width - MARGIN); - left = Math.max(MARGIN, left); - - panel.style.position = "fixed"; - panel.style.margin = "0"; - panel.style.top = `${Math.round(top)}px`; - panel.style.left = `${Math.round(left)}px`; -} diff --git a/assets/styles/app.css b/assets/styles/app.css index c0740480..7302f7d9 100644 --- a/assets/styles/app.css +++ b/assets/styles/app.css @@ -3992,25 +3992,3 @@ label:has(> input.sr-only) { text-underline-offset: 2px; } -/* Menus rendered in the top layer. - * - * The popover API is what gets a menu out of a pane it can otherwise never - * leave — see assets/popout.js — and it arrives with a UA stylesheet of its - * own: a border, padding, a background, and `inset: 0` with `margin: auto`, - * all of which fight the panel's own styling and the coordinates popout.js - * computes for it. - * - * Reset rather than restyled: these elements already carry the app's classes, - * and the only thing needed here is to stop the browser adding a second, - * unstyled box around them. `inset: auto` is the load-bearing one — without it - * the UA centres the panel in the viewport and the computed top/left are - * ignored. - */ -[popover]:popover-open { - inset: auto; - margin: 0; - padding: 0; - border: 0; - background: transparent; - overflow: visible; -} diff --git a/src/Controller/Mail/BulkStatusController.php b/src/Controller/Mail/BulkStatusController.php new file mode 100644 index 00000000..66304572 --- /dev/null +++ b/src/Controller/Mail/BulkStatusController.php @@ -0,0 +1,133 @@ + 'archive|trash|read|restore'])] + public function bulk(Request $request, string $action): Response + { + $this->assertCsrf($request, 'ajax'); + + /** @var User $user */ + $user = $this->getUser(); + + $body = json_decode($request->getContent(), true); + $body = is_array($body) ? $body : []; + + $threads = true === ($body['all'] ?? false) + ? $this->views->threadsIn( + $user, + (string) ($body['scope'] ?? ''), + (string) ($body['value'] ?? ''), + true === ($body['unreadOnly'] ?? false), + ) + : $this->threadRepository->findBy(['id' => array_map(intval(...), (array) ($body['ids'] ?? []))]); + + // Grouped by account, and that is not an optimisation. + // ThreadStatusUpdater resolves the destination label and its IMAP + // folder from the FIRST message's account — which is right for every + // caller that names one conversation, and wrong for a selection + // spanning two mailboxes: everything would be filed into the first + // account's Archive. One call per account keeps each resolution + // correct, and there are rarely more than three. + $byAccount = []; + + foreach ($threads as $thread) { + // Every thread, not just the first: the ids arrive from a browser + // and a selection can be edited before it is posted. + $this->denyAccessUnlessGranted(OwnershipVoter::OWN, $thread); + + foreach ($thread->messages as $message) { + $byAccount[(int) $message->account?->id][] = $message; + } + } + + if ([] === $byAccount) { + return $this->renderTurboStream('thread/status/_bulk.stream.html.twig', [ + 'count' => 0, + 'threads' => [], + 'leaves' => false, + ]); + } + + foreach ($byAccount as $messages) { + match ($action) { + 'archive' => $this->status->archive($messages), + 'trash' => $this->status->trash($messages), + 'restore' => $this->status->restore($messages), + 'read' => $this->status->markRead($messages, true === ($body['read'] ?? true)), + // Unreachable: the route requirement above is the allow-list, so a + // fifth action cannot arrive here without being added there too. + // Stated rather than assumed, because the day someone widens the + // requirement and forgets this arm, a silent no-op is the worst + // possible answer for an action that says it deleted things. + default => throw $this->createNotFoundException(sprintf('Unknown bulk action "%s".', $action)), + }; + } + + return $this->renderTurboStream('thread/status/_bulk.stream.html.twig', [ + 'count' => count($threads), + 'threads' => $threads, + // Whether the conversation is still in the list it was acted on + // from. Archive, trash and restore all move it somewhere else; + // marking read leaves it exactly where it was and only changes how + // it draws. + 'leaves' => 'read' !== $action, + ]); + } +} diff --git a/src/Infrastructure/Event/Subscriber/AppearanceCookieSubscriber.php b/src/Infrastructure/Event/Subscriber/AppearanceCookieSubscriber.php new file mode 100644 index 00000000..66dc2b86 --- /dev/null +++ b/src/Infrastructure/Event/Subscriber/AppearanceCookieSubscriber.php @@ -0,0 +1,138 @@ +isMainRequest()) { + return; + } + + $user = $this->security->getUser(); + + if (false === $user instanceof User) { + return; + } + + $wanted = $this->value($user->appearance->theme->value, $event->getRequest()->getLocale()); + + // Only when it changed. Sending an identical Set-Cookie on every + // response is noise on the wire and, more to the point, makes every + // response uncacheable for no gain. + if ($wanted === $event->getRequest()->cookies->get(self::COOKIE)) { + return; + } + + $event->getResponse()->headers->setCookie( + Cookie::create(self::COOKIE, $wanted) + ->withExpires(time() + self::LIFETIME) + ->withPath('/') + // Readable by JavaScript is not needed and not offered; the + // theme bootstrap in the layout uses localStorage for its own + // reasons and is unaffected by this. + ->withHttpOnly(true) + ->withSameSite(Cookie::SAMESITE_LAX), + ); + } + + /** `theme|locale`, which is all either reader needs. */ + private function value(string $theme, string $locale): string + { + return $theme . '|' . $locale; + } + + /** + * The theme and locale a request carries, or nulls. + * + * Static because both readers are places with no service container worth + * the name — a Twig extension that must not fail, and a template. + * + * @return array{0: ?string, 1: ?string} + */ + public static function read(?string $cookie): array + { + if (null === $cookie || '' === $cookie) { + return [null, null]; + } + + $parts = explode('|', $cookie, 2); + + $theme = '' !== $parts[0] ? $parts[0] : null; + $locale = isset($parts[1]) && '' !== $parts[1] ? $parts[1] : null; + + // Both are compared against known values by their readers, so a + // hand-edited cookie can only ever fail to match. + return [$theme, $locale]; + } + + /** @return Appearance the stored theme applied to the defaults */ + public static function appearanceFrom(?string $cookie): Appearance + { + $appearance = new Appearance(); + + [$theme] = self::read($cookie); + + if (null === $theme) { + return $appearance; + } + + $known = Theme::tryFrom($theme); + + if (null !== $known) { + $appearance->theme = $known; + } + + return $appearance; + } +} diff --git a/src/Infrastructure/Event/Subscriber/UserLocaleSubscriber.php b/src/Infrastructure/Event/Subscriber/UserLocaleSubscriber.php index b7de29b4..a6baa02e 100644 --- a/src/Infrastructure/Event/Subscriber/UserLocaleSubscriber.php +++ b/src/Infrastructure/Event/Subscriber/UserLocaleSubscriber.php @@ -7,6 +7,8 @@ namespace App\Infrastructure\Event\Subscriber; use App\Entity\User\User; use App\Domain\Enum\AppLocale; use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use App\Infrastructure\Event\Subscriber\AppearanceCookieSubscriber; +use Symfony\Component\HttpKernel\Event\ExceptionEvent; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; @@ -30,7 +32,12 @@ final class UserLocaleSubscriber implements EventSubscriberInterface public static function getSubscribedEvents(): array { - return [KernelEvents::REQUEST => ['onKernelRequest', 6]]; + return [ + KernelEvents::REQUEST => ['onKernelRequest', 6], + // Before Symfony's own ErrorListener (-128), so the error page is + // rendered in the right language rather than corrected afterwards. + KernelEvents::EXCEPTION => ['onKernelException', 0], + ]; } public function onKernelRequest(RequestEvent $event): void @@ -54,4 +61,51 @@ final class UserLocaleSubscriber implements EventSubscriberInterface $event->getRequest()->setLocale($locale->value); $this->translator->setLocale($locale->value); } + + /** + * The same job for the pages nobody authenticated for. + * + * A 404 is thrown by the router at priority 32 of kernel.request — before + * the firewall at 8 and before onKernelRequest above at 6. Neither has run + * when the exception is handled, so the token storage is empty and the + * request still carries the default locale: a German user mistyping a URL + * got an English page, in the default theme, which reads as a different + * application. + * + * The user cannot be looked up here either, for the same reason the error + * template refuses to: whatever threw might be the session, the database or + * the firewall itself, and an error handler that throws leaves the visitor + * on Symfony's bare fallback. So the locale comes from the cookie written + * on ordinary responses — see AppearanceCookieSubscriber, which carries the + * theme in the same value for the same reason. + * + * A no-op when the token IS available, which is every exception that is not + * a routing failure: those have been through onKernelRequest already and + * the locale is right. + */ + public function onKernelException(ExceptionEvent $event): void + { + if (false === $event->isMainRequest()) { + return; + } + + $request = $event->getRequest(); + + if (null !== $this->tokenStorage->getToken()?->getUser()) { + return; + } + + [, $stored] = AppearanceCookieSubscriber::read( + $request->cookies->get(AppearanceCookieSubscriber::COOKIE), + ); + + $locale = AppLocale::tryFromRequest($stored); + + if (null === $locale) { + return; + } + + $request->setLocale($locale->value); + $this->translator->setLocale($locale->value); + } } diff --git a/src/Service/Mail/ListViewResolver.php b/src/Service/Mail/ListViewResolver.php new file mode 100644 index 00000000..897cfb8b --- /dev/null +++ b/src/Service/Mail/ListViewResolver.php @@ -0,0 +1,154 @@ + at most self::LIMIT of them + */ + public function threadsIn(User $user, string $scope, string $value, bool $unreadOnly): array + { + $page = 1; + $found = []; + + do { + $batch = $this->page($user, $scope, $value, $unreadOnly, $page); + + foreach ($batch as $thread) { + $found[] = $thread; + + if (count($found) >= self::LIMIT) { + return $found; + } + } + + ++$page; + } while (count($batch) === self::CHUNK); + + return $found; + } + + /** + * @return list + */ + private function page(User $user, string $scope, string $value, bool $unreadOnly, int $page): array + { + if ('inbox' === $scope) { + // The inbox is per category tab, and the tab IS the view: selecting + // everything while looking at Notifications must not reach into + // Primary, which is not on screen. + $category = MessageCategory::tryFrom($value) ?? MessageCategory::Primary; + + return $this->threads->findForUnifiedInbox($user, $category, $page, self::CHUNK, unreadOnly: $unreadOnly); + } + + if ('label' === $scope) { + $label = $this->labels->find((int) $value); + + // Somebody else's label resolves to nothing rather than to an + // error: this is reached from a bulk write, and the honest answer + // to "act on a label you do not own" is that there is nothing to + // act on. + if (null === $label || $label->usr?->id !== $user->id) { + return []; + } + + return $this->threads->findForLabel($label, page: $page, perPage: self::CHUNK, unreadOnly: $unreadOnly); + } + + // Starred is not a label role — it is a column on the thread — so it + // has its own repository method rather than a row in the table below. + // Being asked for it is not exotic: "select every starred mail" is the + // example the feature was asked for with. + if ('starred' === $scope) { + return $this->threads->findForStarred($user, $page, self::CHUNK, unreadOnly: $unreadOnly); + } + + $role = self::ROLES[$scope] ?? null; + + if (null === $role) { + return []; + } + + return $this->threads->findForRole($user, $role, $page, self::CHUNK, unreadOnly: $unreadOnly); + } + + /** + * The scopes that are a system role, by the name the list template already + * uses for itself (`{% block list_scope %}`), so the client has nothing new + * to learn and the two cannot drift. + */ + private const array ROLES = [ + 'archive' => LabelRole::Archive, + 'trash' => LabelRole::Trash, + 'spam' => LabelRole::Spam, + 'sent' => LabelRole::Sent, + 'drafts' => LabelRole::Drafts, + 'snoozed' => LabelRole::Snoozed, + ]; +} diff --git a/src/Service/Mail/ThreadStatusUpdater.php b/src/Service/Mail/ThreadStatusUpdater.php index ff9f54f1..d1ffe6cd 100644 --- a/src/Service/Mail/ThreadStatusUpdater.php +++ b/src/Service/Mail/ThreadStatusUpdater.php @@ -289,7 +289,34 @@ final readonly class ThreadStatusUpdater } } - $messages[0]->thread->unreadCount = $unread; + // Per thread, counted from that thread's own messages. + // + // `$messages[0]->thread->unreadCount = $unread` was right while every + // caller named one conversation: the total of the set WAS the total of + // the thread. A bulk action passes several threads at once, and that + // line then wrote the whole selection's count onto the first thread and + // left the others untouched — so four conversations were marked read in + // the database and three rows went on rendering as unread, because + // `data-unread` reads the thread's counter rather than its messages. + $counts = []; + + foreach ($messages as $message) { + $thread = $message->thread; + + if (null === $thread) { + continue; + } + + $counts[spl_object_id($thread)] ??= ['thread' => $thread, 'unread' => 0]; + + if (null === $message->seenAt) { + ++$counts[spl_object_id($thread)]['unread']; + } + } + + foreach ($counts as ['thread' => $thread, 'unread' => $threadUnread]) { + $thread->unreadCount = $threadUnread; + } $this->propagator->markRead($messages, $read); $this->recordJmapUpdates($messages); @@ -549,7 +576,30 @@ final readonly class ThreadStatusUpdater */ private function finish(array $messages): void { - $this->threadLabelSynchronizer->sync($messages[0]->thread); + // Every distinct thread, not just the first one's. + // + // Each of these methods used to be reached from a route naming ONE + // conversation, so `$messages[0]->thread` was the whole set and saying + // so was honest. A bulk action passes messages from many threads at + // once, and a thread whose labels are not resynced keeps the Inbox + // label its messages no longer have — so the list goes on showing a + // conversation that has been archived. It answers 200, reports the + // right count, and changes nothing on screen, which is the most + // expensive kind of correct. + $threads = []; + + foreach ($messages as $message) { + $thread = $message->thread; + + if (null !== $thread) { + $threads[spl_object_id($thread)] = $thread; + } + } + + foreach ($threads as $thread) { + $this->threadLabelSynchronizer->sync($thread); + } + $this->recordJmapUpdates($messages); $this->em->flush(); } diff --git a/src/Twig/AppearanceExtension.php b/src/Twig/AppearanceExtension.php index 3fb335c9..4a07298e 100644 --- a/src/Twig/AppearanceExtension.php +++ b/src/Twig/AppearanceExtension.php @@ -5,6 +5,8 @@ declare(strict_types=1); namespace App\Twig; use App\Entity\Embeddable\Appearance; +use Symfony\Component\HttpFoundation\RequestStack; +use App\Infrastructure\Event\Subscriber\AppearanceCookieSubscriber; use App\Entity\User\User; use App\Service\Appearance\AppearanceRenderer; use App\Service\Appearance\BackgroundResolver; @@ -25,6 +27,9 @@ final class AppearanceExtension extends AbstractExtension * for both to come out of the same method. */ private readonly BackgroundResolver $backgrounds, + // For the no-user case above: the cookie is the only place the theme + // survives a request the firewall never ran for. + private readonly RequestStack $requests, ) { } @@ -45,11 +50,23 @@ final class AppearanceExtension extends AbstractExtension { $user = $this->security->getUser(); - if (false === $user instanceof User) { - return new Appearance(); + if ($user instanceof User) { + return $user->appearance; } - return $user->appearance; + // No user is not always "not signed in". An error page is rendered with + // an empty token storage even for somebody who is: a 404 is thrown by + // the router at priority 32 of kernel.request, before the firewall at + // 8, so nothing has authenticated by the time the exception is handled. + // Falling straight through to the defaults is what put a German user in + // a beige "Papier" 404 while every other page was their own theme. + // + // The cookie is written on ordinary responses, where the user IS known + // — see AppearanceCookieSubscriber, which also explains why the error + // template cannot simply look the user up for itself. + return AppearanceCookieSubscriber::appearanceFrom( + $this->requests->getCurrentRequest()?->cookies->get(AppearanceCookieSubscriber::COOKIE), + ); } public function appearanceClass(): string diff --git a/templates/_layout/_mailbox.html.twig b/templates/_layout/_mailbox.html.twig index 5f4cd50b..14049b49 100644 --- a/templates/_layout/_mailbox.html.twig +++ b/templates/_layout/_mailbox.html.twig @@ -10,6 +10,12 @@ conditional above and the fragment layout has no such block to inherit. #} {% block body_controllers %}core--mercure mail--mail-pane{% endblock %} +{# The one part of a view that varies within its scope: the category tab for the + inbox, the label id for a label view. Empty for every list where the scope is + the whole answer. Defined here so `block('list_scope_value')` resolves for + every list rather than only the two that care. #} +{% block list_scope_value %}{% endblock %} + {% set selectedMessage = selectedMessage|default(null) %} {# @@ -221,10 +227,18 @@ which of the two is happening. {# ── Gmail-style toolbar ──────────────────────────────────────────── #} {% block inbox_toolbar %} + {# list_scope is the same block the sync scope uses, so a view + names itself once. list_scope_value carries the one thing that + varies within a scope: the category tab for the inbox, the label + id for a label view. Both go to the server's closed set — see + App\Service\Mail\ListViewResolver. #} {% include '_partials/_list_toolbar.html.twig' with { - page: page ?? 1, - total: total ?? 0, - per_page: per_page ?? 50, + page: page ?? 1, + total: total ?? 0, + per_page: per_page ?? 50, + list_scope: block('list_scope'), + list_scope_value: block('list_scope_value'), + unread_only: unread_only ?? false, } %} {% endblock %} diff --git a/templates/_partials/_list_toolbar.html.twig b/templates/_partials/_list_toolbar.html.twig index faec041d..6afbbb92 100644 --- a/templates/_partials/_list_toolbar.html.twig +++ b/templates/_partials/_list_toolbar.html.twig @@ -80,14 +80,44 @@ The button itself carries `group` and `relative`. {% import _self as m %}