Skip to content

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

Merged
merged 6 commits into from
Dec 14, 2020

Conversation

justinmc
Copy link
Contributor

@justinmc justinmc commented Dec 5, 2020

Description

The pivot point is the selection edge that does not move:

Pivot point left Pivot point right
pivot_left pivot_right

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:

  • Select some text, hold shift, and move with the arrow keys. Expect the correct behavior.
  • Also, I updated a couple of tests that were expecting reversed base/extent.

@justinmc justinmc self-assigned this Dec 5, 2020
@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 5, 2020
@google-cla google-cla bot added the cla: yes label Dec 5, 2020
@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Dec 10, 2020
@justinmc justinmc marked this pull request as ready for review December 10, 2020 22:55
@justinmc
Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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;
Copy link
Contributor

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

@fluttergithubbot fluttergithubbot merged commit fb3dba7 into flutter:master Dec 14, 2020
@justinmc justinmc deleted the base-extent-order branch December 14, 2020 22:04
@justinmc justinmc added the a: text input Entering text in a text field or keyboard related problems label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add awareness for cursor position in selection-related behaviors across desktop platforms
3 participants