Skip to content

Make TextArea measurement side-effect free. [fork-internal review]#1

Merged
0k-dot-computer merged 2 commits into
mainfrom
textarea-side-effect-free-measure
Jun 12, 2026
Merged

Make TextArea measurement side-effect free. [fork-internal review]#1
0k-dot-computer merged 2 commits into
mainfrom
textarea-side-effect-free-measure

Conversation

@0k-dot-computer

@0k-dot-computer 0k-dot-computer commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Fork-internal review PR — do not merge into upstream from here. Base and head are both in 0k-dot-computer/xilem; upstream (linebender/xilem) cannot see this. The real PR is opened only after sign-off on the exact text.

Rev 2 (2026-06-12): reworked from the minimal scratch-layout fix to the house pattern per Lane's review — TextArea now measures through Label's layout cache (linebender#1665), extracted into a shared TextLayoutCache.

What this is

Re-derivation of our masonry fork patch 05-text-area-measure-cache.patch on upstream HEAD (5d72ad41), targeting upstream's own TODO in TextArea::measure:
// TODO: Remove this hack and do efficient side-effect free measurement with no alignment.

Two commits, each buildable and green:

  1. Extract Label's text layout cache into a shared TextLayoutCache. Pure refactor — TextLayout + the caching logic from Improve Label text layout caching. linebender/xilem#1665 move verbatim from label.rs to a crate-private widgets/text_layout_cache.rs, parameterized on text/styles instead of reading Label's fields. Label render snapshots unchanged (234/234 tests green at this commit).
  2. Make TextArea measurement side-effect free. measure goes through its own TextLayoutCache built from the editor's current text and styles (no alignment — doesn't affect measured size); the live PlainEditor is never touched during measurement. Cache cleared wherever text/styles/fonts change, mirroring Label::clear_cache (style setters, reset_text, keyboard/IME/paste edit paths, FontsChanged).

Validation evidence

  • measure_does_not_mutate_editor fails on pristine HEAD (speculative measure at min/max-content nudges the editor generation every layout pass), passes with the fix.
  • measure_reflects_edits guards the invalidation: typing must grow max-content width. Counterfactually verified — removing one clear_measure_cache() call makes it fail.
  • cargo test -p masonry: 236 passed incl. all render snapshots (pixel parity for both Label and TextArea); cargo fmt --check, cargo clippy -p masonry --all-features --tests (0 warnings), cargo check --workspace --all-features: clean.

Why the house pattern (vs the minimal fix)

The maintainer (xStrom) solved this exact problem for Label in linebender#1665 with a multi-entry cache keyed by max_advance (satisfies/more_constrained reuse logic). Measuring through the shared cache gives TextArea the same layout reuse — min/max-content probes and repeated measures hit the cache instead of rebuilding — and is a concrete step toward the existing TODO about unifying TextArea's and Label's measure paths.

Cost: correctness now depends on invalidation completeness (the clear sites). Mitigated by mirroring Label's discipline exactly + the counterfactually-verified test.

Things for Lane to weigh

  1. Linebender LLM policy: AI-generated PR descriptions are banned; LLM usage must be disclosed up front. The draft body + disclosure raw material is in the latest pr-body-draft.md comment below — you must rewrite and own it.
  2. Conflict risk went up: the invalidation hunks sit inside the same event-handling code that open PRs TextArea: on Ime::Disabled use finish_compose() instead of clear_compose() linebender/xilem#1692 (IME) and masonry: adopt ui-events text input events linebender/xilem#1700 (ui-events adoption) touch. Mergeable today; expect rebases if those land first.
  3. Commit 1 moves ~120 lines of the maintainer's 4-month-old code verbatim — reviewable, but it's a refactor of his crown-jewel caching. Alternative shapes he might prefer: cache type in masonry_core, or unifying the max_advance calculation too (we deliberately stopped short).
  4. Validation finding kept honest: our fork notes' claim that the hack trips the measurement-cache panic does NOT reproduce upstream (the save/restore keeps returned lengths deterministic; our April panics were in our own widgets). The PR claims only the demonstrated defects: generation nudge, selection refresh through a transient layout, double relayout.

🤖 Generated with Claude Code

@0k-dot-computer

Copy link
Copy Markdown
Owner Author

DRAFT — NOT FOR DIRECT SUBMISSION

Linebender's LLM policy (https://linebender.org/wiki/llm-policy/) bans
AI-generated PR descriptions and requires up-front disclosure of LLM usage.
This draft is raw material only: Lane must rewrite the description in his
own words, own every claim in it, and add his own disclosure paragraph.
A factual basis for that disclosure is at the bottom of this file.


Suggested title

Make TextArea measurement side-effect free.

Draft body (raw material — rewrite before any upstream use)

TextArea::measure does its layout work through the live PlainEditor: it
sets the measured width, lays out, then sets the old width back and lays out
again, behind the existing
// TODO: Remove this hack and do efficient side-effect free measurement with no alignment.
Besides up to two extra full layouts per measure call, the save/restore isn't
faithful: every PlainEditor relayout nudges the generation and re-refreshes
the selection against the transient layout, so a speculative measure mutates
editor state behind the widget's back.

This implements the TODO. measure now builds a dedicated scratch
parley::Layout from the editor's current text and styles — the same
construction as PlainEditor's internal layout, minus the alignment, which
doesn't affect the measured size. The live editor is never touched during
measurement.

The new test (measure_does_not_mutate_editor) fails on main: a parent that
measures its child at min-/max-content bumps the editor generation on every
layout pass, even when nothing about the text area changed.

Notes:

  • The scratch layout skips the IME preedit underline that
    PlainEditor::update_layout pushes; underline is a decoration and doesn't
    affect layout metrics.
  • PlainEditor's quantize flag has no getter, so the scratch builder
    hardcodes the default true (Label does the same with 1.0, true).
  • Trade-off: when measure is called with the max_advance the editor
    already has, the old code reused the editor's cached layout for free; the
    scratch layout is rebuilt on every measure call instead. In exchange, the
    differing-width probes (min-/max-content) no longer do two full editor
    relayouts each, including alignment and selection refresh. A Label-style
    layout cache (Improve Label text layout caching. linebender/xilem#1665) could recover the reuse and would fit the existing
    TODO about unifying this code with Label::measure; left out here to keep
    the change minimal.
  • The test reads editor.generation() directly (it lives in text_area.rs's
    test module). Without debug assertions the second pass hits the measurement
    cache and the test passes vacuously; with debug assertions (dev and ci
    profiles) measurement always re-runs, so the test is live in CI.

The hack dates from the new layout system (linebender#1560).

Factual development notes for Lane's disclosure paragraph

How this change was developed, for the up-front LLM disclosure the policy
requires (write it in your own words):

  • The change originated as a patch carried in the 0k.computer downstream fork
    of masonry (written with LLM assistance there as well).
  • It was then re-derived against upstream HEAD (5d72ad4) by an LLM coding
    agent (Claude, operated by Lane Spangler): the agent re-analyzed the bug in
    upstream's code and parley 0.8's PlainEditor, wrote the test first and
    confirmed it fails on pristine HEAD, ported and reworked the fix (using
    get_styles()/get_scale() instead of the fork's edit_styles() snapshot,
    which itself dirtied the editor), and ran cargo test -p masonry
    (235 passed incl. render snapshots), cargo fmt --check,
    cargo clippy -p masonry --all-features --tests, and
    cargo check --workspace --all-features locally.
  • The code and this description draft are LLM-authored; the description must
    therefore be rewritten and owned by the human submitter per policy.

Pure refactor: `TextLayout` and the caching logic introduced for `Label`
move verbatim to a new crate-private `widgets::text_layout_cache` module,
parameterized on the text and styles instead of reading `Label`'s fields.
This makes the cache usable by `TextArea` measurement in the next commit.

No behavior change; `Label`'s render snapshot tests are unchanged.
Previously `TextArea::measure` did its layout work through the live
`PlainEditor`, temporarily changing its width and then changing it back,
relayouting twice in the process. Beyond the wasted work, the restore
isn't faithful: every relayout nudges the editor's generation and
refreshes the selection against the transient layout, so a speculative
measure invalidates editor state that `measure` must leave untouched.

Instead, measure through a `TextLayoutCache` (the same caching that
`Label` uses) built from the editor's current text and styles, with no
alignment, which doesn't affect the measured size. The cache is cleared
whenever text, styles, or fonts change, mirroring `Label`. The editor is
no longer touched during measurement, resolving the layout-system TODO
about side-effect free measurement.

The first new test fails on the previous commit: a parent measuring its
child at min- and max-content nudged the editor generation on every
layout pass. The second test guards the cache invalidation: inserting
text must grow the measured max-content width.
@0k-dot-computer 0k-dot-computer force-pushed the textarea-side-effect-free-measure branch from 53b648f to 835d541 Compare June 12, 2026 20:29
@0k-dot-computer

Copy link
Copy Markdown
Owner Author

DRAFT — NOT FOR DIRECT SUBMISSION (rev 2, house-pattern rework)

Linebender's LLM policy (https://linebender.org/wiki/llm-policy/) bans
AI-generated PR descriptions and requires up-front disclosure of LLM usage.
This draft is raw material only: Lane must rewrite the description in his
own words, own every claim in it, and add his own disclosure paragraph.
Factual material for that disclosure is at the bottom of this file.


Suggested title

Make TextArea measurement side-effect free.

Draft body (raw material — rewrite before any upstream use)

TextArea::measure does its layout work through the live PlainEditor: it
sets the measured width, lays out, then sets the old width back and lays out
again, behind the existing
// TODO: Remove this hack and do efficient side-effect free measurement with no alignment.
Besides up to two extra full layouts per measure call, the save/restore isn't
faithful: every PlainEditor relayout nudges the generation and re-refreshes
the selection against the transient layout, so a speculative measure mutates
editor state behind the widget's back.

This implements the TODO using the caching approach Label got in linebender#1665:

  1. The first commit is a pure refactor — TextLayout and the cache logic
    from Improve Label text layout caching. linebender/xilem#1665 move verbatim out of label.rs into a crate-private
    widgets/text_layout_cache.rs (TextLayoutCache), parameterized on the
    text and styles instead of reading Label's fields. No behavior change;
    Label's render snapshots are unchanged.
  2. The second commit makes TextArea::measure go through its own
    TextLayoutCache, built from the editor's current text and styles, with
    no alignment (which doesn't affect the measured size). The live editor is
    never touched during measurement. The cache is cleared whenever text,
    styles, or fonts change, mirroring Label::clear_cache — in the style
    setters, reset_text, the keyboard/IME/paste edit paths, and
    Update::FontsChanged.

Measurement also gets Label's layout reuse this way: min-/max-content
probes and repeated measures hit the cache (satisfies) instead of
relayouting the editor twice per differing-width measure.

Two new tests:

  • measure_does_not_mutate_editor fails on main: a parent that measures its
    child at min-/max-content bumps the editor generation on every layout
    pass, even when nothing about the text area changed.
  • measure_reflects_edits guards the cache invalidation: inserting text
    must grow the measured max-content width. (Verified that removing one of
    the clear_measure_cache() calls makes it fail.)

Notes:

  • The cached layouts skip the IME preedit underline that
    PlainEditor::update_layout pushes; underline is a decoration and doesn't
    affect layout metrics. The preedit text is included (built from
    raw_text()), and compose changes clear the cache via the IME path.
  • The cache builds at scale 1.0, quantized — same as Label (and the
    existing Tracking issue for scale factor / dpi handling linebender/xilem#1264 TODO about scale moves with the code). TextArea never
    reconfigures scale/quantization on its editor.
  • This deliberately stops short of unifying the max_advance calculation
    that the other TODO in this function mentions — that's a follow-up that
    doesn't need to block removing the editor mutation.
  • The measure_does_not_mutate_editor test reads editor.generation()
    directly (it lives in text_area.rs's test module). Without debug
    assertions the second pass hits masonry's measurement cache and the test
    is vacuous; with debug assertions (dev and ci profiles) measurement
    always re-runs, so it's live in CI.

The hack dates from the new layout system (linebender#1560); the caching it now uses
is from linebender#1665.

Factual development notes for Lane's disclosure paragraph

How this change was developed, for the up-front LLM disclosure the policy
requires (write it in your own words):

  • What prompted it: In April 2026, automated UI eval runs of our app
    (built on a vendored masonry fork) kept crashing in a loop on masonry's
    measurement-cache stability debug assertion ("Widget 'TextInput' …
    measurement … returned 356 but cache already had 400"), aborting the UI
    process mid-run and cascading through the test suite. Root-causing that
    led to an audit of every measure() implementation for hidden state. The
    actual panicking widgets were our own (a widget returning a cached
    last_size with a 400px fallback — the literal 356-vs-400 in the panic);
    TextArea's save/restore hack was rewritten to side-effect-free
    measurement during that audit, because the hack's hidden editor mutation
    made measure-purity hard to reason about while debugging, and the code's
    own TODO already flagged it. That rewrite has been carried in our fork
    since (2026-04-19).
  • How this PR was produced: the change was re-derived against upstream
    HEAD (5d72ad4) by an LLM coding agent (Claude, operated by Lane
    Spangler): it re-analyzed the bug in upstream's code and parley 0.8's
    PlainEditor, wrote the failing test first and confirmed it fails on
    pristine HEAD, and — per Lane's review of the first draft — reworked the
    fix from a single scratch layout to reuse Label's Improve Label text layout caching. linebender/xilem#1665 cache via the
    extracted TextLayoutCache, adding the invalidation sites and a
    counterfactually-verified invalidation test. Gates run locally:
    cargo test -p masonry (236 passed incl. render snapshots),
    cargo fmt --check, cargo clippy -p masonry --all-features --tests,
    cargo check --workspace --all-features.
  • Honesty note carried into the PR: we did NOT claim the cache-panic
    link upstream — analysis showed upstream's save/restore keeps returned
    lengths deterministic, so the panic we hit was our own widgets' bug. The
    claimed defects are only the demonstrated ones (generation nudge,
    selection refresh through a transient layout, double relayout).
  • The code and this description draft are LLM-authored; the description
    must therefore be rewritten and owned by the human submitter per policy.

@0k-dot-computer

Copy link
Copy Markdown
Owner Author

Disposition: further Label/TextArea unification (Lane asked whether to push for it)

Checked: no anchor issue exists on linebender/xilem or linebender/parley for text-widget unification — only the two in-code TODOs. Recommendation is to sequence, not expand this PR:

  1. max_advance calc helper (the literal TODO): mechanical ~30-line dedupe, LineBreaking vs word_wrap is the only delta. Offer as a small follow-up PR after this one lands, or fold in if the reviewer requests it.
  2. Unified measure semantics: the remaining divergence is deliberate-looking behavior (Label returns content width; TextArea returns the given space, MinContent → 0 — reasonable for an editable field). Changing either is a design call, not a refactor — maintainer's decision.
  3. Full pipeline unification (TextArea's live paint/selection/IME layout on the cache too): blocked on parley — PlainEditor can't adopt an external layout; needs the "split up PlainEditor::layout" surgery. Cross-repo design work in code xStrom owns → discuss-first (issue/Zulip), not a drive-by PR.

Strategic note: c0k's fork burden is fully discharged when THIS PR merges + pin bump (patch 05 retires). Deeper unification doesn't reduce carried divergence — it's goodwill work, best attempted after a merged contribution establishes standing.

🤖 Generated with Claude Code

@0k-dot-computer

Copy link
Copy Markdown
Owner Author

Draft delta (rev 2.1): the "deliberately stops short" note in pr-body-draft.md is now a calibrated follow-up offer — (a) explicitly offer the max_advance dedupe (their literal TODO), (b) name parley-side dirty-tracking invalidation + deeper Label/TextArea unification as direction-seeking only, (c) mark the commit-1 TextLayoutCache extraction as offered-not-insisted (reshape/split on request). Rationale: offers create obligations — only the cheap concrete one is promised; the deep unification is a question, not a workplan. Lane: these are YOUR commitments to honor if accepted — trim anything you wouldn't enjoy following through on.

🤖 Generated with Claude Code

@0k-dot-computer 0k-dot-computer merged commit 2ae2fa1 into main Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant