[web] tighten paragraph bounds estimate#31272
Conversation
yjbanov
force-pushed
the
infinite-width-text
branch
from
b4d5782 to
a2f7d8f
Compare
|
Gold has detected about 3 new digest(s) on patchset 2. |
yjbanov
force-pushed
the
infinite-width-text
branch
from
a2f7d8f to
c959048
Compare
|
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. |
|
Gold has detected about 4 new digest(s) on patchset 2. |
|
Yikes! I clicked in the wrong spot in the flutter-gold UI. I left the image as "?" |
|
|
||
| _paintBounds = ui.Rect.fromLTRB( | ||
| boundsLeft, | ||
| 0, |
There was a problem hiding this comment.
Is the top always 0? What happens if the font has tall ascenders or something like that, does that get computed earlier with the line heights?
There was a problem hiding this comment.
Good point!
The previous version already used the (0, paragraph.height) range for the vertical bounds (see recording_canvas.dart, where offset.dy was used as the top and top + engineParagraph.height as the bottom). That's still the case after this PR.
What is changing are the horizontal bounds. Your concern is still valid for the horizontal case (what if a glyph paints outside its layout bounds due to some swash). Currently dart:ui doesn't provide this information to the framework, so this problem already exists (e.g. the framework may clip or prune contents, thinking that it's not visible). This can probably be worked around by the developer by using extra padding if they know that they are using some font that results in this kind of out-of-bounds painting, and I think horizontal extrusions are less common than vertical ones due to the risk of overlapping with adjacent glyphs.
Maybe this is largely solvable by adding some padding proactively? For example, we could add a certain % of the font size on both sides. WDYT? @mdebbar I'm curious about your thoughts on the topic too.
There was a problem hiding this comment.
I agree with Yegor. Weird characters that paint outside of their bounds are already broken because of clipping, etc.
At this time, I would say it's fair to assume that the top paint bound is 0, especially that there's no concept of vertical alignment (yet) in paragraphs.
There was a problem hiding this comment.
I don't know enough about this, so 🚀
| underTest.endRecording(); | ||
| expect(paragraph.width, 110); | ||
| expect(paragraph.height, 60); | ||
| expect(underTest.pictureBounds, const Rect.fromLTRB(0, 0, 100, 60)); |
There was a problem hiding this comment.
This is expected to render:
┌───────┐
│A │
│AAAAA │
│AAA │
└───────┘
And the one below:
┌───────┐
│ A │
│ AAAAA │
│ AAA │
└───────┘
(Same height, but the bounding box around As is smaller than the container paragraph, also the paragraph needs to be narrow enough to cause a carriage return around the "AAAAA" bit that is 100px wide)
There was a problem hiding this comment.
This is exactly documented in the general_golden_test, genius :P
There was a problem hiding this comment.
LOL, we used the exact same example :) You get the bonus points for using better ASCII art though.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. |
yjbanov
force-pushed
the
infinite-width-text
branch
from
c959048 to
bc06c77
Compare
|
Golden file changes are available for triage from new commit, Click here to view. |
yjbanov
force-pushed
the
infinite-width-text
branch
from
bc06c77 to
498624f
Compare
|
Gold has detected about 1 new digest(s) on patchset 2. |
|
Gold has detected about 4 new digest(s) on patchset 2. |
* [web] tighten paragraph bounds estimate
4 participants
Use the actual text bounds when estimating paragraph paint bounds instead of the paragraph size. Paragraph size can frequently be bigger than the text. Common situations:
double.infinitywidth constraint. While the constraint is infinite, that doesn't make the text infinitely wide. The text will still only take as much space as it needs.TextAlign.centeris used the text is shifted towards the center of the paragraph, so the left and right bounds of the text are padded away from the left and right walls of the paragraph.In the example below center alignment is used. Notice that the text bounds (green box) is smaller than the paragraph bounds (black box):
Fixes: flutter/flutter#97756