Fixed an issue with controlled value messing up editor.selection #3652
Conversation
|
|
||
| hasRange(editor: ReactEditor, range: Range): boolean { | ||
| const { anchor, focus } = range | ||
| return ( |
There was a problem hiding this comment.
this probably should also handle .offset of both anchor and focus?
| // but Slate's value is not being updated through any operation | ||
| // and thus it doesn't transform selection on its own | ||
| if (selection && !ReactEditor.hasRange(editor, selection)) { | ||
| editor.selection = ReactEditor.toSlateRange(editor, domSelection) |
There was a problem hiding this comment.
I'm not entirely sure if this covers everything - it's a quite simple fix. Truth to be told old & new values can be vastly different so technically weird things might always happen here. OTOH we only should have selection set when user is interacting with the element and during that that no overly funky stuff should happen with value between renders.
|
Hi @Andarist , thanks again for the contribution. I'm still looking at this and the change seems to be an improvement. I'm curious though about the initial use case though. The identity of the slate nodes should not be changing super frequently, but we've seen users run into this by trying to do round trip serialization/deserialization in onChange, or introduce their own immutability into redux by doing a shallow clone of the editor value via a spread operator. Both of those should be avoidable by making serialization a one way operation, and by relying on the immutability already provided by immer. Am I correct in understanding that the original issue was encountered through one of the above workflows? Thanks again for all of your contributions, we appreciate the help you're giving the library |
I've created a wrapper around
As described above, not exactly - I just keep a different representation of my controlled value which gets fed to Slate, and thus I need to convert it to Slate nodes. I don't make copies of the structure just to ensure it's not mutated or anything like that.
No worries, I always try to give back to projects I'm using 😉. In general, I feel less confident in this change than the other one though. Not that I think the problem doesn't exist, but rather because it's somewhat a hard problem and can't (?) be handled super-gracefully, the proposed approach is more of a best-effort attempt than anything else. This has to be reviewed with caution. |
🦋 Changeset detectedLatest commit: ca37760 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for this @Andarist! |
…stormtaylor#3652) * Fixed an issue with controlled value messing up editor.selection * Create fifty-ducks-sip.md Co-authored-by: Ian Storm Taylor <ian@ianstormtaylor.com>
Is this adding or improving a feature or fixing a bug?
fixing a bug
Description
The problem can be observed here by focusing on the editable component and pressing ENTER.
What happens? I've made a mistake in serialization/deserialization of Slate's value and thus I've only serialized
node[0]instead of all nodes. As a result of that, I was not inserting the created node at all, and after render Slate has crashed when trying to sync selection with domSelection.The syncing works OK when it's the other way around - syncing selection to domSelection, but my problem is that in this case it's the other way around. domSelection should be synced to selection and this has been failing because no operations were happening and Slate had no chance to update
editor.selection.I've fixed the problem in my code - but I thought I would still explore the problem and potential fix in Slate itself. It's somewhat easy to handle some cases around controlled values wrongly which might lead to a total app crash.
I would understand if you would not like to fix this as this is really a sort of best-effort logic to handle this. It would be much better for Slate if all changes would go through operations as it would be able to apply its logic for updating various states. OTOH - controlled values are popular so you probably would like to support them. Potentially it could be possible to compute set of operations when reconciling old and new values and apply them so everything would get handled in uniform manner, but it seems overly complex to try implementing something along those lines and I suspect there always would be some edge cases with it.
What's the new behavior?
It doesn't crash in the described scenario 😉
How does this change work?
If the selected path is being removed (or not actually inserted) when the value is being controlled then I assume we can use current
domSelectionconverted to slate range as neweditor.selection.Have you checked that...?
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)