Skip to content

Commit 72a76fd

Browse files
authored
Make the mobile-overflow failure name the element that overflows (#1177)
assertNoHorizontalOverflow said only "expected <= 1, received 35" — no element, no CSS. CI's book detail page carried that number for several releases and it was annotated "pre-existing" in PR after PR, with a real user-facing bug (#1170) sitting behind the label. On failure it now lists the offending elements deepest-first with the properties that cause this bug class, and falls back to margin-box and scrollWidth-vs-clientWidth probes when no border box crosses the edge — the case CI actually hits. It ran once and named the culprit: the detail page <h1> holding 48px more content than its box. Fixed in #1178. Children absorbed by a scrollable ancestor are excluded; the first draft listed catalog book cards as 139px past the edge on a page whose real overflow was 0. Pinned by e2e/overflow-diagnostics.spec.ts (7/7, desktop and mobile), including the pseudo-element case. A right margin was tested and does NOT extend scrollable overflow in Chromium, so that probe stays a secondary signal rather than an asserted behaviour. Tests only, no production code. The advisory E2E red on this branch is the bug it diagnosed, not a regression it introduced.
1 parent a46e9ca commit 72a76fd

2 files changed

Lines changed: 311 additions & 2 deletions

File tree

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { test, expect } from '@playwright/test';
2+
import { describeOverflowingElements, pageOverflow, assertNoHorizontalOverflow } from './utils';
3+
4+
/*
5+
* Pins the failure DIAGNOSTIC, not a product behaviour.
6+
*
7+
* `assertNoHorizontalOverflow` guards a whole class of mobile-reflow bugs, but
8+
* for a long time its failure said only "expected <= 1, received 35" — no element,
9+
* no CSS. CI's book detail page carried exactly that for several releases and the
10+
* failure was annotated "pre-existing" rather than fixed, because nothing in the
11+
* message told anyone where to look (`notes/e2e-mobile-overflow-ci-triage.md`).
12+
*
13+
* A diagnostic that silently stops naming the offender would put us straight back
14+
* there, and it would do so invisibly — every spec would still pass. Hence a test.
15+
*/
16+
17+
test('the overflow reporter names the offending element and its CSS', async ({ page }) => {
18+
await page.setViewportSize({ width: 390, height: 844 });
19+
await page.goto('/app');
20+
21+
// Baseline first: if the page under test already overflows, a passing assertion
22+
// below would prove nothing about the element we inject.
23+
expect(await pageOverflow(page)).toBeLessThanOrEqual(1);
24+
25+
await page.evaluate(() => {
26+
const el = document.createElement('div');
27+
el.setAttribute('data-testid', 'overflow-probe');
28+
el.textContent = 'probe';
29+
el.style.cssText =
30+
'position:absolute;top:0;left:0;width:480px;height:8px;white-space:nowrap';
31+
document.body.appendChild(el);
32+
});
33+
34+
const overflow = await pageOverflow(page);
35+
expect(overflow, 'the probe should push the document past 390px').toBeGreaterThan(1);
36+
37+
const report = await describeOverflowingElements(page);
38+
expect(report, 'the report must name the element by test id').toContain('overflow-probe');
39+
expect(report, 'the report must carry the CSS that causes this bug class').toContain(
40+
'white-space=nowrap',
41+
);
42+
expect(report, 'the report must say how far past the edge it lands').toMatch(/\d+px past/);
43+
44+
// And the assertion itself must surface that report, not just the number —
45+
// this is the path a real failing spec takes.
46+
const failure = await assertNoHorizontalOverflow(page).then(
47+
() => null,
48+
(e: Error) => e.message,
49+
);
50+
expect(failure, 'the assertion should have failed while the probe is present').toBeTruthy();
51+
expect(failure!).toContain('overflow-probe');
52+
53+
await page.evaluate(() => document.querySelector('[data-testid="overflow-probe"]')?.remove());
54+
await assertNoHorizontalOverflow(page);
55+
});
56+
57+
test('the overflow reporter explains overflow no border box accounts for', async ({ page }) => {
58+
await page.setViewportSize({ width: 390, height: 844 });
59+
await page.goto('/app');
60+
expect(await pageOverflow(page)).toBeLessThanOrEqual(1);
61+
62+
// A pseudo-element is the case that actually reproduces this shape, and it is a
63+
// strong candidate for CI's own 35px: `::after` has no node, so it is invisible
64+
// to querySelectorAll, yet it widens its parent all the same. Its parent then
65+
// reads `scrollWidth > clientWidth` while every border box stays inside the
66+
// viewport — which is exactly the state CI reports.
67+
//
68+
// Note a right MARGIN does not produce this in Chromium: it does not extend the
69+
// document's scrollable overflow, so it cannot be the cause on its own. The
70+
// reporter still measures margin boxes, but as a secondary signal.
71+
await page.evaluate(() => {
72+
const host = document.createElement('div');
73+
host.setAttribute('data-testid', 'pseudo-host');
74+
host.style.cssText = 'position:absolute;top:0;left:0;width:100px;height:10px';
75+
const style = document.createElement('style');
76+
style.setAttribute('data-testid', 'pseudo-style');
77+
style.textContent =
78+
'[data-testid="pseudo-host"]::after{content:"";display:block;width:900px;height:10px}';
79+
document.head.appendChild(style);
80+
document.body.appendChild(host);
81+
});
82+
83+
expect(
84+
await pageOverflow(page),
85+
'the pseudo-element should widen the document',
86+
).toBeGreaterThan(1);
87+
88+
const report = await describeOverflowingElements(page);
89+
expect(report, 'must say the border boxes are all inside the edge').toContain(
90+
'no element’s border box crosses the viewport edge',
91+
);
92+
expect(report, 'must localise the box holding the oversized content').toContain(
93+
'more content than its box',
94+
);
95+
expect(report, 'must name that box').toContain('pseudo-host');
96+
expect(report, 'must flag that the box carries a pseudo-element').toContain('pseudo=::after');
97+
98+
await page.evaluate(() => {
99+
document.querySelector('[data-testid="pseudo-host"]')?.remove();
100+
document.querySelector('[data-testid="pseudo-style"]')?.remove();
101+
});
102+
await assertNoHorizontalOverflow(page);
103+
});
104+
105+
test('the overflow reporter ignores children a scrollable ancestor absorbs', async ({ page }) => {
106+
await page.setViewportSize({ width: 390, height: 844 });
107+
await page.goto('/app');
108+
109+
// A carousel/shelf row holds children well past the viewport edge on purpose;
110+
// the scroll container absorbs them, so the document does NOT overflow. Naming
111+
// them in a failure report sends the reader after elements that are behaving —
112+
// which is exactly what this reporter did on its first draft, listing book-card
113+
// titles 139px "past" the edge on a page whose real overflow was 0.
114+
await page.evaluate(() => {
115+
const scroller = document.createElement('div');
116+
scroller.style.cssText = 'position:absolute;top:0;left:0;width:200px;overflow-x:auto';
117+
const wide = document.createElement('div');
118+
wide.setAttribute('data-testid', 'absorbed-probe');
119+
wide.style.cssText = 'width:900px;height:8px;white-space:nowrap';
120+
scroller.appendChild(wide);
121+
document.body.appendChild(scroller);
122+
});
123+
124+
expect(
125+
await pageOverflow(page),
126+
'a scrollable ancestor should absorb its children — the document must not overflow',
127+
).toBeLessThanOrEqual(1);
128+
expect(
129+
await describeOverflowingElements(page),
130+
'an absorbed child must not be reported as an offender',
131+
).not.toContain('absorbed-probe');
132+
133+
await page.evaluate(() =>
134+
document.querySelector('[data-testid="absorbed-probe"]')?.parentElement?.remove(),
135+
);
136+
});

frontend/e2e/utils.ts

Lines changed: 175 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,92 @@ export function assertNoPageErrors(errors: string[]) {
2424
}
2525

2626
/** No horizontal body overflow — the signature of the mobile-reflow regressions
27-
* (#288 banner, #576 drawer, edit-cover at 375px). */
27+
* (#288 banner, #576 drawer, edit-cover at 375px).
28+
*
29+
* On failure this names the elements that stick out past the viewport. Without
30+
* that, the failure reads "expected <= 1, received 35" and says nothing about
31+
* WHICH element is 35px wide of the edge — which is exactly why CI's detail-page
32+
* overflow sat behind a "pre-existing" label across several releases while nobody
33+
* could act on it (`notes/e2e-mobile-overflow-ci-triage.md`). The dump only runs
34+
* when the assertion is about to fail, so it costs a passing run nothing. */
2835
export async function assertNoHorizontalOverflow(page: Page) {
2936
const overflow = await pageOverflow(page);
30-
expect(overflow, 'page scrolls horizontally (mobile reflow regression)').toBeLessThanOrEqual(1);
37+
const detail = overflow > 1 ? `\n${await describeOverflowingElements(page)}` : '';
38+
expect(
39+
overflow,
40+
`page scrolls horizontally (mobile reflow regression)${detail}`,
41+
).toBeLessThanOrEqual(1);
42+
}
43+
44+
/** The elements extending past the viewport's right edge, deepest first.
45+
*
46+
* Deepest-first matters: an overflowing leaf drags every ancestor out with it, so
47+
* the shallow entries are consequences and the deep ones are the cause. Reports
48+
* the computed properties that actually produce this class of bug (`white-space`,
49+
* `max-width`, `overflow-wrap`, `min-width`) so the fix is usually readable
50+
* straight off the failure message.
51+
*
52+
* Elements clipped by a scrollable ancestor are EXCLUDED. A carousel or shelf row
53+
* with `overflow-x: auto` legitimately holds children past the viewport edge, and
54+
* their excess never reaches `documentElement.scrollWidth` — the catalog grid
55+
* reports dozens of them while the page overflow is 0. Listing those would point
56+
* the reader at innocent elements, which is worse than printing nothing. */
57+
export async function describeOverflowingElements(page: Page, limit = 8): Promise<string> {
58+
const rows = await page.evaluate((max) => {
59+
const viewport = document.documentElement.clientWidth;
60+
const out: Array<Record<string, string | number>> = [];
61+
/** True when some ancestor clips/scrolls horizontally, absorbing the excess. */
62+
const isAbsorbed = (el: HTMLElement) => {
63+
for (let p = el.parentElement; p && p !== document.documentElement; p = p.parentElement) {
64+
const ox = getComputedStyle(p).overflowX;
65+
if (ox === 'auto' || ox === 'scroll' || ox === 'hidden' || ox === 'clip') return true;
66+
}
67+
return false;
68+
};
69+
document.querySelectorAll<HTMLElement>('*').forEach((el) => {
70+
const r = el.getBoundingClientRect();
71+
if (r.width === 0 && r.height === 0) return;
72+
if (r.right <= viewport + 1) return;
73+
if (isAbsorbed(el)) return;
74+
const cs = getComputedStyle(el);
75+
let depth = 0;
76+
for (let p: HTMLElement | null = el; p?.parentElement; p = p.parentElement) depth += 1;
77+
out.push({
78+
depth,
79+
tag: el.tagName.toLowerCase(),
80+
cls: String((el as unknown as { className?: string }).className ?? '').slice(0, 60),
81+
testid: el.getAttribute('data-testid') ?? '',
82+
text: (el.textContent ?? '').trim().replace(/\s+/g, ' ').slice(0, 40),
83+
past: Math.round(r.right - viewport),
84+
width: Math.round(r.width),
85+
whiteSpace: cs.whiteSpace,
86+
maxWidth: cs.maxWidth,
87+
overflowWrap: cs.overflowWrap,
88+
minWidth: cs.minWidth,
89+
});
90+
});
91+
out.sort((a, b) => (b.depth as number) - (a.depth as number));
92+
return out.slice(0, max);
93+
}, limit);
94+
95+
if (rows.length === 0) {
96+
// A border box can sit inside the viewport and still push the document wider:
97+
// scrollable overflow counts a child's MARGIN box, and a pseudo-element has no
98+
// node for the sweep above to find. CI's long-standing 35px detail-page
99+
// overflow lands here — every element is within the edge — so an empty list
100+
// would be the least useful thing to print. Fall back to the two measurements
101+
// that do explain this case.
102+
return describeIndirectOverflow(page, limit);
103+
}
104+
return rows
105+
.map(
106+
(r) =>
107+
` ${r.past}px past @ depth ${r.depth}: <${r.tag}${r.testid ? ` data-testid="${r.testid}"` : ''} class="${r.cls}">` +
108+
` w=${r.width} white-space=${r.whiteSpace} max-width=${r.maxWidth}` +
109+
` overflow-wrap=${r.overflowWrap} min-width=${r.minWidth}` +
110+
(r.text ? `\n text: ${JSON.stringify(r.text)}` : ''),
111+
)
112+
.join('\n');
31113
}
32114

33115
/** Horizontal overflow in px, for specs that need to compare one render against
@@ -40,3 +122,94 @@ export async function pageOverflow(page: Page): Promise<number> {
40122
return el.scrollWidth - el.clientWidth;
41123
});
42124
}
125+
126+
/** Explains overflow that no element's border box accounts for.
127+
*
128+
* Two causes produce a wider document while every rect stays inside the viewport:
129+
*
130+
* 1. A **margin**. Scrollable overflow includes a child's margin box, so an
131+
* element ending at 388px with `margin-right: 37px` widens the document to
132+
* 425px while its own `right` is comfortably within the edge.
133+
* 2. Content wider than its own box — a **pseudo-element**, a transform, or a
134+
* min-width floor — on an element that is not a scroll container. Such an
135+
* element has `scrollWidth > clientWidth` while its rect stays put, and it
136+
* hands the excess up to its parent.
137+
*
138+
* Reporting the containers in (2) deepest-first localises the cause to one box
139+
* even when the thing inside it has no node of its own. */
140+
async function describeIndirectOverflow(page: Page, limit: number): Promise<string> {
141+
const found = await page.evaluate((max) => {
142+
const viewport = document.documentElement.clientWidth;
143+
const depthOf = (el: HTMLElement) => {
144+
let d = 0;
145+
for (let p: HTMLElement | null = el; p?.parentElement; p = p.parentElement) d += 1;
146+
return d;
147+
};
148+
const scrolls = (el: HTMLElement) => {
149+
const ox = getComputedStyle(el).overflowX;
150+
return ox === 'auto' || ox === 'scroll' || ox === 'hidden' || ox === 'clip';
151+
};
152+
const absorbed = (el: HTMLElement) => {
153+
for (let p = el.parentElement; p && p !== document.documentElement; p = p.parentElement) {
154+
if (scrolls(p)) return true;
155+
}
156+
return false;
157+
};
158+
159+
const margins: string[] = [];
160+
const inner: string[] = [];
161+
162+
document.querySelectorAll<HTMLElement>('*').forEach((el) => {
163+
const r = el.getBoundingClientRect();
164+
if (r.width === 0 && r.height === 0) return;
165+
if (absorbed(el)) return;
166+
const cs = getComputedStyle(el);
167+
const label =
168+
`<${el.tagName.toLowerCase()}` +
169+
`${el.getAttribute('data-testid') ? ` data-testid="${el.getAttribute('data-testid')}"` : ''}` +
170+
` class="${String((el as unknown as { className?: string }).className ?? '').slice(0, 60)}">`;
171+
172+
const marginRight = parseFloat(cs.marginRight) || 0;
173+
if (marginRight > 0 && r.right + marginRight > viewport + 1) {
174+
margins.push(
175+
` ${Math.round(r.right + marginRight - viewport)}px past (margin box) @ depth ${depthOf(el)}: ${label}` +
176+
` right=${Math.round(r.right)} margin-right=${cs.marginRight}`,
177+
);
178+
}
179+
180+
const excess = el.scrollWidth - el.clientWidth;
181+
if (excess > 1 && !scrolls(el)) {
182+
const before = getComputedStyle(el, '::before').content;
183+
const after = getComputedStyle(el, '::after').content;
184+
const pseudo = [
185+
before && before !== 'none' ? '::before' : '',
186+
after && after !== 'none' ? '::after' : '',
187+
]
188+
.filter(Boolean)
189+
.join('+');
190+
inner.push(
191+
` holds ${excess}px more content than its box @ depth ${depthOf(el)}: ${label}` +
192+
` client=${el.clientWidth} scroll=${el.scrollWidth} min-width=${cs.minWidth}` +
193+
` transform=${cs.transform === 'none' ? 'none' : 'yes'}${pseudo ? ` pseudo=${pseudo}` : ''}`,
194+
);
195+
}
196+
});
197+
198+
const byDepthDesc = (a: string, b: string) =>
199+
Number(/@ depth (\d+)/.exec(b)?.[1] ?? 0) - Number(/@ depth (\d+)/.exec(a)?.[1] ?? 0);
200+
return {
201+
margins: margins.sort(byDepthDesc).slice(0, max),
202+
inner: inner.sort(byDepthDesc).slice(0, max),
203+
};
204+
}, limit);
205+
206+
const parts: string[] = [
207+
' (no element’s border box crosses the viewport edge — the width comes from a margin, a pseudo-element, a transform, or a min-width floor)',
208+
];
209+
if (found.margins.length) parts.push(' margin boxes past the edge:', ...found.margins);
210+
if (found.inner.length) parts.push(' boxes whose content is wider than they are:', ...found.inner);
211+
if (!found.margins.length && !found.inner.length) {
212+
parts.push(' nothing matched either probe — inspect documentElement/body padding directly');
213+
}
214+
return parts.join('\n');
215+
}

0 commit comments

Comments
 (0)