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

UseEditable keeps state.position after blur, and steals focus on rerender #16

Open
MatthewSteel opened this issue Sep 1, 2021 · 4 comments · May be fixed by #27
Open

UseEditable keeps state.position after blur, and steals focus on rerender #16

MatthewSteel opened this issue Sep 1, 2021 · 4 comments · May be fixed by #27

Comments

@MatthewSteel
Copy link

Hi! I have

  • an "editable" div, next to
  • a text input that controls the styling div's text.

The text input is not controlled by use-editable. When the text input's value changes, the editable div rerenders.

If I have never edited the contents of the div, focus stays inside the text input as expected. If I first type some content into the div and then type inside the text input, the focus jumps into the div.

I can put together a simple snippet demonstrating if it'd help you reproduce the issue. But I think the main idea is that

  • state.position starts null here, and things behave fine.
  • Later, when state.position is non-null, use-editable forcibly focuses the div on rerender. I think. Maybe this line.

Not sure of the ideal solution -- listening for blurs, or checking the window's selection or something. I've worked around it by forcing the div to remount on blur, which isn't ideal but it's not blocking me.

(Very cool library, thanks for writing it!)

@krixi
Copy link

krixi commented Sep 18, 2021

+1, this library is great, thank you for providing it.

I wanted to mention that I've noticed this behavior as well. I found that if I click outside any editable areas, then focus isn't stolen - but if I go directly from my editable <pre> area that's controlled by this hook, to a normal text area that isn't controlled by this hook, I see the behavior described here.

@eduludi
Copy link

eduludi commented Mar 8, 2022

I have solve this focus issue by handling element's focus with a local state and using useEditable's disable option:

function EditableText({ as, text, onChange }) {
  const editorRef = useRef(null)
  const [hasFocus, setHasFocus] = useState(false)

  useEditable(editorRef, onChange, { disabled: !hasFocus })

  return React.createElement(
      as,
      {
        ref: editorRef,
        onClick: () => setHasFocus(true),
        onBlur: () => setHasFocus(false),
      },
      text
    )
}


// (EditableText is used in this way)

const [value, setValue] = useState()

const onChange = useCallback((text) => {
  setValue(text)
}, [])

<EditableText as="h1" text={value} onChange={onChange} />

@keyneom
Copy link

keyneom commented Nov 12, 2022

After looking into this a bit. It appears the issue is a result of this useLayoutEffect.

When the current range is set, it draws the focus of the browser automatically. Then it is focused yet again with the call indicated by the OP.

Since no dependency array is passed in, the effect will get called on every react render. As far as I can tell, that is unnecessary. You should be able to declare the dependency array as follows:
[elementRef, onChange, opts, state, state.disconnected, state.observer, state.position]

@kitten I believe the above catches all scenarios in which the position needs to be set, etc. without taking focus from other elements unnecessarily but you could probably better say whether that is true or not.

I'm happy to create a PR if that makes things any easier for you.

@keyneom
Copy link

keyneom commented Nov 12, 2022

It looks like the above works as long as opts is not changing. We could either switch to only using opts.disabled in the dependencies array (which might need eslint-disable-next-line react-hooks/exhaustive-deps) or it would be an expectation for end users to ensure that opts is memoized with something along the lines of this:

const options = useMemo(() => ({
    disabled: disabled,
}), [disabled]);

useEditable(editorRef, onEditableChange, options);

@keyneom keyneom linked a pull request Nov 15, 2022 that will close this issue
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 a pull request may close this issue.

4 participants