1405 lines
60 KiB
Markdown
1405 lines
60 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 |
|
||
| [S14](#s14--vertical-centring-in-buttons-and-fields) | Vertical centring in buttons and fields | 0 |
|
||
| [S15](#s15--form-field-label-geometry) | Form field label geometry | 0 |
|
||
| [S16](#s16--backward-page-navigation) | Backward page navigation | 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.
|
||
|
||
It is also not inert. The pool is started from `PageBuffer.initialize` inside a
|
||
process that already has threads, and CPython warns about exactly this:
|
||
|
||
```
|
||
DeprecationWarning: This process (pid=...) is multi-threaded,
|
||
use of fork() may lead to deadlocks in the child.
|
||
```
|
||
|
||
`tests/layout/test_ereader_image_rendering.py` intermittently hangs at
|
||
interpreter exit as a result — every test reports PASSED, then the process never
|
||
returns. Observed roughly one run in four. A reader that hangs on shutdown once
|
||
in four launches would be a shipped bug; the test suite is just where it shows
|
||
up first. This raises S12 from "wasted work" to "actively harmful".
|
||
|
||
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`
|
||
|
||
---
|
||
|
||
## S14 — Vertical centring in buttons and fields
|
||
|
||
### Problem
|
||
|
||
`ButtonText.render` and `FormFieldText.render` both placed the text baseline at
|
||
`box_top + box_height / 2 + descent / 2`. Centring glyphs whose visual height is
|
||
`ascent + descent` inside a box of height `H` puts the baseline at
|
||
`box_top + H/2 + (ascent - descent)/2`. The two agree only when
|
||
`ascent == 2 * descent`; DejaVu is nearer 4:1, so labels rode high against the
|
||
top edge of the control.
|
||
|
||
`ButtonText` also sized itself as `font_size + padding`, but the text's visual
|
||
height exceeds the nominal size — DejaVu at 14px measures 17 — so the button was
|
||
too short to centre its own label in.
|
||
|
||
### Evidence
|
||
|
||
A 14px "Save Document" button with 6px vertical padding, measuring the label's
|
||
ink against the button rectangle:
|
||
|
||
```
|
||
gap above text: 5px
|
||
gap below text: 11px
|
||
```
|
||
|
||
### Design
|
||
|
||
- `baseline = area_top + (area_height - (ascent + descent)) / 2 + ascent` in both
|
||
renderers.
|
||
- `ButtonText._padded_height` derives from `ascent + descent`, guarded so a mock
|
||
or unusual font object falls back to the nominal size.
|
||
|
||
### Acceptance criteria
|
||
|
||
- Label ink is centred within ±2px at font sizes 10, 14 and 20.
|
||
- Label ink stays inside the button rectangle.
|
||
- Button height is at least `ascent + descent + vertical padding`.
|
||
- A form field's value is centred within its input box (±3px).
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/functional.py`
|
||
|
||
### Note
|
||
|
||
`docs/images/example_07_pressed_state.png` was stale — no example regenerates it;
|
||
`07_pressed_state_demo.py` writes `demo_07_pressed.png` at the repository root
|
||
and the docs copy had been placed by hand. It has been refreshed. Worth wiring
|
||
the demo to write straight to `docs/images/` so it cannot drift again.
|
||
|
||
---
|
||
|
||
## S15 — Form field label geometry
|
||
|
||
### Problem
|
||
|
||
`FormFieldText` treats its origin as the control's top-left: `size` and
|
||
`in_object` both measure down from it. But it drew the label by calling
|
||
`Text.render` at that origin, and Text anchors on the **baseline**, so the
|
||
label's glyphs landed *above* the origin — outside the box the control claims,
|
||
on top of whatever was there. In a stacked form that is the previous field's
|
||
input box, which is what
|
||
`docs/images/example_10_forms.png` showed: every label but the first crowding
|
||
and touching the box above it.
|
||
|
||
The height was also computed as `font_size + 5 + field_height`, understating the
|
||
label by the difference between nominal size and ink height, which left the gap
|
||
between label and box smaller than the intended 5px.
|
||
|
||
### Design
|
||
|
||
- The origin is documented as the top-left of the whole control.
|
||
- Rendering offsets the label down by its ascent, so the glyphs occupy
|
||
`[origin.y, origin.y + ascent + descent]`.
|
||
- `LABEL_GAP` names the 5px gap, and `field_area_offset` gives the distance from
|
||
the origin to the top of the input box. `render`, `handle_click` and the height
|
||
calculation all derive from it, instead of each recomputing `font_size + 5`.
|
||
|
||
### Acceptance criteria
|
||
|
||
- No label ink is drawn above the control's origin.
|
||
- All ink lies within `[origin.y, origin.y + size[1]]`.
|
||
- Consecutive fields laid out by `form_layouter` do not overlap.
|
||
- A click in the input area focuses the field; a click on the label does not.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/concrete/functional.py`
|
||
|
||
---
|
||
|
||
## S16 — Backward page navigation
|
||
|
||
### Problem
|
||
|
||
`render_page_backward` *searched* for the previous page's start: estimate a block
|
||
index, lay out forward, compare the end against the target, bisect on the block
|
||
difference, repeat up to ten times. Both the estimator and the adjuster pinned
|
||
`word_index` to 0 and moved only `block_index`.
|
||
|
||
Pages routinely start mid-block. Any such start was therefore **not in the search
|
||
space**, the loop could never match, and it fell through to a fallback that
|
||
jumped several blocks back or to the document start.
|
||
|
||
### Evidence
|
||
|
||
A document of short paragraphs around one 1200-word paragraph. Forward pagination
|
||
gives page starts at `(0,0), (2,208), (2,494), (2,780), (2,1057)`. Asking for the
|
||
page that ends where each of those begins:
|
||
|
||
```
|
||
from page 1 -> got (0,0) expected (0,0) ok (1 forward layout)
|
||
from page 2 -> got (0,0) expected (2,208) WRONG (10 forward layouts)
|
||
from page 3 -> got (0,0) expected (2,494) WRONG (10 forward layouts)
|
||
from page 4 -> got (0,0) expected (2,780) WRONG (10 forward layouts)
|
||
```
|
||
|
||
Every mid-paragraph case threw the reader to the start of the document after ten
|
||
full page layouts. The bisection was also unsound within its own space: a
|
||
document of 40 small paragraphs, where every page *does* start on a block
|
||
boundary, failed too.
|
||
|
||
This is complementary to S11 rather than caused by it. Before S11 forward
|
||
pagination dead-ended at the first page-spanning block, so mid-block starts were
|
||
never produced and the block-granular search looked adequate.
|
||
|
||
### Design
|
||
|
||
Pagination is a pure function: laying out from `q` yields a page and the position
|
||
it stopped at, `next(q)`. The page before `P` is the `q` with `next(q) == P`.
|
||
That is found by **replaying the chain forward from an anchor**, not by guessing
|
||
`q`. Three sources, in order:
|
||
|
||
1. **The recorded chain.** `render_page_forward` now records
|
||
`(font_scale, next(q)) -> q`. Stepping back to anywhere the reader has been is
|
||
exact and costs one layout. Keyed by font scale, since changing it
|
||
repaginates.
|
||
2. **Replay from an anchor.** Anchors are block starts, nearest first: the block
|
||
containing `P`, then up to `MAX_BACKWARD_ANCHORS` earlier ones, then the
|
||
document start. Lay out forward from the anchor until a page ends exactly on
|
||
`P`; that page's start is the answer. `MAX_REPLAY_PAGES` caps the walk so one
|
||
page turn cannot traverse a whole chapter.
|
||
3. **Nearest start before `P`.** If no chain passes exactly through `P` — which
|
||
happens when `P` was reached by a jump or a restored bookmark rather than by
|
||
reading forward, so it lies on no natural chain — return the last page start
|
||
before it. That overlaps `P`'s page slightly rather than skipping content,
|
||
which is the safe direction to be wrong in.
|
||
|
||
The estimator and the bisecting adjuster are deleted.
|
||
|
||
**What "correct" means here.** Each backward step returns a page ending exactly
|
||
where the reader currently is, so paging back never skips or repeats content.
|
||
That chain can differ from the one you would have seen reading forward from page
|
||
one, if you entered the document by a jump — pagination from a different starting
|
||
point is genuinely a different chain, and no algorithm can recover the original
|
||
without replaying from the start.
|
||
|
||
### Measurements
|
||
|
||
Same document, after the change:
|
||
|
||
```
|
||
warm (chain recorded by the forward pass): 4/4 exact, 1 layout each
|
||
cold, fresh layouter per call: 12/13 exact, worst 17 layouts
|
||
cold, one layouter, repeated back presses: 4 layouts per turn typical
|
||
```
|
||
|
||
The single inexact case is a target that lies on the canonical chain but not on
|
||
any chain reachable from a nearby anchor; it returns a start 15 words early,
|
||
i.e. a slightly overlapping page.
|
||
|
||
### Acceptance criteria
|
||
|
||
- For every page of a document, `render_page_backward(start[i])` returns
|
||
`start[i-1]` — verified for both a mid-paragraph-paginating document and one
|
||
where every page starts on a block boundary.
|
||
- Laying out forward from the returned position ends exactly on the requested
|
||
position.
|
||
- Forward-then-back returns to the original position.
|
||
- At the document start, backward stays there; an empty document is safe.
|
||
- Cost stays within a small bounded number of forward layouts.
|
||
|
||
### Files
|
||
|
||
`pyWebLayout/layout/ereader_layout.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.
|