Add preferredLineHeightAtOffset to obtain measured metrics for full cursor height#24265
Add preferredLineHeightAtOffset to obtain measured metrics for full cursor height#24265GaryQian wants to merge 15 commits intoflutter:masterfrom
Conversation
|
cc @Hixie This adds API TextPainter.PreferredLineHeightAtOffset(int offset) to attempt to get the measured line height at the provided offset in the string. |
There should be an issue that itemizes the additional work; refer to it in the description. |
|
Might be helpful with #20694 too |
This doesn't quite make it look like that in iOS. Without a taller script/glyph, we will still have the old heights of the cursor (at least with this patch). There needs to be more work to obtain a cursor that matches something like the cursor in iOS. |
|
Right, iOS would be closer to BoxHeightStyle.includeLineSpacingMiddle. Though are you sure the current PR is how it works on Android too? On multi-line text fields with small glyphs, it seems like adjacent lines' cursors touch regardless. |
|
It looks like many android apps have similar behavior to iOS. What we currently do for the cursor is completely wrong and is just guessing at the potential height of a line. This PR does not yet modify it such that we replicate iOS or Android, but rather just makes sure the cursor at least covers the full height of the glyph it is associated with. I can make a separate patch that can accomplish this but it will likely be more complicated as we would have to rework the way we get the coordinates for the beginning of the cursor as well as the height. |
|
This needs tests. The tests should check all the code here (there's lots of new code to test), including especially testing how the surrogate checking code handles low and high surrogates. |
|
Doing OS fidelity in a separate PR is fine. As long as you're moving in a monotonic direction and aren't dismantling code or tests you're writing now to do it :) |
|
Pushed new changes, The TextField now attempts to measure actual metrics before defaulting to the predicted metrics we used before in most places. This resolves the Chinese (and other languages) clipping issue, as well as the cursor height. I removed the hard-coded caret height reduction of 2 pixels, as it better matches both Android as well as iOS. I also exposed needsLayout as a getter in TextPainter, which is useful to check when certain methods dependent on layout are available for use. Will add tests now. |
GaryQian
force-pushed
the
cursorheight
branch
from
28619ec to
596ec87
Compare
| // removed and replaced with a less biased alternative. We can choose what | ||
| // character to use based of the locale and the ScriptCategory field of | ||
| // MaterialLocalization. | ||
| builder.addText(' '); |
There was a problem hiding this comment.
what's wrong with the space character?
The code here doesn't know about Material, let alone localizations or anything like that.
There was a problem hiding this comment.
The space used in alphabetic scripts (0x0020) is not the same dimensions as the space used in ideographic scripts (0x3000). Alphabetic space is both shorter and narrower. This is causing our guess at line height to be too short when the user is expecting to type scripts like CJK, which impacts the height of the text field as well as the height of the cursor.
We have access to a Locale object from dart:ui here, I figured we might be able to do something with it.
There was a problem hiding this comment.
Hm. Really ideally we'd never change the caret size... it's a pity we don't have a way to just pick the "right" size for all text. Maybe we could measure the string U+0020 U+3000 ?
The locale from dart:ui isn't helpful here. We have no idea what the locale of the actual TextPainter's text is (it might have nothing to do with the locales in dart:ui).
There was a problem hiding this comment.
Measuring a string with both U+0020 and U+3000 would make the height almost always be the height of U+3000 as it is consistently taller than U+0020. This is essentially the same as just defaulting to U+3000 which would cause fields with alphabetic text to behave strangely (box too tall, caret too tall, etc).
GaryQian
force-pushed
the
cursorheight
branch
from
de0e0a0 to
aea3db4
Compare
zoechi
added
a: text input
| caretRect = editable.getLocalRectForCaret(const TextPosition(offset: 4)); | ||
| // TODO(garyq): Not a round number because of rounding in LibTxt height | ||
| // metrics. This rounding is slated to be removed as it is no longer | ||
| // needed. This test should be fixed when the rounding is removed. |
There was a problem hiding this comment.
please include a link to the relevant issue, so that audits of issue links will reveal this
|
cc @goderbauer for deeper review |
|
When can this problem be solved?I still feel no change on the v1.1.2 version. |
|
So it turns out that the full solution to this requires additional work by supporting a paragraphStyle-level leading/height that functions as a min line height for the text. Since this patch would only be a temporary fix and introduces another significant bug, I will be pursuing the full proper fix instead. I will post more details about it soon. |
|
#26085 Is a tracking issue that enumerates the work needed to implement "Strut" which will allow better defined line heights and ability to add "buffer zone" line padding that allows taller fonts to not impact line height. |
|
A modified version of this may be able to be implemented and landed after the strut features are added to LibTxt and the framework. Strut should be able to prevent the jumping text that this PR in its current state causes. The new changes will likely be done in a separate PR. |
|
Closing this PR for now, and a new strut-based fix will be written. |
7 participants
This fixes #24182, #23934, #20694
This should also fix #20183 as well as other similar bugs reporting clipping of chinese and other tall-glyphed scripts.
Before, we were blindly using the height of a latin
' 'space character as the height of both the TextField as well as the cursor height. This PR calculates the actual height of the laid out text when it has been laid out and falls back to the old method before layout. I also track the max line height, and return the max height when there is a script taller than latin' 'being used in the TextField.This does introduce the possibility of the TextField height changing as taller glyphs are typed (for example, a string like (
'Hello World 则没有这个问题'will cause the TextField to become taller when the Chinese is typed, as well as the overall text to shift slightly down), but it does seem consistent with some of the text editors in the wild.There should be further work in the future to more consistently align the positions of ideographic/hanging script to alphabetic characters, as what we currently do seems slightly off in certain situations. (Tracked at #24317)