fix(functional): form field labels no longer overprint the field above (S15)
FormFieldText treats its origin as the control's top-left - size and in_object both measure down from it - but drew the label by calling Text.render at that origin, and Text anchors on the baseline. The label's glyphs therefore landed above the origin, outside the box the control claims, on top of whatever was there. In a stacked form that is the preceding field's input box, which is what example_10_forms.png showed: every label but the first crowding the box above it. The label is now offset down by its ascent, so it occupies the space the control accounts for. Height derives from the label's ink height rather than the nominal font size, which had also eaten into the 5px gap between label and box. LABEL_GAP names that 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 now share it instead of each recomputing font_size + 5. Also recorded under S12: the broken process pool is not merely wasted work. It forks from a process that already has threads, and tests/layout/test_ereader_image_rendering.py hangs at interpreter exit roughly one run in four - every test passes, then the process never returns.
This commit is contained in:
@@ -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
|
||||
|
||||
Binary file not shown.
|
Before Width: | Height: | Size: 31 KiB After Width: | Height: | Size: 31 KiB |
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user