diff --git a/docs/LAYOUT_REMEDIATION_SPEC.md b/docs/LAYOUT_REMEDIATION_SPEC.md index c963739..a75e0e7 100644 --- a/docs/LAYOUT_REMEDIATION_SPEC.md +++ b/docs/LAYOUT_REMEDIATION_SPEC.md @@ -560,6 +560,22 @@ Four separate geometry defects: 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 diff --git a/docs/images/example_05_html_table_with_images.png b/docs/images/example_05_html_table_with_images.png index 80cf98c..e25a835 100644 Binary files a/docs/images/example_05_html_table_with_images.png and b/docs/images/example_05_html_table_with_images.png differ diff --git a/pyWebLayout/concrete/page.py b/pyWebLayout/concrete/page.py index 3128214..b08e8e9 100644 --- a/pyWebLayout/concrete/page.py +++ b/pyWebLayout/concrete/page.py @@ -15,6 +15,10 @@ class Page(Renderable, Queriable): contains a given point. """ + # Mode of the render canvas. The measurement context matches it so that text + # width caching keys stay consistent between layout and rendering. + _CANVAS_MODE = 'RGBA' + def __init__(self, size: Tuple[int, int], style: Optional[PageStyle] = None, origin: Tuple[int, int] = (0, 0)): """ @@ -32,6 +36,7 @@ class Page(Renderable, Queriable): self._children: List[Renderable] = [] self._canvas: Optional[Image.Image] = None self._draw: Optional[ImageDraw.Draw] = None + self._measurement_draw: Optional[ImageDraw.ImageDraw] = None # Initialize y_offset to start of content area # Position the first line so its baseline is close to the top boundary # For subsequent lines, baseline-to-baseline spacing is used @@ -168,13 +173,38 @@ class Page(Renderable, Queriable): @property def draw(self) -> Optional[ImageDraw.Draw]: - """Get the ImageDraw object for drawing on this page's canvas""" - if self._draw is None: + """ + Get the ImageDraw object bound to this page's render canvas. + + Rebuilt whenever the canvas has been invalidated: a draw context + outlives the image it was created from, so checking only _draw would + hand back a context pointing at a discarded canvas. + """ + if self._draw is None or self._canvas is None: # Initialize canvas and draw context if not already done self._canvas = self._create_canvas() self._draw = ImageDraw.Draw(self._canvas) return self._draw + @property + def measurement_draw(self) -> ImageDraw.ImageDraw: + """ + A scratch draw context for text metrics during layout. + + Layout asks for text widths constantly, but has no reason to touch the + render canvas - and the canvas is invalidated on every add_child, so + measuring through `draw` would allocate a full-page image per line. + This context is 1x1 and never invalidated. + + Its mode matches the render canvas because Text keys its width cache on + the draw mode; a mismatch would double every cache entry. Children built + against it are re-bound to the real canvas by render_children. + """ + if self._measurement_draw is None: + scratch = Image.new(self._CANVAS_MODE, (1, 1)) + self._measurement_draw = ImageDraw.Draw(scratch) + return self._measurement_draw + def add_child(self, child: Renderable) -> 'Page': """ Add a child renderable object to this page. @@ -333,7 +363,7 @@ class Page(Renderable, Queriable): PIL Image with background and borders applied """ # Create base image - canvas = Image.new('RGBA', self._size, (*self._style.background_color, 255)) + canvas = Image.new(self._CANVAS_MODE, self._size, (*self._style.background_color, 255)) # Draw borders if needed if self._style.border_width > 0: diff --git a/pyWebLayout/layout/document_layouter.py b/pyWebLayout/layout/document_layouter.py index 95d071e..3f19b2c 100644 --- a/pyWebLayout/layout/document_layouter.py +++ b/pyWebLayout/layout/document_layouter.py @@ -165,7 +165,7 @@ def paragraph_layouter(paragraph: Paragraph, # Create a temporary Text object to calculate word width if word: - temp_text = Text.from_word(word, page.draw) + temp_text = Text.from_word(word, page.measurement_draw) temp_text.width else: pass @@ -174,7 +174,7 @@ def paragraph_layouter(paragraph: Paragraph, spacing=word_spacing_constraints, origin=(x_cursor, y_cursor), size=(page.available_width, baseline_spacing), - draw=page.draw, + draw=page.measurement_draw, font=font, halign=text_align ) @@ -225,7 +225,7 @@ def paragraph_layouter(paragraph: Paragraph, return False, i, overflow_text # Check if the word will fit on the new line before adding it - temp_text = Text.from_word(word, page.draw) + temp_text = Text.from_word(word, page.measurement_draw) if temp_text.width > current_line.size[0]: # Word is too wide for the line, we need to hyphenate it if len(word.text) >= 6: @@ -234,13 +234,13 @@ def paragraph_layouter(paragraph: Paragraph, (Text( pair[0], word.style, - page.draw, + page.measurement_draw, line=current_line, source=word), Text( pair[1], word.style, - page.draw, + page.measurement_draw, line=current_line, source=word)) for pair in word.possible_hyphenation()] if len(splits) > 0: @@ -455,7 +455,7 @@ def button_layouter(button: Button, available_height = page.remaining_height # Create ButtonText renderable - button_text = ButtonText(button, font, page.draw, padding=padding) + button_text = ButtonText(button, font, page.measurement_draw, padding=padding) # Check if button fits on current page button_height = button_text.size[1] @@ -505,7 +505,8 @@ def form_field_layouter(field: FormField, page: Page, font: Optional[Font] = Non available_height = page.remaining_height # Create FormFieldText renderable - field_text = FormFieldText(field, font, page.draw, field_height=field_height) + field_text = FormFieldText(field, font, page.measurement_draw, + field_height=field_height) # Check if field fits on current page total_field_height = field_text.size[1] diff --git a/tests/concrete/test_canvas_lifecycle.py b/tests/concrete/test_canvas_lifecycle.py new file mode 100644 index 0000000..8caddb0 --- /dev/null +++ b/tests/concrete/test_canvas_lifecycle.py @@ -0,0 +1,129 @@ +""" +Regression tests for the page draw/canvas lifecycle (spec S3). + +add_child invalidates the canvas but left _draw pointing at it, and the draw +property only rebuilt when _draw was None. Callers therefore received a context +bound to a discarded image while page._canvas stayed None - which is how images +inside table cells ended up as grey placeholders: table_layouter passed +canvas=None through to the cell renderer. + +Fixing that alone would make layout allocate a full-page canvas per line, since +layout measures text through the page. Measurement now goes through a dedicated +scratch context. +""" + +import pytest +from PIL import Image + +from pyWebLayout.abstract.block import Image as AbstractImage, Paragraph +from pyWebLayout.abstract.inline import Word +from pyWebLayout.concrete.page import Page +from pyWebLayout.layout.document_layouter import DocumentLayouter +from pyWebLayout.style import Font +from pyWebLayout.style.page_style import PageStyle + + +@pytest.fixture +def font(): + return Font(font_size=12) + + +@pytest.fixture +def page(): + return Page(size=(400, 600), style=PageStyle()) + + +def paragraph_of(font, count=40): + paragraph = Paragraph(font) + for i in range(count): + paragraph.add_word(Word(f"word{i}", font)) + return paragraph + + +class TestDrawIsNeverStale: + + def test_draw_matches_canvas_after_add_child(self, page, font): + page.draw # force canvas creation + DocumentLayouter(page).layout_paragraph(paragraph_of(font)) + + assert page.draw.im is page._canvas.im, \ + "draw must be bound to the page's current canvas" + + def test_canvas_is_present_after_layout(self, page, font): + DocumentLayouter(page).layout_paragraph(paragraph_of(font)) + page.draw + + assert page._canvas is not None + + def test_repeated_draw_access_is_stable(self, page): + first = page.draw + assert page.draw is first, "draw must not be rebuilt while the canvas stands" + + +class TestMeasurementDoesNotAllocateCanvases: + + def test_layout_allocates_no_page_canvas(self, page, font, monkeypatch): + calls = [] + original = Page._create_canvas + + def counting(self): + calls.append(1) + return original(self) + + monkeypatch.setattr(Page, "_create_canvas", counting) + DocumentLayouter(page).layout_paragraph(paragraph_of(font, 400)) + + assert calls == [], \ + f"layout allocated {len(calls)} full-page canvases; it should allocate none" + + def test_measurement_context_is_tiny_and_matches_canvas_mode(self, page): + scratch = page.measurement_draw + assert scratch.im.size == (1, 1) + assert scratch.mode == Page._CANVAS_MODE + + def test_measurement_context_is_stable(self, page, font): + first = page.measurement_draw + DocumentLayouter(page).layout_paragraph(paragraph_of(font)) + assert page.measurement_draw is first, \ + "the scratch context must survive canvas invalidation" + + +class TestRenderIsRepeatable: + + def test_two_renders_are_identical(self, page, font): + DocumentLayouter(page).layout_paragraph(paragraph_of(font)) + + first = page.render().copy() + second = page.render().copy() + + assert first.tobytes() == second.tobytes() + + +class TestImageInCellGetsARealCanvas: + """The concrete symptom: table images degraded to placeholders.""" + + @pytest.fixture + def image_path(self, tmp_path): + path = tmp_path / "swatch.png" + Image.new("RGB", (40, 30), (10, 200, 10)).save(path) + return str(path) + + def test_table_after_paragraph_receives_a_canvas(self, page, font, image_path): + from pyWebLayout.abstract.block import Table, TableCell, TableRow + from pyWebLayout.layout.document_layouter import table_layouter + + layouter = DocumentLayouter(page) + layouter.layout_paragraph(paragraph_of(font, 10)) + + table = Table() + row = TableRow() + cell = TableCell() + cell.add_block(AbstractImage(image_path)) + row.add_cell(cell) + table.add_row(row) + + # The canvas is invalidated by the preceding add_child; the table must + # still be handed a real one. + assert table_layouter(table, page) or True # placement may fail on space + assert page._canvas is not None, \ + "table layout must not run against a None canvas"