Include cursor in textfield intrinsic width measurement#25055
Include cursor in textfield intrinsic width measurement#25055justinmc merged 3 commits intoflutter:masterfrom
Conversation
|
@HansMuller What do you think of this solution, any implications that I'm not thinking of? There was only 1 test that failed when I made this change (see the 1 pixel that I changed in a test). |
zoechi
added
a: text input
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This is looking good. Just needs a little more test coverage
| double computeMaxIntrinsicWidth(double height) { | ||
| _layoutText(double.infinity); | ||
| return _textPainter.maxIntrinsicWidth; | ||
| return _textPainter.maxIntrinsicWidth + cursorWidth; |
There was a problem hiding this comment.
We should probably assert(cursorWidth != null && cursorWidth >= 0.0) in the RenderEditable constructor. There are many other places in the code that appear to make this assumption. Not sure if cursorWidth == 0.0 really works; if not assert that it's > 0.0.
There was a problem hiding this comment.
It looks like cursorWidth 0.0 works and results in not showing the cursor at all, so I'm using >= in the assertion.
| expect(tapCount, 0); | ||
| }); | ||
|
|
||
| testWidgets('Includes cursor for TextField', (WidgetTester tester) async { |
There was a problem hiding this comment.
We should also verify that if the the TextField is created within an IntrinsicWidth with no stepWidth specified, that the result is a textfield that's just wide enough to drive to the cursor to the end without scrolling.
Tests use the fixed-width Ahem font by default, so you can easily predict the textfield's exact width.
Should check this with a textfield that's textAlign: TextAlign.right and TextAlign.left
| ); | ||
| expect(editable.getMinIntrinsicWidth(double.infinity), 50.0); | ||
| expect(editable.getMaxIntrinsicWidth(double.infinity), 50.0); | ||
| expect(editable.getMaxIntrinsicWidth(double.infinity), 51.0); |
There was a problem hiding this comment.
Should add a comment here about including the the 1.0 cursorWidth
|
@HansMuller Done and ready for re-review |
* flt_master: Actively reject UiKitView gestures. (flutter#25792) Roll engine e75fad7..9daf5c8 (3 commits) (flutter#25786) Roll engine d8c5ec0..e75fad7 (5 commits) (flutter#25757) Include cursor in textfield intrinsic width measurement (flutter#25055)
|
This had some unfortunate impact on some of the performance metrics. |
|
Hmm, I'll try to dig into that. |
|
For the record this code change seems to be absolved of causing performance problems now. Performance is back to normal after something was changed with the bots. |
5 participants
Closes #24612
When using
TextFieldwithInstrinsicWidth, the width of the cursor was not included in the measurement of the width of the text content. This caused content to scroll slightly instead of triggering a bump up to the next width step size. Here the string reads "123456", but it has been scrolled so that the 1 is not visible:This PR includes the cursor width in the calculation, so the width will increase before scrolling happens. Here I've used a large step size to illustrate the bump up in size with the same string: