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

Fixed #3871 (can reproduction when i click 4 times) #4933

Closed
wants to merge 7 commits into from

Conversation

sennpang
Copy link
Contributor

@sennpang sennpang commented Apr 11, 2022

Description
When i click 4 times, the selection is not expected, like #3871

QQ20220414-174930-HD-before.mp4

fixed

QQ20220414-175158-HD-after.mp4

Issue
Fixes: #4930 #3871

Example
https://user-images.githubusercontent.com/82301/162357857-514f3563-a256-45e2-ab11-21f6c019ab0c.mp4

Context
In the case of triple clicking on the Editable component itself, the node returned from toSlateNode is the editor node, and the path returned from findPath is of zero length.

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 Apr 11, 2022

🦋 Changeset detected

Latest commit: 2305262

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 Patch

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

setTimeout(() => {
Transforms.select(editor, range)
Editor.unhangRange(editor, range)
}, 100)
Copy link

@scottlu scottlu Apr 11, 2022

Choose a reason for hiding this comment

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

Given the timeout, two potential issues: 1) the range could be invalid by the time the timer expires (editor content could have changed). Calling select() and unhangRange() with an invalid range could cause unexpected behavior including a crash. 2) the component could have unmounted before the timer expires, and these apis should not be called in that case. Making the code synchronous would remove these problems.

If it can't be synchronous / must use setTimeout, then:

#1: I don't know of a fix for this one. It's not clear how you could know the range is valid when the timer fires.
#2: You can check if the component has been unmounted by setting a ref boolean in a useEffect cleanup callback. Then that ref's value can be checked in this setTimeout callback function to see if the component has unmounted. If it has, then don't make these api calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add if (!ref.current) return in setTimeout callback

Copy link

Choose a reason for hiding this comment

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

The node, path, and range may not be right after 100ms if editor.children changed within that time. That could be solved by putting all the logic inside the setTimeout callback like this:

if (event.detail >= TRIPLE_CLICK) {
  setTimeout(() => {
    if (ref.current) {
      const node = ReactEditor.toSlateNode(editor, event.target)
      const path = ReactEditor.findPath(editor, node)
      if (path.length) {
        const start = Editor.start(editor, [path[0]])
        const end = Editor.end(editor, [path[0]])
        const range = Editor.range(editor, start, end)
        Transforms.select(editor, range)
        Editor.unhangRange(editor, range)
      }
    }
  }, 100);
  return;
}

Copy link
Contributor Author

@sennpang sennpang Apr 13, 2022

Choose a reason for hiding this comment

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

putting all the logic inside the setTimeout callback have another problem, the event.target maybe is not the current target , it will set the selection to prev target
onDOMSelectionChange function in packages/slate-react/src/components/editable.tsx
image
when i change to Transforms.unhangRange(editor, range), triple and more click is fine, but i don't know is there have some potential issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottlu i delete the setTimeout function, and use state.isTripleClick to check 3 or more clicks, on onDOMSelectionChange event to decide how to set the selection, what do you think?

@sennpang sennpang reopened this Apr 14, 2022
@sennpang sennpang closed this Apr 14, 2022
@sennpang sennpang reopened this Apr 14, 2022
@sennpang sennpang changed the title Fixed crash when triple clicking #4930 #3871 Fixed #3871 (can reproduction when i click 4 times) Apr 14, 2022
@sennpang sennpang closed this Apr 27, 2022
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.

Easy repro crash when triple clicking
2 participants