-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Ensure selection collapses if user tries to replace with matching text #1661
Conversation
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.
Hooray!
dancing_taco
I'll get this merged today/tomorrowish, and then do a release now that we have this fix solidified.
@@ -29,13 +29,17 @@ const DEFAULT_SELECTION = { | |||
isBackward: false, | |||
}; | |||
|
|||
//const collapsedSelection = new SelectionState(DEFAULT_SELECTION); |
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.
Thanks for catching this~
@@ -125,5 +129,25 @@ test('editor selectionstate is updated if new text matches current selection', ( | |||
expect(editor.update).toHaveBeenCalledTimes(1); | |||
|
|||
const newEditorState = editor.update.mock.calls[0][0]; | |||
expect(newEditorState.getCurrentContent()).toMatchSnapshot(); | |||
expect(newEditorState.getSelection()).toMatchSnapshot(); |
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 narrows the focus of what this is testing - I think that's ok, just flagging to make sure it's intentional.
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.
Yeah, the test was originally trying to vet SelectionState specifically, and actually serializing ContentState wasn't correct.
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.
@flarnie is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
@flarnie has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: **Summary** This PR ensures that the selection is always collapsed for the cases where a user replaces the current selection with matching text. I admittedly didn't really dig into the original issue when fixing the previous crash :) **Test Plan** Small repro: 1. Type in "abc" 1. Select "c" from right-to-left 1. Type "c" 1. Select "c" from left-to-right 1. Type "c" Thanks to JLarky for bringing this to my intention! Closes facebookarchive/draft-js#1661 Differential Revision: D7055502 fbshipit-source-id: ee4cf80d292ee715e40c017e4c6bd18f806b0e24
Summary: **Summary** This PR ensures that the selection is always collapsed for the cases where a user replaces the current selection with matching text. I admittedly didn't really dig into the original issue when fixing the previous crash :) **Test Plan** Small repro: 1. Type in "abc" 1. Select "c" from right-to-left 1. Type "c" 1. Select "c" from left-to-right 1. Type "c" Thanks to JLarky for bringing this to my intention! Closes facebookarchive/draft-js#1661 Differential Revision: D7055502 fbshipit-source-id: ee4cf80d292ee715e40c017e4c6bd18f806b0e24
Summary
This PR ensures that the selection is always collapsed for the cases where a user replaces the current selection with matching text.
I admittedly didn't really dig into the original issue when fixing the previous crash :)
Test Plan
Small repro:
Thanks to @JLarky for bringing this to my intention!