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

Use native selectionchange event as opposed to React's onSelect #1135

Closed
YurkaninRyan opened this issue Sep 18, 2017 · 16 comments
Closed

Use native selectionchange event as opposed to React's onSelect #1135

YurkaninRyan opened this issue Sep 18, 2017 · 16 comments
Labels

Comments

@YurkaninRyan
Copy link
Collaborator

I want to take a shot at implementing this but before I do I want to see what people think.

Currently, I believe Slate is relying on React's onSelect event. The problem with that is that it only fires when the user releases their mouse. This causes a desync between the selection state in the editor state and what is actually on the screen mid-highlight. Consider this workflow:

  1. User types
  2. Some kind of polling mechanism sets of some async actions, (in my case this would be an autosaving service)
  3. User starts highlighting text
  4. mid-highlight a prop gets updated because of the async autosave call
  5. Editor rerenders and resets the selection, causing it to collapse
@ianstormtaylor
Copy link
Owner

@YurkaninRyan what prop is the async call updating?

@YurkaninRyan
Copy link
Collaborator Author

It's updating my isAutosaving prop, which doesn't go directly on the editor, but causes the wrapper around the editor to rerender, the editor seems to rerender along with it everytime

@ianstormtaylor
Copy link
Owner

Open to the idea of this. Consider another case that might help you think about it:

If we have collaborative editing, there are going to be cases where new operations come in from the server, and applying those shouldn't cause the scrollToSelection logic to fire. However, they should cause the selection to be updated if the new changes would have shifted the selection at all.

Ideally though I'd really rather not have to go around React I think.

@ianstormtaylor
Copy link
Owner

Put differently, right now we have a 1:1 connection between the editor re-rendering and the selection being updated, but I think not all situations will be okay with that. So maybe there's something to investigate about making that logic smarter or more selective, and maybe it doesn't end up needing to hack around React? Not sure.

Another idea would be to augment React's event-handling if it's not ideal for this case, if they were open to it. They've been open to onSelect-related changes in the past I think.

@YurkaninRyan
Copy link
Collaborator Author

What we don't have though is a 1:1 with the user updating the selection and the selection model updating

I found this issue but it doesn't seem to be gaining much traction facebook/react#5785

@ianstormtaylor
Copy link
Owner

@YurkaninRyan very fair. I'd love to see a solution that keeps it all inside React, by getting a PR merged that adds the new event to React's event.

@YurkaninRyan
Copy link
Collaborator Author

I'm going to try to get it in there!

@YurkaninRyan
Copy link
Collaborator Author

For anyone that comes here and has a similar problem here's the status, i'm trying to get this in at React core, and i'm waiting for some feedback on my approach from the React team.

But, if you need to fix this problem now, here is what you can do.

Outside your editor, when your editor mounts attach a selectionchange event listener.
When you're editor fires a mousedown event, set a flag that allows for that selection change event to make changes.
When your editor fires mouseup set that flag to false.
When selectionchange fires, and the editor is the active element, create a selection state, and use change's select to set it.

@danburzo
Copy link
Contributor

Having a onselectionchange that picks up on the DOM event would be pretty useful in the codebase, we may be able to catch some IME events that replace some text, if they first trigger a selection change.

@YurkaninRyan
Copy link
Collaborator Author

One thing i'm struggling with is turning the window selection into a valid slate selection, I had to write a lot of custom logic to prevent errors from being thrown. Would it be worth some time to expose that selection logic somehow? I'm thinking of an API that you call that grabs the current window selection and just returns a slate selection or null if the selection isn't valid @ianstormtaylor

@ianstormtaylor
Copy link
Owner

@YurkaninRyan definitely open to that idea! Similar in concept/goal to the findDOMNode helper.

@YurkaninRyan
Copy link
Collaborator Author

YurkaninRyan commented Sep 22, 2017

Perfect, that's exactly how I saw it, i'm going to port over what I have and try to think of ways to test it

@AlbertHilb
Copy link
Contributor

AlbertHilb commented Sep 23, 2017

Hi @YurkaninRyan.

One thing i'm struggling with is turning the window selection into a valid slate selection

Isn't slate-react/utils/getPoint sufficient?

@echenley
Copy link
Contributor

echenley commented Oct 10, 2017

As a temporary workaround, I considered trying something to the effect of this (as suggested by @YurkaninRyan and @AlbertHilb):

import getPoint from 'slate-react/lib/utils/get-point'
// ...

class SlateEditor extends React.Component {
  // ...
  componentDidMount() {
    document.addEventListener('selectionchange', this.onSelectionChange)
  }

  componentWillUnmount() {
    document.removeEventListener('selectionchange', this.onSelectionChange)
  }

  onSelectionChange(e: Event) {
    const { state } = this.props
    const { editor, isDragging } = this

    // isDragging is toggled on mousedown and mouseup
    if (editor && isDragging) {
      const selection = document.getSelection()

      if (!selection) {
        return null
      }

      const { anchorNode, anchorOffset, focusNode, focusOffset } = selection

      if (!anchorNode || !focusNode) {
        return null
      }

      const anchorPoint = getPoint(anchorNode, anchorOffset, state, editor)
      const focusPoint = getPoint(focusNode, focusOffset, state, editor)

      if (!anchorPoint || !focusPoint) {
        return null
      }

      const newSelection = Selection.create({
        anchorKey: anchorPoint.key,
        anchorOffset: anchorPoint.offset,
        focusKey: focusPoint.key,
        focusOffset: focusPoint.offset,
      })

      return this.onChange(state.change.select(newSelection).focus())
    }
  }
  // ...
}

As a proof of concept it does appear to work, though it's pretty quirky as far as where the cursor ends up after the text is inserted. I am not sure I'm comfortable putting this hack in production for this reason. Also, onSelectionChange is fired like a zillion times per second, so probably worth debouncing it.

Related issue: #802

@ianstormtaylor
Copy link
Owner

@YurkaninRyan FWIW, with #1231 a new findRange helper is exposed that returns a Slate Range (previously Selection) from a DOM Selection/Range.

@ianstormtaylor
Copy link
Owner

Now that we're also using the native onbeforeinput event, circumventing React's handling of it in newer browsers, I think it would make sense to handle onselectionchange in the same way. If anyone was down to make a PR with this fix I'd gladly accept it!

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

No branches or pull requests

5 participants