add_child invalidated the canvas but left _draw bound to it, and the draw property only rebuilt when _draw was None. Callers therefore got a context pointing at a discarded image while page._canvas stayed None. table_layouter reads page._canvas directly, so every image inside a table cell laid out after any other content silently degraded to a grey [Image: WxH] placeholder. The property now rebuilds when either half is missing. On its own that would make layout allocate a full-page RGBA canvas per line, because layout measures text through the page - so measurement moves to page.measurement_draw, a 1x1 scratch context that is never invalidated. Its mode matches the render canvas so that Text's width cache does not hold two entries per word. Children built against the scratch context are re-bound to the live canvas by render_children, which already synchronised _draw and _canvas; that behaviour was incidental and is now load-bearing and documented as such. Regenerating the examples shows table images rendering as images rather than placeholders. The empty header row in the second table of example 05 is unrelated and pre-existing - row height ignores cell padding, so text is clipped as padding grows - recorded as evidence under S6.
1200 lines
51 KiB
Markdown
1200 lines
51 KiB
Markdown
# Layout Remediation Spec
|
||
|
||
Remediation plan for the defects found in the block/table rendering path
|
||
(audit of 2026-08-06), plus two outstanding defects in the pagination and
|
||
background-rendering paths (S11, S12). Twelve specs, sequenced into five phases.
|
||
|
||
Every problem statement below was reproduced against the code at `2a543d0`; the
|
||
reproductions are quoted verbatim so each spec has a falsifiable "before" state.
|
||
|
||
**S11 is the most user-visible defect in this document** and the cheapest to
|
||
fix: a single paragraph larger than one page dead-ends the reader permanently.
|
||
It is independent of every other spec here.
|
||
|
||
## Contents
|
||
|
||
| ID | Spec | Phase |
|
||
|----|------|-------|
|
||
| [S1](#s1--inline-content-in-non-paragraph-containers) | Inline content in non-paragraph containers | 0 |
|
||
| [S2](#s2--page-geometry-origin-and-content-rect) | Page geometry: origin and content rect | 1 |
|
||
| [S3](#s3--drawcanvas-lifecycle) | Draw/canvas lifecycle | 1 |
|
||
| [S4](#s4--one-block-dispatch-one-measurement) | One block dispatch, one measurement | 2 |
|
||
| [S5](#s5--cells-as-sub-layouts) | Cells as sub-layouts | 2 |
|
||
| [S6](#s6--table-grid-model) | Table grid model | 3 |
|
||
| [S7](#s7--retained-mode-table-rendering) | Retained-mode table rendering | 3 |
|
||
| [S8](#s8--table-and-list-pagination) | Table and list pagination | 4 |
|
||
| [S9](#s9--interactivity-inside-tables) | Interactivity inside tables | 4 |
|
||
| [S10](#s10--contracts-and-hygiene) | Contracts and hygiene | 5 |
|
||
| [S11](#s11--partial-block-progress-is-discarded) | Partial-block progress is discarded | 0 |
|
||
| [S12](#s12--background-rendering) | Background rendering | 4 |
|
||
| [S13](#s13--word-spacing-and-alignment) | Word spacing and alignment | 0 |
|
||
|
||
## Design invariants
|
||
|
||
These are the rules the specs exist to establish. After remediation, a reviewer
|
||
should be able to reject a change by pointing at one of these.
|
||
|
||
1. **One dispatch.** There is exactly one function that maps an abstract block to
|
||
concrete objects on a page. Tables, the document layouter and the ereader all
|
||
call it. No component re-implements paragraph layout.
|
||
2. **Retained mode everywhere.** Layout produces children; `Page.render()` draws
|
||
them. Nothing paints during layout. A page can be rendered any number of times
|
||
and produce identical output.
|
||
3. **Measure and render agree by construction.** Reported height comes from the
|
||
same objects that get drawn, never from a parallel estimator.
|
||
4. **A cell is a page.** Any block the layout engine can place on a page can be
|
||
placed in a table cell, a list item or a blockquote, with no per-container
|
||
type switch.
|
||
5. **Style flows from the document.** No layout component invents a font, a size
|
||
or a path. Absolute font paths never appear outside `style/fonts.py`.
|
||
|
||
## Phasing
|
||
|
||
```
|
||
S11 (independent, ship today)
|
||
S1 (independent, ship first)
|
||
S12 (independent; decide delete-vs-fix before touching S8)
|
||
S2 ─┬─ S4 ── S5 ─┬─ S6 ── S7 ─┬─ S8
|
||
S3 ─┘ │ └─ S9
|
||
└──────────────── S10 (opportunistic, any time after S7)
|
||
```
|
||
|
||
S11 is a few lines and unblocks reading the affected books; it ships on its own,
|
||
immediately. S1 is independent of everything else and fixes silent content loss,
|
||
so it ships next regardless of appetite for the rest. S12 is independent but
|
||
should be decided before S8, since table pagination changes the cost model that
|
||
justifies background rendering at all. S2+S3 are small and unlock S4/S5, which
|
||
are the bulk of the work. S6/S7 are where the table actually becomes correct.
|
||
S8/S9 are integration. S10 is cleanup.
|
||
|
||
---
|
||
|
||
## S1 — Inline content in non-paragraph containers
|
||
|
||
### Problem
|
||
|
||
Inline tags (`a`, `b`, `strong`, `em`, `span`, `code`, …) are registered to
|
||
[`ignore_handler`](../pyWebLayout/io/readers/html_extraction.py#L825-L828)
|
||
because they are meant to be consumed by
|
||
[`extract_text_content()`](../pyWebLayout/io/readers/html_extraction.py#L364-L466).
|
||
But only `paragraph_handler` and `heading_handler` ever call that function.
|
||
`div_handler`, `list_item_handler`, `table_cell_handler` and
|
||
`table_header_cell_handler` iterate children and call `process_element` per
|
||
child, so inline tags return `None` and their text is discarded. `div_handler`
|
||
additionally ignores `NavigableString` children outright.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
<p>hello <b>world</b> again</p> -> Paragraph ['hello', 'world', 'again'] ok
|
||
<div>hello <b>world</b> again</div> -> (no blocks at all) lost
|
||
<li>hello <b>world</b> again</li> -> HList with no words lost
|
||
<td>hello <b>world</b> again</td> -> Table with no words lost
|
||
<td><a href="u">link</a> text</td> -> Paragraph ['text'] link lost
|
||
```
|
||
|
||
Bare text nodes that *do* survive (in `td`) each become their own `Paragraph`,
|
||
so `a <b>b</b> c` would fragment onto three lines even once `<b>` is handled.
|
||
|
||
### Design
|
||
|
||
Introduce a single helper that every block container uses to process its
|
||
children, replacing the four hand-rolled loops. It walks children in order,
|
||
accumulating consecutive *inline* children (tags and text) into a run, and
|
||
flushing that run into one `Paragraph` whenever a *block* child interrupts it or
|
||
the children are exhausted.
|
||
|
||
Inline-ness is decided by one predicate, not by each caller:
|
||
|
||
```python
|
||
# html_extraction.py
|
||
|
||
INLINE_TAGS: FrozenSet[str] = frozenset({
|
||
"a", "span", "strong", "b", "em", "i", "u", "s", "del", "ins", "mark",
|
||
"small", "sub", "sup", "code", "q", "cite", "abbr", "time", "br",
|
||
})
|
||
|
||
def is_inline(node) -> bool:
|
||
"""True for text nodes and inline tags; False for block tags."""
|
||
|
||
def process_block_children(element: Tag, context: StyleContext) -> List[Block]:
|
||
"""
|
||
Process an element's children into a block list, coalescing runs of inline
|
||
content into paragraphs.
|
||
|
||
This is the single entry point for any container that may hold a mix of
|
||
inline and block content: div, li, td, th, blockquote, figure, section...
|
||
"""
|
||
```
|
||
|
||
`process_block_children` delegates each inline run to the existing
|
||
`extract_text_content`, which already handles `<a>` → `LinkedWord`, style
|
||
nesting and background inheritance — it is called on a synthetic run rather
|
||
than on the whole element. The simplest correct implementation wraps the run's
|
||
nodes in a detached `Tag` and calls `extract_text_content` on it; that keeps
|
||
one code path for inline styling.
|
||
|
||
`<br>` inside a run terminates the current paragraph and starts a new one,
|
||
replacing the current no-op [`line_break_handler`](../pyWebLayout/io/readers/html_extraction.py#L791-L794).
|
||
|
||
Callers become one line each:
|
||
|
||
```python
|
||
def div_handler(element, context): return process_block_children(element, context)
|
||
def list_item_handler(element, context): item = ListItem(...); item._blocks = process_block_children(...)
|
||
def table_cell_handler(element, context): cell = TableCell(...); for b in process_block_children(...): cell.add_block(b)
|
||
```
|
||
|
||
`paragraph_handler`/`heading_handler` keep their current behaviour (they are
|
||
already correct); their image-splitting logic in
|
||
[html_extraction.py:492-552](../pyWebLayout/io/readers/html_extraction.py#L492-L552)
|
||
is subsumed by `process_block_children` and should be folded in — an `<img>` is
|
||
a block child, so the "text before, image after" split falls out of the general
|
||
algorithm for free.
|
||
|
||
### Acceptance criteria
|
||
|
||
- All four markup samples in *Evidence* produce the same words as the `<p>`
|
||
control, in document order, with `LinkedWord` preserved for `<a href>`.
|
||
- `<div>a <b>b</b> c</div>` yields **one** `Paragraph` of three words, not three
|
||
paragraphs.
|
||
- `<td>text<p>para</p>more</td>` yields three blocks in order: Paragraph, Paragraph,
|
||
Paragraph — inline runs on either side of a block child are not merged.
|
||
- `<td>a<br>b</td>` yields two paragraphs.
|
||
- `<p>text<img src=x>more</p>` behaves as it does today (regression).
|
||
- Round-trip test over `tests/io_tests/` fixtures shows no block-count or
|
||
word-count regressions.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/io/readers/html_extraction.py`, `tests/io_tests/test_html_extraction.py`
|
||
|
||
### Risk
|
||
|
||
Low, and contained to the reader. The change *adds* content where there was
|
||
none, so existing assertions on parsed structure may need their expected counts
|
||
raised. Grep for tests asserting `len(blocks) == N` before starting.
|
||
|
||
---
|
||
|
||
## S2 — Page geometry: origin and content rect
|
||
|
||
### Problem
|
||
|
||
Two issues, one cause: `Page` assumes it is rooted at (0,0) and has no notion of
|
||
a content rectangle.
|
||
|
||
1. **Horizontal padding is ignored.** `paragraph_layouter` sets
|
||
`x_cursor = page.border_size` ([document_layouter.py:154](../pyWebLayout/layout/document_layouter.py#L154))
|
||
while line width is `page.available_width` (which *does* subtract both
|
||
paddings). With `border_width=2, padding=(40,40,40,40)` on a 400px page, the
|
||
line lands at `origin.x = 2, width = 316`: text hugs the border and the whole
|
||
80px padding budget accumulates on the right. Vertical padding is honoured
|
||
([page.py:34](../pyWebLayout/concrete/page.py#L34)), so the asymmetry is
|
||
horizontal only.
|
||
2. **A page cannot be placed inside another page**, which is the prerequisite for
|
||
S5 (cells as sub-layouts).
|
||
|
||
### Design
|
||
|
||
Give `Page` an origin and derive a content rect from it. Default `(0, 0)` keeps
|
||
every existing caller behaviourally identical apart from the padding fix.
|
||
|
||
```python
|
||
class Page(Renderable, Queriable):
|
||
def __init__(self, size, style=None, origin: Tuple[int, int] = (0, 0)): ...
|
||
|
||
@property
|
||
def origin(self) -> np.ndarray:
|
||
"""Absolute top-left of the page box."""
|
||
|
||
@property
|
||
def content_origin(self) -> Tuple[int, int]:
|
||
"""Absolute top-left of the content box (origin + border + padding)."""
|
||
|
||
@property
|
||
def content_rect(self) -> Tuple[int, int, int, int]:
|
||
"""(x, y, w, h) of the content box in absolute coordinates."""
|
||
|
||
@property
|
||
def remaining_height(self) -> int:
|
||
"""Content-box height still available below _current_y_offset."""
|
||
```
|
||
|
||
`remaining_height` replaces the ad-hoc
|
||
`page.size[1] - page._current_y_offset - page.border_size` computed inline by
|
||
`image_layouter`, `table_layouter`, `button_layouter` and `form_layouter` — all
|
||
four of which subtract the border but not the bottom padding, so every block
|
||
type may currently be placed up to `padding_bottom` past its boundary.
|
||
|
||
The existing [`free_space()`](../pyWebLayout/concrete/page.py#L41-L43) has the
|
||
same defect (it returns full page width, and height ignoring bottom border and
|
||
padding) and has no callers; delete it in favour of `content_rect` /
|
||
`remaining_height`.
|
||
|
||
All layouters switch from `page.border_size` to `page.content_origin[0]` for the
|
||
x cursor, and `_current_y_offset` is initialised to `content_origin[1]`.
|
||
`can_fit_line`'s `max_y` becomes `content_rect.y + content_rect.h`.
|
||
|
||
`_current_y_offset` stays absolute, so no arithmetic elsewhere changes sign.
|
||
|
||
### Acceptance criteria
|
||
|
||
- With `size=(400,300), border_width=2, padding=(40,40,40,40)`: the first line
|
||
has `origin == (42, 42)` and `size[0] == 316`; rendered ink starts at
|
||
x ≥ 42 and ends at x ≤ 358.
|
||
- No block is placed within `padding_bottom` of the page bottom, for every block
|
||
type (paragraph, image, table, button, form).
|
||
- A `Page(size=(100,50), origin=(200,300))` places its first line at
|
||
`(200 + border + padding_left, 300 + border + padding_top)`.
|
||
- Existing rendering tests with default `padding=(20,20,20,20)` shift right by
|
||
20px — golden images in `docs/images/` and `test_output/` must be regenerated
|
||
and eyeballed once, deliberately, as part of this spec.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/page.py`, `pyWebLayout/concrete/dynamic_page.py`,
|
||
`pyWebLayout/layout/document_layouter.py`
|
||
|
||
### Risk
|
||
|
||
Medium — it moves every existing rendering by `padding_left`. That is the point,
|
||
but it invalidates every golden image at once. Do it in its own commit, separate
|
||
from anything else, so the diff of regenerated images is reviewable.
|
||
|
||
---
|
||
|
||
## S3 — Draw/canvas lifecycle
|
||
|
||
### Problem
|
||
|
||
[`Page.add_child`](../pyWebLayout/concrete/page.py#L140-L154) sets
|
||
`self._canvas = None` but leaves `self._draw` bound to the discarded canvas, and
|
||
the [`draw` property](../pyWebLayout/concrete/page.py#L131-L138) only rebuilds
|
||
when `_draw is None`. So after the first `add_child`, `page.draw` hands out a
|
||
draw context pointing at an orphaned image while `page._canvas` stays `None`.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
after first .draw: _canvas set? True
|
||
after add_child: _canvas set? False _draw is stale? True
|
||
page.draw returns same stale object? True page._canvas still None? True
|
||
=> a table laid out now receives canvas = None
|
||
```
|
||
|
||
Downstream: `table_layouter` passes `canvas=None` into `TableRenderer`, so every
|
||
image inside a cell silently degrades to a grey `[Image: WxH]` placeholder
|
||
([table.py:297-320](../pyWebLayout/concrete/table.py#L297-L320)).
|
||
|
||
Note the trap: naively fixing the property so it rebuilds whenever
|
||
`_canvas is None` makes layout allocate a full-page RGBA canvas on *every*
|
||
`add_child`, because layout calls `page.draw` to measure text. The fix has to
|
||
separate the two uses.
|
||
|
||
### Design
|
||
|
||
Split measurement from rendering.
|
||
|
||
```python
|
||
class Page:
|
||
@property
|
||
def measurement_draw(self) -> ImageDraw.ImageDraw:
|
||
"""
|
||
Persistent 1x1 scratch draw context used for text metrics during layout.
|
||
Never invalidated; matches the render canvas's mode so that
|
||
Text width caching keys stay consistent.
|
||
"""
|
||
|
||
@property
|
||
def draw(self) -> ImageDraw.ImageDraw:
|
||
"""Draw context bound to the live render canvas, rebuilt if invalidated."""
|
||
```
|
||
|
||
- Layouters construct `Line`/`Text` with `page.measurement_draw`.
|
||
- `Page.render_children` already rebinds `child._draw = self._draw` and
|
||
`child._canvas = self._canvas` before rendering
|
||
([page.py:256-269](../pyWebLayout/concrete/page.py#L256-L269)), so children
|
||
built against the scratch context draw onto the real canvas at render time.
|
||
That rebinding becomes load-bearing rather than incidental — document it as
|
||
such, and extend it to recurse into child pages (S5).
|
||
- `draw` rebuilds when `self._draw is None or self._canvas is None`.
|
||
|
||
Because `Text._calculate_dimensions` keys its width cache on `self._draw.mode`
|
||
([text.py](../pyWebLayout/concrete/text.py)), the scratch image must be created
|
||
in the same mode as the render canvas (`RGBA`) or the cache will hold two
|
||
entries per word.
|
||
|
||
### Acceptance criteria
|
||
|
||
- Laying out 100 paragraphs allocates **zero** full-page canvases (assert via a
|
||
counter patched onto `_create_canvas`).
|
||
- After any sequence of `add_child` calls, `page.draw.im` is the same image
|
||
object as `page._canvas`.
|
||
- A table containing an image, laid out after a paragraph, renders the real
|
||
image and not the placeholder.
|
||
- `page.render()` called twice returns pixel-identical images.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/page.py`, `pyWebLayout/layout/document_layouter.py`
|
||
|
||
---
|
||
|
||
## S4 — One block dispatch, one measurement
|
||
|
||
### Problem
|
||
|
||
There are three implementations of "lay out content in a box", which must agree
|
||
and do not:
|
||
|
||
| Purpose | Location | Word spacing | Hyphenation |
|
||
|---|---|---|---|
|
||
| Rendering | [`_render_cell_content`](../pyWebLayout/concrete/table.py#L110-L242) | `0.25–0.5 × font_size` | real, via `Line.add_word` |
|
||
| Height | [`_estimate_wrapped_lines`](../pyWebLayout/concrete/table.py#L578-L639) | `0.25 × font_size` | "assume one line" |
|
||
| Width | [`layout_cell_content`](../pyWebLayout/layout/table_optimizer.py#L117-L186) | hardcoded `(3, 6)` | none |
|
||
|
||
`layout_cell_content` additionally appends to `line._text_objects` directly,
|
||
bypassing the fitting logic it is trying to predict.
|
||
|
||
Separately, block *dispatch* is duplicated three times:
|
||
[`DocumentLayouter.layout_document`](../pyWebLayout/layout/document_layouter.py#L683-L723),
|
||
[`EreaderLayout._layout_block_on_page`](../pyWebLayout/layout/ereader_layout.py#L493-L519),
|
||
and the `isinstance` chain in `_render_cell_content`. They support different
|
||
block types, which is why tables render paragraphs but not lists, and the
|
||
ereader renders neither.
|
||
|
||
### Design
|
||
|
||
Two new public functions, one module each.
|
||
|
||
**Dispatch** — `pyWebLayout/layout/block_layouter.py`:
|
||
|
||
```python
|
||
@dataclass
|
||
class LayoutResult:
|
||
complete: bool # block fully placed
|
||
next_word: Optional[int] = None # resume index if not complete
|
||
pretext: Optional[Text] = None # hyphenated remainder
|
||
next_item: Optional[int] = None # resume index for lists
|
||
next_row: Optional[int] = None # resume index for tables
|
||
|
||
def layout_block(block: Block,
|
||
page: Page,
|
||
start_word: int = 0,
|
||
pretext: Optional[Text] = None,
|
||
**resume) -> LayoutResult:
|
||
"""
|
||
Place one abstract block onto a page, starting from a resume point.
|
||
The single dispatch point for Paragraph, Heading, Image, Table, HList,
|
||
Quote, CodeBlock, HorizontalRule, PageBreak, Button, Form.
|
||
"""
|
||
```
|
||
|
||
The existing `paragraph_layouter`, `image_layouter`, `table_layouter`,
|
||
`button_layouter`, `form_layouter` stay as the per-type implementations;
|
||
`layout_block` is the registry in front of them. `DocumentLayouter`,
|
||
`EreaderLayout` and the cell layout of S5 all call it, and gaining a block type
|
||
means adding one entry, once.
|
||
|
||
**Measurement** — `pyWebLayout/layout/measure.py`:
|
||
|
||
```python
|
||
@dataclass
|
||
class BlockMeasure:
|
||
min_width: int # narrowest the block can be without overflow
|
||
max_width: int # width at which the block never wraps
|
||
|
||
def measure_block(block: Block, style_ctx: RenderingContext) -> BlockMeasure:
|
||
"""Intrinsic width demands of a block, per the CSS min-content/max-content model."""
|
||
```
|
||
|
||
Rules per type:
|
||
- `Paragraph`/`Heading`: `min_width` = widest unbreakable run — a word wider
|
||
than its font's `min_hyphenation_width` contributes its longest hyphenation
|
||
fragment, not its full width; `max_width` = Σ word widths + (n−1) × min spacing.
|
||
- `Image`: intrinsic width for both.
|
||
- `Table`: recurse (nested tables measure through the S6 grid).
|
||
- `HList`: item measure + marker indent.
|
||
|
||
Measurement uses each word's *own* style, so it is correct for mixed formatting
|
||
— unlike all three current implementations, which assume one font for the box.
|
||
|
||
Heights are **not** part of measurement. Height comes from running
|
||
`layout_block` at the final assigned width and reading `_current_y_offset`.
|
||
`_estimate_wrapped_lines` and `layout_cell_content` are deleted, and
|
||
`DynamicPage.get_min_width`/`get_preferred_width` are reimplemented in terms of
|
||
`measure_block` (they currently walk `Line._text_objects`, which only works if
|
||
someone has pre-poked the lines).
|
||
|
||
### Acceptance criteria
|
||
|
||
- `_estimate_wrapped_lines` and `layout_cell_content` no longer exist.
|
||
- Property test: for a corpus of paragraphs and widths, laying out at
|
||
`measure_block(b).max_width` produces exactly one line, and laying out at
|
||
`min_width` produces no line that overflows its box.
|
||
- `layout_block` handles every type in the union above; adding a type to the
|
||
registry makes it work in documents, cells and the ereader simultaneously
|
||
(assert with one test that exercises all three call sites).
|
||
- Mixed-font paragraph: a paragraph whose words have different sizes measures
|
||
wider than the same word count at the smallest size.
|
||
|
||
### Files
|
||
|
||
new `pyWebLayout/layout/block_layouter.py`, new `pyWebLayout/layout/measure.py`,
|
||
`pyWebLayout/layout/document_layouter.py`, `pyWebLayout/layout/table_optimizer.py`,
|
||
`pyWebLayout/concrete/dynamic_page.py`, `pyWebLayout/concrete/table.py`
|
||
|
||
### Risk
|
||
|
||
Largest spec by volume, but mostly deletion. Land `measure.py` first with tests
|
||
against the current optimizer's outputs to establish a baseline, then swap the
|
||
optimizer over, then delete.
|
||
|
||
---
|
||
|
||
## S5 — Cells as sub-layouts
|
||
|
||
### Problem
|
||
|
||
[`TableCellRenderer._render_cell_content`](../pyWebLayout/concrete/table.py#L110-L242)
|
||
hand-rolls line breaking and handles only `Paragraph`, `Heading` and `Image`
|
||
([table.py:141-146](../pyWebLayout/concrete/table.py#L141-L146)); lists, nested
|
||
tables, quotes, code blocks, buttons and forms are silently dropped. It also
|
||
discards styling: it hardcodes
|
||
`/usr/share/fonts/truetype/dejavu/DejaVuSans.ttf` (a Debian path — **it does not
|
||
exist on the dev machine**; 6 such literals across the package) and
|
||
`font_size = 12`, then rebuilds every word as a plain `Word(text, font)`
|
||
([table.py:156-171](../pyWebLayout/concrete/table.py#L156-L171)), throwing away
|
||
bold/italic/colour/size, the bundled-font system, the font-family override, and
|
||
`LinkedWord` link targets.
|
||
|
||
`DynamicPage` — the abstraction that exists precisely for this — is used only
|
||
for measurement, despite `3bcd1bf` claiming cells can hold anything.
|
||
|
||
### Design
|
||
|
||
A cell owns a `DynamicPage` positioned at the cell's content origin, sharing the
|
||
parent's canvas, filled by `layout_block` (S4).
|
||
|
||
```python
|
||
class TableCellRenderer(Box):
|
||
def __init__(self, cell, origin, size, style, is_header_section=False):
|
||
self._page = DynamicPage(
|
||
style=PageStyle(padding=style.cell_padding, border_width=0,
|
||
background_color=...),
|
||
origin=origin, # S2
|
||
)
|
||
self._page.layout(size)
|
||
for block in cell.blocks():
|
||
layout_block(block, self._page) # S4
|
||
|
||
def render(self):
|
||
self._draw_background_and_border()
|
||
self._page.render_children() # children already positioned absolutely
|
||
```
|
||
|
||
Consequences that fall out rather than being coded:
|
||
|
||
- **Arbitrary content works.** Nested tables, lists, buttons — anything in the
|
||
S4 registry. Invariant 4 is satisfied structurally, not by a type switch.
|
||
- **Styling is preserved**, because `layout_block` uses each word's own style.
|
||
Header cells get bold via `get_or_create_font(weight=FontWeight.BOLD)`
|
||
([core/base.py:186](../pyWebLayout/core/base.py#L186)) rather than by
|
||
substituting a font *file path*. No absolute paths outside `style/fonts.py`.
|
||
- **Cell height is the laid-out height** (`_page._current_y_offset - origin.y`),
|
||
so S6's row heights are exact by construction (invariant 3).
|
||
- **Overflow is bounded**: `can_fit_line` against the cell page's content rect
|
||
stops content at the cell edge instead of painting over neighbours.
|
||
|
||
`TableCellRenderer` keeps its constructor shape so `TableRowRenderer` is
|
||
unaffected, but loses the `draw`/`canvas` parameters — the canvas arrives at
|
||
render time via the S3 rebinding, which must now recurse into child pages:
|
||
|
||
```python
|
||
def render_children(self):
|
||
for child in self._children:
|
||
if isinstance(child, Page):
|
||
child.attach_surface(self._canvas, self._draw) # recurse
|
||
...
|
||
```
|
||
|
||
### Acceptance criteria
|
||
|
||
- A cell containing an `HList`, a nested `Table`, a `Quote` and a `Button`
|
||
renders all four (today: a bare list and a nested table draw nothing but the
|
||
cell border).
|
||
- A cell whose words carry bold/italic/24px/red styling renders with those
|
||
attributes; assert on the `Text` objects' `_style`, not on pixels.
|
||
- `grep -rn "/usr/share/fonts" pyWebLayout/` returns 0 hits.
|
||
- A cell containing a `LinkedWord` produces a `LinkText` concrete object
|
||
(prerequisite for S9).
|
||
- Cell content that exceeds the cell's height is clipped at the cell boundary
|
||
and does not overpaint the next row.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/concrete/dynamic_page.py`,
|
||
`pyWebLayout/concrete/page.py`
|
||
|
||
---
|
||
|
||
## S6 — Table grid model
|
||
|
||
### Problem
|
||
|
||
Four separate geometry defects:
|
||
|
||
1. **`size` is wrong.** [table.py:445-446](../pyWebLayout/concrete/table.py#L445-L446)
|
||
computes total height as `sum(self._row_heights.values())`, but `_row_heights`
|
||
is a **three-entry dict** (`header`/`body`/`footer`), not one entry per row.
|
||
A 10-row table reports height **44px**; its bottom-most drawn pixel is at
|
||
**y=409**. The caption is drawn but not counted either.
|
||
2. **Uniform row heights.** [`_calculate_row_height_for_section`](../pyWebLayout/concrete/table.py#L499-L576)
|
||
takes the max across all rows in a section, so one verbose cell inflates every
|
||
row in the table.
|
||
3. **colspan is not counted.** [`get_column_count`](../pyWebLayout/layout/table_optimizer.py#L189-L205)
|
||
returns `first_row.cell_count`. For `<tr><td colspan=2>` followed by
|
||
`<tr><td><td><td>` it returns **1**, and
|
||
[`TableRowRenderer.render`](../pyWebLayout/concrete/table.py#L383) silently
|
||
drops every cell past `len(column_widths)` — two of three cells never render.
|
||
4. **rowspan is parsed and stored but never read** by any renderer or measurer;
|
||
spanned rows just shift left.
|
||
5. **Row height ignores the cell padding it must contain.** The 40px minimum in
|
||
`_calculate_row_height_for_section` is a constant, so a larger `cell_padding`
|
||
eats into the content box rather than growing the row, and
|
||
`_render_cell_content` then clips the text against `available_height`.
|
||
Rendering the same header at two paddings:
|
||
|
||
```
|
||
padding=(8,10,8,10) border=1: header h=40, ink=593
|
||
padding=(10,12,10,12) border=2: header h=40, ink=288
|
||
```
|
||
|
||
Both rows are 40px tall; the second silently loses half its text. This is
|
||
visible in `docs/images/example_05_html_table_with_images.png`, whose second
|
||
table renders an empty header row. It is the same measure/render disagreement
|
||
as defect 1, and S5 removes it by construction: the cell page's content box
|
||
*is* the box its padding leaves.
|
||
|
||
### Design
|
||
|
||
A resolved grid, built once, that every other stage consumes.
|
||
|
||
```python
|
||
# pyWebLayout/layout/table_grid.py
|
||
|
||
@dataclass(frozen=True)
|
||
class GridCell:
|
||
cell: TableCell
|
||
row: int # resolved row index across all sections
|
||
col: int # resolved column index
|
||
colspan: int
|
||
rowspan: int
|
||
section: str # "header" | "body" | "footer"
|
||
|
||
class TableGrid:
|
||
"""
|
||
Occupancy-resolved view of an abstract Table.
|
||
|
||
Built by the standard HTML table algorithm: walk rows in order, maintaining a
|
||
set of slots occupied by open rowspans, placing each cell at the first free
|
||
column and marking its span.
|
||
"""
|
||
n_cols: int # max over rows of sum(colspan), not first row's cell count
|
||
n_rows: int
|
||
def cells(self) -> Iterator[GridCell]: ...
|
||
def row(self, index: int) -> List[GridCell]: ...
|
||
def cell_at(self, row: int, col: int) -> Optional[GridCell]: ...
|
||
```
|
||
|
||
**Column widths.** `optimize_table_layout` takes a `TableGrid` and per-cell
|
||
`measure_block` results (S4). Span distribution follows CSS: a cell spanning *k*
|
||
columns imposes its demand on the *sum* of those columns, and only widens them
|
||
(proportionally to their current demand) if the sum falls short. Single-column
|
||
demands are applied first so spanning cells cannot dominate.
|
||
|
||
**Row heights.** Per row, not per section: lay out every cell in the row at its
|
||
assigned width (S5) and take the max of the resulting content heights. A cell
|
||
with `rowspan = k` contributes to row *r+k−1* only if the accumulated height of
|
||
rows *r…r+k−1* is less than the cell needs, and the shortfall is distributed
|
||
across those rows.
|
||
|
||
**Size.** `TableRenderer._size` = caption height + Σ per-row heights + borders,
|
||
computed from the same per-row heights that `render()` walks. The two must come
|
||
from one list; a test asserts they cannot diverge.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `<tr><td colspan=2></tr><tr><td><td><td></tr>` → `grid.n_cols == 3`, and all
|
||
four cells render.
|
||
- `<td rowspan=2>` spans two rows vertically; the cell below it in the next row
|
||
is not shifted left.
|
||
- 10-row table: `renderer.size[1]` equals `last_drawn_pixel_y - origin_y + border`
|
||
(±1 for border rounding). This is the direct regression test for defect 1 —
|
||
today it is 44 vs 409.
|
||
- A table with one tall row and nine short ones is shorter than 10 × tall row.
|
||
- A captioned table's `size[1]` includes the caption.
|
||
- Fuzz: for randomly generated colspan/rowspan tables, no two rendered cells'
|
||
rectangles overlap, and every cell in the abstract table appears exactly once
|
||
in the grid.
|
||
|
||
### Files
|
||
|
||
new `pyWebLayout/layout/table_grid.py`, `pyWebLayout/layout/table_optimizer.py`,
|
||
`pyWebLayout/concrete/table.py`
|
||
|
||
---
|
||
|
||
## S7 — Retained-mode table rendering
|
||
|
||
### Problem
|
||
|
||
Every other block type is retained-mode: layouters build objects, `add_child`
|
||
them, and `Page.render()` draws them onto a fresh canvas. Tables are
|
||
immediate-mode: [`table_layouter`](../pyWebLayout/layout/document_layouter.py#L351-L402)
|
||
grabs `page.draw`, paints directly, bumps `_current_y_offset`, and never adds
|
||
anything to `page._children`.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
layout_table -> True
|
||
pixels on canvas right after layout: 28328
|
||
pixels after page.render(): 0
|
||
children on page: 0
|
||
```
|
||
|
||
`Page.render()` rebuilds the canvas ([page.py:279](../pyWebLayout/concrete/page.py#L279))
|
||
and the table is not a child, so it is erased. Lay out a table *and* a
|
||
paragraph, render, and only the paragraph survives. The examples appear to work
|
||
only because they read `page._canvas` directly instead of calling `render()`.
|
||
|
||
Because `size` lies (S6), the fit check
|
||
`if table_height > available_height: return False` is also meaningless: a ~950px
|
||
table "fits" a 300px page, draws past the border, and leaves
|
||
`_current_y_offset = 64` so the next paragraph overlaps it.
|
||
|
||
### Design
|
||
|
||
- `TableRenderer.__init__` measures only — it builds the grid, resolves widths,
|
||
lays out cells (S5) and computes `size`. It draws nothing and takes no
|
||
`draw`/`canvas` parameters.
|
||
- `TableRenderer.render()` draws, and is called by `Page.render_children()`.
|
||
- `table_layouter` becomes structurally identical to `image_layouter`:
|
||
|
||
```python
|
||
def table_layouter(table, page, style=None) -> bool:
|
||
renderer = TableRenderer(table, origin=(page.content_origin[0], page._current_y_offset),
|
||
available_width=page.available_width, style=style)
|
||
if renderer.size[1] > page.remaining_height:
|
||
return False # honest check, honest size (S6)
|
||
page.add_child(renderer) # retained mode
|
||
return True
|
||
```
|
||
|
||
- `_row_renderers` / `_cell_renderers` are rebuilt per `render()` call, not
|
||
appended to (today they grow without bound on re-render).
|
||
|
||
### Acceptance criteria
|
||
|
||
- `layout_table(...)` then `page.render()` yields a canvas containing the table.
|
||
- `page.render()` twice → pixel-identical images.
|
||
- Table + following paragraph: both present, non-overlapping, paragraph starts
|
||
below `table.size[1]`.
|
||
- A table taller than the remaining space returns `False` and adds no child.
|
||
- `len(page.children) == 1` after laying out one table.
|
||
- Rendering the same `TableRenderer` five times leaves
|
||
`len(renderer._row_renderers)` equal to the row count.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/layout/document_layouter.py`
|
||
|
||
---
|
||
|
||
## S8 — Table and list pagination
|
||
|
||
### Problem
|
||
|
||
[`EreaderLayout._layout_table_on_page`](../pyWebLayout/layout/ereader_layout.py#L598-L611)
|
||
skips tables outright ("For now, skip tables"), and `_layout_list_on_page` does
|
||
the same for lists. The ereader — the library's main consumer — renders neither.
|
||
`RenderingPosition` already carries `table_row`, `table_col` and
|
||
`list_item_index` ([ereader_layout.py:38-40](../pyWebLayout/layout/ereader_layout.py#L33-L45)),
|
||
so the state model anticipated this; only the layout half is missing.
|
||
`examples/13_table_pagination_demo.py` documents pagination behaviour that the
|
||
library does not implement.
|
||
|
||
### Design
|
||
|
||
With S4 (`layout_block` returning a resume point) and S6/S7 (honest per-row
|
||
geometry), pagination is row-splitting rather than new machinery.
|
||
|
||
```python
|
||
def table_layouter(table, page, style=None, start_row: int = 0) -> LayoutResult:
|
||
"""
|
||
Place as many rows as fit from start_row.
|
||
Returns complete=False with next_row set when the table is split.
|
||
"""
|
||
```
|
||
|
||
Rules:
|
||
- Split only on row boundaries. A single row taller than a full page is placed
|
||
anyway and clipped — with a `log.warning`, not silently.
|
||
- Header rows repeat at the top of each continuation fragment. This is a
|
||
`TableStyle` flag (`repeat_header: bool = True`), because for a two-row table
|
||
it is noise.
|
||
- Column widths are resolved once for the whole table and reused across
|
||
fragments, so columns line up across pages.
|
||
|
||
`EreaderLayout._layout_table_on_page` / `_layout_list_on_page` delegate to
|
||
`layout_block` and map `LayoutResult.next_row` / `next_item` onto
|
||
`RenderingPosition`. The existing bidirectional navigation
|
||
(`render_page_backward`) must round-trip through table positions — that is the
|
||
part most likely to bite, so it gets its own test.
|
||
|
||
### Acceptance criteria
|
||
|
||
- A 60-row table across a 3-page document renders every row exactly once,
|
||
no duplicates, no gaps.
|
||
- Forward then backward navigation across a table returns to the identical
|
||
starting position (extends `tests/layout/test_navigation_consistency.py`).
|
||
- With `repeat_header=True`, every fragment starts with the header row.
|
||
- A serialized `RenderingPosition` inside a table restores to the same page after
|
||
reload.
|
||
- Lists paginate mid-list and resume at the right item, with markers continuing
|
||
their numbering.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/document_layouter.py`,
|
||
`pyWebLayout/concrete/table.py`
|
||
|
||
### Risk
|
||
|
||
The ereader's backward-navigation estimator
|
||
([ereader_layout.py:400-465](../pyWebLayout/layout/ereader_layout.py#L430-L465))
|
||
assumes blocks are cheap to re-lay-out repeatedly. Tables are not. Expect to
|
||
need a per-table layout cache keyed on `(table_id, available_width, font_scale)`
|
||
before this is usable at speed.
|
||
|
||
---
|
||
|
||
## S9 — Interactivity inside tables
|
||
|
||
### Problem
|
||
|
||
Nothing inside a table is hit-testable: `query_point` into a rendered table
|
||
returns `object_type="empty"`. Links, images and buttons in cells are invisible
|
||
to the query/selection/callback system the rest of the library is built on.
|
||
`examples/14_interactive_table.py` works around this by hand-painting buttons
|
||
*on top of* the table and doing its own coordinate maths — the demo fakes the
|
||
feature. Meanwhile `TableCellRenderer` sets bounds on `InteractiveImage`
|
||
([table.py:322-327](../pyWebLayout/concrete/table.py#L322-L327)), a third,
|
||
parallel interaction mechanism.
|
||
|
||
### Design
|
||
|
||
S5 and S7 make this mostly free: cells are pages, tables are children, and
|
||
`Page.query_point` already recurses into children that implement `query_point`
|
||
([page.py:345-358](../pyWebLayout/concrete/page.py#L345-L358)).
|
||
|
||
Required:
|
||
- `TableRenderer` and `TableCellRenderer` implement `Queriable.in_object` (they
|
||
are `Box` subclasses, so bounds already exist) and `query_point`, delegating to
|
||
the cell's `DynamicPage`.
|
||
- Because child pages are positioned in absolute coordinates (S2), **no
|
||
coordinate translation is needed** — a translating implementation would be a
|
||
sign S2 was not applied.
|
||
- Callback registration cascades: a cell page's `CallbackRegistry` merges into
|
||
the owning page's registry at `add_child` time, so `page.callbacks` remains the
|
||
single lookup point.
|
||
- `InteractiveImage.set_rendered_bounds` becomes redundant for the table path;
|
||
keep it for direct users but stop calling it from the cell renderer.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `page.query_point(p)` for a point over a link in a cell returns
|
||
`object_type == "link"` with the correct `link_target`.
|
||
- A `Button` in a cell fires its callback through the normal
|
||
`page.callbacks` path.
|
||
- `examples/14_interactive_table.py` is rewritten to put real `Button` blocks in
|
||
cells and delete its manual overlay maths — the example shrinks substantially,
|
||
which is the acceptance signal.
|
||
- `query_range` selection spanning a table returns the cell text in document
|
||
order.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/table.py`, `pyWebLayout/concrete/page.py`,
|
||
`pyWebLayout/core/callback_registry.py`, `examples/14_interactive_table.py`
|
||
|
||
---
|
||
|
||
## S10 — Contracts and hygiene
|
||
|
||
Small independent items. Each is a one-commit change; none blocks the others.
|
||
|
||
### 10.1 Render contract
|
||
|
||
`TableRenderer.render()`, `TableRowRenderer.render()` and
|
||
`TableCellRenderer.render()` are annotated `-> Image.Image` and all
|
||
`return None`. After S7, decide and enforce one contract: `render()` draws onto
|
||
the bound canvas and returns `None`; only `Page.render()` returns an image.
|
||
Update `Renderable.render`'s docstring in
|
||
[core/base.py:16-22](../pyWebLayout/core/base.py#L16-L22), which currently
|
||
promises a `PIL.Image`, and annotate accordingly.
|
||
|
||
### 10.2 Abstract/concrete leak
|
||
|
||
[concrete/__init__.py:18](../pyWebLayout/concrete/__init__.py) re-exports the
|
||
**abstract** `Table, TableRow as Row, TableCell as Cell` from the concrete
|
||
package, aliased to look concrete, while `TableRenderer` is not exported at all.
|
||
This contradicts `ARCHITECTURE.md`. Remove the re-export, export the renderers,
|
||
and add the `Cell`/`Row` names to a deprecation shim for one release if anything
|
||
external depends on them.
|
||
|
||
### 10.3 Dead code
|
||
|
||
- [table.py:229-242](../pyWebLayout/concrete/table.py#L229-L242): fallback
|
||
reading `self._cell._text_content`, an attribute that exists nowhere.
|
||
- `TableCellRenderer._children`: written never, read never.
|
||
- `dynamic_page.py`: `import numpy as np` unused;
|
||
`render_partial`/`has_more_content`/`reset_pagination` are an unused
|
||
pagination API superseded by S8 — delete or wire up, do not leave both.
|
||
- [document_layouter.py:157-161](../pyWebLayout/layout/document_layouter.py#L157-L161):
|
||
`temp_text.width` computed and discarded; `else: pass`.
|
||
|
||
### 10.4 Exception handling
|
||
|
||
13 bare `except Exception:` / `except BaseException:` in the package. The worst
|
||
is [table.py:331-333](../pyWebLayout/concrete/table.py#L331-L333), which swallows
|
||
every image failure into a 20px gap with no diagnostic. Policy: catch the
|
||
specific exception, log at `warning` with `exc_info=True`, and render a visible
|
||
placeholder for content failures. `except BaseException` (which catches
|
||
`KeyboardInterrupt`) is never correct here.
|
||
|
||
### 10.5 Image source resolution
|
||
|
||
[`_render_image_in_cell`](../pyWebLayout/concrete/table.py#L252-L266) probes six
|
||
attribute names (`source`, `_source`, `path`, `src`, `_path`, `_src`) to find an
|
||
image path. Give `abstract.block.Image` one documented accessor and use it.
|
||
|
||
### Acceptance criteria
|
||
|
||
- `grep -rn "except BaseException" pyWebLayout/` → 0 hits.
|
||
- No `hasattr` chain longer than one alternative anywhere in `concrete/table.py`.
|
||
- `python -W error -c "import pyWebLayout"` clean; flake8 reports no unused
|
||
imports in touched files.
|
||
|
||
---
|
||
|
||
## S11 — Partial-block progress is discarded
|
||
|
||
### Problem
|
||
|
||
`BidirectionalLayouter.render_page_forward` discards the resume position of a
|
||
block that only partially fitted:
|
||
|
||
```python
|
||
success, new_pos = self._layout_block_on_page(scaled_block, page, current_pos, font_scale)
|
||
if not success:
|
||
# Block doesn't fit, we're done with this page
|
||
break # <-- new_pos dropped on the floor
|
||
...
|
||
current_pos = new_pos
|
||
return page, current_pos
|
||
```
|
||
[ereader_layout.py:341-347](../pyWebLayout/layout/ereader_layout.py#L330-L362)
|
||
|
||
`paragraph_layouter` correctly returns the index of the first word that did not
|
||
fit, and `_layout_paragraph_on_page` correctly packs it into `new_pos.word_index`
|
||
([ereader_layout.py:570-582](../pyWebLayout/layout/ereader_layout.py#L570-L582)).
|
||
The information exists and is thrown away one frame up the stack. The returned
|
||
"next position" is therefore the *same* position the page started at.
|
||
|
||
The bug is invisible for a document whose paragraphs each fit on a page — the
|
||
`not success` path is only taken when nothing more fits, which for normal prose
|
||
means the block boundary. It bites exactly when one block spans a page boundary,
|
||
i.e. any paragraph longer than a page.
|
||
|
||
### Evidence
|
||
|
||
A 2877-word paragraph at 800×600:
|
||
|
||
```
|
||
_layout_block_on_page -> success=False, new_pos.word_index=271 (start was 0)
|
||
render_page_forward -> next position (b0, w0) (start was b0, w0)
|
||
|
||
page 0: start(b0,w0) -> next(b0,w0) lines=26 DEAD END
|
||
```
|
||
|
||
The page renders 26 lines of real content, then reports that the reader has made
|
||
no progress. Navigation is stuck on that page permanently; the book is
|
||
unreadable from that block onward.
|
||
|
||
### Design
|
||
|
||
Distinguish *nothing placed* from *something placed*. Only the former should
|
||
leave the position untouched.
|
||
|
||
```python
|
||
if not success:
|
||
if self._position_compare(new_pos, current_pos) > 0:
|
||
# Partially placed: keep the progress, end the page here.
|
||
current_pos = new_pos
|
||
break
|
||
```
|
||
|
||
`_position_compare` already exists and is used by the backward navigation
|
||
([ereader_layout.py:439](../pyWebLayout/layout/ereader_layout.py#L439)), so
|
||
ordering semantics stay in one place.
|
||
|
||
Two supporting changes, because a silent dead-end should not be possible again:
|
||
|
||
1. **A no-progress guard in the navigation loop.** `EreaderManager.next_page`
|
||
asserts that the returned position is strictly greater than the requested
|
||
one; if not, it logs an error naming the block index and force-advances
|
||
`block_index += 1`. A malformed block should cost the reader one block, not
|
||
the rest of the book.
|
||
2. **The same audit for the backward path.** `render_page_backward`'s refinement
|
||
loop ([ereader_layout.py:400-465](../pyWebLayout/layout/ereader_layout.py#L430-L465))
|
||
already has fallbacks for failing to move backward, which is the symmetric
|
||
symptom — check whether those fallbacks were compensating for this bug and
|
||
simplify them if so.
|
||
|
||
### Acceptance criteria
|
||
|
||
- A 2877-word single-paragraph document paginates to completion: 12 pages at
|
||
800×600, every word appearing exactly once, positions strictly increasing.
|
||
(Verified against the patched implementation: pages advance
|
||
w0 → w271 → w531 → … → w2684 → end.)
|
||
- Forward-then-backward across a page-spanning paragraph round-trips to the
|
||
original position.
|
||
- A block that genuinely places nothing (e.g. an image taller than the page on
|
||
an empty page) still returns the unchanged position, and the no-progress guard
|
||
force-advances it with a logged error rather than looping.
|
||
- Regression test asserts `next_position > position` for **every** page of a
|
||
full-book pagination run over the EPUB fixtures.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/ereader_layout.py`, `pyWebLayout/layout/ereader_manager.py`
|
||
|
||
### Risk
|
||
|
||
Low, and it makes pagination strictly more correct. Watch for page-count changes
|
||
in `tests/layout/test_navigation_consistency.py` — any fixture with a
|
||
page-spanning paragraph will now produce more pages, which is the fix working.
|
||
|
||
---
|
||
|
||
## S12 — Background rendering
|
||
|
||
### Problem
|
||
|
||
`PageBuffer` starts a `ProcessPoolExecutor(max_workers=4)`
|
||
([page_buffer.py:117](../pyWebLayout/layout/page_buffer.py#L117)) and submits
|
||
page renders to it. **Every job fails.** `_render_page_worker` returns
|
||
`pickle.dumps(page)` ([page_buffer.py:50](../pyWebLayout/layout/page_buffer.py#L50)),
|
||
and a `Page` holds a live PIL canvas, which is not picklable.
|
||
|
||
### Evidence
|
||
|
||
```
|
||
args pickle OK: 877.5 KB in 21 ms # submit succeeds
|
||
real pool round trip -> job FAILED: TypeError cannot pickle 'ImagingCore' object
|
||
```
|
||
|
||
So the cost is paid in full and the benefit is zero: 4 forked processes, the
|
||
entire block list pickled and shipped per job (~880KB for a 200-block document),
|
||
a full page laid out in the worker — then the result is thrown away by
|
||
`check_completed_renders`, which swallows the exception into a bare `print`
|
||
([page_buffer.py:274-276](../pyWebLayout/layout/page_buffer.py#L274-L276)). On a
|
||
4-core Pi with 512MB this is actively harmful: four interpreter copies plus four
|
||
copies of the book, to populate a cache that never populates.
|
||
|
||
Four further defects in the same file, which matter only if the decision is to
|
||
keep it:
|
||
|
||
1. `_render_page_worker` builds `BidirectionalLayouter(blocks, page_style, font_family_override=...)`
|
||
**without `page_size`**, so it silently uses the default `(800, 600)`
|
||
([page_buffer.py:44](../pyWebLayout/layout/page_buffer.py#L44)). Fixing the
|
||
pickling alone would poison the cache with wrong-size pages.
|
||
2. `check_completed_renders` caches every result with `is_backward=False`
|
||
([page_buffer.py:268](../pyWebLayout/layout/page_buffer.py#L268)), so backward
|
||
renders land in the forward buffer.
|
||
3. `_queue_forward_renders` / `_queue_backward_renders` `break` at the end of
|
||
their first loop body, so despite `for i in range(self.buffer_size)` they
|
||
queue at most one page each.
|
||
4. Failures are reported with `print`, not the logger.
|
||
|
||
### Design
|
||
|
||
**Recommendation: delete the process pool.** Replace it with synchronous
|
||
readahead, gated on a measurement.
|
||
|
||
The justification for multiprocessing was sub-second navigation. That premise
|
||
predates the text caches now landing in `concrete/text.py`, whose own
|
||
measurements put a page at ~30ms on desktop once warm, with
|
||
`EreaderManager.prewarm_caches` ([ereader_manager.py:222](../pyWebLayout/layout/ereader_manager.py#L222))
|
||
priming the working set at open time. If a page costs tens of milliseconds, a
|
||
process pool cannot pay for its own IPC, let alone its memory on the target
|
||
device.
|
||
|
||
```python
|
||
class PageBuffer:
|
||
def __init__(self, buffer_size: int = 5):
|
||
"""LRU page cache with synchronous readahead. No worker processes."""
|
||
|
||
def readahead(self, position: RenderingPosition, n: int = 1) -> None:
|
||
"""
|
||
Render and cache the next n pages on the calling thread.
|
||
Called after a navigation completes, when the reader is idle.
|
||
"""
|
||
```
|
||
|
||
This keeps the LRU buffers, the position maps and the invalidation logic — all
|
||
of which are fine — and removes the executor, the worker function, the pickling
|
||
and the `threading.Lock` that only guarded the pending-render dict.
|
||
|
||
**Gate:** measure first, on the Pi, with the caches warm. Record page render time
|
||
at the target page size in the spec's PR description. If p95 is under ~150ms,
|
||
delete the pool. If it is materially worse, the fallback is a **single worker
|
||
thread**, not processes: layout is PIL-bound, PIL releases the GIL for
|
||
rasterisation, and a thread shares the block list instead of copying it. Fixing
|
||
the process pool properly would require making the entire concrete tree
|
||
picklable (Page → Line → Text → Font → `FreeTypeFont`), which is a large amount
|
||
of surface area to maintain for a cache.
|
||
|
||
Whichever way the gate goes, defects 1–4 above are fixed or deleted with the code
|
||
that contains them, and failures are logged with `exc_info=True` rather than
|
||
printed.
|
||
|
||
### Acceptance criteria
|
||
|
||
- No `ProcessPoolExecutor` in the package, or — if the gate says keep it — a test
|
||
that submits a real job through a real pool and asserts it **succeeds**. The
|
||
absence of such a test is why this shipped broken.
|
||
- Measured page-render p95 on the target device recorded in the PR, before and
|
||
after.
|
||
- Peak RSS while paginating a full book drops by roughly the pool's share
|
||
(expect ~4 interpreter copies' worth on a 4-core device).
|
||
- Readahead of *n* pages populates the cache with *n* pages (today: one queued,
|
||
zero cached).
|
||
- Backward-rendered pages land in the backward buffer.
|
||
- Cache invalidation on font-scale and font-family change still clears both
|
||
buffers.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/page_buffer.py`, `pyWebLayout/layout/ereader_manager.py`
|
||
|
||
### Risk
|
||
|
||
Low. The feature currently contributes nothing but overhead, so removing it
|
||
cannot regress rendering; the only risk is navigation latency, which is what the
|
||
gate measures.
|
||
|
||
---
|
||
|
||
## S13 — Word spacing and alignment
|
||
|
||
### Problem
|
||
|
||
Three defects, all visible as a right edge that wobbles from line to line.
|
||
|
||
1. **Ragged alignments stretched their gaps.** `LeftAlignmentHandler` distributed
|
||
the line's residual space across its word gaps, clamped to `max_spacing`. A
|
||
line whose residual divided to less than `max_spacing` was stretched flush;
|
||
one that exceeded it was not. So left-aligned text was justified *sometimes*,
|
||
by a different amount on each line. `CenterRightAlignmentHandler` did the same,
|
||
and additionally returned `ideal_space` while computing its start position from
|
||
a different value (`actual_spacing`), so centred lines were not centred.
|
||
2. **The last line of a justified paragraph was justified.** A three-word tail was
|
||
spread across the full measure.
|
||
3. **Justified lines fell 1–2px short.** `base_spacing = int(residual // gaps)`
|
||
with `remainder = int(residual % gaps)` discards the fractional part of both
|
||
terms, and word widths are fractional.
|
||
|
||
### Design
|
||
|
||
- Ragged alignments (left, centre, right) use a **constant** word space: the
|
||
font's own space advance, clamped to `[min_spacing, max_spacing]`, passed to
|
||
the handler as `natural_spacing`. They never absorb residual space — that
|
||
belongs in the margin. When a line cannot fit at natural spacing they report
|
||
overflow rather than tightening, so line breaking moves the word instead of
|
||
rendering deciding to squeeze it.
|
||
- `Line` carries `is_paragraph_end`, set by `paragraph_layouter` on the line
|
||
holding a paragraph's final word. `render_alignment_handler` substitutes flush
|
||
left for justify on that line only. A paragraph continued onto the next page
|
||
never reaches the marking code, so its lines stay justified — correct.
|
||
- Justification distributes the residual by **cumulative rounding**
|
||
(`round(total * i / gaps)` differenced), so the gaps sum to the residual
|
||
exactly and every line ends at the same x.
|
||
- Alignment becomes configurable: `PageStyle.default_alignment`, defaulting to
|
||
`JUSTIFY`, replaces the hardcoded `Alignment.LEFT` in `paragraph_layouter`.
|
||
`AbstractStyle.text_align` / `ConcreteStyle.text_align` now default to `None`
|
||
meaning "not specified", so HTML that sets no `text-align` inherits the page
|
||
default while explicit CSS still wins. Headings are never justified.
|
||
|
||
### Acceptance criteria
|
||
|
||
- Left-aligned word gaps are constant within a line and across lines (±1px).
|
||
- Left-aligned text does not end flush on every line — a flush edge means it was
|
||
justified.
|
||
- Justified body lines end within 2px of the margin; measured advance ends are
|
||
identical across lines, with ≤1px of ink variation from side bearings.
|
||
- The final line of a completed justified paragraph is not stretched.
|
||
- Centred lines have equal margins either side (±2px).
|
||
- Headings are flush left even when the page default is justify.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/text.py`, `pyWebLayout/layout/document_layouter.py`,
|
||
`pyWebLayout/style/page_style.py`, `pyWebLayout/style/abstract_style.py`,
|
||
`pyWebLayout/style/concrete_style.py`
|
||
|
||
---
|
||
|
||
## Test plan
|
||
|
||
Findings were reproduced with four probe scripts; each becomes a regression test
|
||
rather than being thrown away.
|
||
|
||
| Regression test | Guards | Currently |
|
||
|---|---|---|
|
||
| `test_table_survives_page_render` | S7 | fails (0 px after render) |
|
||
| `test_table_size_matches_drawn_extent` | S6 | fails (44 vs 409) |
|
||
| `test_table_fit_check_rejects_oversized` | S6/S7 | fails (returns True) |
|
||
| `test_colspan_column_count` | S6 | fails (1 vs 3) |
|
||
| `test_rowspan_occupancy` | S6 | fails (ignored) |
|
||
| `test_inline_content_in_div_li_td` | S1 | fails (content lost) |
|
||
| `test_page_draw_not_stale_after_add_child` | S3 | fails (stale) |
|
||
| `test_horizontal_padding_honoured` | S2 | fails (x=2 vs 42) |
|
||
| `test_arbitrary_blocks_in_cell` | S5 | fails (dropped) |
|
||
| `test_query_point_into_table_cell` | S9 | fails ("empty") |
|
||
| `test_page_spanning_paragraph_advances` | S11 | fails (dead-ends at page 0) |
|
||
| `test_background_render_job_succeeds` | S12 | fails (unpicklable Page) |
|
||
|
||
Note that `tests/concrete/test_table_rendering.py:541-553` currently asserts only
|
||
`height > 0`, which is why the size defect survived. Assertions of that shape
|
||
should be replaced wherever the tests touch geometry.
|
||
|
||
Golden images in `docs/images/` are regenerated **once** under S2 and reviewed
|
||
deliberately; after that they are treated as fixtures and any diff is a
|
||
regression.
|
||
|
||
## Out of scope
|
||
|
||
Called out so their absence is a decision rather than an oversight:
|
||
|
||
- CSS percentage widths in `parse_html_width`
|
||
([table_optimizer.py:283-284](../pyWebLayout/layout/table_optimizer.py#L283-L284)) —
|
||
needs a containing-block model.
|
||
- `border-collapse: separate`, per-cell borders, per-cell background colour.
|
||
- Vertical alignment within cells (`valign`); everything is top-aligned.
|
||
- RTL and vertical writing modes.
|
||
- Floats and absolute positioning.
|