Adjust handle anchor offset for collapsed text selection for Android #169715
Conversation
…extSelectionControls
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
a: text input
|
@kenzieschmoll |
|
@muhammadkamel code reviewers are assigned during weekly triage, please give us time to do so instead of pinging maintainers. They may not have any knowledge of this part of the codebase. |
Hi @Piinks Got it! |
There was a problem hiding this comment.
Thank you for fixing this! A one line fix but I think it makes it look a lot better.
Can you add a test? Probably something in text_selection_test.dart that shows the handle, gets the handle's Rect and the cursor's Rect, and then verifies the position of the handle in relation to the cursor.
I took a screenshot of my Pixel 9 Android 16 running the Google Keep app and zoomed in to confirm that the handle position matches what is shown in this PR 👍 .
| @@ -112,7 +112,7 @@ class MaterialTextSelectionControls extends TextSelectionControls { | |||
| @override | |||
| Offset getHandleAnchor(TextSelectionHandleType type, double textLineHeight) { | |||
| return switch (type) { | |||
| TextSelectionHandleType.collapsed => const Offset(_kHandleSize / 2, -4), | |||
| TextSelectionHandleType.collapsed => const Offset(_kHandleSize / 2.18, -4.5), | |||
There was a problem hiding this comment.
Can you add a comment explaining how you came up with these values? Something like:
// These values were eyeballed to match a physical Pixel 9 Running Android 16.
|
Also, are you aware of an issue for this? |
Updated the calculation of the anchor point for TextSelectionHandleType.collapsed to properly account for the cursor width. Changed from: Offset(_kHandleSize / 2, -4.6) to: Offset((_kHandleSize - cursorWidth) / 2, -4.6) This ensures that the anchor aligns with the actual visual center of the cursor. This change is necessary because the total handle size is 48x48 pixels, which is larger than the actual cursor width (2px), so centering must be adjusted accordingly.
|
Hi @justinmc I have updated the calculation of the anchor point for TextSelectionHandleType.collapsed to properly account for the cursor width. Changed from: This ensures that the anchor aligns with the actual visual center of the cursor. |
|
Also, this is a new screenshot after applying this calculation: |
The text selection anchor/handle in InputField was slightly misaligned, shifting slightly to the left. This PR fixes the handle positioning to ensure proper alignment with the text cursor.
Before:
After fix:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.