Center iOS caret, remove constant offsets that do not scale #31687
Conversation
GaryQian
added
a: text input
|
Tests? Maybe a golden? |
|
Unfortunately we haven't added a golden from #27851 yet. Could you add a golden test on the CupertinoTextField and TextField for the caret? |
There was a problem hiding this comment.
Just for sake of the PR title, if you're changing iOS and non-iOS behavior, remove the iOS part from the description (if I understood correctly)
There was a problem hiding this comment.
There was a value that is added here and then subtracted later, this just removes the redundant math for android
There was a problem hiding this comment.
This is a bit hard to read. Maybe make it a local variable with a self-documenting name?
There was a problem hiding this comment.
This might not be meaningful since we'll never call updateFloatingCursor on Android
There was a problem hiding this comment.
OK, I can remove this then, to prevent any unnecessary pain in future breakages.
There was a problem hiding this comment.
You'd want to put the thing you want to golden test against in a repaintboundary. Otherwise you end up with a really big image like https://github.com/flutter/goldens/blob/master/packages/flutter/test/cupertino/text_field_cursor_test.0.0.png.
You'd also want to isolate it from things like the debug banner etc that aren't specifically the scenario you're trying to test.
|
I tested the branch a bit locally. It seems like on CupertinoTextField, this introduces a small vertical shift in the text when dragging the selection handle. https://streamable.com/d0nia We should resolve it before merging. |
|
Have confirmed that the jitter is not due to this patch. It repros on iPhone XR on master. May have to do with an unintended side effect of a21a1f4? |
5 participants
Description
This vertically centers the caret over the text and removes the constant vertical position offset, which was causing the caret to be at a bad height (too high) especially for small fonts.
After this PR lands, the remaining issues regarding caret primarily deal with determining what to set the height to. This change should bring it to a strictly better and reasonable state for both iOS and Android, but may not quite replicate the exact platform behavior.
Related Issues
#31661
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?