Repository navigation
[web_paragraph] Fix placeholder positioning and BoxHeightStyle.tight boxes - #193734
Conversation
d8ea669 to
1d285b5
Compare
…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.
1d285b5 to
efe1f92
Compare
…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
There was a problem hiding this comment.
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.
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…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)
…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]>
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…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
…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]>
…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
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
PlaceholderBlock.calculatePlaceholderTopsetblock.advancefrom the line ascent as it was at that moment before foldingblock.ascentinto the line. A placeholder taller than the preceding text (or a later, taller placeholder on the same line) grewline.fontBoundingBoxAscentafterwards inline.updateBoundingBox(block), leaving placeholderadvance.toppositioned against a stale ascent.getBoxesForPlaceholdersreturned those stale rects, so a placeholder box could sit above the line's top (negativetop) and disagree withgetBoxesForRangefor the same range.BoxHeightStyle.tightreturned the height-scaled box for text.getBoxesForRangeusedmultipliedFontBoundingBoxAscent/multipliedHeight, i.e. the box included the extra leading added byTextStyle.height. SkParagraph'sTextLine::getRectsForRangereturns the unscaled glyph bounds fortight(Rect::MakeXYWH(..., -run.ascent(), ..., run.descent())relative to the baseline). The same code path was also applied toPlaceholderBlock, whoseadvanceis already line-relative, producing a double-shifted box.Fix
PlaceholderBlock&addLine: split placeholder layout into two passes:block.calculatePlaceholderMetrics(line.fontBoundingBoxAscent, line.fontBoundingBoxDescent)to compute each placeholder'sascent/descentand folds them into the line metrics vialine.updateBoundingBox(block).line.fontBoundingBoxAscentis final, callsblock.calculatePlaceholderTop(line.fontBoundingBoxAscent)to computetop = lineAscent - ascentand setblock.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.advanceis already relative to the line, so no extra ascent shift..translate(...)onfirstRectforTextBlock, sincefirstRectonly providesleftandwidthwhiletopandbottomare computed perBoxHeightStyle.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. Onmaster:getBoxesForPlaceholdersreturns(0, -12, 50, 18)for the first placeholder whilegetBoxesForRange(0, 1)returns(0, 14, 50, 44).BoxHeightStyle.tight uses unscaled glyph height and respects TextHeightBehavior—height: 3.0withevenleading;tightbox height equals the unscaled line height and is centered in the 60px line. Onmasterthetightbox is 60px tall instead of 22px.paragraph_get_boxes_test.dartandparagraph_placeholders_test.dartpass.--full,preferWebParagraph+forceTestFontsvsorigin/masterb7315d76579e):web_ui):16/16webparagraph/*(+198 ~5 -1, +2 new tests passing) +60/60test/ui/*(+1354 ~5 -41).packages/flutter,894/894suites across8/8shards):+16712 ~1237 -459vs baseline+16710 ~1237 -461(+3 existing tests fixed; 1 apparent golden diff intest/cupertino/text_field_test.dart: Overflow clipBehavior none goldenwhere onorigin/mastertwo bugs cancelled out — missingStrutStyle(forceStrutHeight: true)from [web_paragraph] Apply StrutStyle to line metrics and placeholders #193774 inflated the line ascent by 83px while the old single-passcalculatePlaceholderTopshifted the placeholder by-83pxdetached from its'Hi'text; resolved once [web_paragraph] Apply StrutStyle to line metrics and placeholders #193774 lands).test/painting/text_painter_test.dart: TextPainter line-height half-leading with leading trim.Fixed corpus tests vs
origin/master(3)packages/flutter::test/painting/text_painter_test.dart: TextPainter line-height TextLeadingDistribution falls back to paragraph stylepackages/flutter::test/painting/text_painter_test.dart: TextPainter line-height half-leadingpackages/flutter::test/painting/text_painter_test.dart: TextPainter widget spanPre-launch Checklist
///).