-
Notifications
You must be signed in to change notification settings - Fork 28.7k
Correct text selection pivot points #71756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…orrects pivot points.
dd3f50b
to
967a9fa
Compare
e2544ec
to
3a304d5
Compare
Reviewers: Can you think of any reason why we were swapping base/extent before? This seems to work better without the swap and doesn't seem to break major tests, but I'm keeping an eye out. |
baseOffset = math.min(fromPosition.offset, toPosition.offset); | ||
extentOffset = math.max(fromPosition.offset, toPosition.offset); | ||
} | ||
final int baseOffset = fromPosition.offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to revert the changes made in #29395. According to @mdebbar
The problem is if you drag from left to right, the text will be selected as expected. But if you drag from right to left, then baseOffset will be greater than extentOffset and it'll be collapsed down to a cursor. So dragging from right to left is broken, and this PR fixes it.
Not sure why the tests are passing. But it sounds like we're not handling baseOffset > extentOffset correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for tracking that PR down. It seems like the bug that that fixed, where the cursor would collapsed when selecting right to left, no longer happens. Everything still seems to be fine with this PR here by my testing.
I should point out that we're still reversing base and extent in the underlying TextRange. That seems to have been around for a long time, though.
flutter/packages/flutter/lib/src/services/text_editing.dart
Lines 18 to 26 in 72aa23e
const TextSelection({ | |
required this.baseOffset, | |
required this.extentOffset, | |
this.affinity = TextAffinity.downstream, | |
this.isDirectional = false, | |
}) : super( | |
start: baseOffset < extentOffset ? baseOffset : extentOffset, | |
end: baseOffset < extentOffset ? extentOffset : baseOffset, | |
); |
extentOffset = math.max(fromPosition.offset, toPosition.offset); | ||
} | ||
final int baseOffset = fromPosition.offset; | ||
final int extentOffset = toPosition == null ? fromPosition.offset : toPosition.offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: from the previous code it could be toPosition?.offset ?? fromPosition.offset
Description
The pivot point is the selection edge that does not move:
Currently, Flutter always puts the pivot point on the right, but as you can see in the gifs above of native OSX, other platforms put it where the selection began. This PR updates Flutter to do the same.
There are more complicated platform-specific behaviors in other selection scenarios, such as when using word and line modifier keys, but that isn't handled by this PR. See #57679 (comment).
Related Issues
Closes #57679
Tests
I added the following tests: