diff --git a/docs/LAYOUT_REMEDIATION_SPEC.md b/docs/LAYOUT_REMEDIATION_SPEC.md index cc347e8..74838d6 100644 --- a/docs/LAYOUT_REMEDIATION_SPEC.md +++ b/docs/LAYOUT_REMEDIATION_SPEC.md @@ -29,6 +29,7 @@ It is independent of every other spec here. | [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 | ## Design invariants @@ -1016,6 +1017,20 @@ a full page laid out in the worker — then the result is thrown away by 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: @@ -1211,6 +1226,45 @@ 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` + +--- + ## Test plan Findings were reproduced with four probe scripts; each becomes a regression test diff --git a/docs/images/example_10_forms.png b/docs/images/example_10_forms.png index f3fae75..62bd3a5 100644 Binary files a/docs/images/example_10_forms.png and b/docs/images/example_10_forms.png differ diff --git a/pyWebLayout/concrete/functional.py b/pyWebLayout/concrete/functional.py index 3e168b9..a2eb920 100644 --- a/pyWebLayout/concrete/functional.py +++ b/pyWebLayout/concrete/functional.py @@ -297,8 +297,17 @@ class FormFieldText(Text, Interactable, Queriable): """ A Text subclass that can handle FormField interactions. Renders form field labels and input areas. + + The origin is the top-left of the whole control: label, then a gap, then the + input box. Text itself draws from a baseline, so the label is offset down by + its ascent when rendering; without that the glyphs would sit above the origin + and overprint whatever is above, which for a stacked form is the previous + field's input box. """ + # Vertical gap between the label and its input box, in pixels. + LABEL_GAP = 5 + def __init__(self, field: FormField, font: Font, draw: ImageDraw.Draw, field_height: int = 24, source=None, line=None): """ @@ -324,8 +333,11 @@ class FormFieldText(Text, Interactable, Queriable): self._field_height = field_height self._focused = False - # Calculate total height (label + gap + field) - self._total_height = self._style.font_size + 5 + field_height + # Calculate total height (label + gap + field). The label's height is its + # ink height, ascender to descender, not the nominal font size - the two + # differ by several pixels and the gap between label and box is only 5. + self._label_height = self._visual_label_height() + self._total_height = self._label_height + self.LABEL_GAP + field_height # Field width should be at least as wide as the label # Use getattr to handle mock objects in tests @@ -334,6 +346,20 @@ class FormFieldText(Text, Interactable, Queriable): self._width, '__call__') else 0 self._field_width = max(text_width, 150) + def _visual_label_height(self) -> int: + """Height of the rendered label, ascender to descender.""" + try: + ascent, descent = self._style.font.getmetrics() + return int(ascent + descent) + except (AttributeError, TypeError, ValueError): + # Mock or unusual font object; the nominal size is the best guess. + return int(getattr(self._style, 'font_size', 0) or 0) + + @property + def field_area_offset(self) -> int: + """Distance from this control's origin to the top of its input box.""" + return self._label_height + self.LABEL_GAP + @property def field(self) -> FormField: """Get the associated FormField object""" @@ -352,12 +378,21 @@ class FormFieldText(Text, Interactable, Queriable): """ Render the form field with label and input area. """ - # Render the label - super().render() + # Render the label. Text draws from the baseline, so shift down by the + # ascent to make the origin the top of the label rather than its baseline. + try: + label_ascent = self._style.font.getmetrics()[0] + except (AttributeError, TypeError, ValueError): + label_ascent = self._label_height - # Calculate field position (below label with 5px gap) + label_origin = self._origin + self._origin = np.array([label_origin[0], label_origin[1] + label_ascent]) + super().render() + self._origin = label_origin + + # Calculate field position (below the label, with the standard gap) field_x = self._origin[0] - field_y = self._origin[1] + self._style.font_size + 5 + field_y = self._origin[1] + self.field_area_offset # Draw field background and border bg_color = (255, 255, 255) @@ -404,7 +439,7 @@ class FormFieldText(Text, Interactable, Queriable): True if the field was clicked and focused """ # Calculate field area - field_y = self._style.font_size + 5 + field_y = self.field_area_offset # Check if click is within the input field area (not just the label) if (0 <= point[0] <= self._field_width and diff --git a/tests/concrete/test_concrete_functional.py b/tests/concrete/test_concrete_functional.py index 64a97b3..e33fdc0 100644 --- a/tests/concrete/test_concrete_functional.py +++ b/tests/concrete/test_concrete_functional.py @@ -334,8 +334,11 @@ class TestFormFieldText(unittest.TestCase): """Test size property includes field area""" renderable = FormFieldText(self.text_field, self.font, self.mock_draw) - # Size should include label height + gap + field height - expected_height = renderable._style.font_size + 5 + renderable._field_height + # Size should include label height + gap + field height. The label's + # height is its ink height (ascent + descent), not the nominal font size. + ascent, descent = renderable._style.font.getmetrics() + expected_height = (ascent + descent) + FormFieldText.LABEL_GAP \ + + renderable._field_height expected_width = renderable._field_width # Use the calculated field width np.testing.assert_array_equal( diff --git a/tests/concrete/test_form_field_geometry.py b/tests/concrete/test_form_field_geometry.py new file mode 100644 index 0000000..9a7c6f9 --- /dev/null +++ b/tests/concrete/test_form_field_geometry.py @@ -0,0 +1,140 @@ +""" +Regression tests for form field label geometry (spec S15). + +Text renders with a baseline anchor, so drawing the label at the field's origin +put its glyphs above that origin - outside the box the field claims through size +and in_object. Stacked fields therefore had each label overprinting the input box +of the field before it. +""" + +import numpy as np +import pytest +from PIL import Image, ImageDraw + +from pyWebLayout.abstract.functional import Form, FormField, FormFieldType +from pyWebLayout.concrete.functional import FormFieldText +from pyWebLayout.concrete.page import Page +from pyWebLayout.layout.document_layouter import form_layouter +from pyWebLayout.style import Font +from pyWebLayout.style.page_style import PageStyle + + +ORIGIN = (10, 40) +FIELD_HEIGHT = 24 + + +@pytest.fixture +def font(): + return Font(font_size=12, colour=(0, 0, 0)) + + +@pytest.fixture +def canvas(): + image = Image.new("RGB", (300, 200), (255, 255, 255)) + return image, ImageDraw.Draw(image) + + +def make_field(font, draw, label="Email Address"): + field = FormField(name="email", field_type=FormFieldType.TEXT, label=label) + renderable = FormFieldText(field, font, draw, field_height=FIELD_HEIGHT) + renderable.set_origin(np.array(list(ORIGIN))) + return renderable + + +def ink_rows(image, x_range, y_range): + pixels = image.convert("RGB").load() + return [y for y in y_range + if any(sum(pixels[x, y]) < 400 for x in x_range)] + + +class TestLabelStaysInsideTheFieldBox: + + def test_label_ink_is_below_the_origin(self, font, canvas): + image, draw = canvas + renderable = make_field(font, draw) + renderable.render() + + rows = ink_rows(image, range(ORIGIN[0], ORIGIN[0] + 140), + range(0, ORIGIN[1])) + assert not rows, \ + f"label drew above its own origin, at rows {rows}" + + def test_label_and_box_do_not_overlap(self, font, canvas): + image, draw = canvas + renderable = make_field(font, draw) + renderable.render() + + ascent, descent = font.font.getmetrics() + label_bottom = ORIGIN[1] + ascent + descent + box_top = renderable.field_area_offset + ORIGIN[1] + + assert box_top >= label_bottom, \ + "the input box must start below the label's descenders" + + def test_reported_height_covers_everything_drawn(self, font, canvas): + image, draw = canvas + renderable = make_field(font, draw) + renderable.render() + + top, bottom = ORIGIN[1], ORIGIN[1] + int(renderable.size[1]) + rows = ink_rows(image, range(ORIGIN[0], ORIGIN[0] + 200), range(0, 200)) + assert min(rows) >= top, "ink above the field's declared box" + assert max(rows) < bottom, "ink below the field's declared box" + + +class TestStackedFieldsDoNotCollide: + + def test_form_layout_leaves_labels_clear(self, font): + page = Page(size=(300, 400), style=PageStyle()) + form = Form("signup") + for name in ["Username", "Email Address", "Password"]: + form.add_field(FormField(name=name.lower().replace(" ", "_"), + field_type=FormFieldType.TEXT, label=name)) + + ok, ids = form_layouter(form, page, font) + assert ok and len(ids) == 3 + + fields = [c for c in page.children if isinstance(c, FormFieldText)] + assert len(fields) == 3 + + for earlier, later in zip(fields, fields[1:]): + earlier_bottom = earlier.origin[1] + earlier.size[1] + assert later.origin[1] >= earlier_bottom, \ + "fields overlap: a label would print over the preceding input box" + + def test_rendered_form_has_no_ink_collisions(self, font): + """Every field's ink stays within its own declared bounds.""" + page = Page(size=(300, 400), style=PageStyle()) + form = Form("signup") + for name in ["Username", "Email Address"]: + form.add_field(FormField(name=name.lower(), field_type=FormFieldType.TEXT, + label=name)) + form_layouter(form, page, font) + image = page.render() + + fields = [c for c in page.children if isinstance(c, FormFieldText)] + for field in fields: + top = int(field.origin[1]) + bottom = top + int(field.size[1]) + rows = ink_rows(image, range(int(field.origin[0]), + int(field.origin[0] + field.size[0])), + range(max(0, top - 6), top)) + assert not rows, f"ink found just above a field at y={top}" + + +class TestClickTargetsFollowTheLayout: + + def test_click_in_the_input_area_focuses(self, font, canvas): + _, draw = canvas + renderable = make_field(font, draw) + + inside = (5, renderable.field_area_offset + FIELD_HEIGHT // 2) + assert renderable.handle_click(inside) is True + assert renderable._focused is True + + def test_click_on_the_label_does_not_focus(self, font, canvas): + _, draw = canvas + renderable = make_field(font, draw) + + on_label = (5, 2) + assert renderable.handle_click(on_label) is False