Keep multiline CupertinoTextField placeholders in bounds#183622
Keep multiline CupertinoTextField placeholders in bounds#183622mayanksharma9 wants to merge 3 commits intoflutter:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses a layout regression in CupertinoTextField where a multiline placeholder could be rendered partially out of bounds. The fix adjusts the layout logic in _RenderBaselineAlignedStack to consider the combined height of the editable text and the placeholder for vertical alignment, ensuring both remain within the field's bounds. A new widget test has been added to verify this behavior and prevent future regressions.
victorsanni
left a comment
There was a problem hiding this comment.
Hi, this should target master not main.
|
Retargeted to master. Thanks for catching that. |
| final double dy = Alignment( | ||
| 0.0, | ||
| textAlignVertical.y, | ||
| ).alongOffset(Offset(0.0, size.height - alignedGroupHeight)).dy; |
There was a problem hiding this comment.
Are we losing information about size.width by using zero horizontal offset? Might this affect positioning in RTL scenarios?
| final double placeholderBottom = placeholderTop + (placeholder?.size.height ?? 0.0); | ||
| final double alignedGroupTop = math.min(0.0, placeholderTop); | ||
| final double alignedGroupBottom = math.max(editableText.size.height, placeholderBottom); | ||
| final double alignedGroupHeight = alignedGroupBottom - alignedGroupTop; |
There was a problem hiding this comment.
Instead of these manual calculations, can we use Rect.expandToInclude to create the aligned group box?
|
Thanks for the detailed feedback. I pushed e9ff419 and updated the layout math to build the combined bounds with Rect.expandToInclude, then compute vertical placement directly from textAlignVertical.y so we do not rely on Alignment with a synthetic horizontal offset. I also added an RTL regression test for the multiline placeholder bounds case. Could you take another look? |
|
Note to reviewers: this user appears to be running an unsupervised or poorly-supervised agent, and has received a warning and temporary ban. If interaction resumes next week, approach the review with caution. |
|
Hi @mayanksharma9 thank you for contributing. Given the velocity and quality of changes we received from this account recently, we put a 7 day hold on actions in the Flutter org. Please review our AI contribution guidelines before continuing to contribute. We'll be happy to continue accepting contributions that follow these guidelines once the hold lifts. Thank you! 💙 |
| final Rect placeholderRect = Offset(0.0, placeholderTop) & (placeholder?.size ?? Size.zero); | ||
| final Rect alignedGroupRect = editableTextRect.expandToInclude(placeholderRect); | ||
| final double availableDy = size.height - alignedGroupRect.height; | ||
| final double dy = (textAlignVertical.y + 1.0) * availableDy / 2.0; |
There was a problem hiding this comment.
Can you explain the arithmetic here?
|
Thanks for your contribution! We are not able to merge this PR at the moment due to the Material/Cupertino code freeze (#184093). As soon as the decoupling is complete, we will post a way to port this PR to the new packages to land. In the meantime, I'll convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Closing as the submitting account has been banned. |
This fixes a recent regression in
CupertinoTextFieldplaceholder layout.When a
CupertinoTextFieldhas a multiline placeholder andmaxLines: null, the placeholder is baseline-aligned relative to the editable text, but the stack was vertically aligning only the editable child. That could place the placeholder partially outside the text field bounds.This change keeps the baseline alignment behavior, but vertically aligns the combined placeholder/editable-text group instead of just the editable child. That keeps multiline placeholders inside the field bounds while preserving the existing baseline-alignment behavior for single-line placeholders.
Fixes #183529
Tests
./bin/flutter test packages/flutter/test/cupertino/text_field_test.dart --plain-name 'Multiline placeholder remains within text field bounds when maxLines is null'./bin/flutter test packages/flutter/test/cupertino/text_field_test.dart --plain-name 'Placeholder is baseline aligned with text'Pre-launch Checklist
///).