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(); });