Skip to content
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

Delayed setState(value) results in Cannot resolve a DOM point from Slate point #3575

Closed
FezVrasta opened this issue Apr 2, 2020 · 28 comments · Fixed by #4540
Closed

Delayed setState(value) results in Cannot resolve a DOM point from Slate point #3575

FezVrasta opened this issue Apr 2, 2020 · 28 comments · Fixed by #4540

Comments

@FezVrasta
Copy link

Do you want to request a feature or report a bug?

What's the current behavior?

Repro: https://codesandbox.io/s/slate-reproductions-1sbiz

Kapture 2020-04-02 at 17 24 18

Slate: 0.57.1
Browser: Chrome
OS: Mac

What's the expected behavior?

The above repro shows how a delayed setState(value) call systematically results in the error Cannot resolve a DOM point from Slate point.

In my case, I'm using Apollo to keep some text in sync with the server, I have optimistic updates in place, but it's still not quick enough to not trigger this error.

I also encountered the same issue while trying to sync with some quite large (and badly written) Redux store.

@cameracker
Copy link
Collaborator

Thanks for posting this issue.

I think this is caused by the selection state of the editor briefly being in a state that isn't possible to achieve in its value state because they are being applied out of sync.

So, understanding that the onChange here is contrived and not going to usually include setTimeout (?), my recommendation is to ensure that your onChange loop is as tight as possible and if you must really do something that takes a meaningful amount of time, subscribe to onChange only to get updates and don't pass that value back to slate so it can continue to control its own internal value

One "fix" for this bug is to move the selection state back into the value state. However, that's a non trivial change and wont be done any time soon, if at all. It was a deliberate decision to keep the selection state internal.

But a lot of people have these issues so maybe over time it'll be enough to justify that transition

@agnislav
Copy link

agnislav commented Apr 4, 2020

Hi guys.

I'm pretty new with Slate (a few weeks), but the current behavior looks reasonable for me. I'd be grateful if you can explain what's wrong with it.

So, how I see it:

  • slate editor object contains the state of the editor
  • editor.children contains value while editor.selection contains the current state of selection
  • editor.children is exposed outside via value and onChange <Editor> props and not updated by slate itself
  • editor.selection is purely internal stuff, which is clear as the selection can be made over a couple of nodes.
  • this means that a developer is responsible to keep editor value in immediate sync with the slate
  • also, this means that if a developer changes the value from outside, he's responsible to update the selection as well.

Would be great to hear which of the points above aren't correct and why.

@huypham50
Copy link

Same issue (Chrome Latest). Typing too fast (intentionally) will cause the problem above.

In my case, I'm updating the cache inside useEffect hook, React blames the component though.

Wonder if there's a fix here.

@FezVrasta
Copy link
Author

I feel like Slate should wait to move the cursor until the new value has been received, I think Quill does the same

@czechdave
Copy link
Contributor

I would argue that this is by design. Why would you want your UI to have an intentional lag for your user when they type? If you need to do anything async with your editor value you should separate it from the user's UI state. In my Slate implementations I do something like this:

  const [value, setValue] = useState(/* ... */)
  const onChange = newValue => {
    setValue(newValue);
    if (newValue !== value) {
      // set newValue in redux state, post to server etc.
    }
  }

@FezVrasta
Copy link
Author

FezVrasta commented Apr 10, 2020

As pointed out by @phamstack, this state mismatch can happen even on seemingly synchronous operations.

React state updates are by nature asynchronous, so there's isn't any guarantee that they will run quickly enough to satisfy Slate's requirements.

The possible solutions I can think of are:

  1. Make Slate keep an internal value state, that gets kept in sync with what's passed in by props (difficult);
  2. Make Slate wait to run selection operations until the content is updated (may introduce lag);
  3. Make Slate silently fail - gracefully handle these errors;

@czechdave
Copy link
Contributor

czechdave commented Apr 10, 2020

While you are correct that React updates are by nature asynchronous, Slate uses the ReactDOM.unstable_batchedUpdates helper to mitigate that:
https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/plugin/with-react.ts#L86-L100

If you could point out in a code-sandbox a "seemingly asynchronous" operation that causes this issue we could see if there is anything to fix here.

@FezVrasta
Copy link
Author

I haven't a reproduction example for that case, maybe @phamstack can provide one?

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Apr 28, 2020

This is a problem when doing collaborative editing. Say user A has focus on line 2. User B removes the line above (line 1). As far as I can tell there is no way I can adjust the selection for user A before editable (https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/components/editable.tsx#L162) in slate-react tries to update the selection itself and fails because now line 2 no longer exists.

So far I have forked slate-react and just fail silently, and then adjust the selection myself.

@FezVrasta
Copy link
Author

An option to silently fail, and maybe fallback to the closest valid position would probably a great idea

@gabriel-peracio
Copy link

I have the same problem described by @skogsmaskin. My application already has collaboration built-in, which powers other widgets (slate is just one of them). I am opting out of slate's collaborative handling and instead piggybacking on my own app's infrastructure to make things more uniform

This error bites me constantly - when collaborating and also when using undo/redo (which, again, works through my own app code for consistency rather than using slate's system).

User types paragraph A, then types paragraph B, then clicks undo. Paragraph A no longer exists, the selection is invalid, slate crashes.

I also cannot store/reset the selection as part of the undo/redo mechanism, unless I use slate's system.

skogsmaskin added a commit to skogsmaskin/slate that referenced this issue May 6, 2020
skogsmaskin added a commit to skogsmaskin/slate that referenced this issue May 6, 2020
skogsmaskin added a commit to skogsmaskin/slate that referenced this issue May 6, 2020
In a collaboration session this would happen all the time.

Find some way of dealing properly with this.

Track issue: ianstormtaylor#3575
@cameracker
Copy link
Collaborator

@gabriel-peracio , could you provide a little bit more information about how you're handling these external transformations? If you're going about this by mutating the value manually via immer and injecting it back in with setValue (for instance) there's a lot of state consistency logic that is being bypassed that you may need to make sure to restore yourself.

@TheSpyder
Copy link
Collaborator

TheSpyder commented May 23, 2020

Slate core has a couple of ways to deal with this:

  • both document contents (editor.children) and selection (editor.selection) are simple properties and can be changed at the same time.
  • in collaboration, when an operation is applied to the editor, it will automatically update the selection (along with all other Ref instances).

So perhaps the React editor should expose the ability to include selection in the state, as the core does, but I also wonder if we need some documentation around getting started with collaboration. The Slate design goal is (as far as I can tell) to synchronise based on operations, not state, for the best user experience.

skogsmaskin added a commit to skogsmaskin/slate that referenced this issue Jun 4, 2020
In a collaboration session this would happen all the time.

Find some way of dealing properly with this.

Track issue: ianstormtaylor#3575
skogsmaskin added a commit to skogsmaskin/slate that referenced this issue Aug 26, 2020
In a collaboration session this would happen all the time.

Find some way of dealing properly with this.

Track issue: ianstormtaylor#3575
@zaqqaz
Copy link

zaqqaz commented Mar 6, 2021

Same issue here with remote updates as @skogsmaskin & @gabriel-peracio described, as current workaround I'm removing selection during the remote update (collaboration mode or history updates), and set it back once value updated:

    React.useEffect(() => {
        setValue((currentValue) => {
            /**
             * Slate can't properly handle selection range for async updates.
             * https://github.com/ianstormtaylor/slate/issues/3575
             *
             * We need to reset selection only for real remote updates
             * (collaboration or undo/redo events)
             * In case of direct update currentValue will be update before remote
             * so they will be equal.
             */
            if (currentValue !== remoteValue) {
                editor.selection = null;
            }

            return remoteValue;
        });
    }, [editor, remoteValue]);

    React.useEffect(() => {
        if (elementSelectedForEdit && !editor.selection) {
            const lastCharPosition = Editor.end(editor, []);
            Transforms.select(editor, lastCharPosition);
            ReactEditor.focus(editor);
        }
    }, [editor, elementSelectedForEdit, value]);

In this example it's expected that focus will be moved to the end of the string even if update happens somewhere in between (I think that this ux is better in the case if you have batching logic for remote updates), but in case if you want to keep last selection position, just store it (i.e. the ref object) and check in second hook that this selection valid for current range and set after.

But in general I think this issue should be fixed in the slate itself.

@phyllisstein
Copy link

It sounded like @czechdave was requesting a reproduction case that we would expect to behave more or less synchronously. I wonder if this would serve: https://codesandbox.io/s/condescending-surf-rzcin.

It's essentially the boilerplate from the React docs, except that it stores editor state in a Recoil atom. While Slate works as expected with editor state managed by React's useState hook, moving the editor value to Recoil's useRecoilState hook crashes with "Cannot resolve DOM point from Slate point."

Recoil is new and a little funky, but its updates normally happen in the same lifecycle as useState. It would be reasonable to expect its behavior to be synchronous-ish.

@rohankeskar19
Copy link

Thanks for posting this issue.

I think this is caused by the selection state of the editor briefly being in a state that isn't possible to achieve in its value state because they are being applied out of sync.

So, understanding that the onChange here is contrived and not going to usually include setTimeout (?), my recommendation is to ensure that your onChange loop is as tight as possible and if you must really do something that takes a meaningful amount of time, subscribe to onChange only to get updates and don't pass that value back to slate so it can continue to control its own internal value

One "fix" for this bug is to move the selection state back into the value state. However, that's a non trivial change and wont be done any time soon, if at all. It was a deliberate decision to keep the selection state internal.

But a lot of people have these issues so maybe over time it'll be enough to justify that transition

I'm doing something like notion when the user types '/' then a popup appears to select a block type and after selecting a block I want to remove that slash so I'm taking a backup when user types '/' and then resetting the value with backup after selecting a block, So how can I achieve this without passing a value to slate?

@tiberiuichim
Copy link

@rohankeskar19 You could use the Transforms interface to remove the current line.

@rohankeskar19
Copy link

Is there a fix for this yet, I really need this please

@cameracker
Copy link
Collaborator

@rohankeskar19 there isnt a bug to fix here really, its that you need not try to intercept or delay the onChange event.

@gabriel-peracio
Copy link

gabriel-peracio commented Jul 12, 2021

there isnt a bug to fix here really

I think trying to abstract away the selection turned out to be a mistake, and it is now too expensive to fix. I'm not sure this is a "bug" per se. On the one hand, it's fine to tell people to not modify the onChange event - on the other hand:

  • They clearly want to
  • There is no other mechanism to achieve what we want to do
  • Slate should gracefully handle invalid selections somehow rather than throwing (or at least letting people configure it to do so). Prosemirror has a map api for exactly that purpose, where you pipe your selection through the transactions and it spits a new (valid) selection on the other end

In any case this was an architecture decision that robbed a ton of flexibility from the API

I'm not a collaborator on this project nor particularly familiar with the codebase, so my opinion should be taken with a grain of salt.

For what it's worth, I ended up deciding on prosemirror as my editor instead of slate - I felt like I was fighting with slate every time I tried to do something even slightly non-obvious

@rohankeskar19
Copy link

@rohankeskar19 there isnt a bug to fix here really, its that you need not try to intercept or delay the onChange event.

But in collaboration when I pass new value from database, It causes issues with the selection, Is there a workaround for that?

@cameracker
Copy link
Collaborator

cameracker commented Jul 12, 2021

Theres a lot of questions I have with how youre trying to use the onChange event, but here are some general rules:

  1. The first thing your onChange should do is put the new value in state. Then your editor should be using this state for its value.
  2. Anything youre doing to put the value into the database or redux should be one way. Send it out, don’t accept that value back. Serializing and the deserializing the value in onChange needlessly delays the event which will kill typing performance
  3. If youre mutating editor state in onChange, again, see rule 1

Edit: I missed the collaborative editing question. This issue is not related to that. For collaborative editing, you will probably need to reconcile the selection but im not sure specifically how you should do that. I recommend you open a github question and bring it to the #collaboration channel on slack.

@rohankeskar19
Copy link

I don't have any issues with sending the data inside onChange, What I'm saying is when I change the value passed to Editable in the value prop, I'm also getting the same error

`

{canShowToolbar() && (
<Toolbar
isBold={state.isBold}
isItalic={state.isItalic}
isUnderlined={state.isUnderlined}
isStrikeThrough={state.isStrikeThrough}
isCode={state.isCode}
label={"Paragraph"}
convertToUnorderedList={convertToUnorderedList}
convertToOrderedList={convertToOrderedList}
convertToTodoList={convertToTodoList}
setTextFormat={setTextFormat}
setBackgroundColor={setBackgroundColor}
setTextColor={setTextColor}
addLinkMark={addLinkMark}
removeLinkMark={removeLinkMark}
isBackgroundColorActive={state.isBackgroundColorActive}
isTextColorActive={state.isTextColorActive}
/>
)}

      <Editable
        renderLeaf={renderLeaf}
        renderElement={renderElement}
        onKeyDown={keyDownHandler}
        onKeyUp={keyUpHandler}
        onPaste={handlePaste}
        onSelect={onSelectHandler}
        value={state.blocks}
        style={{ height: "120%", minHeight: "900px" }}
        placeholder="Type '/' for commands"
      />
    </Slate>

`
Here I'm passing state.blocks to the value prop which will be coming from database

Theres a lot of questions I have with how youre trying to use the onChange event, but here are some general rules:

  1. The first thing your onChange should do is put the new value in state. Then your editor should be using this state for its value.
  2. Anything youre doing to put the value into the database or redux should be one way. Send it out, don’t accept that value back. Serializing and the deserializing the value in onChange needlessly delays the event which will kill typing performance
  3. If youre mutating editor state in onChange, again, see rule 1

I don't have any issues with sending the data inside onChange, What I'm saying is when I change the value passed to Editable in the value prop, I'm also getting the same error

<Slate editor={editor} onChange={handleChange} value={state.blocks}>
             {canShowToolbar() && (
            <Toolbar
              isBold={state.isBold}
              isItalic={state.isItalic}
              isUnderlined={state.isUnderlined}
              isStrikeThrough={state.isStrikeThrough}
              isCode={state.isCode}
              label={"Paragraph"}
              convertToUnorderedList={convertToUnorderedList}
              convertToOrderedList={convertToOrderedList}
              convertToTodoList={convertToTodoList}
              setTextFormat={setTextFormat}
              setBackgroundColor={setBackgroundColor}
              setTextColor={setTextColor}
              addLinkMark={addLinkMark}
              removeLinkMark={removeLinkMark}
              isBackgroundColorActive={state.isBackgroundColorActive}
              isTextColorActive={state.isTextColorActive}
            />
          )}

          <Editable
            renderLeaf={renderLeaf}
            renderElement={renderElement}
            onKeyDown={keyDownHandler}
            onKeyUp={keyUpHandler}
            onPaste={handlePaste}
            onSelect={onSelectHandler}
            value={state.blocks}
            style={{ height: "120%", minHeight: "900px" }}
            placeholder="Type '/' for commands"
          />
        </Slate>

Here I'm passing state.blocks to the value prop which will be coming from database

@cameracker
Copy link
Collaborator

@rohankeskar19 sorry, I missed your question. Its kind of tangential to this issue though.

@bryanph
Copy link
Contributor

bryanph commented Sep 20, 2021

As far as I can tell, the value prop is really only meant to be used as an initial value. When applying a different value prop (one that was not returned from onChange) later on this can cause all kinds of problems because editor.selection, and the history stack from slate-history (if you use that) can still contain values related to the old slate value. Perhaps it's a good idea to rename the value prop to initialValue instead?

Might also need to rework the slate provider code in https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/slate.tsx a bit so it actually directly handles a re-render instead of doing this through the parent component (changing the value causes key to change which will cause a rerender).

@TheSpyder
Copy link
Collaborator

It's still needed to read the editor contents, but yes value should only be provided on startup. If we could make it readonly, we would.

Modifications to value must be done via Slate APIs - and only via the APIs - I'm not sure where our documentation falls over trying to explain this but clearly it does.

@bryanph
Copy link
Contributor

bryanph commented Sep 20, 2021

@TheSpyder The Slate provider does rely on the value property to be set to exactly to what is returned from onChange though (because of this line)

(note the key dependency) which is where the misunderstanding comes from I think. Most react developers are used to components being either controlled or uncontrolled, this sits kind of somewhere inbetween. Although it is not clear to me why it is done this way.

(I made a PR to demonstrate what I mean #4540)

@TheSpyder
Copy link
Collaborator

I'm not a React developer, so I can't really comment on why the Slate component was done that way. I am only familiar with the Slate core, how it works and what it expects :)

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

Successfully merging a pull request may close this issue.