text: Fix text align for autosize fields #18494
Conversation
kjarosh
added
text
| num_ticks = 1 | ||
|
|
||
| [image_comparisons.output] | ||
| tolerance = 128 |
There was a problem hiding this comment.
To be sure, why such a high tolerance?
There was a problem hiding this comment.
It might seem that the tolerance is pretty high, but for tests which check the placement of black rectangles over white background, it basically means a half-pixel tolerance. It's pretty common that we're off by half a pixel, e.g. in cases of border corners (different line rasterization algorithms), or glyph placement (font hinting probably?).
There was a problem hiding this comment.
But is it everywhere that's wrong, or just some places? Can we reduce the tolerance and increase the outliers? I'm a little worried by "every pixel can be half wrong" 😅
There was a problem hiding this comment.
I could alternatively do low tolerance + max_outliers, but TBH after making so many text tests I got bored of counting these outliers and making sure they are at the right places.
Edit: I didn't see the comment above when writing this.
There was a problem hiding this comment.
But is it everywhere that's wrong, or just some places? Can we reduce the tolerance and increase the outliers? I'm a little worried by "every pixel can be half wrong" 😅
In this case it's 144 outliers mainly due to border corners being off. When I do low tolerance + 144 outliers it's difficult to know whether there are so many outliers due to small variations in tolerance scattered all over the place (like here), or due to big variations in tolerance in one place (e.g. the glyph is wrongly positioned). So both approaches have their cons. :/
There was a problem hiding this comment.
The solution would be to provide max outliers with max tolerance for each one, so that we could say "there are max 144 outliers, but each one shouldn't be more off than 128".
This makes it easier to pass parameters down and reduces the number of parameters in methods.
This refactor moves code from lower_from_text_spans to LayoutContext.
This refactor moves code from lower_from_text_spans to LayoutContext.
This patch moves code around so that LayoutBox impl is not divided in half.
When horizontal autosize is effective, text width is calculated dynamically and is equal to the width of the longest line in text. That's why this patch makes the text width optional, and in case it's None, the text is laid out two times—first time in order to calculate the proper text width, and the second time knowing what width should be used.
This test verifies how text align is handled when autosize and/or word wrap is set.
Dinnerbone
force-pushed
the
autosize-align
branch
from
53ceab5
to
a9c7575
Compare
kjarosh
removed
the
waiting-on-review
Fixes #18030, progresses #18490, fixes #10660, fixes #1440, fixes #3156.
Supersedes #18092, supersedes #12598.
Basically, when horizontal autosize is effective, we don't know the width of the text to be laid out and we have to calculate it dynamically.
Before this PR, the width was assumed to be the width of the field, which made non-left-aligned text display weirdly.
This PR lays out text two times when autosize is on: the first time to calculate the proper text width, and the second time to provide the final layout knowing the proper width.
Note that we could do it more efficiently by doing multi-pass layout, first pass – basic horizontal layout + vertical layout, second pass – full horizontal layout; but that requires rewriting most of the current layout code. We could do that in the future when we have more test coverage for text fields though (and we're absolutely sure that's how it works)!
Note 2: this PR moves some code around, so make sure to review commit-by-commit and don't be afraid of the number of lines changed.