diff --git a/assets/controllers/settings/account_order_controller.js b/assets/controllers/settings/account_order_controller.js new file mode 100644 index 00000000..9204ea7e --- /dev/null +++ b/assets/controllers/settings/account_order_controller.js @@ -0,0 +1,110 @@ +import { Controller } from "@hotwired/stimulus" +import Sortable from "sortablejs" + +/** + * Display order for the account list: drag on a pointer, buttons everywhere else. + * + * Uses sortablejs directly rather than @stimulus-components/sortable, for the + * same reason rules/rule_order_controller does: that wrapper builds its own + * multipart body and offers no way to attach a CSRF token, which is why the + * account reorder endpoint had none for as long as it did. Sending the whole + * order as JSON also makes the write idempotent — a duplicated request lands on + * the same arrangement instead of shifting a row twice. + * + * This order is COSMETIC. It decides where rows sit and nothing else; which + * account new mail is sent from is the "make primary" button on the row, which + * posts its own form. Dragging used to do both silently. + */ +export default class extends Controller { + static targets = ["list"] + static values = { url: String, csrf: String } + + connect() { + this.sortable = Sortable.create(this.listTarget, { + handle: ".account-drag-handle", + animation: 150, + // Without this a touch-drag scrolls the page instead of moving the + // row; the handle also carries touch-none for the same reason. + forceFallback: false, + onEnd: () => { + this.syncEdges() + this.persist() + }, + }) + + this.syncEdges() + } + + disconnect() { + this.sortable?.destroy() + } + + /** Keyboard- and touch-reachable equivalent of a drag. */ + move(event) { + const row = event.currentTarget.closest("[data-account-id]") + const direction = event.currentTarget.dataset.direction === "up" ? -1 : 1 + const rows = [...this.listTarget.querySelectorAll("[data-account-id]")] + const index = rows.indexOf(row) + const target = index + direction + + if (target < 0 || target >= rows.length) { + return + } + + // Move in the DOM first so the list is already correct if the request + // is slow; the server is told what the user can already see. + direction === -1 + ? rows[target].before(row) + : rows[target].after(row) + + this.syncEdges() + + // Keep focus on the control that was pressed, or a keyboard user loses + // their place after every step. Re-queried because syncEdges() may have + // just disabled this very button — if it did, move focus to the other + // direction rather than leaving it on a dead control. + const pressed = event.currentTarget.dataset.direction + const same = row.querySelector(`[data-direction="${pressed}"]`) + const other = row.querySelector(`[data-direction="${pressed === "up" ? "down" : "up"}"]`) + + ;(same?.disabled ? other : same)?.focus() + + this.persist() + } + + /** + * Re-evaluate which move buttons are dead ends. + * + * The server renders these disabled states from loop.first/loop.last, but a + * move rearranges the DOM without re-rendering — so without this the top + * row keeps an enabled "up" and the row that used to be first has a + * permanently disabled one. + */ + syncEdges() { + const rows = [...this.listTarget.querySelectorAll("[data-account-id]")] + + rows.forEach((row, index) => { + const up = row.querySelector('[data-direction="up"]') + const down = row.querySelector('[data-direction="down"]') + + if (up) up.disabled = index === 0 + if (down) down.disabled = index === rows.length - 1 + }) + } + + persist() { + const ids = [...this.listTarget.querySelectorAll("[data-account-id]")].map( + (row) => Number(row.dataset.accountId), + ) + + fetch(this.urlValue, { + method: "POST", + headers: { "Content-Type": "application/json", "X-CSRF-Token": this.csrfValue }, + body: JSON.stringify({ ids }), + keepalive: true, + }).catch(() => { + // The order on screen is what the user chose; a failed write means + // the next load shows the stored order again, which is honest. + }) + } +} diff --git a/assets/controllers/settings/appearance_controller.js b/assets/controllers/settings/appearance_controller.js index 6972404d..e13172ce 100644 --- a/assets/controllers/settings/appearance_controller.js +++ b/assets/controllers/settings/appearance_controller.js @@ -36,17 +36,27 @@ export default class extends Controller { this.queue(); } + /** + * The tiles name their theme in `data-theme-name`, NOT `data-theme`. + * + * `data-theme` is the palette switch: every block in app.css that declares + * one is selected by that attribute, so a tile carrying it re-declared the + * whole palette on itself and its label was painted in the colours of the + * theme it names rather than the theme you are looking at. That is what put + * "Nord" on screen at 1.22:1 in Solar. The blocks are :root-scoped now as + * well, so neither half of the fault can come back on its own. + */ pickTheme(event) { - const theme = event.currentTarget.dataset.theme; + const theme = event.currentTarget.dataset.themeName; this.themeTarget.value = theme; this.root.dataset.theme = theme; this.root.classList.toggle('dark', this.resolvesDark(event.currentTarget)); - this.element.querySelectorAll('[data-theme]').forEach((button) => { - button.classList.toggle('ring-2', button.dataset.theme === theme); - button.classList.toggle('ring-accent', button.dataset.theme === theme); + this.element.querySelectorAll('[data-theme-name]').forEach((button) => { + button.classList.toggle('ring-2', button.dataset.themeName === theme); + button.classList.toggle('ring-accent', button.dataset.themeName === theme); }); // A theme may seed knobs too, the way a layout does. The accent has to @@ -85,7 +95,7 @@ export default class extends Controller { * light palette until the next navigation put it back. */ resolvesDark(button) { - if (button.dataset.theme === 'system') { + if (button.dataset.themeName === 'system') { return window.matchMedia('(prefers-color-scheme: dark)').matches; } diff --git a/assets/controllers/ui/sidebar_controller.js b/assets/controllers/ui/sidebar_controller.js index 1c769892..e6b24a0c 100644 --- a/assets/controllers/ui/sidebar_controller.js +++ b/assets/controllers/ui/sidebar_controller.js @@ -90,7 +90,7 @@ function patchNewDots(counts) { export default class extends Controller { static targets = ["link", "badge", "scroller", "tree"]; - static values = { countsUrl: String }; + static values = { countsUrl: String, i18n: Object }; connect() { // Both before the first paint of this element, so nothing is seen in @@ -185,6 +185,13 @@ export default class extends Controller { badge.textContent = count; badge.classList.toggle("hidden", count === 0); + + // The number and the name it is read out with are one statement. + // Patching only the text left a screen reader saying "3 unread" + // over a badge that had moved on to 7 — and on Trash and Drafts it + // would have said "unread" over a total, which is the very thing + // the two badge shapes exist to keep apart. + this._nameBadge(badge, count); }); patchNewDots(counts); @@ -192,6 +199,23 @@ export default class extends Controller { this._updateTitle(counts); } + /** + * Re-label one badge for the count it now shows. + * + * A badge with no `data-badge-kind`, or a page that shipped no strings, is + * left exactly as the server rendered it: a wrong name is worse than the + * one that is already correct. + */ + _nameBadge(badge, count) { + const template = this.hasI18nValue ? this.i18nValue[badge.dataset.badgeKind] : null; + + if (!template) { + return; + } + + badge.setAttribute("aria-label", template.replace("%count%", String(count))); + } + /** * Keep the "(n)" in the tab in step with the badges. * diff --git a/assets/stimulus_bootstrap.js b/assets/stimulus_bootstrap.js index 5a5422a9..7efc9680 100644 --- a/assets/stimulus_bootstrap.js +++ b/assets/stimulus_bootstrap.js @@ -1,5 +1,4 @@ import { startStimulusApp } from '@symfony/stimulus-bundle'; -import Sortable from '@stimulus-components/sortable'; const app = startStimulusApp(); @@ -13,6 +12,13 @@ const app = startStimulusApp(); // prod goes quiet. app.debug = document.documentElement.dataset.appDebug === 'true'; -app.register('sortable', Sortable); +// No global 'sortable' controller any more. @stimulus-components/sortable was +// registered here and driven the accounts list; it builds its own request body +// and gives you nowhere to attach a CSRF token, which is precisely why the +// account reorder endpoint went without one. Both draggable lists — rules and +// accounts — now wrap sortablejs in a controller of their own that posts the +// whole order as JSON with a token, so the wrapper is gone rather than left +// registered for the next list to reach for. +// // register any custom, 3rd party controllers here // app.register('some_controller_name', SomeImportedController); diff --git a/assets/styles/app.css b/assets/styles/app.css index d12d59bf..02ca7b23 100644 --- a/assets/styles/app.css +++ b/assets/styles/app.css @@ -203,7 +203,7 @@ Everything that separates one thing from another here is the ink colour at a low alpha, including the shadow: a neutral-black drop shadow under a warm cream card is the one grey thing on the screen. */ -[data-theme="paper"] { +:root[data-theme="paper"] { color-scheme: light; --rgb-surface: 247 245 239; @@ -280,7 +280,7 @@ every primary button paint it behind light text, so it has to be deep enough to carry them — a pale sage, which is what the reference uses for 40px avatar circles, becomes a washed-out slab nothing can be read on. */ -[data-theme="dark"] { +:root[data-theme="dark"] { color-scheme: dark; --rgb-surface: 28 27 26; @@ -349,7 +349,7 @@ to borrow the neutral grey base for --rgb-ink-soft, which was both colder than everything around it and DARKER than the --rgb-ink-muted below it, so the two steps ran backwards. */ -[data-theme="nord"] { +:root[data-theme="nord"] { color-scheme: dark; --rgb-surface: 46 52 64; /* nord0 #2e3440 */ @@ -406,7 +406,7 @@ backwards step Nord's did, for the same reason: --rgb-ink-soft was a neutral grey inherited from the dark base while the ink either side of it was tinted. */ -[data-theme="dusk"] { +:root[data-theme="dusk"] { color-scheme: dark; --rgb-surface: 30 27 46; @@ -462,7 +462,7 @@ the wrong sixteen: a cool slate --rgb-ink-soft between two warm greys, a #f3f4f6 inset panel on a #fdf6e3 page, and the base link blue where Solarized has a blue of its own. */ -[data-theme="solar"] { +:root[data-theme="solar"] { color-scheme: light; --rgb-surface: 253 246 227; /* base3 #fdf6e3 */ diff --git a/importmap.php b/importmap.php index 20d6a282..cbd19b94 100644 --- a/importmap.php +++ b/importmap.php @@ -35,7 +35,6 @@ return [ 'tom-select/dist/css/tom-select.default.css' => ['version' => '2.6.2', 'type' => 'css'], 'tom-select/dist/css/tom-select.bootstrap4.css' => ['version' => '2.6.2', 'type' => 'css'], 'tom-select/dist/css/tom-select.bootstrap5.css' => ['version' => '2.6.2', 'type' => 'css'], - '@stimulus-components/sortable' => ['version' => '5.0.3'], 'sortablejs' => ['version' => '1.15.6'], '@rails/request.js' => ['version' => '0.0.12'], 'emoji-picker-element' => ['version' => '1.29.1'], diff --git a/migrations/Version20260813120000.php b/migrations/Version20260813120000.php new file mode 100644 index 00000000..d171dea0 --- /dev/null +++ b/migrations/Version20260813120000.php @@ -0,0 +1,74 @@ +addSql('ALTER TABLE account ADD color_index INT DEFAULT 0 NOT NULL'); + + // The colour every one of these accounts is wearing today. + $this->addSql('UPDATE account SET color_index = sort_order'); + + // Exactly one primary per user. Demote any extras first — two primaries + // is a state the old derivation could not produce but an import can, + // and findOneBy() would then pick between them arbitrarily. + $this->addSql(<<<'SQL' + UPDATE account SET is_primary = false + WHERE is_primary = true + AND id NOT IN ( + SELECT MIN(id) FROM account WHERE is_primary = true GROUP BY usr_id + ) + SQL); + + // Then promote the lowest-ordered account of any user left without one. + $this->addSql(<<<'SQL' + UPDATE account SET is_primary = true + WHERE id IN ( + SELECT DISTINCT ON (usr_id) id + FROM account + WHERE usr_id NOT IN (SELECT usr_id FROM account WHERE is_primary = true) + ORDER BY usr_id, sort_order ASC, id ASC + ) + SQL); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE account DROP color_index'); + } +} diff --git a/src/Command/Test/SeedTestEmailCommand.php b/src/Command/Test/SeedTestEmailCommand.php index 4529d3c6..310ee10f 100644 --- a/src/Command/Test/SeedTestEmailCommand.php +++ b/src/Command/Test/SeedTestEmailCommand.php @@ -17,6 +17,7 @@ use App\Repository\Mail\MessageRepository; use App\Repository\Mail\MessageThreadRepository; use App\Repository\User\UserRepository; use App\Service\Label\LabelResolver; +use App\Service\Mail\AccountCreator; use DateTimeImmutable; use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\Console\Attribute\AsCommand; @@ -72,6 +73,7 @@ final class SeedTestEmailCommand extends Command private readonly MessageThreadRepository $threadRepository, private readonly LabelResolver $labelResolver, private readonly StateManager $stateManager, + private readonly AccountCreator $accountCreator, #[Autowire('%kernel.environment%')] private readonly string $environment, ) { @@ -123,6 +125,15 @@ final class SeedTestEmailCommand extends Command $this->entityManager->flush(); } + // isPrimary is a stored choice rather than "whichever account sorts + // first", so a hand-built fixture has to state it — otherwise the + // seeded mailbox has no primary, no PRIMARY badge in settings, and does + // not resemble an account any real path would have produced. + $this->accountCreator->ensurePrimary( + $this->accountRepository->findForUserOrdered($user), + ); + $this->entityManager->flush(); + $this->wipeThreads($account); $inboxLabel = $this->labelResolver->systemLabel(LabelRole::Inbox, $account); diff --git a/src/Controller/Mail/MailController.php b/src/Controller/Mail/MailController.php index 0ac38d7b..10a5fbb6 100644 --- a/src/Controller/Mail/MailController.php +++ b/src/Controller/Mail/MailController.php @@ -462,7 +462,7 @@ final class MailController extends AbstractController } #[Route('/account/{account}/folders', name: 'account_folders', requirements: ['account' => '\d+'])] - public function accountFolders(Account $account): Response + public function accountFolders(Request $request, Account $account): Response { $this->denyAccessUnlessGranted('ROLE_USER'); @@ -481,9 +481,33 @@ final class MailController extends AbstractController return $this->render('mail/_account_labels.html.twig', [ 'account' => $account, 'labels' => $labels, + 'frameId' => $this->folderFrameId($request, $account), ]); } + /** + * The id the answering frame must wear. + * + * The desktop rail and the mobile drawer both render the sidebar into the + * same document, so each account's folder frame needs an id of its own — + * and Turbo matches a frame response by the id of the frame that asked for + * it, so the id has to come back on the reply too. + * + * Taken from the request rather than derived, because only the caller knows + * which of the two it is; validated hard, because it lands in an id + * attribute. Anything that is not this account's own frame falls back to + * the bare name, which is the desktop one. + */ + private function folderFrameId(Request $request, Account $account): string + { + $default = 'account-folders-' . $account->id; + $requested = (string) $request->query->get('frame', ''); + + return in_array($requested, [$default, $default . '-drawer'], true) + ? $requested + : $default; + } + /** * Remembers which account's folder list is open, so the sidebar can render * it already expanded rather than expanding it afterwards. diff --git a/src/Controller/Settings/AccountController.php b/src/Controller/Settings/AccountController.php index f7f97970..dc917daa 100644 --- a/src/Controller/Settings/AccountController.php +++ b/src/Controller/Settings/AccountController.php @@ -143,38 +143,112 @@ final class AccountController extends AbstractController $this->entityManager->remove($account); $this->entityManager->flush(); - $this->resequence($this->accountRepository->findForUserOrdered($this->getUser())); + $remaining = $this->accountRepository->findForUserOrdered($this->getUser()); + + $this->resequence($remaining); + + // Deleting the primary is one of the two ways the "exactly one" rule + // can be lost now that it is a flag rather than position 0 — the other + // being the very first account, which create() covers. Without this the + // user would be left with accounts and no default sender at all. + $this->accountCreator->ensurePrimary($remaining); + $this->entityManager->flush(); return $this->streamAccountList($request, 'account.removed'); } - #[Route('/{id}/reorder', name: 'reorder', methods: ['PATCH'])] - public function reorder(Request $request, Account $account): Response + /** + * The display order of the account list, and nothing else. + * + * A collection route taking the whole arrangement as JSON, exactly like + * MailRuleController::reorder. It replaces a per-account PATCH driven by + * @stimulus-components/sortable, which built its own multipart body and + * offered nowhere to put a token — so this was the one mutation on the page + * with no CSRF check at all. The rules list had already solved that by + * dropping the wrapper for sortablejs; the accounts list now does the same. + * + * Sending the whole order also makes the write idempotent: a duplicated + * request lands on the same arrangement instead of shifting a row twice. + * + * What it deliberately does NOT do is touch isPrimary. That coupling — top + * of the list means "sends your mail" — is the reported fault: dragging a + * row to tidy a list silently changed which address Compose sent from, with + * nothing on screen saying it would. Choosing the sender is makePrimary() + * below, and it is a button that says what it does. + */ + #[Route('/reorder', name: 'reorder', methods: ['POST'])] + public function reorder(Request $request): JsonResponse { - $this->denyUnlessOwner($account); + if (false === $this->isCsrfTokenValid('account_reorder', (string) $request->headers->get('X-CSRF-Token'))) { + throw $this->createAccessDeniedException('Invalid CSRF token.'); + } - // No CSRF token here: @stimulus-components/sortable owns the request body - // (it only sends account[position]). The action is authenticated, - // same-origin, and non-destructive. Say the word if you want parity with - // toggle/delete and I'll wire an X-CSRF-Token header + meta tag. - $position = (int) ($request->getPayload()->all('account')['position'] ?? 0); + $ids = $request->toArray()['ids'] ?? null; - $ordered = $this->accountRepository->findForUserOrdered($this->getUser()); + if (false === is_array($ids)) { + return $this->json(['ok' => false], Response::HTTP_BAD_REQUEST); + } - // Pull the dragged account out, re-insert at the target index. - $ordered = array_values(array_filter($ordered, static function (Account $candidate) use ($account): bool { - return $candidate->id !== $account->id; - })); + $ids = array_values(array_filter(array_map('intval', $ids))); + $accounts = $this->accountRepository->findForUserOrdered($this->getUser()); - $targetIndex = max(0, min(count($ordered), $position - 1)); - array_splice($ordered, $targetIndex, 0, [$account]); + // Indexed by id, so an id that is not this user's simply finds nothing + // — the arrangement is built from rows we already fetched as theirs, + // never from the ids the request asked for. + $byId = []; + + foreach ($accounts as $account) { + $byId[(int) $account->id] = $account; + } + + $ordered = []; + + foreach ($ids as $id) { + if (true === array_key_exists($id, $byId)) { + $ordered[] = $byId[$id]; + + unset($byId[$id]); + } + } + + // Anything the payload left out keeps its existing relative order at + // the end, so a stale tab cannot delete rows from the list by omission. + foreach ($byId as $account) { + $ordered[] = $account; + } $this->resequence($ordered); $this->entityManager->flush(); - return $this->render('account/_reorder.stream.html.twig', [ - 'manageableAccounts' => $this->accountRepository->findForUserOrdered($this->getUser()), - ], new Response(headers: ['Content-Type' => 'text/vnd.turbo-stream.html'])); + return $this->json(['ok' => true]); + } + + /** + * Choose the account new messages are sent from. + * + * The explicit half of splitting order from identity. It used to be that + * whatever sat at position 0 was primary, so this was reachable only by + * dragging — and a person dragging a row is arranging a list, not changing + * their return address. Now the list order says nothing about it and this + * button is the only thing that does. + */ + #[Route('/{id}/primary', name: 'primary', methods: ['POST'])] + public function makePrimary(Request $request, Account $account): Response + { + $this->denyUnlessOwner($account); + + if (false === $this->isCsrfTokenValid('account-primary' . $account->id, (string) $request->request->get('_token'))) { + throw $this->createAccessDeniedException(); + } + + $this->accountCreator->makePrimary( + $account, + $this->accountRepository->findForUserOrdered($this->getUser()), + ); + + $this->entityManager->flush(); + + return $this->streamAccountList($request, 'account.primary_set'); } #[Route('/test-connection', name: 'test_connection', methods: ['POST'])] diff --git a/src/Entity/Mail/Account.php b/src/Entity/Mail/Account.php index b7017e8e..273b29e5 100644 --- a/src/Entity/Mail/Account.php +++ b/src/Entity/Mail/Account.php @@ -31,12 +31,43 @@ class Account extends AccountModel #[ORM\Column(length: 255, nullable: true)] public ?string $name = null; + /** + * Where the account sits in the user's own arrangement of the list. + * + * DISPLAY ONLY. It used to decide two other things as well — which account + * was primary and which colour the account wore — so tidying the list by + * dragging a row silently reassigned the address Compose sent from, and + * repainted every account dot in the app. Both now live in fields of their + * own; this one moves rows and nothing else. + */ #[ORM\Column(options: ['default' => 0])] public int $sortOrder = 0; + /** + * The account Compose starts from. Exactly one per user, chosen explicitly. + * + * Derived from sortOrder === 0 until it was found that a drag rewrote it, + * which is not something a person tidying a list is asking for and which + * the UI never mentioned. AccountCreator::ensurePrimary() keeps the "exactly + * one" part true across creation and deletion. + */ #[ORM\Column] public bool $isPrimary = false; + /** + * Which entry of the account palette this account paints its dot with. + * + * Assigned once, at creation, and then left alone — that is the whole point + * of it. The dot was keyed off sortOrder, so a reorder swapped the colours + * of two accounts: the mark whose only job is "this is the same account you + * saw on that message" changed meaning under the user, on the sidebar and + * on every list row at once. Dense and lowest-free at creation, so the first + * eight accounts are still guaranteed distinct, which is why sortOrder was + * used in the first place. + */ + #[ORM\Column(options: ['default' => 0])] + public int $colorIndex = 0; + #[ORM\ManyToOne(inversedBy: 'accounts')] #[ORM\JoinColumn(nullable: false)] public ?User $usr = null; diff --git a/src/Service/Backup/ConfigBackupUserRestorer.php b/src/Service/Backup/ConfigBackupUserRestorer.php index 5b213577..25454f1a 100644 --- a/src/Service/Backup/ConfigBackupUserRestorer.php +++ b/src/Service/Backup/ConfigBackupUserRestorer.php @@ -21,6 +21,7 @@ use App\Entity\Rule\MailRule; use App\Entity\User\ApiToken; use App\Entity\User\User; use App\Repository\User\UserRepository; +use App\Service\Mail\AccountCreator; use DateTimeImmutable; use Doctrine\ORM\EntityManagerInterface; use Throwable; @@ -96,6 +97,7 @@ final readonly class ConfigBackupUserRestorer public function __construct( private UserRepository $users, private EntityManagerInterface $entityManager, + private AccountCreator $accountCreator, ) { } @@ -257,6 +259,11 @@ final readonly class ConfigBackupUserRestorer $account->email = $this->text($row, 'email'); $account->sortOrder = $this->number($row, 'sortOrder') ?? 0; $account->isPrimary = true === ($row['isPrimary'] ?? false); + // Falls back to sortOrder, which is exactly what the colour used to + // be derived from — so a file written before colorIndex existed + // restores the colours that backup's accounts were actually wearing. + $account->colorIndex = $this->number($row, 'colorIndex') + ?? $account->sortOrder; $account->imapHost = $this->text($row, 'imapHost'); $account->imapPort = $this->number($row, 'imapPort'); $account->imapEncryption = $this->text($row, 'imapEncryption'); @@ -288,6 +295,15 @@ final readonly class ConfigBackupUserRestorer $created[$this->number($row, 'id') ?? 0] = $account; } + // A backup written before isPrimary was authoritative can carry none at + // all, or — from an install that had drifted — two. Either way the user + // must come out of a restore with exactly one sending account, and the + // one the old position-0 rule would have chosen. + $ordered = $created; + usort($ordered, static fn (Account $a, Account $b): int => $a->sortOrder <=> $b->sortOrder); + + $this->accountCreator->ensurePrimary($ordered); + return $created; } diff --git a/src/Service/Backup/ConfigBackupUsers.php b/src/Service/Backup/ConfigBackupUsers.php index dc58c7e8..fb63c2b6 100644 --- a/src/Service/Backup/ConfigBackupUsers.php +++ b/src/Service/Backup/ConfigBackupUsers.php @@ -336,6 +336,11 @@ final readonly class ConfigBackupUsers 'email' => $account->email, 'sortOrder' => $account->sortOrder, 'isPrimary' => $account->isPrimary, + // The account's dot colour, which is assigned at creation and + // is not recoverable from anything else in this file — restore + // it and every account keeps the mark the user recognises it + // by; leave it out and they all come back sharing colour 0. + 'colorIndex' => $account->colorIndex, 'imapHost' => $account->imapHost, 'imapPort' => $account->imapPort, 'imapEncryption' => $account->imapEncryption, diff --git a/src/Service/Mail/AccountCreator.php b/src/Service/Mail/AccountCreator.php index 925007e5..7a8991a4 100644 --- a/src/Service/Mail/AccountCreator.php +++ b/src/Service/Mail/AccountCreator.php @@ -8,6 +8,7 @@ use App\Entity\Mail\Account; use App\Entity\User\User; use App\Repository\Mail\AccountRepository; use App\Service\Calendar\CalendarProvisioner; +use App\Twig\SenderAvatarExtension; use DateTimeImmutable; use Doctrine\ORM\EntityManagerInterface; use Throwable; @@ -61,14 +62,24 @@ final readonly class AccountCreator $account->isActive = true; $account->usr = $user; - // Appended, then the whole list renumbered: sortOrder and isPrimary are - // positional, so the first account added has to come out primary - // without anything having to special-case "is this the first one". - $ordered = $this->accounts->findForUserOrdered($user); + $existing = $this->accounts->findForUserOrdered($user); + + // The colour is picked BEFORE the account joins the list, from the + // colours already in use — it is the account's own from here on and + // never moves again. + $account->colorIndex = $this->freeColorIndex($existing); + + // Appended, then the whole list renumbered, so sortOrder stays the + // dense 0-based position the list is drawn from. + $ordered = $existing; $ordered[] = $account; $this->resequence($ordered); + // A first account is primary because there is nothing else it could be, + // not because it landed at position 0. + $this->ensurePrimary($ordered); + $this->entityManager->persist($account); $this->entityManager->flush(); @@ -114,6 +125,16 @@ final readonly class AccountCreator } /** + * Writes sequential sortOrder over the list, in the order given. + * + * DISPLAY ORDER ONLY. This used to set isPrimary on the row that came out + * first, which meant dragging an account to the top of the settings list — + * a gesture about tidying a list — silently changed the address every new + * message would be sent from, with nothing on screen saying so. The two are + * separate now: this moves rows, makePrimary chooses the sender. + * + * Caller is responsible for flushing. + * * @param iterable $orderedAccounts */ public function resequence(iterable $orderedAccounts): void @@ -122,9 +143,88 @@ final readonly class AccountCreator foreach ($orderedAccounts as $account) { $account->sortOrder = $index; - $account->isPrimary = 0 === $index; ++$index; } } + + /** + * Exactly one primary, no matter what just happened to the list. + * + * The invariant used to be free: position 0 always existed and was always + * one row. Now that the flag is chosen rather than derived, the two ways it + * can be lost have to be closed by hand — deleting the account that held it, + * and adding the very first account, which has nothing to inherit from. A + * list that has drifted (an import with two, a restore with none) is + * repaired the way the old derivation would have answered it: the + * lowest-ordered account wins, so nobody's From address moves. + * + * Caller is responsible for flushing. + * + * @param iterable $orderedAccounts in the user's display order + */ + public function ensurePrimary(iterable $orderedAccounts): void + { + $accounts = is_array($orderedAccounts) ? $orderedAccounts : iterator_to_array($orderedAccounts); + $primary = null; + + foreach ($accounts as $account) { + if (true === $account->isPrimary && null === $primary) { + $primary = $account; + + continue; + } + + $account->isPrimary = false; + } + + if (null !== $primary || [] === $accounts) { + return; + } + + $accounts[array_key_first($accounts)]->isPrimary = true; + } + + /** + * Make one account the primary, and the only one. + * + * @param iterable $siblings every account of the same user + */ + public function makePrimary(Account $account, iterable $siblings): void + { + foreach ($siblings as $sibling) { + $sibling->isPrimary = false; + } + + $account->isPrimary = true; + } + + /** + * The lowest palette slot nobody is using yet. + * + * The palette has eight entries and the point of the dot is telling + * accounts apart, so "lowest free" keeps the first eight guaranteed + * distinct — the property that made sortOrder attractive here in the first + * place — without inheriting its habit of changing when the list moves. + * Past eight accounts a colour is reused, which is what the modulo in + * SenderAvatarExtension already did. + * + * @param iterable $existing + */ + private function freeColorIndex(iterable $existing): int + { + $taken = []; + + foreach ($existing as $account) { + $taken[$account->colorIndex] = true; + } + + for ($index = 0; $index < SenderAvatarExtension::ACCOUNT_COLOURS; ++$index) { + if (false === array_key_exists($index, $taken)) { + return $index; + } + } + + return count($taken) % SenderAvatarExtension::ACCOUNT_COLOURS; + } } diff --git a/src/Service/Mail/ThreadParticipants.php b/src/Service/Mail/ThreadParticipants.php index 7b7bcf7d..b6c09c65 100644 --- a/src/Service/Mail/ThreadParticipants.php +++ b/src/Service/Mail/ThreadParticipants.php @@ -66,7 +66,7 @@ final class ThreadParticipants : $this->displayName($message->fromName, $address); } - return array_values($names); + return $this->collapse(array_values($names)); } /** @@ -96,7 +96,29 @@ final class ThreadParticipants $names[$address] ??= $this->displayName($entry['name'] ?? null, $address); } - return array_values($names); + return $this->collapse(array_values($names)); + } + + /** + * One name per person, not one name per address. + * + * De-duplication happens by address first, because that is what identifies + * a participant — but several addresses can resolve to the SAME display + * name, and then the column prints it twice. The reported case is a thread + * carrying two of the reader's own addresses: both map to "me", and the row + * read "me, me". A sender writing from two of their own addresses under one + * display name is the same fault with a different name in it. + * + * The first occurrence wins, so the order participants joined the + * conversation in — which is the whole point of this column — survives. + * + * @param list $names + * + * @return list + */ + private function collapse(array $names): array + { + return array_values(array_unique($names)); } private function newest(MessageThread $thread): ?Message diff --git a/src/Service/OAuth/OAuthAccountLinker.php b/src/Service/OAuth/OAuthAccountLinker.php index 0aafd43b..e25385eb 100644 --- a/src/Service/OAuth/OAuthAccountLinker.php +++ b/src/Service/OAuth/OAuthAccountLinker.php @@ -10,6 +10,7 @@ use App\Domain\Exception\AccountIdentityMismatch; use App\Entity\Mail\Account; use App\Entity\User\User; use App\Repository\Mail\AccountRepository; +use App\Service\Mail\AccountCreator; use App\Service\Mail\AliasSeeder; use App\Service\Push\PushSubscriptionRegistry; use DateTimeImmutable; @@ -32,6 +33,7 @@ final readonly class OAuthAccountLinker private EntityManagerInterface $em, private PushSubscriptionRegistry $pushRegistry, private AliasSeeder $aliasSeeder, + private AccountCreator $accountCreator, ) { } @@ -225,6 +227,15 @@ final readonly class OAuthAccountLinker $this->em->persist($account); $this->em->flush(); + // isPrimary is a stored choice now, not "whichever account sits at + // position 0" — so a path that creates an account without going through + // AccountCreator has to say who the primary is, or a user whose only + // account is an OAuth one ends up with none at all and no badge saying + // which address their mail goes out from. Existing primaries are never + // stolen; this only fills a gap. + $this->accountCreator->ensurePrimary($this->accounts->findForUserOrdered($user)); + $this->em->flush(); + return $account; } diff --git a/src/Twig/LabelNameExtension.php b/src/Twig/LabelNameExtension.php new file mode 100644 index 00000000..4746687d --- /dev/null +++ b/src/Twig/LabelNameExtension.php @@ -0,0 +1,63 @@ +name(...)), + new TwigFilter('role_name', $this->roleName(...)), + ]; + } + + public function name(Label $label): string + { + $role = $label->role; + + return null === $role + ? (string) $label->name + : $this->roleName($role); + } + + /** + * The sidebar's own key, deliberately: sharing it is what keeps the two + * lists from drifting, and a `label.system.*` family beside it would be the + * same strings maintained twice. + */ + public function roleName(LabelRole $role): string + { + return $this->translator->trans('sidebar.nav.' . $role->value); + } +} diff --git a/src/Twig/SenderAvatarExtension.php b/src/Twig/SenderAvatarExtension.php index 423af25d..ced91369 100644 --- a/src/Twig/SenderAvatarExtension.php +++ b/src/Twig/SenderAvatarExtension.php @@ -79,22 +79,34 @@ final class SenderAvatarExtension extends AbstractExtension ]; } + /** How many distinct account colours there are to hand out. */ + public const int ACCOUNT_COLOURS = 8; + /** - * A distinct colour per account, by POSITION rather than by hash. + * A distinct colour per account, ASSIGNED rather than derived. * - * Accounts are a small ordered set — a handful, arranged by the user — not - * the unbounded stream of senders `accountTone()` was written for. Hashing - * an address into eight buckets makes a collision a coin-flip at four - * accounts (it was: two shared a colour in the sidebar), and a dot whose - * whole job is to tell accounts apart must not put two of them in the same - * paint. `sortOrder` is the dense 0-based position AccountCreator keeps, so - * the first eight accounts are guaranteed different and the same account is - * the same colour wherever it appears — the sidebar dot and the message - * row's dot included, which is the point of it. + * Accounts are a small set — a handful, arranged by the user — not the + * unbounded stream of senders `accountTone()` was written for. Hashing an + * address into eight buckets makes a collision a coin-flip at four accounts + * (it was: two shared a colour in the sidebar), and a dot whose whole job is + * to tell accounts apart must not put two of them in the same paint. + * + * So it was keyed off `sortOrder`, which is dense and 0-based and gave the + * first eight accounts guaranteed-different colours. The cost only showed + * up later: sortOrder is the user's ARRANGEMENT, so dragging a row swapped + * two accounts' colours everywhere at once — the sidebar dot and the corner + * mark on every message row — and a mark that means "this is the same + * account you saw over there" stopped being true of anything the user had + * already learned. + * + * `Account::$colorIndex` is the same dense 0-based number, handed out at + * creation from the lowest free slot and then never touched. Same guarantee, + * and now the same account really is the same colour wherever and whenever + * it appears. */ - public function accountColor(int $sortOrder): string + public function accountColor(int $colorIndex): string { - return self::DOT_TONES[(($sortOrder % count(self::DOT_TONES)) + count(self::DOT_TONES)) % count(self::DOT_TONES)]; + return self::DOT_TONES[(($colorIndex % count(self::DOT_TONES)) + count(self::DOT_TONES)) % count(self::DOT_TONES)]; } public function accountTone(?string $seed): string diff --git a/templates/_partials/_account_list.html.twig b/templates/_partials/_account_list.html.twig index 4ee8d9f9..42a6c7aa 100644 --- a/templates/_partials/_account_list.html.twig +++ b/templates/_partials/_account_list.html.twig @@ -1,10 +1,18 @@ {% set _drawerClose = _drawerClose is defined ? _drawerClose : '' %} +{# '' for the desktop rail, '-drawer' for the mobile one. Both render this + partial into the same document, so every id below has to carry it. #} +{% set _idSuffix = _idSuffix is defined ? _idSuffix : '' %} {% for account in accounts %} {% set expanded = app.user.expandedAccountId == account.id %} + {% set frameId = 'account-folders-' ~ account.id ~ _idSuffix %} + {# The frame id travels with the request too. Turbo matches the response + against the id of the frame that asked, so a drawer frame fetching a + reply that says `account-folders-20` finds nothing to swap and the + folders never appear. #}
@@ -37,8 +45,27 @@ green/grey status light, which made every connected account identical and told the list dots apart from nothing. Inactive accounts dim rather than change hue: still recognisably the - same account, visibly asleep. #} - + same account, visibly asleep. + + Two things were wrong with it. It read `sortOrder`, so + reordering the accounts swapped their colours and the mark + stopped identifying anything; that is `colorIndex` now, which + is assigned once and never moves. And it had no name of any + kind, so an orange circle beside one address and a green one + beside another could not be interpreted at all — the obvious + reading being a status light, which it is not and never was. + Account health is where status is reported; this says which + account, and now says so out loud. The one piece of state it + does carry — switched off — is named too, and agrees with the + toggle in Settings that put it there. #} + {{ account.displayAddress }} {# aria-expanded, because this is a disclosure button and which way @@ -57,7 +84,7 @@ aria-label="{{ 'sidebar.accounts.toggle'|trans({ '%account%': account.displayAddress }) }}" title="{{ 'sidebar.accounts.toggle'|trans({ '%account%': account.displayAddress }) }}" aria-expanded="{{ expanded ? 'true' : 'false' }}" - aria-controls="account-folders-{{ account.id }}" + aria-controls="{{ frameId }}" data-settings--account-expand-target="toggle" class="p-2 rounded-lg text-gray-400 cursor-pointer hover:text-ink" data-action="click->settings--account-expand#toggle" @@ -73,7 +100,7 @@ arrives one request later is a list that visibly blinks on each navigation. Any other account still loads its frame on demand. #} diff --git a/templates/_partials/_label_menu.html.twig b/templates/_partials/_label_menu.html.twig index 3ce1f891..1c79a19a 100644 --- a/templates/_partials/_label_menu.html.twig +++ b/templates/_partials/_label_menu.html.twig @@ -50,7 +50,7 @@ Variables: class="fa-solid fa-check w-3.5 text-accent {{ activeLabels is defined and label.id in activeLabels ? '' : 'invisible' }}" aria-hidden="true"> - {{ label.name }} + {{ label|label_name }} {% endfor %} {% endif %} diff --git a/templates/_partials/_sidebar.html.twig b/templates/_partials/_sidebar.html.twig index 7495a963..74fa077b 100644 --- a/templates/_partials/_sidebar.html.twig +++ b/templates/_partials/_sidebar.html.twig @@ -7,21 +7,36 @@ app_mail_sidebar_counts. {% macro unread_badge(count, key) %} {{ count }} {% endmacro %} {# -The same badge for a number that is a total rather than an unread count — -Trash and Drafts, see SidebarCounts::TOTAL_ROLES. +The badge for a number that is a TOTAL rather than an unread count — Trash and +Drafts, see SidebarCounts::TOTAL_ROLES. -Neutral rather than accented, and that is the whole point of it existing: an -accent badge means "these want you", and 193 items in the bin do not. Wearing -the same red as the Inbox badge is what made the Trash count read as 188 unread -messages demanding attention rather than as how much is in there. +The rule is right: nobody triages a bin, and the useful number there is how much +is in it. What was wrong is that saying so relied on colour alone. Both numbers +were the same filled pill in the same place at the same size, one red and one a +faint grey, and a bare "5" in a pill is a bare "5" — the shape said "badge", not +"how many". Colour is also the one channel a person may not have. + +So the two now differ in SHAPE as well as tone: unread is a filled round pill +(it is a call to action and looks like one), a total is an outlined rounded +rectangle that reads as a tally. And both carry an accessible name that says +which kind of number it is, because to a screen reader they were both the word +"five". #} {% macro total_badge(count, key) %} - {{ count }} {% endmacro %} @@ -64,7 +79,7 @@ keeps its accessible name either way. text-ink-faint hover:text-ink hover:bg-hover opacity-0 group-hover/label:opacity-100 focus-visible:opacity-100 transition-opacity cursor-pointer" - aria-label="{{ 'settings.labels.edit'|trans({ '%name%': label.name }) }}" + aria-label="{{ 'settings.labels.edit'|trans({ '%name%': label|label_name }) }}" > @@ -99,7 +114,7 @@ keeps its accessible name either way. The label's own colour dot on the far left is identity, not status, which is why this cannot be folded into it. #} - {{ label.name }} + {{ label|label_name }} {{ macros.label_new_dot(label) }} {{ macros.unread_badge(sidebarCounts.forLabel(label), 'label:' ~ label.id) }} @@ -120,7 +135,7 @@ keeps its accessible name either way. class="flex-1 flex items-center justify-between gap-2 truncate min-w-0" data-ui--sidebar-target="link" data-action="{{ drawerClose }}"> - {{ label.name }} + {{ label|label_name }} {{ macros.unread_badge(sidebarCounts.forLabel(label), 'label:' ~ label.id) }} {{ macros.label_edit(label) }} @@ -146,7 +161,7 @@ App\Twig\NewMailMarkers::roleKey() and ::labelKey(), which build the other half. {% macro label_new_dot(label) %} {% import '_partials/_new_dot.html.twig' as shared %} - {{ shared.new_dot(newMail.forLabel(label), 'new:label:' ~ label.id, label.name) }} + {{ shared.new_dot(newMail.forLabel(label), 'new:label:' ~ label.id, label|label_name) }} {% endmacro %} {% macro starred_new_dot() %} @@ -179,6 +194,13 @@ controller instead — the scroll position and which trees are open.
- {% include '_partials/_account_list.html.twig' with { _drawerClose: _drawerClose } %} + {# The same suffix the list containers either side of this use. Both + sidebars render this partial on every page, so without it each + account's folder frame and expand toggle existed twice under one + id — invalid HTML, and Turbo resolves a frame target BY id, so + the drawer and the desktop rail were one bad guess apart from + swapping folder lists. #} + {% include '_partials/_account_list.html.twig' with { + _drawerClose: _drawerClose, + _idSuffix: _drawerMode is defined and _drawerMode ? '-drawer' : '', + } %}
diff --git a/templates/_partials/_thread_row.html.twig b/templates/_partials/_thread_row.html.twig index e0c3bfc3..3b6f6411 100644 --- a/templates/_partials/_thread_row.html.twig +++ b/templates/_partials/_thread_row.html.twig @@ -181,7 +181,7 @@ Threads only. A bare Message row is a message, and only a conversation arrives. same everywhere it appears, only in a unified multi-account list. #} {% if showAccountDot %} {% set accountName = rowAccount.name ?: (rowAccount.email ?: rowAccount.username) %} - {% endif %} diff --git a/templates/account/_reorder.stream.html.twig b/templates/account/_reorder.stream.html.twig deleted file mode 100644 index 078f0497..00000000 --- a/templates/account/_reorder.stream.html.twig +++ /dev/null @@ -1,15 +0,0 @@ - - - - - - - diff --git a/templates/mail/_account_folder_rows.html.twig b/templates/mail/_account_folder_rows.html.twig index 47723808..4c36f55c 100644 --- a/templates/mail/_account_folder_rows.html.twig +++ b/templates/mail/_account_folder_rows.html.twig @@ -30,7 +30,7 @@ class="nav-item flex items-center justify-between py-1.5 text-xs transition-colors text-ink-muted" style="--label-depth: {{ 12 + label.depth * 12 }}px" > - {{ label.name }} + {{ label|label_name }} {% if not label.isSystem %} {# Counted within this account, and keyed that way too: the sidebar's own label rows carry `label:` and mean every diff --git a/templates/mail/_account_labels.html.twig b/templates/mail/_account_labels.html.twig index 2d2695d4..a4a6a6c6 100644 --- a/templates/mail/_account_labels.html.twig +++ b/templates/mail/_account_labels.html.twig @@ -1,5 +1,10 @@ {# The frame response for an account the user has just expanded. The rows are - shared with the sidebar's own inline render — see _account_folder_rows. #} - + shared with the sidebar's own inline render — see _account_folder_rows. + + `frameId` rather than a literal: the desktop rail and the mobile drawer each + render a frame for this account and they cannot share an id, so the reply has + to wear whichever one asked. MailController::folderFrameId() decides, and + only ever returns one of the two. #} + {% include 'mail/_account_folder_rows.html.twig' with { account: account, labels: labels } only %} diff --git a/templates/settings/_accounts.html.twig b/templates/settings/_accounts.html.twig index 93582ca5..a6087b29 100644 --- a/templates/settings/_accounts.html.twig +++ b/templates/settings/_accounts.html.twig @@ -3,24 +3,48 @@ {{ 'settings.accounts.empty'|trans }}

{% else %} + {# Says what dragging does, before it is dragged. This list used to reassign + the primary account as a side effect of being tidied, and nothing on + screen mentioned it; the honest fix is that it no longer does, but the + list should still say which of the two it is. #} +

{{ 'settings.accounts.order_hint'|trans }}

+ + {# sortablejs by way of our own controller, not @stimulus-components/sortable: + that wrapper builds its own request body with nowhere to put a CSRF + token, which is exactly why this endpoint went without one. The rules + list had already moved off it for the same reason. #}
    + data-controller="settings--account-order" + data-settings--account-order-url-value="{{ path('app_account_reorder') }}" + data-settings--account-order-csrf-value="{{ csrf_token('account_reorder') }}" + data-settings--account-order-target="list"> {% for account in manageableAccounts %} {% set isPassword = account.authType == 'password' %} -
  • +
  • - + {# Buttons as well as a handle: a drag is unreachable from a + keyboard and fiddly on a phone, and the rules list beside + this one already worked that way. #} +
    + + + + + +
    @@ -31,14 +55,17 @@ {# Read the flag, not the position. This badge used to be `loop.first`, which made it a statement about whatever order the list happened to arrive in — and - the settings page sorts alphabetically while - `isPrimary` is derived from the user's own - arrangement (AccountCreator::resequence). The two - disagreed whenever the alphabet did, so the account - badged PRIMARY here and the account compose actually - sent from were different accounts. #} + `isPrimary` was then itself derived from position, so + the badge moved when the list was dragged and took + the user's From address with it. + + The flag is now chosen rather than derived, and the + badge reports it. It is also a title, because "which + account sends my mail" is not something a coloured + pill on its own answers. #} {% if account.isPrimary %} - + {{ 'settings.accounts.primary'|trans }} {% endif %} @@ -52,6 +79,30 @@
    + {# The explicit way to choose the sending account, which is + the half that was missing: the only way to change it used + to be to drag a row to the top, which is not something a + person tidying a list is asking for and which the list + never said it would do. + + Only offered on an account that could actually send — + a disabled account as the default From would be a From + nothing can go out of. #} + {% if not account.isPrimary and account.isActive %} +
    + + +
    + {% endif %} + {% if isPassword %}