Skip to content

[web_paragraph] Fix placeholder positioning and BoxHeightStyle.tight boxes - #193734

Merged
Rusino merged 5 commits into
flutter:masterfrom
Rusino:webparagraph_placeholder_boxes
Oct 16, 2026
Merged

Rusino merged 5 commits into
flutter:masterfrom
Rusino:webparagraph_placeholder_boxes

Conversation

@Rusino

@Rusino Rusino commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Part of #193731. Split out of #193403 (second of four independent PRs; the others are #193733 and #193736, and #193403 itself will be rebased on top once these land).

Problem

  • Placeholder vertical position was computed too early. PlaceholderBlock.calculatePlaceholderTop set block.advance from the line ascent as it was at that moment before folding block.ascent into the line. A placeholder taller than the preceding text (or a later, taller placeholder on the same line) grew line.fontBoundingBoxAscent afterwards in line.updateBoundingBox(block), leaving placeholder advance.top positioned against a stale ascent. getBoxesForPlaceholders returned those stale rects, so a placeholder box could sit above the line's top (negative top) and disagree with getBoxesForRange for the same range.
  • BoxHeightStyle.tight returned the height-scaled box for text. getBoxesForRange used multipliedFontBoundingBoxAscent / multipliedHeight, i.e. the box included the extra leading added by TextStyle.height. SkParagraph's TextLine::getRectsForRange returns the unscaled glyph bounds for tight (Rect::MakeXYWH(..., -run.ascent(), ..., run.descent()) relative to the baseline). The same code path was also applied to PlaceholderBlock, whose advance is already line-relative, producing a double-shifted box.

Fix

  • PlaceholderBlock & addLine: split placeholder layout into two passes:
    • Pass 1 calls block.calculatePlaceholderMetrics(line.fontBoundingBoxAscent, line.fontBoundingBoxDescent) to compute each placeholder's ascent/descent and folds them into the line metrics via line.updateBoundingBox(block).
    • Pass 2, after line.fontBoundingBoxAscent is final, calls block.calculatePlaceholderTop(line.fontBoundingBoxAscent) to compute top = lineAscent - ascent and set block.advance = Rect.fromLTWH(spanShiftFromLineStart, top, span.width, span.height).
  • getBoxesForRange, BoxHeightStyle.tight:
    • TextBlock: top = line.advance.top + line.fontBoundingBoxAscent - block.rawFontBoundingBoxAscent, height = rawFontBoundingBoxAscent + rawFontBoundingBoxDescent (unscaled glyph bounds positioned at the baseline, matching SkParagraph).
    • PlaceholderBlock: top = firstRect.top + line.advance.top, height = span.height — block.advance is already relative to the line, so no extra ascent shift.
    • Removed the unused vertical .translate(...) on firstRect for TextBlock, since firstRect only provides left and width while top and bottom are computed per BoxHeightStyle.

Tests

  • paragraph_get_boxes_test.dart:
    • getBoxesForRange and getBoxesForPlaceholders return matching boxes for placeholders — two placeholders on one line where the second is taller; checks both APIs agree, top >= 0, and the box sizes equal the placeholder sizes. On master: getBoxesForPlaceholders returns (0, -12, 50, 18) for the first placeholder while getBoxesForRange(0, 1) returns (0, 14, 50, 44).
    • BoxHeightStyle.tight uses unscaled glyph height and respects TextHeightBehavior — height: 3.0 with even leading; tight box height equals the unscaled line height and is centered in the 60px line. On master the tight box is 60px tall instead of 22px.
  • Existing paragraph_get_boxes_test.dart and paragraph_placeholders_test.dart pass.
  • Full WebParagraph corpus (--full, preferWebParagraph + forceTestFonts vs origin/master b7315d76579e):
Fixed corpus tests vs origin/master (3)
  • packages/flutter :: test/painting/text_painter_test.dart: TextPainter line-height TextLeadingDistribution falls back to paragraph style
  • packages/flutter :: test/painting/text_painter_test.dart: TextPainter line-height half-leading
  • packages/flutter :: test/painting/text_painter_test.dart: TextPainter widget span

Pre-launch Checklist

…boxes

Split placeholder metric calculation and vertical positioning into two passes so PlaceholderBlock.advance is computed after all placeholders have updated the line ascent, and return unscaled glyph bounds for TextBlock and line-relative bounds for PlaceholderBlock under BoxHeightStyle.tight.
@Rusino
Rusino force-pushed the webparagraph_placeholder_boxes branch from 1d285b5 to efe1f92 Compare October 9, 2026 21:29
@Rusino Rusino added the CICD Run CI/CD label Oct 12, 2026
Hzj-jie pushed a commit to zijiehe-google-com/flutter that referenced this pull request Oct 12, 2026
…flutter#193733)

Part of flutter#193731. Split out of flutter#193403 (first of four independent PRs;
the others are flutter#193734 and flutter#193736, and flutter#193403 itself will be rebased
on top once these land).

### Problem
- `ChildStyleNode._height` converted `style.height ==
ui.kTextHeightNone` to `null` *before* falling back to `parent._height`,
so a child `TextStyle(height: ui.kTextHeightNone)` could never override
a non-null parent height (`ParagraphStyle(height: 3.0)` +
`pushStyle(TextStyle(height: kTextHeightNone))` still laid out the line
at 3x).
- `LineBlock` only checked `span.style.height == null`, so
`ParagraphStyle(height: ui.kTextHeightNone)` (`0.0`) multiplied the line
ascent/descent by `0.0` instead of disabling the height multiplier.

### Fix
- `ChildStyleNode._height` keeps `style.height` as-is: `style.height ??
parent._height`. `kTextHeightNone` now reaches `LineBlock`, which is the
one place that interprets it.
- `LineBlock`: `height == null || height == ui.kTextHeightNone` → raw
font ascent/descent, no scaling (same as the native path, where
`paragraph_builder.cc` sets `has_height_override = style.height !=
kTextHeightNone`).
- `LineBlock` uses `StyleManager.defaultFontSize` instead of a hardcoded
`14.0`.
- `WebStrutStyle` already normalizes `kTextHeightNone` to `null` in its
constructor, so struts need no change.

### Tests
- `paragraph_get_boxes_test.dart`: `layout with kTextHeightNone disables
parent height multiplier`, `layout with
ParagraphStyle(kTextHeightNone)`.
- **WebParagraph corpus (`preferWebParagraph` + `forceTestFonts` vs
`origin/master` `b7315d76579e`)**: 20,012 tests (18,765 executed, 1,247
skipped) — **503 -> 503 failures (0 fixed standalone, +2 new tests
passing, 0 regressions)**.
- *Note:* Existing corpus tests for `kTextHeightNone` also depend on
other issues (`test/ui/paragraph_builder_test.dart: kTextHeightNone
StrutStyle` is unlocked in flutter#193774 once `StrutStyle` is applied;
`test/painting/text_painter_test.dart: kTextHeightNone unsets the text
height multiplier` still fails due to Chromium's integer-rounded
`FlutterTest` font metrics at `10px` where `ascent + descent = 11 !=
10`).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@Rusino
Rusino requested a review from mdebbar October 15, 2026 14:38
@mdebbar
mdebbar marked this pull request as ready for review October 15, 2026 15:16

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors placeholder layout in TextLayout by separating metric calculation from positioning, ensuring placeholders are positioned only after all ascents are folded into the line. It also updates BoxHeightStyle.tight to handle PlaceholderBlock and TextBlock differently, using unscaled glyph bounds for the latter. Corresponding tests are added to verify these behaviors. Feedback suggests simplifying the height calculation for placeholders in BoxHeightStyle.tight by using firstRect.height directly instead of accessing block.span.height.

@flutter-dashboard

Copy link
Copy Markdown

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #193734 at sha f194c20

@flutter-dashboard flutter-dashboard Bot added the will affect goldens Changes to golden files label Oct 15, 2026
mdebbar
mdebbar previously approved these changes Oct 15, 2026
…assert

The placeholder test now asserts the baseline-aligned placeholder ends on
the alphabetic baseline and the bottom-aligned one on the line bottom,
so a stale ascent from a single-pass layout is caught instead of passing
with top == 0. The tight-box assert compared bottom - top against the
very values it was computed from, so it is removed; the comment above it
is rewrapped to fit 100 columns.

Assisted-by: Claude Fable 5.1 (High)
@Rusino
Rusino requested a review from mdebbar October 15, 2026 16:09
mdebbar added a commit to mdebbar/flutter that referenced this pull request Oct 15, 2026
…ult to proportional (flutter#193736)

Part of flutter#193731. Split out of flutter#193403 (third of four independent PRs;
the others are flutter#193733 and flutter#193734, and flutter#193403 is stacked on top of
this PR).

### Problem
- **`TextHeightBehavior.leadingDistribution` was ignored.**
`WebParagraphStyle` never passed
`textHeightBehavior.leadingDistribution` to the root `WebTextStyle`, and
`RootStyleNode._leadingDistribution` always returned `null`, so the
value could not reach `LineBlock`. Only an explicit
`TextStyle(leadingDistribution: …)` had any effect.
- **The default leading distribution was `even`.** `LineBlock` treated
`null` the same as `TextLeadingDistribution.even` (half-leading).
SkParagraph (and therefore the native engine and CanvasKit) default to
`proportional`: `TextStyle::fHalfLeading` is `false` unless
`leadingDistribution == even` is set, and `dart:ui`'s
`TextHeightBehavior.leadingDistribution` defaults to
`TextLeadingDistribution.proportional`.
- **Negative half-leading clamped `fontBoundingBoxDescent` to `0.0`.**
`TextLine` initialized `fontBoundingBoxAscent` and
`fontBoundingBoxDescent` to `0.0` and accumulated every block with
`math.max(0.0, ...)`. With `TextLeadingDistribution.even` and a small
`height` multiplier (where `height * fontSize < rawAscent +
rawDescent`), `extraLeading / 2` is negative and
`multipliedFontBoundingBoxDescent` can become negative; `math.max(0.0,
...)` clamped it to `0.0`, inflating the line height above `height *
fontSize`.

### Fix
- `WebParagraphStyle` propagates
`textHeightBehavior?.leadingDistribution` into the root `WebTextStyle`;
`RootStyleNode._leadingDistribution` returns
`style.leadingDistribution`. Child `TextStyle`s still override it
through `ChildStyleNode` as before.
- `LineBlock`: `null` now falls through to the `proportional` branch
(ascent and descent are scaled by `height * fontSize / (ascent +
descent)`), `even` keeps the half-leading behavior.
- `TextLine.updateBoundingBox`: track `_hasLineMetrics` so the first
block on the line initializes `fontBoundingBoxAscent` and
`fontBoundingBoxDescent` directly (preserving a negative `descent`),
while subsequent blocks take `math.max`.

### Behavior change
For any paragraph with a `height` multiplier and no explicit
`leadingDistribution`, the **line height is unchanged** but the
**baseline moves**: the extra leading is now split in proportion to the
font's ascent/descent instead of 50/50. This brings WebParagraph in line
with CanvasKit/Skwasm output for the same `ParagraphStyle`; WebParagraph
is not the default renderer yet, so no app-facing change on stable.

### Tests
- New `paragraph_line_height_test.dart`:
- `Height multiplier scales ascent and descent proportionally by
default` — checks the scaled line keeps `height * fontSize`, that
ascent/descent scale by the same factor by default, that
`TextHeightBehavior(leadingDistribution: even)` adds equal leading on
both sides, and that the two differ. Fails on `master` (default was
`even` and the THB value was ignored).
- `Half-leading with a small height multiplier preserves negative
descent` — checks that `height: 0.1` with `TextLeadingDistribution.even`
keeps a negative `descent` and total height `0.1 * fontSize`.
- Existing `webparagraph/*` and `ui/paragraph_builder_test.dart` suites
pass.
- **WebParagraph corpus (`preferWebParagraph` + `forceTestFonts` vs
`origin/master` `b7315d76579e`)**: **0 fixed standalone, +2 new tests
passing, 0 regressions** (base of the flutter#193736 -> flutter#193773 -> flutter#193774 ->
flutter#193403 stack that fixes **34** corpus tests; framework `TextPainter
line-height` tests in `test/painting/text_painter_test.dart` also call
`getBoxesForSelection` with `BoxHeightStyle.tight` and are unlocked when
combined with flutter#193734).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

---------

Co-authored-by: Mouad Debbar <[email protected]>
@flutter-dashboard

Copy link
Copy Markdown

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #193734 at sha bca6f0d

@Rusino
Rusino added this pull request to the merge queue Oct 16, 2026
Merged via the queue into flutter:master with commit 77c305e Oct 16, 2026
18 checks passed
@Rusino
Rusino deleted the webparagraph_placeholder_boxes branch October 16, 2026 21:15
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
…flutter#193733)

Part of flutter#193731. Split out of flutter#193403 (first of four independent PRs;
the others are flutter#193734 and flutter#193736, and flutter#193403 itself will be rebased
on top once these land).

### Problem
- `ChildStyleNode._height` converted `style.height ==
ui.kTextHeightNone` to `null` *before* falling back to `parent._height`,
so a child `TextStyle(height: ui.kTextHeightNone)` could never override
a non-null parent height (`ParagraphStyle(height: 3.0)` +
`pushStyle(TextStyle(height: kTextHeightNone))` still laid out the line
at 3x).
- `LineBlock` only checked `span.style.height == null`, so
`ParagraphStyle(height: ui.kTextHeightNone)` (`0.0`) multiplied the line
ascent/descent by `0.0` instead of disabling the height multiplier.

### Fix
- `ChildStyleNode._height` keeps `style.height` as-is: `style.height ??
parent._height`. `kTextHeightNone` now reaches `LineBlock`, which is the
one place that interprets it.
- `LineBlock`: `height == null || height == ui.kTextHeightNone` → raw
font ascent/descent, no scaling (same as the native path, where
`paragraph_builder.cc` sets `has_height_override = style.height !=
kTextHeightNone`).
- `LineBlock` uses `StyleManager.defaultFontSize` instead of a hardcoded
`14.0`.
- `WebStrutStyle` already normalizes `kTextHeightNone` to `null` in its
constructor, so struts need no change.

### Tests
- `paragraph_get_boxes_test.dart`: `layout with kTextHeightNone disables
parent height multiplier`, `layout with
ParagraphStyle(kTextHeightNone)`.
- **WebParagraph corpus (`preferWebParagraph` + `forceTestFonts` vs
`origin/master` `b7315d76579e`)**: 20,012 tests (18,765 executed, 1,247
skipped) — **503 -> 503 failures (0 fixed standalone, +2 new tests
passing, 0 regressions)**.
- *Note:* Existing corpus tests for `kTextHeightNone` also depend on
other issues (`test/ui/paragraph_builder_test.dart: kTextHeightNone
StrutStyle` is unlocked in flutter#193774 once `StrutStyle` is applied;
`test/painting/text_painter_test.dart: kTextHeightNone unsets the text
height multiplier` still fails due to Chromium's integer-rounded
`FlutterTest` font metrics at `10px` where `ascent + descent = 11 !=
10`).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
…ult to proportional (flutter#193736)

Part of flutter#193731. Split out of flutter#193403 (third of four independent PRs;
the others are flutter#193733 and flutter#193734, and flutter#193403 is stacked on top of
this PR).

### Problem
- **`TextHeightBehavior.leadingDistribution` was ignored.**
`WebParagraphStyle` never passed
`textHeightBehavior.leadingDistribution` to the root `WebTextStyle`, and
`RootStyleNode._leadingDistribution` always returned `null`, so the
value could not reach `LineBlock`. Only an explicit
`TextStyle(leadingDistribution: …)` had any effect.
- **The default leading distribution was `even`.** `LineBlock` treated
`null` the same as `TextLeadingDistribution.even` (half-leading).
SkParagraph (and therefore the native engine and CanvasKit) default to
`proportional`: `TextStyle::fHalfLeading` is `false` unless
`leadingDistribution == even` is set, and `dart:ui`'s
`TextHeightBehavior.leadingDistribution` defaults to
`TextLeadingDistribution.proportional`.
- **Negative half-leading clamped `fontBoundingBoxDescent` to `0.0`.**
`TextLine` initialized `fontBoundingBoxAscent` and
`fontBoundingBoxDescent` to `0.0` and accumulated every block with
`math.max(0.0, ...)`. With `TextLeadingDistribution.even` and a small
`height` multiplier (where `height * fontSize < rawAscent +
rawDescent`), `extraLeading / 2` is negative and
`multipliedFontBoundingBoxDescent` can become negative; `math.max(0.0,
...)` clamped it to `0.0`, inflating the line height above `height *
fontSize`.

### Fix
- `WebParagraphStyle` propagates
`textHeightBehavior?.leadingDistribution` into the root `WebTextStyle`;
`RootStyleNode._leadingDistribution` returns
`style.leadingDistribution`. Child `TextStyle`s still override it
through `ChildStyleNode` as before.
- `LineBlock`: `null` now falls through to the `proportional` branch
(ascent and descent are scaled by `height * fontSize / (ascent +
descent)`), `even` keeps the half-leading behavior.
- `TextLine.updateBoundingBox`: track `_hasLineMetrics` so the first
block on the line initializes `fontBoundingBoxAscent` and
`fontBoundingBoxDescent` directly (preserving a negative `descent`),
while subsequent blocks take `math.max`.

### Behavior change
For any paragraph with a `height` multiplier and no explicit
`leadingDistribution`, the **line height is unchanged** but the
**baseline moves**: the extra leading is now split in proportion to the
font's ascent/descent instead of 50/50. This brings WebParagraph in line
with CanvasKit/Skwasm output for the same `ParagraphStyle`; WebParagraph
is not the default renderer yet, so no app-facing change on stable.

### Tests
- New `paragraph_line_height_test.dart`:
- `Height multiplier scales ascent and descent proportionally by
default` — checks the scaled line keeps `height * fontSize`, that
ascent/descent scale by the same factor by default, that
`TextHeightBehavior(leadingDistribution: even)` adds equal leading on
both sides, and that the two differ. Fails on `master` (default was
`even` and the THB value was ignored).
- `Half-leading with a small height multiplier preserves negative
descent` — checks that `height: 0.1` with `TextLeadingDistribution.even`
keeps a negative `descent` and total height `0.1 * fontSize`.
- Existing `webparagraph/*` and `ui/paragraph_builder_test.dart` suites
pass.
- **WebParagraph corpus (`preferWebParagraph` + `forceTestFonts` vs
`origin/master` `b7315d76579e`)**: **0 fixed standalone, +2 new tests
passing, 0 regressions** (base of the flutter#193736 -> flutter#193773 -> flutter#193774 ->
flutter#193403 stack that fixes **34** corpus tests; framework `TextPainter
line-height` tests in `test/painting/text_painter_test.dart` also call
`getBoxesForSelection` with `BoxHeightStyle.tight` and are unlocked when
combined with flutter#193734).

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

---------

Co-authored-by: Mouad Debbar <[email protected]>
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
…boxes (flutter#193734)

Part of flutter#193731. Split out of flutter#193403 (second of four independent PRs;
the others are flutter#193733 and flutter#193736, and flutter#193403 itself will be rebased
on top once these land).

### Problem
- **Placeholder vertical position was computed too early.**
`PlaceholderBlock.calculatePlaceholderTop` set `block.advance` from the
line ascent *as it was at that moment* before folding `block.ascent`
into the line. A placeholder taller than the preceding text (or a later,
taller placeholder on the same line) grew `line.fontBoundingBoxAscent`
afterwards in `line.updateBoundingBox(block)`, leaving placeholder
`advance.top` positioned against a stale ascent.
`getBoxesForPlaceholders` returned those stale rects, so a placeholder
box could sit above the line's top (negative `top`) and disagree with
`getBoxesForRange` for the same range.
- **`BoxHeightStyle.tight` returned the height-scaled box for text.**
`getBoxesForRange` used `multipliedFontBoundingBoxAscent` /
`multipliedHeight`, i.e. the box included the extra leading added by
`TextStyle.height`. SkParagraph's `TextLine::getRectsForRange` returns
the unscaled glyph bounds for `tight` (`Rect::MakeXYWH(...,
-run.ascent(), ..., run.descent())` relative to the baseline). The same
code path was also applied to `PlaceholderBlock`, whose `advance` is
already line-relative, producing a double-shifted box.

### Fix
- `PlaceholderBlock` & `addLine`: split placeholder layout into two
passes:
- Pass 1 calls
`block.calculatePlaceholderMetrics(line.fontBoundingBoxAscent,
line.fontBoundingBoxDescent)` to compute each placeholder's
`ascent`/`descent` and folds them into the line metrics via
`line.updateBoundingBox(block)`.
- Pass 2, after `line.fontBoundingBoxAscent` is final, calls
`block.calculatePlaceholderTop(line.fontBoundingBoxAscent)` to compute
`top = lineAscent - ascent` and set `block.advance =
Rect.fromLTWH(spanShiftFromLineStart, top, span.width, span.height)`.
- `getBoxesForRange`, `BoxHeightStyle.tight`:
- `TextBlock`: `top = line.advance.top + line.fontBoundingBoxAscent -
block.rawFontBoundingBoxAscent`, `height = rawFontBoundingBoxAscent +
rawFontBoundingBoxDescent` (unscaled glyph bounds positioned at the
baseline, matching SkParagraph).
- `PlaceholderBlock`: `top = firstRect.top + line.advance.top`, `height
= span.height` — `block.advance` is already relative to the line, so no
extra ascent shift.
- Removed the unused vertical `.translate(...)` on `firstRect` for
`TextBlock`, since `firstRect` only provides `left` and `width` while
`top` and `bottom` are computed per `BoxHeightStyle`.

### Tests
- `paragraph_get_boxes_test.dart`:
- `getBoxesForRange and getBoxesForPlaceholders return matching boxes
for placeholders` — two placeholders on one line where the second is
taller; checks both APIs agree, `top >= 0`, and the box sizes equal the
placeholder sizes. On `master`: `getBoxesForPlaceholders` returns `(0,
-12, 50, 18)` for the first placeholder while `getBoxesForRange(0, 1)`
returns `(0, 14, 50, 44)`.
- `BoxHeightStyle.tight uses unscaled glyph height and respects
TextHeightBehavior` — `height: 3.0` with `even` leading; `tight` box
height equals the unscaled line height and is centered in the 60px line.
On `master` the `tight` box is 60px tall instead of 22px.
- Existing `paragraph_get_boxes_test.dart` and
`paragraph_placeholders_test.dart` pass.
- **Full WebParagraph corpus (`--full`, `preferWebParagraph` +
`forceTestFonts` vs `origin/master` `b7315d76579e`)**:
- Part 1 (`web_ui`): `16/16` `webparagraph/*` (`+198 ~5 -1`, **+2** new
tests passing) + `60/60` `test/ui/*` (`+1354 ~5 -41`).
- Part 2 (`packages/flutter`, `894/894` suites across `8/8` shards):
`+16712 ~1237 -459` vs baseline `+16710 ~1237 -461` (**+3** existing
tests fixed; **1** apparent golden diff in
`test/cupertino/text_field_test.dart: Overflow clipBehavior none golden`
where on `origin/master` two bugs cancelled out — missing
`StrutStyle(forceStrutHeight: true)` from flutter#193774 inflated the line
ascent by 83px while the old single-pass `calculatePlaceholderTop`
shifted the placeholder by `-83px` detached from its `'Hi'` text;
resolved once flutter#193774 lands).
- Jointly with flutter#193736 + flutter#193773 also unlocks
`test/painting/text_painter_test.dart: TextPainter line-height
half-leading with leading trim`.

<details>
<summary>Fixed corpus tests vs <code>origin/master</code> (3)</summary>

- `packages/flutter` :: `test/painting/text_painter_test.dart:
TextPainter line-height TextLeadingDistribution falls back to paragraph
style`
- `packages/flutter` :: `test/painting/text_painter_test.dart:
TextPainter line-height half-leading`
- `packages/flutter` :: `test/painting/text_painter_test.dart:
TextPainter widget span`
</details>

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD engine flutter/engine related. See also e: labels. platform-web Web applications specifically team-web Owned by Web platform team will affect goldens Changes to golden files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants