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

Retain editor selection when using ReactEditor.focus() #5516

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

josephmr
Copy link
Contributor

Description
When calling ReactEditor.focus() the current editor selection is lost and the caret is placed at the beginning of the document. This change retains the current editor selection when giving the editor focus.

Issue
Fixes: (link to issue)

Example
Before:

Screen.Recording.2023-09-21.at.11.51.59.AM.mov

After:

Screen.Recording.2023-09-21.at.11.49.03.AM.mov

Context
Before focusing the editor we set the DOM selection so that the corresponding onDOMSelectionChange handler in editable.tsx does not reset the editor selection when it fires after the el.focus() call.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023

🦋 Changeset detected

Latest commit: bcc4085

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Minor

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

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mild concerns this could break something else, but let's give it a try. Thanks!

@dylans dylans merged commit 300dc57 into ianstormtaylor:main Sep 21, 2023
11 checks passed
@github-actions github-actions bot mentioned this pull request Sep 21, 2023
@zbeyens
Copy link
Contributor

zbeyens commented Oct 4, 2023

From this change, any ReactEditor.focus(editor) call after a transform will trigger that error udecode/plate#2674

The workaround is to use setTimeout, which shows a wrong selection in the state between.

2023-10-04.at.09.55.30.mp4

We need to either fix it so it can resolve the DOM node, or revert this @josephmr

@12joan
Copy link
Contributor

12joan commented Oct 4, 2023

Is it possible we could add an option to ReactEditor.focus that controls this behaviour?

I don't think resolving the DOM node immediately after a transform is possible, since the DOM node you're trying to select hasn't been rendered to React yet.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Oct 18, 2023

This change seems to break a lot of our tests, unfortunately.

What about something like this at the beginning of the static focus method:

focus: editor => {
  // If the editor has pending operations, focus should be requested
  // after those changes are applied. Retry in the next tick.
  if (editor.operations.length > 0) {
    setTimeout(() => {
      ReactEditor.focus(editor)
    })
    return
  }
 // ...the rest of the focus code
}

@zbeyens
Copy link
Contributor

zbeyens commented Oct 18, 2023

@dylans @josephmr Many consumers having issues from that release udecode/plate#2674. We should release @skogsmaskin's suggestion as a quick fix.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Oct 19, 2023

I drafted a PR with my suggestion here - #5527

I haven't been able to confirm if this will actually fix your particular issue @zbeyens would you be able to confirm?

I'm not even sure if this is the right solution, but to me, it sounds like a good idea to not interfere with document selections, while the ReactEditor might potentially try to create a new one through transforms.

This does at least fix very similar issues we have been troubled with in our code.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Oct 20, 2023

There seems to be a similar issue with the Editable onBlur [event handler.] (https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/editable.tsx#L984) that only manifested itself here with React 18.0.0 and Chrome.

There it's also trying to map a DOM node with this function: ReactEditor.hasSelectableTarget(editor, event.target) which sometimes fails very similar to the bug we are discussing.

@aodinok
Copy link

aodinok commented Nov 1, 2023

This change is also breaking our tests, slate is unable to resolve node on focus call.
Having ability to control that behaviour would be nice 🙏

Cannot resolve a DOM point from Slate point: {"path":[0,0],"offset":20}
    at Object.toDOMPoint (react-editor.ts:598:13)
    at Object.toDOMRange (react-editor.ts:611:35)
    at Object.focus

@sneridagh
Copy link

I am having the same issue since this PR was merged. When I try to set a link with a selection active.

index.es.js:644 Uncaught Error: Cannot resolve a DOM node from Slate node: {"text":"about"}
    at Object.toDOMNode (index.es.js:644:1)
    at Object.toDOMPoint (index.es.js:651:1)
    at Object.toDOMRange (index.es.js:723:1)
    at Object.focus (index.es.js:565:1)
    at Object.onChangeValue (index.js:82:1)
    at AddLinkForm.onSubmit (AddLinkForm.jsx:210:1)

AddLinkForm is my link plugin.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Nov 7, 2023

I opened up a PR today, which should fix the Cannot resolve a DOM node from Slate node error.

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

Successfully merging this pull request may close these issues.

ReactEditor.focus places the caret at the wrong position
7 participants