Include fallback line spacing in BoringLayout Metrics#51304
Closed
NickGerleman wants to merge 3 commits into
Closed
Include fallback line spacing in BoringLayout Metrics#51304NickGerleman wants to merge 3 commits into
NickGerleman wants to merge 3 commits into
Conversation
Summary: X-link: facebook/yoga#1811 ## Resubmit This was backed out due to being up the stack from another change that was backed out, but should be safe by itself. ## Original We want to know if an artifact created during measurement can fully be reused after final layout, but the final layout is allowed to be slightly larger due to pixel grid rounding (while still allowing reuse). It's hard to tell after the fact, whether it is larger because of this rounding (though the measure is used), or if it may be a pixel larger for valid reasons. We can expose the unsnapped dimensions of a node to give us this information, and to correlate measurement artifacts. This is most of the time the same as the layout's measured dimension, though I don't think it's safe to use this, since anything else measuring the node after could clobber this (I think `YGNodeLayoutGetOverflow` may also be prone to this as a bug). Changelog: [Internal] Reviewed By: joevilches Differential Revision: D74673119
Summary: We do a lot of incorrect `cei()` in text measurement, compensated later by other incorrect bits. There are a couple of interesting bits here: A "desired width" is how large a hypothetical text layout would like to be. It is a floating point value, and to avoid truncation, a container must be larger than the desired width. So ceiling this is correct. We are also passed available width, which may be an exact specification, not at a subpixel boundary. Ceiling this is totally incorrect, since Yoga will disregard our ceiled version, and we just created a layout, larger than our actual measure will be. We must instead floor `availableWidth`, and we create our layout based off of that, to not give an extra physical pixel, that may not be given to us, if later layout rounding doesn't go our way. We then ceil `desiredWidth` earlier. Finally, when we have an exact measuremode, we create the layout so that it uses guaranteedWidth`, but act as if it takes the full available space, per-contract, which may be a subpixel size larger. This means we cannot create layouts which cause truncation. I used this change as an opportunity to clean up `createLayout()`, since we are gating anyways, to remove the duplicated paths, and to avoid the unnecessary `TextDirectionHeuristic` work for BoringLayout case, and since `StaticLayoutBuilder` already defaults to the heuristic we are manually setting. Changelog: [Android][Fixed] - Fix more text rounding bugs Reviewed By: joevilches Differential Revision: D74685353
Summary: A potential source of truncation we previously experienced with D65171352 is difference in how TextView calculates line spacing with StaticLayout. As of Android 13, this also appies to how TextView creates the metrics of BoringLayout. Oddly, this is not also applied to `BoringLayout.make()` (I think it ends up only impacting metrics), so we don't pass the flag there. See https://cs.android.com/android/_/android/platform/frameworks/base/+/78c774defb238c05c42b34a12b6b3b0c64844ed7 This could cause some theoretical truncation of simple single line text, so let's match TextView expectations. Changelog: [Android][Fixed] - Include fallback line spacing in BoringLayout Differential Revision: D74694483
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74694483 |
Contributor
|
This pull request has been merged in 2fe6c1a. |
Collaborator
|
This pull request was successfully merged by @NickGerleman in 2fe6c1a When will my fix make it into a release? | How to file a pick request? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
A potential source of truncation we previously experienced with D65171352 is difference in how TextView calculates line spacing with StaticLayout.
As of Android 13, this also appies to how TextView creates the metrics of BoringLayout. Oddly, this is not also applied to
BoringLayout.make()(I think it ends up only impacting metrics), so we don't pass the flag there. See https://cs.android.com/android/_/android/platform/frameworks/base/+/78c774defb238c05c42b34a12b6b3b0c64844ed7This could cause some theoretical truncation of simple single line text, so let's match TextView expectations.
Changelog:
[Android][Fixed] - Include fallback line spacing in BoringLayout
Differential Revision: D74694483