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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop dragging only when user interacts with keyboard #15916

Merged
merged 1 commit into from Oct 18, 2017

Conversation

Projects
None yet
4 participants
@as-cii
Member

as-cii commented Oct 17, 2017

Refs: #15407

Previously, we prevented users from dragging the selection further when the buffer was about to change. This was problematic because any change in the buffer, even one that was performed "automatically" by a package, would cancel the dragging action and result in a confusing experience for the user. On the other hand, we want to prevent users from accidentally selecting text when they perform an edit (see #15217, #15405).

This pull-request addresses both concerns by canceling the dragging as soon as the user interacts with the keyboard, instead of canceling the dragging when the buffer is about to change. One downside of this approach is that it changes the behavior of pressing a key that does not result in a buffer change, e.g. Shift, Right-Arrow, etc. In fact, pressing one of those keys will now prevent users from dragging the selection further. This seems like an acceptable tradeoff, but we are open to revisiting it in case it introduces pain in some other area of the system.

馃崘'd with @jasonrudolph

/cc: @nathansobo @Ben3eeE @ungb

Stop dragging only when user interacts with keyboard
Previously, we used to prevent the user from dragging the selection
further when the buffer was about to change. This was problematic
because any change in the buffer, even one that was performed
"automatically" by a package, would cancel the dragging action and
result in a confusing experience for the user.

On the other hand, we want to prevent users from accidentally selecting
text when they perform an edit (see #15217, #15405).

This commit addresses both concerns by canceling the dragging as soon as
the user interacts with the keyboard, instead of canceling the dragging
when the buffer is about to change.

One downside of this approach is that it changes the behavior of
pressing a keystroke that does not result in a buffer change, e.g.
Shift, Arrow Keys, etc.

Signed-off-by: Jason Rudolph <jasonrudolph@github.com>
@nathansobo

Looks great. We could potentially use textInput events instead of keydown, but I think this is probably fine.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 18, 2017

Member

The current implementation also fixes atom/find-and-replace#520 which is a bad experience for touchpad users.

I think that this makes sense and now the interruption is entirely in the users control. Instead of being interrupted by automatic changes that can be outside of their control. I vote that we ship this and revisit if we get issues for it.

Member

Ben3eeE commented Oct 18, 2017

The current implementation also fixes atom/find-and-replace#520 which is a bad experience for touchpad users.

I think that this makes sense and now the interruption is entirely in the users control. Instead of being interrupted by automatic changes that can be outside of their control. I vote that we ship this and revisit if we get issues for it.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 18, 2017

Member

Looks great. We could potentially use textInput events instead of keydown, but I think this is probably fine.

Thanks for the review. Yeah, unfortunately I think using keydown is necessary because the user could hit Backspace, Delete or paste content too.

I think that this makes sense and now the interruption is entirely in the users control. Instead of being interrupted by automatic changes that can be outside of their control. I vote that we ship this and revisit if we get issues for it.

Nice! Thanks for testing this.

Member

as-cii commented Oct 18, 2017

Looks great. We could potentially use textInput events instead of keydown, but I think this is probably fine.

Thanks for the review. Yeah, unfortunately I think using keydown is necessary because the user could hit Backspace, Delete or paste content too.

I think that this makes sense and now the interruption is entirely in the users control. Instead of being interrupted by automatic changes that can be outside of their control. I vote that we ship this and revisit if we get issues for it.

Nice! Thanks for testing this.

@as-cii as-cii merged commit 280253f into master Oct 18, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@as-cii as-cii deleted the as-jr-stop-dragging-when-interacting-with-keyboard branch Oct 18, 2017

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 18, 2017

Member

I will cherry-pick this to beta only, so that it can go out on stable during the next railcar.

Member

as-cii commented Oct 18, 2017

I will cherry-pick this to beta only, so that it can go out on stable during the next railcar.

as-cii added a commit that referenced this pull request Oct 18, 2017

Merge pull request #15916 from atom/as-jr-stop-dragging-when-interact鈥
鈥ng-with-keyboard

Stop dragging only when user interacts with keyboard
@alexdevero

This comment has been minimized.

Show comment
Hide comment
@alexdevero

alexdevero Nov 1, 2017

@as-cii Is there a way to disable this? It causes issues when I want to select multiple selections of text - make one selection, hold CTRL and select another. Now, this no longer works.

alexdevero commented Nov 1, 2017

@as-cii Is there a way to disable this? It causes issues when I want to select multiple selections of text - make one selection, hold CTRL and select another. Now, this no longer works.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 1, 2017

Member

@alexdevero Can you open an issue and reference this PR? This is something that we want to address but it's easy that it will get lost if we don't have an issue to track.

Member

Ben3eeE commented Nov 1, 2017

@alexdevero Can you open an issue and reference this PR? This is something that we want to address but it's easy that it will get lost if we don't have an issue to track.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

@Ben3eeE I can open an issue. This is a pretty serious regression.

Contributor

nathansobo commented Nov 1, 2017

@Ben3eeE I can open an issue. This is a pretty serious regression.

@alexdevero

This comment has been minimized.

Show comment
Hide comment
@alexdevero

alexdevero commented Nov 1, 2017

Thank you @nathansobo.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Nov 1, 2017

Member

@nathansobo I agree. I would have opened it myself as soon as I could if nothing else 馃榾

I think we should address this while 1.22 is still in beta.

Member

Ben3eeE commented Nov 1, 2017

@nathansobo I agree. I would have opened it myself as soon as I could if nothing else 馃榾

I think we should address this while 1.22 is still in beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment