Skip to content

Commit 00913cf

Browse files
committed
Measure the overflow the long tags add, not the page's absolute overflow
The first version of this spec asserted zero horizontal overflow outright. In CI it failed with 35px — the same figure `hidden-books.spec.ts:252` reports, on a different spec and a different navigation, with the tag row stubbed to a known state. The CI fixture's detail page carries ~35px of overflow from a cause that has nothing to do with tags (notes/e2e-mobile-overflow-ci-triage.md). An absolute assertion there is permanently red for an unrelated reason and says nothing about the pills. Measure the delta instead: render the page with its own tags, measure, swap in the pathological ones, measure again, and require the long tags to add nothing. Pre-fix that delta is 323px (324 on mobile); post-fix it is 0, on any baseline. `pageOverflow` splits out of `assertNoHorizontalOverflow` so a spec can compare two renders instead of asserting a global property.
1 parent 7fdd6b6 commit 00913cf

2 files changed

Lines changed: 32 additions & 9 deletions

File tree

frontend/e2e/book-detail.spec.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { test, expect, Page } from '@playwright/test';
2-
import { collectPageErrors, assertNoPageErrors, assertNoHorizontalOverflow } from './utils';
2+
import { collectPageErrors, assertNoPageErrors, assertNoHorizontalOverflow, pageOverflow } from './utils';
33

44
/*
55
* Book-detail completeness pass:
@@ -192,7 +192,13 @@ test('book detail with a "More by" strip has no horizontal overflow on mobile',
192192
// specs — all of which log in as admin — never saw this.
193193
//
194194
// Deterministic on any seed: the role and the tag list are both stubbed.
195-
test('long tag names do not push the read-only detail page into horizontal scroll', async ({ page }) => {
195+
//
196+
// Measures the DELTA the long tags add, not absolute overflow. The CI fixture's
197+
// detail page carries ~35px of horizontal overflow from an unrelated cause
198+
// (notes/e2e-mobile-overflow-ci-triage.md), and an absolute assertion here would
199+
// fail on that instead of on the tag row, reporting nothing about the pills. The
200+
// delta is what this fix owns: pre-fix it was ~300px, post-fix it is 0.
201+
test('long tag names add no horizontal overflow to the read-only detail page', async ({ page }) => {
196202
await page.setViewportSize({ width: 390, height: 844 });
197203
await page.goto('/app');
198204
const bookId = await firstBookId(page);
@@ -207,19 +213,28 @@ test('long tag names do not push the read-only detail page into horizontal scrol
207213
await route.fulfill({ response: res, json: me });
208214
});
209215

216+
await page.goto(`/app/book/${bookId}`, { waitUntil: 'domcontentloaded' });
217+
await expect(page.locator('main h1')).toBeVisible({ timeout: 10_000 });
218+
const baseline = await pageOverflow(page);
219+
210220
// A real LoC heading plus a single unbroken token wider than the viewport.
221+
const longTag = 'France -- History -- Revolution, 1789-1799 -- Fiction';
211222
await page.route(`**/api/v1/books/${bookId}`, async (route) => {
212223
const res = await route.fetch();
213224
const book = await res.json();
214225
book.tags = [
215-
{ id: 990001, name: 'France -- History -- Revolution, 1789-1799 -- Fiction' },
226+
{ id: 990001, name: longTag },
216227
{ id: 990002, name: 'Bildungsroman'.repeat(8) },
217228
];
218229
await route.fulfill({ response: res, json: book });
219230
});
220231

221-
await page.goto(`/app/book/${bookId}`, { waitUntil: 'domcontentloaded' });
222-
await expect(page.locator('#book-tags')).toBeVisible({ timeout: 10_000 });
223-
await expect(page.getByText('France -- History -- Revolution, 1789-1799 -- Fiction')).toBeVisible();
224-
await assertNoHorizontalOverflow(page);
232+
await page.reload({ waitUntil: 'domcontentloaded' });
233+
await expect(page.getByText(longTag)).toBeVisible({ timeout: 10_000 });
234+
const withLongTags = await pageOverflow(page);
235+
236+
expect(
237+
withLongTags - baseline,
238+
`long tags widened the page by ${withLongTags - baseline}px (baseline ${baseline}px)`,
239+
).toBeLessThanOrEqual(1);
225240
});

frontend/e2e/utils.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,17 @@ export function assertNoPageErrors(errors: string[]) {
2626
/** No horizontal body overflow — the signature of the mobile-reflow regressions
2727
* (#288 banner, #576 drawer, edit-cover at 375px). */
2828
export async function assertNoHorizontalOverflow(page: Page) {
29-
const overflow = await page.evaluate(() => {
29+
const overflow = await pageOverflow(page);
30+
expect(overflow, 'page scrolls horizontally (mobile reflow regression)').toBeLessThanOrEqual(1);
31+
}
32+
33+
/** Horizontal overflow in px, for specs that need to compare one render against
34+
* another rather than assert the absolute property. Asserting "no overflow at
35+
* all" makes a spec fail for any unrelated overflow the page happens to carry,
36+
* which tells you nothing about the element under test. */
37+
export async function pageOverflow(page: Page): Promise<number> {
38+
return page.evaluate(() => {
3039
const el = document.documentElement;
3140
return el.scrollWidth - el.clientWidth;
3241
});
33-
expect(overflow, 'page scrolls horizontally (mobile reflow regression)').toBeLessThanOrEqual(1);
3442
}

0 commit comments

Comments
 (0)