From 994d0a9c0d4516c8229fe664ff1020d432276825 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Mon, 28 Sep 2026 16:37:33 +0200 Subject: [PATCH] fix(sidebar): the open row no longer fades in from black on every load Every sidebar row went out with no colour class, and ui--sidebar gave the open one bg-accent-soft and text-accent (and every other row text-ink-muted) on connect. By then the browser had already styled the page, and the rows carry transition-colors for their hover, so on every full load and every Turbo visit the open row faded in over 150ms from the inherited black and a transparent pill, and the rest faded from black to their muted ink. Measured in headless Chromium against the test stack: app.css loaded at ~155ms, first paint at ~200ms with the rows black, the controller's pass at ~250ms, transitionrun on every visible row in the next frame. The stylesheet order was never the cause, and neither was the per-frame sampler: plain transitionrun listeners see the same thing. The server now renders the state. SidebarRowStateExtension restates ui--sidebar#_matches against the main request, and every link row in the sidebar, the account list and an expanded account's folders takes its classes from sidebar_row_state(). The controller's pass is then a no-op on everything the server drew, the first style the browser resolves is the final one, and the hover transitions are untouched (still ~130ms in and out). The pass stays for frame navigations and for rows that arrive later. After: the open row's first sample is already its final colour, and no sidebar row runs a transition on load, on a Turbo visit or in the mobile drawer. Two transitions remain at load and neither is a sidebar row: the Mercure dot, which is neutral until the stream reports, by design, and the topbar calendar toggle, which is still rendered as "mail" and corrected on connect. SidebarRowStateTest pins the rendered state, the matching rule, and that the class lists agree with the controller's. Co-Authored-By: Claude Opus 5.5 --- assets/controllers/ui/sidebar_controller.js | 10 ++ src/Twig/SidebarRowStateExtension.php | 123 +++++++++++++++++ templates/_partials/_account_list.html.twig | 10 +- templates/_partials/_sidebar.html.twig | 67 +++++---- templates/mail/_account_folder_rows.html.twig | 10 +- tests/Controller/SidebarRowStateTest.php | 128 ++++++++++++++++++ 6 files changed, 319 insertions(+), 29 deletions(-) create mode 100644 src/Twig/SidebarRowStateExtension.php create mode 100644 tests/Controller/SidebarRowStateTest.php diff --git a/assets/controllers/ui/sidebar_controller.js b/assets/controllers/ui/sidebar_controller.js index b2af56fc..46b767f9 100644 --- a/assets/controllers/ui/sidebar_controller.js +++ b/assets/controllers/ui/sidebar_controller.js @@ -4,6 +4,12 @@ import { WRITE_EVENT } from "../../mail_writes.js"; // `is-active` carries no colour of its own — it is the hook the sidebar's // Gmail-style pill shape hangs off in app.css. +// +// The server renders these same two sets on every row it draws (OPEN and +// CLOSED in App\Twig\SidebarRowStateExtension), so on those rows the pass in +// connect() changes nothing. Keep the lists identical: a row the server drew +// one way and this pass redraws the other is a row that fades on every load, +// from whatever it was painted as. SidebarRowStateTest compares the two files. const ACTIVE_CLASSES = ["is-active", "bg-accent-soft", "text-accent", "font-medium"]; // A parent of the open row. Its own class rather than the active set: the // trail should read as "you are in here", not as a second selection. @@ -667,6 +673,10 @@ export default class extends Controller { * compare matched nothing and those rows never highlighted at all. The * account has to agree too — the same label under another account is a * different view, and both rows lighting up says the wrong thing. + * + * Restated in PHP as SidebarRowStateExtension::isOpen(), which decides the + * rows the server renders. A change here that is not made there is a row + * the two disagree about, and it fades on every load. */ _matches(link) { const target = new URL(link.href, window.location.origin); diff --git a/src/Twig/SidebarRowStateExtension.php b/src/Twig/SidebarRowStateExtension.php new file mode 100644 index 00000000..65cc16fa --- /dev/null +++ b/src/Twig/SidebarRowStateExtension.php @@ -0,0 +1,123 @@ +rowState(...)), + ]; + } + + /** + * The classes for the row whose link points at $href. + */ + public function rowState(string $href): string + { + return true === $this->isOpen($href) ? self::OPEN : self::CLOSED; + } + + public function isOpen(string $href): bool + { + $request = $this->requestStack->getMainRequest(); + $target = parse_url($href); + + if (null === $request || false === $target || false === isset($target['path'])) { + return false; + } + + // What `window.location.pathname` is to the controller: base URL + // included, and still percent-encoded — which is how path() writes the + // href, so the two compare like for like. + $path = $request->getBaseUrl() . $request->getPathInfo(); + + if ($path !== $target['path'] && false === str_starts_with($path, $target['path'] . '/')) { + return false; + } + + parse_str($target['query'] ?? '', $query); + + $wanted = $query['account'] ?? null; + + if (null === $wanted) { + return true; + } + + // all() rather than get(): get() throws on `?account[]=`, and a + // malformed query string must not be able to take the sidebar down. + $current = $request->query->all()['account'] ?? null; + + return is_string($current) && $wanted === $current; + } +} diff --git a/templates/_partials/_account_list.html.twig b/templates/_partials/_account_list.html.twig index e7409066..a837cccc 100644 --- a/templates/_partials/_account_list.html.twig +++ b/templates/_partials/_account_list.html.twig @@ -26,13 +26,17 @@ other sidebar row — a rounded-lg box on this wrapper was the last square highlight left in the sidebar. It has to be the wrapper and not the anchor: on the anchor the pill stops before the chevron and - cuts the row in half. #} -
+ cuts the row in half. + + Open or closed is rendered, not applied on connect — see + App\Twig\SidebarRowStateExtension. #} + {% set _href = path('app_mail_account', { account: account.id }) %} +
{# Named explicitly rather than by the address inside it: that span is `truncate`, and a name a screen reader reads as "kontakt@…" because CSS ran out of room is not a name. #} 0 %} {# Keyed by full name, in the same stored set as the two section @@ -192,7 +193,7 @@ account inside ThreadStatusUpdater. data-action="toggle->ui--sidebar#rememberCollapse" {{ sidebarCounts.isCollapsed(_treeKey) ? '' : 'open' }}> {% if _showSnoozed %} -
+ {% set _href = path('app_mail_snoozed') %} +
{# The tier rides on the LINK, not on the pill around it. The pill is what you see — .nav-item draws it — but the row's height comes from padding-block, and padding on the wrapper is area the anchor does not @@ -595,7 +611,7 @@ controller instead — the scroll position and which trees are open. the same to the pixel as when the anchor WAS the row. #} {% endif %} {% if _showArchive %} -
{# The tier rides on the LINK, not on the pill around it. The pill is what you see — .nav-item draws it — but the row's height comes from @@ -621,7 +638,7 @@ controller instead — the scroll position and which trees are open. the same to the pixel as when the anchor WAS the row. #} get(UserRepository::class) + ->findOneBy(['email' => 'e2e-admin@plmail.test']); + + if (false === $user instanceof User) { + self::markTestSkipped('run `app:test:seed-user --admin` first'); + } + + $client->loginUser($user); + $crawler = $client->request('GET', '/mail/inbox'); + + $rows = $crawler + ->filter('#sidebar [data-ui--sidebar-target="link"], #sidebar-drawer-inner [data-ui--sidebar-target="link"]') + ->each(static function (Crawler $link): array { + $row = $link->closest('.nav-item') ?? $link; + + return [(string) $link->attr('href'), explode(' ', (string) $row->attr('class'))]; + }); + + self::assertNotEmpty($rows, 'no sidebar rows rendered at all'); + + $open = []; + + foreach ($rows as [$href, $classes]) { + $isOpen = [] === array_diff(explode(' ', SidebarRowStateExtension::OPEN), $classes); + $isClosed = [] === array_diff(explode(' ', SidebarRowStateExtension::CLOSED), $classes); + + // Neither is the regression: a row the controller colours on + // connect, from whatever the browser had already painted it as. + self::assertTrue( + $isOpen xor $isClosed, + sprintf('the row for %s was rendered without its open/closed state: %s', $href, implode(' ', $classes)), + ); + + if ($isOpen) { + $open[] = $href; + } + } + + // Once in the sidebar and once in the drawer, which is the same partial. + self::assertSame(['/mail/inbox', '/mail/inbox'], $open); + } + + /** + * ui--sidebar#_matches, restated — the cases where getting it slightly + * wrong would be invisible until a row faded. + * + * @return iterable + */ + public static function rows(): iterable + { + yield 'a parent label is open while a child is' => ['/mail/label/path/Work/Clients', '/mail/label/path/Work', true]; + yield 'a label sharing a prefix is not its child' => ['/mail/label/path/Workshop', '/mail/label/path/Work', false]; + yield 'an account folder, on that account' => ['/mail/label/5?account=3', '/mail/label/5?account=3', true]; + yield 'an account folder, on another account' => ['/mail/label/5?account=4', '/mail/label/5?account=3', false]; + yield 'a malformed account is closed, not an error' => ['/mail/label/5?account[]=3', '/mail/label/5?account=3', false]; + } + + #[DataProvider('rows')] + public function testARowIsOpenExactlyWhereTheControllerWouldOpenIt(string $page, string $href, bool $open): void + { + $stack = new RequestStack(); + $stack->push(Request::create($page)); + + self::assertSame($open, new SidebarRowStateExtension($stack)->isOpen($href)); + } + + public function testTheServerAndTheControllerDrawARowTheSameWay(): void + { + $source = (string) file_get_contents(dirname(__DIR__, 2) . '/' . self::CONTROLLER); + + foreach (['ACTIVE_CLASSES' => SidebarRowStateExtension::OPEN, 'INACTIVE_CLASSES' => SidebarRowStateExtension::CLOSED] as $constant => $rendered) { + self::assertSame(1, preg_match('/const ' . $constant . '\s*=\s*\[([^\]]*)\]/', $source, $match), sprintf('%s no longer declares %s', self::CONTROLLER, $constant)); + + preg_match_all('/"([^"]+)"/', $match[1], $classes); + + self::assertSame( + explode(' ', $rendered), + $classes[1], + sprintf( + '%s in %s and the classes SidebarRowStateExtension renders have drifted apart. ' + . 'Every row the server draws would then be redrawn on connect — and fade.', + $constant, + self::CONTROLLER, + ), + ); + } + } +}