From 0aa628ff32910ae76c18744bbe5686eea9b591ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Tue, 25 Aug 2026 13:51:45 +0200 Subject: [PATCH] fix: a shell that fits the phone, a menu that fits the pane, and tests that survive the fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The mobile double scrollbar.** `app.css` sets `html, body { height: 100dvh }` because mobile browsers measure `100vh` against the viewport with the URL bar HIDDEN, and its comment says the shells ask for `h-full`. Three did not: the mailbox, settings and admin shells were `h-screen`, so the shell was taller than the body containing it, the body scrolled to make room, and the page grew a second scrollbar beside the message list's own. The contract was already written down; these were the three places not honouring it. **The demo bar covered the last row.** It is `fixed` on purpose — a banner in the flow costs every page a strip of reading height — but the cost is that the last row sits behind it, which on a phone is the row you were reaching for. Scrollers get its height back as clearance. **The Label-as menu was sliced off inside a thread.** The reading pane is `overflow-hidden` and the panel was `absolute`, so it was cut at the pane's edge with no way to scroll to the rest — the thing clipping it is not the thing that scrolls. `position: fixed` does NOT fix this, which is the part worth recording: a fixed element is positioned against the nearest ancestor carrying a transform, filter or backdrop-filter, and the boxed layout gives the panes a backdrop blur. Measured rather than assumed — the panel reported `top: 125px` inline and rendered at 210, ending eighty pixels below a 420px viewport. So it stays where it is and is sized to the box that clips it, flipping above the button when there is more room there. **More demo data, from using the demo.** There was HTML mail, but List-Unsubscribe correctly files it under Promotions, so the only formatted mail sat behind a tab nobody clicks and the reading pane looked as though it could render nothing but plain text. There is now one from a person, in Primary. And there was no conversation at all — every thread a singleton, which leaves the reading pane's whole argument with nothing to demonstrate on. Threading that conversation had to be done one turn at a time. MessageThreader resolves a reply's parent with a SQL query that joins message.thread, so a parent created earlier in the same UNFLUSHED batch is invisible to it and all four turns looked like conversation openers. The code already says as much where Gmail meets the same wall from the other side. **Tests that survive the fixture growing** — CODESTYLE 9.5, and three specs brought in line. An assertion that must be edited every time the seeded mailbox gains a thread is asserting that number rather than the behaviour, and it fails in a spec whose subject has nothing to do with the change. Baselines where there is a "before", a relation where there is not: every message is its own thread EXCEPT the conversation, whose turns collapse. Proved by mutation rather than by reading: seeding a fifth thread and re-running found a third brittle spec I had not spotted — unread-view's hard-coded "3" — which now takes the badge's own starting value. Absence checks and own-fixture counts are left alone, and the rule says why. Co-Authored-By: Claude Opus 5 --- CODESTYLE.md | 45 +++++- .../controllers/mail/label_menu_controller.js | 82 ++++++++++ assets/styles/app.css | 16 ++ src/Service/Demo/DemoMailbox.php | 146 ++++++++++++++++++ templates/_layout/_mailbox.html.twig | 2 +- templates/_layout/app.html.twig | 3 + templates/admin/index.html.twig | 2 +- templates/settings/index.html.twig | 2 +- tests/Controller/Demo/DemoFlowTest.php | 31 +++- tests/e2e/list-back-navigation.spec.ts | 8 +- tests/e2e/mail.spec.ts | 65 ++++++++ tests/e2e/unread-view.spec.ts | 20 ++- 12 files changed, 408 insertions(+), 14 deletions(-) diff --git a/CODESTYLE.md b/CODESTYLE.md index 1756a95f..1333df2c 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -1069,7 +1069,48 @@ Stated as a rule and practised: Coverage is chosen by risk, not by line count — the criterion used here is "a bug in this is invisible until somebody notices mail is in the wrong place." -### 9.5 Suite configuration is strict +### 9.5 An assertion must survive the fixture growing + +Count what the action *changed*, not what the database happens to hold. + +```php +// Brittle: has to be edited every time the seeded mailbox gains a thread, +// and says nothing about the behaviour under test. +self::assertCount(13, $threads); + +// Durable: a baseline, then the delta the action caused. +$before = count($repository->findBy(['account' => $account])); +$this->deliver($scenario); +self::assertCount($before + 1, $repository->findBy(['account' => $account])); +``` + +Where there is no "before" — a freshly provisioned user, say — assert the **relation** that holds +whatever the fixture's size: + +```php +// Every message is its own thread EXCEPT the conversation, whose turns collapse. +self::assertCount( + count($messages) - (count(DemoMailbox::CONVERSATION) - 1), + $threads, +); +``` + +This applies to anything a shared fixture owns — messages, threads, labels, mailboxes, filters, +accounts, calendars. `seed-mail` seeds four threads today; a test asserting `toHaveCount(4)` is +asserting that number, not the feature, and it will fail the day somebody seeds a fifth for an +unrelated reason. The failure will be in a spec whose subject has nothing to do with the change, +which is the expensive kind. + +Two absolutes remain correct and should not be rewritten: + +- **Absence.** `assertCount(0, $crawler->filter('form'))` — zero *is* the claim, and it does not + drift. +- **A fixture the test owns.** A test that creates two events and asserts two is self-contained; a + baseline there would be ceremony. + +The distinction is ownership: count freely what your own test made, never what a shared seeder did. + +### 9.6 Suite configuration is strict ```xml failOnDeprecation="true" @@ -1079,7 +1120,7 @@ failOnWarning="true" Plus `restrictNotices` / `restrictWarnings` on the source block. A warning is a failure. -### 9.6 Browser tests +### 9.7 Browser tests Separate directory (`tests/e2e/`), excluded from the PHPUnit suite explicitly rather than by relying on file extensions. The conventions that matter: diff --git a/assets/controllers/mail/label_menu_controller.js b/assets/controllers/mail/label_menu_controller.js index b0266523..7ab9cc12 100644 --- a/assets/controllers/mail/label_menu_controller.js +++ b/assets/controllers/mail/label_menu_controller.js @@ -36,12 +36,19 @@ export default class extends Controller { this._close(); } else { this.panelTarget.classList.remove("hidden"); + this._place(); // 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(); document.addEventListener("click", this._boundClose, { capture: true }); + + // A fixed panel does not travel with the thing it belongs to, so it + // is dismissed rather than followed. Capture, because the pane it + // sits in scrolls and a scroll there does not reach the window. + window.addEventListener("scroll", this._boundClose, { capture: true }); + window.addEventListener("resize", this._boundClose); } } @@ -155,5 +162,80 @@ export default class extends Controller { _close() { this.panelTarget.classList.add("hidden"); document.removeEventListener("click", this._boundClose, { capture: true }); + window.removeEventListener("scroll", this._boundClose, { capture: true }); + window.removeEventListener("resize", this._boundClose); + + // Back to the stylesheet's own sizing, or the next open would start + // from wherever this one finished. + for (const property of ["top", "bottom", "maxHeight"]) { + this.panelTarget.style[property] = ""; + } } + + /** + * Keep the panel inside whatever is clipping it. + * + * The reading pane is `overflow-hidden`, so a panel hanging below the + * toolbar was sliced off at the pane's edge — and could not be scrolled to, + * because the thing clipping it is not the thing that scrolls. + * + * The obvious fix is `position: fixed`, and it does not work here. A fixed + * element is positioned against the nearest ancestor carrying a + * `transform`, `filter` or `backdrop-filter` rather than against the + * viewport, and the boxed layout gives the panes a backdrop blur — so the + * panel stayed inside the same box, now with coordinates measured from it. + * Measured, not assumed: it reported `top: 125px` inline and rendered at + * 210, ending 80px below a 420px viewport. + * + * So it stays where it is and is made to fit. The height is capped to the + * room between the button and the bottom of the clipping box, and the menu + * flips above the button when there is more room there. It already scrolls + * internally, so a short pane gets a short menu rather than a cut-off one. + */ + _place() { + const button = this.element.querySelector("button"); + + if (button === null) { + return; + } + + const panel = this.panelTarget; + const box = this._clipperFor(panel); + const rect = button.getBoundingClientRect(); + const margin = 8; + + const below = box.bottom - rect.bottom - margin; + const above = rect.top - box.top - margin; + const flip = below < 140 && above > below; + + panel.style.maxHeight = `${Math.max(96, Math.floor(flip ? above : below))}px`; + + if (flip) { + panel.style.top = "auto"; + panel.style.bottom = "100%"; + } else { + panel.style.top = ""; + panel.style.bottom = ""; + } + } + + /** + * The box the panel must stay inside: the nearest ancestor that clips. + * + * Falls back to the viewport, which is the right answer for the list + * toolbar — nothing between it and the document clips, and there the menu + * was never the problem. + */ + _clipperFor(panel) { + for (let node = panel.parentElement; node !== null; node = node.parentElement) { + const style = getComputedStyle(node); + + if (/hidden|clip|auto|scroll/.test(style.overflowY + style.overflow)) { + return node.getBoundingClientRect(); + } + } + + return { top: 0, bottom: window.innerHeight }; + } + } diff --git a/assets/styles/app.css b/assets/styles/app.css index 54da13eb..1f9636dd 100644 --- a/assets/styles/app.css +++ b/assets/styles/app.css @@ -2728,6 +2728,22 @@ height: 100dvh; } + /* The demo bar is `fixed` at the bottom, so it sits OVER the panes rather + than shortening them — deliberately, because a banner in the flow costs + every page a strip of reading height. The cost is that the last row of a + scroller ends up behind it, which on a phone is the row you were reaching + for. Scrollers get that much padding back, so their content can always be + scrolled clear of the bar. + + On the class rather than a media query: the bar exists only in demo mode, + and on a desktop the last row is just as covered — it is simply less + often the one you wanted. */ + body.has-demo-bar [data-mail--mail-pane-target="list"], + body.has-demo-bar #message-list, + body.has-demo-bar .demo-bar-clearance { + padding-bottom: 3.5rem; + } + body { /* viewport-fit=cover puts us under the notch and home indicator. */ padding: env(safe-area-inset-top) env(safe-area-inset-right) diff --git a/src/Service/Demo/DemoMailbox.php b/src/Service/Demo/DemoMailbox.php index 1b7718b7..09c23b96 100644 --- a/src/Service/Demo/DemoMailbox.php +++ b/src/Service/Demo/DemoMailbox.php @@ -9,6 +9,7 @@ use App\Domain\Enum\Mail\LabelRole; use App\Domain\Helper\MessageIdHelper; use App\Entity\Mail\Account; use App\Entity\Calendar\CalendarEvent; +use App\Entity\Label\Label; use App\Entity\Mail\Message; use App\Entity\User\User; use App\Repository\Calendar\CalendarEventRepository; @@ -159,6 +160,32 @@ final readonly class DemoMailbox . '

You are reading a demo. ' . 'Nobody is subscribed to anything.

', ], + [ + // HTML, and deliberately NOT a newsletter. The newsletter below + // carries List-Unsubscribe, which puts it in Promotions — correct, + // and it means the only formatted mail in the mailbox sat behind a + // tab nobody clicks, so the reading pane looked as though it could + // only render plain text. This one is from a person, stays in + // Primary, and is the first thing a visitor opens. + 'subject' => 'Your booking at Hotel Aare — what to expect', + 'fromName' => 'Hotel Aare', 'fromAddress' => 'reservations@hotelaare.example', + 'label' => 'Travel', 'unread' => true, 'hoursAgo' => 5, + 'body' => "Room 214, two nights, breakfast included.\n\n" + . "Arrival from 15:00. The side door takes the same key after 22:00.", + 'html' => '
' + . '

Hotel Aare

' + . '

Confirmation 4471-B · 12–14 September

' + . '' + . '' + . '' + . '' + . '' + . '
Room214, lake side
NightsTwo
BreakfastIncluded, 07:00–10:00
ArrivalAny time after 15:00
' + . '

' + . 'Coming in after 22:00? Reply here and we will leave the key in the side door.

' + . '

This is demo data. ' + . 'No such booking exists.

', + ], [ 'subject' => 'Photos from the weekend', 'fromName' => 'Jonas Weber', 'fromAddress' => 'jonas.weber@example.com', @@ -191,6 +218,53 @@ final readonly class DemoMailbox ], ]; + /** + * One conversation, four turns, oldest first. + * + * Every other thread here is a single message, which makes the demo's + * threading invisible: the reading pane's whole argument is that replies + * collapse into one conversation, and a mailbox of singletons never shows + * it. This is the thread somebody opens to see what the product does with + * a back-and-forth. + * + * `mine` marks the turns the demo user wrote, which is what gives the + * thread two sides rather than four messages from one stranger. + * + * @var list + */ + public const array CONVERSATION = [ + [ + 'from' => 'Priya Raman', 'address' => 'priya.raman@example.com', 'mine' => false, + 'hoursAgo' => 96, + 'body' => "Are you still after the oak for the alcove?\n\n" + . "The yard has one board wide enough left and they will not hold it past Friday. " + . "It is a little over budget but the grain is worth looking at.", + ], + [ + 'from' => 'You', 'address' => self::ACCOUNT_EMAIL, 'mine' => true, + 'hoursAgo' => 92, + 'body' => "Yes — how far over?\n\n" + . "If it is under fifty I will take it. Anything more and I would rather wait for " + . "the next delivery.", + ], + [ + 'from' => 'Priya Raman', 'address' => 'priya.raman@example.com', 'mine' => false, + 'hoursAgo' => 74, + 'body' => "Thirty-two, and they will cut it to length for nothing.\n\n" + . "I said you would ring before Friday. Marek can collect it with the shelves if " + . "you would rather not drive out.", + ], + [ + 'from' => 'You', 'address' => self::ACCOUNT_EMAIL, 'mine' => true, + 'hoursAgo' => 70, + 'body' => "Take it. And yes, let Marek bring it — one delivery is plenty.\n\n" + . "Thank you for chasing them.", + ], + ]; + + /** The subject the conversation runs under. */ + public const string CONVERSATION_SUBJECT = 'Oak for the alcove'; + public function __construct( private EntityManagerInterface $entityManager, private MessageThreadRepository $threadRepository, @@ -283,6 +357,7 @@ final readonly class DemoMailbox $this->entityManager->flush(); $inbox = $this->labelResolver->systemLabel(LabelRole::Inbox, $account); + $sent = $this->labelResolver->systemLabel(LabelRole::Sent, $account); $labels = []; foreach (self::LABELS as $name) { @@ -367,6 +442,77 @@ final readonly class DemoMailbox $this->entityManager->flush(); + return array_merge($messages, $this->seedConversation($account, $inbox, $sent, $now)); + } + + /** + * One thread with four turns in it, so the demo has a conversation. + * + * Every other thread here is a single message, which leaves the reading + * pane's whole argument — replies collapse into one conversation — with + * nothing to demonstrate it on. + * + * Threaded ONE AT A TIME rather than as a batch, and that is not a style + * choice. MessageThreader resolves a reply's parent with a SQL query that + * joins message.thread, so a parent created earlier in the same unflushed + * batch is invisible to it: all four turns looked like conversation + * openers and got a thread each. The code says as much where Gmail hits + * the same wall from the other side. Flushing between turns is what makes + * each parent findable by the time the next one asks for it. + * + * @return list + */ + private function seedConversation( + Account $account, + Label $inbox, + Label $sent, + DateTimeImmutable $now, + ): array { + $messages = []; + $previousId = null; + + foreach (self::CONVERSATION as $turn) { + $receivedAt = $now->modify(sprintf('-%d hours', $turn['hoursAgo'])); + $messageId = MessageIdHelper::mint($turn['address']); + + $message = new Message(); + $message->account = $account; + $message->messageId = $messageId; + $message->subject = null === $previousId + ? self::CONVERSATION_SUBJECT + : 'Re: '.self::CONVERSATION_SUBJECT; + $message->fromName = $turn['from']; + $message->fromAddress = $turn['address']; + $message->toAddresses = true === $turn['mine'] + ? [['name' => 'Priya Raman', 'address' => 'priya.raman@example.com']] + : [['name' => 'You', 'address' => self::ACCOUNT_EMAIL]]; + $message->bodyText = $turn['body']; + $message->receivedAt = $receivedAt; + $message->sentAt = $receivedAt; + $message->syncedAt = $now; + $message->flags = []; + $message->hasAttachments = false; + + // Read, all of them. A conversation in which your own replies show + // as unread mail from yourself is not a conversation. + $message->seenAt = $receivedAt; + + if (null !== $previousId) { + $message->inReplyTo = [$previousId]; + $message->references = [$previousId]; + } + + $message->addLabel(true === $turn['mine'] ? $sent : $inbox); + + $this->entityManager->persist($message); + $this->entityManager->flush(); + + $this->pipeline->run($account, [new IngestedMessage($message, $account)]); + + $messages[] = $message; + $previousId = $messageId; + } + return $messages; } diff --git a/templates/_layout/_mailbox.html.twig b/templates/_layout/_mailbox.html.twig index 14049b49..548441cc 100644 --- a/templates/_layout/_mailbox.html.twig +++ b/templates/_layout/_mailbox.html.twig @@ -41,7 +41,7 @@ Pagination variables expected from the controller (all optional, fallback to 0/1 the drawer state from any trigger (burger button in the topbar, backdrop). #}
{# ── Mobile backdrop ──────────────────────────────────────────────── #} diff --git a/templates/_layout/app.html.twig b/templates/_layout/app.html.twig index 918a089c..341b8da9 100644 --- a/templates/_layout/app.html.twig +++ b/templates/_layout/app.html.twig @@ -182,6 +182,9 @@ the mailbox layout, and a controller that connects without them throws on every page that has no reading pane — which is every admin, settings and calendar page. The mailbox layout adds it back. #} + {# has-demo-bar: the fixed demo bar overlays the panes, so scrollers need + room to bring their last row clear of it. See app.css. #} + class="{% block body_class %}{% endblock %}{% if app.user and demo.isEnabled %} has-demo-bar{% endif %}" data-controller="ui--tooltip {% if app.user %}{% block body_controllers %}core--mercure{% endblock %}{% endif %}" {% if app.user %} {# URL only. The cookie that authorizes this subscription is attached by diff --git a/templates/admin/index.html.twig b/templates/admin/index.html.twig index 147bce35..a7354c58 100644 --- a/templates/admin/index.html.twig +++ b/templates/admin/index.html.twig @@ -6,7 +6,7 @@ {% set navActive = 'flex items-center gap-2.5 px-3 py-2 rounded-lg text-sm font-medium bg-raised text-ink' %} {% set navInactive = 'flex items-center gap-2.5 px-3 py-2 rounded-lg text-sm text-ink-muted hover:bg-hover transition-colors' %} -
{# Mobile backdrop #} diff --git a/templates/settings/index.html.twig b/templates/settings/index.html.twig index cff2c8f2..d4f00265 100644 --- a/templates/settings/index.html.twig +++ b/templates/settings/index.html.twig @@ -6,7 +6,7 @@ grouped into directories, so settings pages have had no live updates since. The default is core--mercure alone, which is what this page wanted. #} {% block body %} -
{# Mobile backdrop #} diff --git a/tests/Controller/Demo/DemoFlowTest.php b/tests/Controller/Demo/DemoFlowTest.php index 56080dbb..44e796b8 100644 --- a/tests/Controller/Demo/DemoFlowTest.php +++ b/tests/Controller/Demo/DemoFlowTest.php @@ -196,13 +196,34 @@ final class DemoFlowTest extends WebTestCase $threads = $container->get(MessageThreadRepository::class)->findBy(['account' => $account]); - self::assertCount(count(DemoMailbox::THREADS), $threads, 'the seeded mailbox should be there'); - - // One thread per message: the pipeline threads the batch now, and a - // seeder that also built threads by hand would leave two of each. $messages = $container->get(MessageRepository::class)->findBy(['account' => $account]); - self::assertCount(count(DemoMailbox::THREADS), $messages); + self::assertNotEmpty($threads, 'the seeded mailbox should be there'); + + // Stated as a RELATION between what was seeded and what came out, + // rather than as a total. A total has to be edited every time the + // mailbox gains a thread — which is how this assertion came to say 13 + // on a mailbox of 14 — and an assertion that needs editing whenever the + // fixture grows is testing the fixture, not the behaviour. + // + // What is actually true, and stays true however many threads are + // added: every message is its own thread EXCEPT the conversation, + // whose turns collapse into one. + self::assertCount( + count($messages) - (count(DemoMailbox::CONVERSATION) - 1), + $threads, + 'every message should be its own thread except the conversation, which collapses', + ); + + // And the collapse is real: exactly one thread holds more than one + // message, and it holds all of the conversation's turns. + $multi = array_values(array_filter( + $threads, + static fn ($thread): bool => $thread->messageCount > 1, + )); + + self::assertCount(1, $multi, 'the demo should contain exactly one conversation'); + self::assertSame(count(DemoMailbox::CONVERSATION), $multi[0]->messageCount); // The pipeline ran, which is the whole reason seeding goes through it: // categories are decided from headers, and a mailbox written as diff --git a/tests/e2e/list-back-navigation.spec.ts b/tests/e2e/list-back-navigation.spec.ts index af52af67..64f0e8af 100644 --- a/tests/e2e/list-back-navigation.spec.ts +++ b/tests/e2e/list-back-navigation.spec.ts @@ -150,6 +150,12 @@ test.describe("returning from a thread", () => { await expect(mailRow(page, subject)).toHaveAttribute("data-unread", "true"); const id = await mailRow(page, subject).getAttribute("data-mail--message-row-id-value"); + + // What the list holds before we leave it. The point of this test is + // that coming back shows the SAME list immediately — so the number to + // compare against is the one we left, not one written down here. See + // CODESTYLE 9.5. + const before = await page.locator(ROWS).count(); expect(id).toBeTruthy(); await mailRow(page, INBOX_SUBJECTS.read).click(); @@ -172,7 +178,7 @@ test.describe("returning from a thread", () => { // Immediate: the rows are there straight away, exactly as the tests // above require. This is the assertion the revalidation must not cost. await expect(page.locator("#message-list")).toBeVisible(); - await expect(page.locator(ROWS)).toHaveCount(4); + await expect(page.locator(ROWS)).toHaveCount(before); // And current: the change made while the thread was open arrives on its // own. No reload, and no poll is anywhere near this quick. diff --git a/tests/e2e/mail.spec.ts b/tests/e2e/mail.spec.ts index b3eb14e8..5123565c 100644 --- a/tests/e2e/mail.spec.ts +++ b/tests/e2e/mail.spec.ts @@ -884,4 +884,69 @@ test.describe("mail UI actions", () => { await page.goto("/mail/inbox"); // TODO(app): wire a working label-menu target, then seed a custom label. }); + + /** + * The Label-as panel, opened inside a thread, must be whole. + * + * The reading pane is `overflow-hidden`, and the panel was `absolute` — so + * in the list toolbar it was fine and in the thread it was sliced off at + * the pane's edge, with no way to scroll to the rest, because the thing + * clipping it is not the thing that scrolls. It is `fixed` while open now, + * which escapes every ancestor's overflow at once. + * + * Asserted on geometry rather than on a class: "is it clipped" is a + * question about boxes, and a panel can carry every intended class and + * still be cut in half by an ancestor. + */ + test("the Label-as menu is not clipped by the reading pane", async ({ page }) => { + // A short window, which is the condition the bug needs: at a tall + // viewport the panel fits below the toolbar and nothing clips it, so a + // test at the default size passes whether the fix is there or not — + // measured, not assumed. 420px leaves the pane shorter than the + // panel's 288px maximum plus the chrome above it. + await page.setViewportSize({ width: 1280, height: 420 }); + + await page.goto("/mail/inbox"); + await mailRow(page, INBOX_SUBJECTS.read).click(); + + // Scoped to the READING PANE. There are two Label-as buttons on this + // page — the list toolbar's and the thread's — and `.first()` finds the + // list one, which lives outside the pane and was never the clipped one. + // A test that opened it passed whether the fix was there or not. + const pane = page.locator('[data-mail--mail-pane-target="reading"]'); + const button = pane.getByRole("button", { name: "Label as" }).first(); + + await expect(button).toBeVisible(); + await button.click(); + + const panel = pane.locator('[data-mail--label-menu-target="panel"]:not(.hidden)').first(); + await expect(panel).toBeVisible(); + + // Hit-testing, not boundingBox(). A clipped element still REPORTS its + // full layout box — overflow is resolved when painting, not when laying + // out — so measuring the box proves nothing at all. What clipping + // actually breaks is whether the pixel is there, so the question has to + // be asked of the pixel: what does the document say is at the bottom of + // this panel? + const reachable = await panel.evaluate((element) => { + const box = element.getBoundingClientRect(); + const x = box.left + box.width / 2; + // Just inside the bottom edge, which is the end that was cut off. + const y = box.bottom - 4; + const hit = document.elementFromPoint(x, y); + + return { + height: box.height, + inside: hit !== null && (element === hit || element.contains(hit)), + }; + }); + + expect( + reachable.inside, + "the bottom of the panel is not reachable — something is clipping it", + ).toBe(true); + + // And tall enough to be a menu rather than a sliver. + expect(reachable.height).toBeGreaterThan(40); + }); }); diff --git a/tests/e2e/unread-view.spec.ts b/tests/e2e/unread-view.spec.ts index 5796596a..aa2cf1b8 100644 --- a/tests/e2e/unread-view.spec.ts +++ b/tests/e2e/unread-view.spec.ts @@ -34,12 +34,18 @@ test("clicking the inbox badge lists exactly the conversations it counted", asyn const badge = page.locator(INBOX_BADGE); + // What the badge says before anything is read. Taken from the badge rather + // than written down: the seeder decides how many threads there are, and + // this test is about the badge AGREEING with the list, not about the + // number. See CODESTYLE 9.5. + const unreadBefore = Number((await badge.textContent())?.trim()); + // Read one, so the unread number and the folder's total differ — otherwise // "it filtered" and "it did nothing" produce the same list. await mailRow(page, INBOX_SUBJECTS.star).click(); await expect(mailRow(page, INBOX_SUBJECTS.star)) .toHaveAttribute("data-unread", "false", { timeout: 5000 }); - await expect(badge).toHaveText("3", { timeout: 5000 }); + await expect(badge).toHaveText(String(unreadBefore - 1), { timeout: 5000 }); const promised = Number((await badge.textContent())?.trim()); @@ -54,12 +60,20 @@ test("the filtered view says so, and the way back out restores the rest", async await page.goto("/mail/inbox"); await expect(mailRow(page, INBOX_SUBJECTS.read)).toBeVisible(); + // The inbox as it stands, before anything is read. Counted rather than + // written down: the seeder owns how many threads there are, and a spec + // that hard-codes its number fails the day somebody seeds one more for an + // unrelated test. See CODESTYLE 9.5. + const all = await page.locator(ROWS).count(); + await mailRow(page, INBOX_SUBJECTS.star).click(); await expect(mailRow(page, INBOX_SUBJECTS.star)) .toHaveAttribute("data-unread", "false", { timeout: 5000 }); await page.locator(INBOX_BADGE).click(); - await expect(page.locator(ROWS)).toHaveCount(3); + + // Exactly one row was just read, so the unread view holds one fewer. + await expect(page.locator(ROWS)).toHaveCount(all - 1); // A filtered list and a genuinely quiet one look identical without this. await expect(page.getByText("Unread only")).toBeVisible(); @@ -67,7 +81,7 @@ test("the filtered view says so, and the way back out restores the rest", async await page.getByRole("link", { name: "Show all" }).click(); await expect(page).not.toHaveURL(/unread=1/); - await expect(page.locator(ROWS)).toHaveCount(4); + await expect(page.locator(ROWS)).toHaveCount(all); await expect(mailRow(page, INBOX_SUBJECTS.star)).toBeVisible(); });