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

Don't select when typing while also holding the left mouse button #15407

Merged
merged 2 commits into from Aug 22, 2017

Conversation

Projects
None yet
3 participants
@as-cii
Member

as-cii commented Aug 22, 2017

Fixes #15217
Fixes #15405

This pull-request addresses a regression that was causing edits performed while users were holding the left mouse button to result in unwanted selections.

Before After
before after

Back in Atom 1.18, we used to handle this subtle behavior via an onWillChange event subscription on the buffer:

disposables.add(@editor.getBuffer().onWillChange(onMouseUp))

And this pull-request uses the same strategy to work around the problem.

/cc: @nathansobo @Ben3eeE @ungb

@Ben3eeE

It seems that there was a test for this in Atom 1.18 that was removed in the editor rendering rewrite PR.

https://github.com/atom/atom/blob/1.18-releases/spec/text-editor-component-spec.js#L2734

The test was added in #8627

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Aug 22, 2017

Member

Yeah, I thought about it but we have changed the way that code path is exercised and testing this specific behavior would be a little trickier now. In particular, we don't synthesize mouseup and mousemove events anymore, but we spy on handleMouseDragUntilMouseUp instead and assume it does the right thing.

Initially I thought about moving onWillChange event handler up the call stack so that we can test it, but that would require duplicating some logic for lines and line numbers drag handlers. Another thought was to synthesize mouse events just for this one test, but it wouldn't blend in with the existing ones and seemed suboptimal.

After some more thinking, in e6b84db I ended up writing a unit test that exercises the lower level bits implemented in handleMouseDragUntilMouseUp by synthesizing mousemove and mouseup events, and keep using the spy in the old specs that tested the higher level logic.

Member

as-cii commented Aug 22, 2017

Yeah, I thought about it but we have changed the way that code path is exercised and testing this specific behavior would be a little trickier now. In particular, we don't synthesize mouseup and mousemove events anymore, but we spy on handleMouseDragUntilMouseUp instead and assume it does the right thing.

Initially I thought about moving onWillChange event handler up the call stack so that we can test it, but that would require duplicating some logic for lines and line numbers drag handlers. Another thought was to synthesize mouse events just for this one test, but it wouldn't blend in with the existing ones and seemed suboptimal.

After some more thinking, in e6b84db I ended up writing a unit test that exercises the lower level bits implemented in handleMouseDragUntilMouseUp by synthesizing mousemove and mouseup events, and keep using the spy in the old specs that tested the higher level logic.

@Ben3eeE

LGTM 👍

@Ben3eeE Ben3eeE referenced this pull request Aug 22, 2017

Closed

Moving lines by control-command up/down goes wrong #15405

1 of 1 task complete
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Aug 22, 2017

Member

It seems that some commands are not working correctly even with this guard. This was an issue before 1.19 was released so it's not a regression or issue with this PR. Just mentioning it here in case someone is testing and discovers this. /cc: atom/find-and-replace#520

Member

Ben3eeE commented Aug 22, 2017

It seems that some commands are not working correctly even with this guard. This was an issue before 1.19 was released so it's not a regression or issue with this PR. Just mentioning it here in case someone is testing and discovers this. /cc: atom/find-and-replace#520

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 22, 2017

Contributor

Nice work. Sorry about this regression.

Contributor

nathansobo commented Aug 22, 2017

Nice work. Sorry about this regression.

@nathansobo nathansobo merged commit a1fdf52 into master Aug 22, 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

@nathansobo nathansobo deleted the as-fix-typing-while-holding-left-mouse-button branch Aug 22, 2017

as-cii added a commit that referenced this pull request Aug 23, 2017

Merge pull request #15407 from atom/as-fix-typing-while-holding-left-…
…mouse-button

Don't select when typing while also holding the left mouse button

as-cii added a commit that referenced this pull request Aug 23, 2017

Merge pull request #15407 from atom/as-fix-typing-while-holding-left-…
…mouse-button

Don't select when typing while also holding the left mouse button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment