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

rerender slate when focus changes. #3988

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

lukesmurray
Copy link
Contributor

@lukesmurray lukesmurray commented Nov 17, 2020

Rerender slate when focus changes. Fixes #3987

Is this adding or improving a feature or fixing a bug?

Fixing a bug

What's the new behavior?

I've shown the old behavior in the initial bug report. When the editor loses focus due to the user pressing tab or escape the useFocused hook does not update. While debugging I found that this is due to the Slate wrapper not rerendering, so the useFocused Context Provider never receives a new value.

Here is a code snippet which can be used to demonstrate the new behavior. Without the new code is focused only logs when you click outside the editor. With the new behavior is focused logs when you tab outside of the editor or press escape.

import React, { useEffect, useMemo, useState } from 'react'
import { createEditor, Node } from 'slate'
import { Editable, Slate, useFocused, withReact } from 'slate-react'

const FocusTester = () => {
  const focused = useFocused()
  useEffect(() => {
    console.log('is focused', focused)
  }, [focused])

  return <></>
}

const FocusExample: React.FC = props => {
  const editor = useMemo(() => withReact(createEditor()), [])
  // Add the initial value when setting up our state.
  const [value, setValue] = useState<Node[]>([
    {
      type: 'paragraph',
      children: [{ text: 'A line of text in a paragraph.' }],
    },
  ])

  return (
    <Slate
      editor={editor}
      value={value}
      onChange={newValue => setValue(newValue)}
    >
      <Editable />
      <FocusTester />
    </Slate>
  )
}

export default FocusExample

How does this change work?

When looking at the code I realized ReactEditor.isFocused does return the correct value at all time already. Blur and Focus events are already getting handled correctly. The only issue is Slate is not rendering when the ReactEditor.isFocused value changes.

In order to resolve the issue I just need to get slate to rerender when focus changes. My solution is in slate.tsx. In my solution I move isFocused to local react state. I listen to focus and blur events at the document level and then update the local state with ReactEditor.isFocused. I also update the local state on every render with useEffect. You can see this implementation in slate.tsx.

If useState returns the same value as the previous state then react will not rerender. So this code should not cause any unnecessary renders.

Have you checked that...?

I wanted to try and avoid the focus and blur handlers in editable because there is a bunch of edge case behavior handled there. All of those edge cases should still be handled the exact same way, because my change is essentially a read only change to read ReactEditor.isFocused when the focus changes on the page.

  • 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.)

Does this fix any issues or need any specific reviewers?

Fixes: #3987
Reviewers:

@ianstormtaylor
Copy link
Owner

Hey @lukesmurray thanks for submitting this. It looks like as currently written it's going to be re-rendering all editors on the page whenever anything is focused/blurred instead of specifically when the editor in question is focused/blurred? Which seems inefficient.

I wonder if that's a reason to move this logic into the onFocus and onBlur handlers to ensure that it's only happening for the specific editor in question.

@lukesmurray
Copy link
Contributor Author

Thanks for reviewing the PR! I didn't think about the multi editor scenario but I just checked and it seems to work correctly.

Here's a repro with multiple editors separated by horizontal rules, you can see as I tab through and click on different editors at most two editors are rererenderd, the one losing focus and the one getting focus.

multi-editor-example

You're right that we call setState whenever the focus changes at the document level but if the editor focus hasn't changed then it's a no-op.

If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects.

The reason this isn't in the onFocus and onBlur handles is because we need to change the value passed to the provider. Unless I'm mistaken if we want to change the value from a nested component we would have to add a setter to the provider which would be changing it's api.

Here's the multi editor code, adapted from the large document example.

import faker from 'faker'
import React, { useCallback, useMemo, useState } from 'react'
import { createEditor } from 'slate'
import { Editable, Slate, withReact } from 'slate-react'

const HEADINGS = 1
const PARAGRAPHS = 1
const EDITORS = 5
const editorValues = []
for (let e = 0; e < EDITORS; e++) {
  const initialValue = []

  for (let h = 0; h < HEADINGS; h++) {
    initialValue.push({
      type: 'heading',
      children: [{ text: faker.lorem.sentence() }],
    })

    for (let p = 0; p < PARAGRAPHS; p++) {
      initialValue.push({
        children: [{ text: faker.lorem.paragraph() }],
      })
    }
  }
  editorValues.push(initialValue)
}

const MultipleEditors = () => {
  return (
    <div>
      {editorValues.map((initialValue, i) => (
        <div key={i}>
          <SlateEditor initialValue={initialValue} />
          <hr />
        </div>
      ))}
    </div>
  )
}

const SlateEditor = (props: { initialValue: any }) => {
  const [value, setValue] = useState(props.initialValue)
  const renderElement = useCallback(props => <Element {...props} />, [])
  const editor = useMemo(() => withReact(createEditor()), [])
  return (
    <Slate editor={editor} value={value} onChange={value => setValue(value)}>
      <Editable renderElement={renderElement} spellCheck autoFocus />
    </Slate>
  )
}

const Element = ({ attributes, children, element }) => {
  switch (element.type) {
    case 'heading':
      return <h1 {...attributes}>{children}</h1>
    default:
      return <p {...attributes}>{children}</p>
  }
}

export default MultipleEditors

@ianstormtaylor
Copy link
Owner

@lukesmurray ah great call. Thank you!

@ianstormtaylor ianstormtaylor merged commit 67a6f73 into ianstormtaylor:main Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Focus does not change when keyboard changes focus
2 participants