Skip to content

Commit ae80915

Browse files
authored
fix(render): measure page height from content, not the body box (#131)
pixelshot clamped the page height to `body.getBoundingClientRect().bottom`, which only bounds the document when the page lets the body size to its content. Sites that pin it to the viewport — `html, body { height: 100% }`, as Wikipedia's Vector 2022 skin does — leave the content overflowing a one-viewport box, so a 19,951px article measured 1,568px and captured a single tile with `complete: true` (issue #124). The rect was also read as a document coordinate while being viewport-relative. A URL with a `#fragment` loads already scrolled, where the bottom edge is negative and `Math.max(bottom, 1)` floors the whole page to 1px. Measure the lowest edge among the body and its element children instead, and add the scroll offset back. That reads the same as before on a self-sizing body, so the clamp still drops the blank tail an inflated `documentElement.scrollHeight` would otherwise buy (root padding, trailing margin), while surviving a pinned body. Both capture paths carried their own copy of the probe; the snippet now lives in one module so they can't drift apart. Verified in stock Chrome, standard and turbo probes agreeing on every page: page before after scrollHeight wikipedia (#124) 1568 19951 19951 wikipedia #fragment 1 19951 19951 simonwillison.net 18252 18252 18252 ourworldindata.org 8713 8713 8713 Closes #124
1 parent 7a3c304 commit ae80915

4 files changed

Lines changed: 92 additions & 5 deletions

File tree

render/src/pixelrag_render/backends/cdp.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535

3636
from PIL import Image
3737

38+
from .page_metrics import CONTENT_BOTTOM_JS
39+
3840
logger = logging.getLogger("pixelrag_render.backends.cdp")
3941

4042
VIEWPORT_W = 875
@@ -344,6 +346,7 @@ def _readiness_expr() -> str:
344346
Returns an async-IIFE expression resolving to the page height to tile.
345347
"""
346348
return f"""(async () => {{
349+
{CONTENT_BOTTOM_JS}
347350
await new Promise(res => {{
348351
if (document.readyState === 'complete') return res();
349352
const t = setTimeout(res, {LOAD_TIMEOUT_MS});
@@ -361,8 +364,7 @@ def _readiness_expr() -> str:
361364
const sh = document.documentElement.scrollHeight;
362365
const body = document.body;
363366
if (body) {{
364-
const bottom = Math.ceil(body.getBoundingClientRect().bottom);
365-
return Math.min(sh, Math.max(bottom, 1));
367+
return Math.min(sh, Math.max(contentBottom(body), 1));
366368
}}
367369
return sh;
368370
}})()"""

render/src/pixelrag_render/backends/fast_cdp.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
import urllib.request
3535
from pathlib import Path
3636

37+
from .page_metrics import CONTENT_BOTTOM_JS
38+
3739
logger = logging.getLogger("pixelrag_render.backends.fast_cdp")
3840

3941
VIEWPORT_WIDTH = 875
@@ -58,8 +60,12 @@
5860
"--disable-features=Translate,MediaRouter,OptimizationHints",
5961
]
6062

61-
# JS: wait for fonts + eager images, then return scrollHeight
62-
_WAIT_FONTS_IMGS = """new Promise(resolve => {
63+
# JS: wait for fonts + eager images, then return the page height to tile
64+
_WAIT_FONTS_IMGS = (
65+
"""new Promise(resolve => {
66+
"""
67+
+ CONTENT_BOTTOM_JS
68+
+ """
6369
const waitEagerImgs = Promise.all(
6470
Array.from(document.images)
6571
.filter(i => !i.complete && i.loading !== 'lazy')
@@ -79,12 +85,13 @@
7985
const sh = document.documentElement.scrollHeight;
8086
const body = document.body;
8187
resolve(body
82-
? Math.min(sh, Math.max(Math.ceil(body.getBoundingClientRect().bottom), 1))
88+
? Math.min(sh, Math.max(contentBottom(body), 1))
8389
: sh);
8490
});
8591
});
8692
});
8793
})"""
94+
)
8895

8996

9097
# ---------------------------------------------------------------------------
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
"""Shared in-page measurement JS for the capture backends.
2+
3+
Kept in one place because both backends measure page height the same way and
4+
must agree: the standard (``cdp``) and turbo (``fast_cdp``) paths each embed
5+
this snippet, and a divergence between them silently changes how much of a page
6+
gets captured depending on which Chrome binary is installed.
7+
8+
No imports — a plain string constant, so either backend can use it without a
9+
dependency edge between them.
10+
"""
11+
12+
# How tall is the document's *content*?
13+
#
14+
# `documentElement.scrollHeight` alone over-reports: padding on the root element
15+
# or a trailing margin inflates it, buying a run of blank tiles at the bottom of
16+
# every such page. So it is clamped to where the content actually ends.
17+
#
18+
# That bound has to be measured from the content, not from the body box. A body
19+
# is only as tall as the document when the page lets it size to its content;
20+
# sites that pin it to the viewport (`html, body { height: 100% }` — Wikipedia's
21+
# Vector 2022 skin among them) leave the content overflowing a one-viewport box,
22+
# and clamping to that box truncates a 20,000px article to a single tile
23+
# (issue #124). Taking the lowest edge among the body and its element children
24+
# reads the same on a self-sizing body and survives a pinned one.
25+
#
26+
# Coordinates are viewport-relative, so scroll offset is added back: a page
27+
# navigated to a `#fragment` lands scrolled down, where a raw rect bottom would
28+
# under-report by exactly the scrolled distance.
29+
CONTENT_BOTTOM_JS = """
30+
function contentBottom(body) {
31+
const offset = window.scrollY || window.pageYOffset || 0;
32+
let bottom = body.getBoundingClientRect().bottom;
33+
for (let el = body.firstElementChild; el; el = el.nextElementSibling) {
34+
const r = el.getBoundingClientRect();
35+
// Skip elements with no box at all (display:none, empty <script>);
36+
// theirs is a zero rect at the origin and would not move `bottom`,
37+
// but skipping keeps the intent explicit.
38+
if (r.width > 0 || r.height > 0) {
39+
bottom = Math.max(bottom, r.bottom);
40+
}
41+
}
42+
return Math.ceil(bottom + offset);
43+
}
44+
"""

tests/test_render.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
to end rather than mocked.
77
"""
88

9+
import json
910
from pathlib import Path
1011

1112
from pixelrag_render import render_file
@@ -29,3 +30,36 @@ def test_render_local_html_to_tiles(tmp_path):
2930
f"no tile images produced in {tile_dir} "
3031
f"(contents: {[p.name for p in tile_dir.iterdir()]})"
3132
)
33+
34+
35+
def test_page_taller_than_a_viewport_bounded_body_is_fully_tiled(tmp_path):
36+
"""A page whose body is pinned to the viewport must still tile in full.
37+
38+
Regression for issue #124. Sites that set ``html, body { height: 100% }``
39+
(Wikipedia's Vector 2022 skin among them) leave the article content
40+
overflowing the body box visibly, so ``body.getBoundingClientRect()`` is one
41+
viewport tall on a 20,000px page. The readiness probe used to clamp the page
42+
height to that rect, capturing a single tile and reporting the viewport as
43+
the page height.
44+
"""
45+
body = "".join(f"<p>line {i:03d}</p>" for i in range(400))
46+
html = tmp_path / "viewport_bounded_body.html"
47+
html.write_text(
48+
'<!DOCTYPE html><html style="height:100%"><body style="height:100%">'
49+
f"{body}</body></html>"
50+
)
51+
out = tmp_path / "tiles"
52+
53+
dirs = render_file(html, out, tile_height=1000, viewport_width=1280)
54+
55+
tile_dir = Path(dirs[0])
56+
manifest = json.loads((tile_dir / "tiles.json").read_text())
57+
tiles = sorted(tile_dir.glob("tile_*.jpg"))
58+
59+
# 400 paragraphs are several viewports tall whatever the default font is;
60+
# assert against the viewport rather than a brittle exact pixel count.
61+
assert manifest["page_height"] > 3000, (
62+
f"page_height {manifest['page_height']} is about one viewport — the "
63+
"content below the fold was never measured"
64+
)
65+
assert len(tiles) > 1, f"expected multiple tiles, got {[t.name for t in tiles]}"

0 commit comments

Comments
 (0)