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

Issue with hooks and onBlur #161

Open
embiem opened this issue Apr 18, 2019 · 25 comments
Open

Issue with hooks and onBlur #161

embiem opened this issue Apr 18, 2019 · 25 comments

Comments

@embiem
Copy link

embiem commented Apr 18, 2019

Hey, very useful component, thanks!

I ran into an issue when trying to use this component in a functional component combined with useState. The issue is that not all prop changes lead to an update of the component, therefore an onBlur like in my example would return the old initial content value.

The issue resides in the current implementation of shouldComponentUpdate.

Please look at this example Codesandbox. I copied this component's current source code over there and just return true in the shouldComponentUpdate and everything works fine. To see the issue, comment the return true and uncomment the original code. If you type something and look in the console, you'll see the following logs:

render log: 
render log: a
render log: as
render log: asd
onBlur log: 

To fix this, I'd suggest going with a return true or make it a PureComponent to make updates based on prop changes.

Maintainer edit

Short answer

Do this

  const text = useRef('');

  const handleChange = evt => {
    text.current = evt.target.value;
  };

  const handleBlur = () => {
    console.log(text.current);
  };

  return <ContentEditable
      html={text.current}
      onBlur={handleBlur}
      onChange={handleChange} />

NOT THAT

  const [text, setText] = useState('');

  const handleChange = evt => {
    setText(evt.target.value);
  };

  const handleBlur = () => {
    console.log(text); // incorrect value
  };

  return <ContentEditable
      html={text}
      onBlur={handleBlur}
      onChange={handleChange} />

Explanation

react-contenteditable has to prevent rendering (using shouldComponentUpdate) very frequently. Otherwise, the caret would jump to the end of the editable element on every key stroke. With useState, you create a new onBlur event handler on every keystroke, but since the ContentEditable is preventing rendering, your event handlers are not taken into account on every keystroke, and it's the handler function that was creating the last time the component was rendered that gets called.

embiem added a commit to embiem/react-contenteditable that referenced this issue Apr 18, 2019
@embiem
Copy link
Author

embiem commented Apr 18, 2019

Added a PR in case you decide to go this way. I can't really see any downsides to this.

The deep equal check of styles might have some historic reasons that might make this a breaking change. So any input on why that check was part of the shouldComponentUpdate function would be good.

@lovasoa
Copy link
Owner

lovasoa commented Apr 19, 2019

I had a look at your codesandbox, and your change breaks the expected behavior of the component. You can't type anywhere in the editable component except at the end.

@embiem
Copy link
Author

embiem commented Apr 19, 2019

You're right. Didn't catch that.

I'll have another focused look at it.

@KevinSheedy
Copy link

KevinSheedy commented Nov 23, 2019

+1 Inside onBlur my state seems to be reset to it's original value.
My workaround is to use innerRef to read the value directly off the domElement

const [text, setText] = useState('');
const contentEditable = useRef();

const handleChange = evt => {
  setText(evt.target.value);
};

const handleBlur = () => {
  console.log(contentEditableRef.current.innerHTML);  // Correct value
  console.log(text); // Incorrect value
};

<ContentEditable
    innerRef={contentEditableRef}
    html={text}
    onBlur={handleBlur}
    onChange={handleChange} />

@BitPhinix
Copy link

Same issue here, can't use your workaround though since I'm using component state.

@conraddamon
Copy link

The same thing happens with onKeyUp and onKeyDown (though not with onChange).

Our ugly workaround is to make sure that something that shouldComponentUpdate cares about changes on each render, in this case className.

@lovasoa
Copy link
Owner

lovasoa commented Mar 7, 2020

A pull request would be welcome :)

@BrianLovelace128
Copy link

Dropping in to say that I just encountered this issue too.

@iamjoshua
Copy link

If no fix is coming, perhaps an update to the README with a warning? I lost a lot of time on this issue before realizing that this component is (as far as I can tell) completely incompatible with react hooks.

@zeevl
Copy link
Contributor

zeevl commented Apr 1, 2020

Done.

#195

@lovasoa
Copy link
Owner

lovasoa commented Apr 1, 2020

I updated the top post to give more context. What could be done to avoid every new user getting bitten by this is to detect the situation and print a message like updating event handlers after the element creation is not supported, see #161

@lovasoa lovasoa pinned this issue Apr 7, 2020
@erganmedia
Copy link

erganmedia commented Apr 9, 2020

@embiem

Explanation

react-contenteditable has to prevent rendering (using shouldComponentUpdate) very frequently. Otherwise, the caret would jump to the end of the editable element on every key stroke. With useState, you create a new onBlur event handler on every keystroke, but since the ContentEditable is preventing rendering, your event handlers are not taken into account on every keystroke, and it's the handler function that was creating the last time the component was rendered that gets called.

your explanation sounds like you should use the useCallback hook to wrap the handleChange function. correct me if I am wrong.

like this
const handleChange = useCallback(evt => { setText(evt.target.value); }, []);

@lovasoa
Copy link
Owner

lovasoa commented Apr 9, 2020

The quoted text is by me, not embiem. The problem is not with the handleChange function, that is correctly called on every change, but with handleBlur. Every time you render your component, you create a new handleBur instance (which closes over the current value of text). But since the contenteditable cancels rendering, the event is not rebound to the new function.

We could implement callback function comparison in shouldComponentUpdate, and then rerender on every callback change. Then, users could use useCallback together with useState without having to use useRef. However, this would break the component for all users who currently rely on the component not rerendering (this would make the caret jump to the end of the element while editing).

@fivethreeo
Copy link

fivethreeo commented Aug 5, 2020

Maybe the caret position can be saved and restored?

https://gist.github.com/timdown/244ae2ea7302e26ba932a43cb0ca3908
https://github.com/timdown/rangy

@ctrlplusb
Copy link

ctrlplusb commented Aug 6, 2020

Hey all;

So I also hit this issue. It appears to be less about the "hooks" feature of React, and more about the paradigm that "hooks" operate within - i.e. the need to create new callback functions containing new scope closures.

The current design of this component, along with the usage of shouldComponentUpdate does not play nicely with this paradigm. I haven't gone into the source in depth, and understand the introduction of shouldComponentUpdate was to prevent the caret position from changing. However, the current design doesn't allow new instances of callback components to be passed down, therefore you can get into a case where an old callback instance is called with a stale closure scope, leading to difficult to debug issues and strange behaviour.

To try and alleviate this I created my own wrapping component which manages refs of each callback and then passes down the callbacks down.

I've had good results from this as a temporary measure. My longer term hope is that we can revisit the internal design of this component as a community and move it forward.

Here is my wrapping component (click to view)
// wrapped-content-editable.js

import React from 'react';
import ReactContentEditable from 'react-contenteditable';

export default function ContentEditable({
  onChange,
  onInput,
  onBlur,
  onKeyPress,
  onKeyDown,
  ...props
}) {
  const onChangeRef = React.useRef(onChange);
  const onInputRef = React.useRef(onInput);
  const onBlurRef = React.useRef(onBlur);
  const onKeyPressRef = React.useRef(onKeyPress);
  const onKeyDownRef = React.useRef(onKeyDown);

  React.useEffect(() => {
    onChangeRef.current = onChange;
  }, [onChange]);
  React.useEffect(() => {
    onInputRef.current = onInput;
  }, [onInput]);
  React.useEffect(() => {
    onBlurRef.current = onBlur;
  }, [onBlur]);
  React.useEffect(() => {
    onKeyPressRef.current = onKeyPress;
  }, [onKeyPress]);
  React.useEffect(() => {
    onKeyDownRef.current = onKeyDown;
  }, [onKeyDown]);

  return (
    <ReactContentEditable
      {...props}
      onChange={
        onChange
          ? (...args) => {
              if (onChangeRef.current) {
                onChangeRef.current(...args);
              }
            }
          : undefined
      }
      onInput={
        onInput
          ? (...args) => {
              if (onInputRef.current) {
                onInputRef.current(...args);
              }
            }
          : undefined
      }
      onBlur={
        onBlur
          ? (...args) => {
              if (onBlurRef.current) {
                onBlurRef.current(...args);
              }
            }
          : undefined
      }
      onKeyPress={
        onKeyPress
          ? (...args) => {
              if (onKeyPressRef.current) {
                onKeyPressRef.current(...args);
              }
            }
          : undefined
      }
      onKeyDown={
        onKeyDown
          ? (...args) => {
              if (onKeyDownRef.current) {
                onKeyDownRef.current(...args);
              }
            }
          : undefined
      }
    />
  );
}

Yeah, I know that looks a bit verbose and repetitive, but I prefer the explicit simplicity. 😊

You can now write your code as described how not to do it in within the OP.

i.e. This is fine now:

function Demo() {
  const [text, setText] = React.useState('Woot! Hooks working');

  const handleChange = evt => {
    setText(evt.target.value);
  };

  const handleBlur = () => {
    console.log(text); // 👍 correct value
  };

  return (
    <ContentEditable html={text} onBlur={handleBlur} onChange={handleChange} />
  );
}

Demo here

@Nabellaleen
Copy link

@ctrlplusb with your solution, trying to use innerRef to programmatically give the focus to the content editable div element, totally break the behaviour, as you can see in the following demo : https://codesandbox.io/s/bold-surf-kilyp

Any tips to fix that ? :/

@Kalyan457
Copy link

When I get the text inside the div, using the onChange event, I get the html formatted text, but not the original text entered by the user. Is there a way to get what the user has typed but not the html formatted text.

I know I can parse the text and remove the html tags, but I don't want to do that as users may type the html itself as the content

@sfalihi
Copy link

sfalihi commented Oct 20, 2020

Hey all;

So I also hit this issue. It appears to be less about the "hooks" feature of React, and more about the paradigm that "hooks" operate within - i.e. the need to create new callback functions containing new scope closures.

The current design of this component, along with the usage of shouldComponentUpdate does not play nicely with this paradigm. I haven't gone into the source in depth, and understand the introduction of shouldComponentUpdate was to prevent the caret position from changing. However, the current design doesn't allow new instances of callback components to be passed down, therefore you can get into a case where an old callback instance is called with a stale closure scope, leading to difficult to debug issues and strange behaviour.

To try and alleviate this I created my own wrapping component which manages refs of each callback and then passes down the callbacks down.

I've had good results from this as a temporary measure. My longer term hope is that we can revisit the internal design of this component as a community and move it forward.

Here is my wrapping component (click to view)
You can now write your code as described how not to do it in within the OP.

i.e. This is fine now:

function Demo() {
  const [text, setText] = React.useState('Woot! Hooks working');

  const handleChange = evt => {
    setText(evt.target.value);
  };

  const handleBlur = () => {
    console.log(text); // 👍 correct value
  };

  return (
    <ContentEditable html={text} onBlur={handleBlur} onChange={handleChange} />
  );
}

Demo here

thank you for your help.
i just had some problem with ref.
passing ref like that doesn't work perfectly. accourding to react website forwadRef documentation, i used bellow and it solved my problem with ref:

import React, { forwardRef } from "react";
import ReactContentEditable from "react-contenteditable";

export default forwardRef(
  ({ onChange, onInput, onBlur, onKeyPress, onKeyDown, ...props }, ref) => {
    const onChangeRef = React.useRef(onChange);
    const onInputRef = React.useRef(onInput);
    const onBlurRef = React.useRef(onBlur);
    const onKeyPressRef = React.useRef(onKeyPress);
    const onKeyDownRef = React.useRef(onKeyDown);

    React.useEffect(() => {
      onChangeRef.current = onChange;
    }, [onChange]);
    React.useEffect(() => {
      onInputRef.current = onInput;
    }, [onInput]);
    React.useEffect(() => {
      onBlurRef.current = onBlur;
    }, [onBlur]);
    React.useEffect(() => {
      onKeyPressRef.current = onKeyPress;
    }, [onKeyPress]);
    React.useEffect(() => {
      onKeyDownRef.current = onKeyDown;
    }, [onKeyDown]);

    return (
      <ReactContentEditable
        {...props}
        innerRef={ref}
        onChange={
          onChange
            ? (...args) => {
                if (onChangeRef.current) {
                  onChangeRef.current(...args);
                }
              }
            : undefined
        }
        onInput={
          onInput
            ? (...args) => {
                if (onInputRef.current) {
                  onInputRef.current(...args);
                }
              }
            : undefined
        }
        onBlur={
          onBlur
            ? (...args) => {
                if (onBlurRef.current) {
                  onBlurRef.current(...args);
                }
              }
            : undefined
        }
        onKeyPress={
          onKeyPress
            ? (...args) => {
                if (onKeyPressRef.current) {
                  onKeyPressRef.current(...args);
                }
              }
            : undefined
        }
        onKeyDown={
          onKeyDown
            ? (...args) => {
                if (onKeyDownRef.current) {
                  onKeyDownRef.current(...args);
                }
              }
            : undefined
        }
      />
    );
  }
);

now sending ref is like this :

function Demo() {
  const contentRef = React.useRef(null); 
  const [text, setText] = React.useState('Woot! Hooks working');

  const handleChange = evt => {
    setText(evt.target.value);
  };

  const handleBlur = () => {
    console.log(text); // 👍 correct value
  };

  return (
    <ContentEditable ref={contentRef} html={text} onBlur={handleBlur} onChange={handleChange} />
  );
}

@rasendubi
Copy link

You can use the following hook to reduce boilerplate

const useRefCallback = <T extends any[]>(
  value: ((...args: T) => void) | undefined,
  deps?: React.DependencyList
): ((...args: T) => void) => {
  const ref = React.useRef(value);

  React.useEffect(() => {
    ref.current = value;
  }, deps ?? [value]);

  const result = React.useCallback((...args: T) => {
    ref.current?.(...args);
  }, []);

  return result;
};
Usage in the wrapping component
import React from 'react';
import ReactContentEditable, { Props } from 'react-contenteditable';

const useRefCallback = <T extends any[]>(
  value: ((...args: T) => void) | undefined,
  deps?: React.DependencyList
): ((...args: T) => void) => {
  const ref = React.useRef(value);

  React.useEffect(() => {
    ref.current = value;
  }, deps ?? [value]);

  const result = React.useCallback((...args: T) => {
    ref.current?.(...args);
  }, []);

  return result;
};

export default function ContentEditable({
  ref,
  onChange,
  onInput,
  onBlur,
  onKeyPress,
  onKeyDown,
  ...props
}: Props) {
  const onChangeRef = useRefCallback(onChange);
  const onInputRef = useRefCallback(onInput);
  const onBlurRef = useRefCallback(onBlur);
  const onKeyPressRef = useRefCallback(onKeyPress);
  const onKeyDownRef = useRefCallback(onKeyDown);

  return (
    <ReactContentEditable
      {...props}
      onChange={onChangeRef}
      onInput={onInputRef}
      onBlur={onBlurRef}
      onKeyPress={onKeyPressRef}
      onKeyDown={onKeyDownRef}
    />
  );
}

If you don't want to copy the wrapping component, you can use useRefCallback as a drop-in replacement for useCallback:

function Demo() {
  const [text, setText] = React.useState('Woot! Hooks working');

  const handleChange = useRefCallback((evt) => {
    setText(evt.target.value);
  }, []);

  const handleBlur = useRefCallback(() => {
    console.log(text); // 👍 correct value
  }, [text]);

  return (
    <ContentEditable html={text} onBlur={handleBlur} onChange={handleChange} />
  );
}

Demo

@EduardoAraujoB
Copy link

Hey all;

So I also hit this issue. It appears to be less about the "hooks" feature of React, and more about the paradigm that "hooks" operate within - i.e. the need to create new callback functions containing new scope closures.

The current design of this component, along with the usage of shouldComponentUpdate does not play nicely with this paradigm. I haven't gone into the source in depth, and understand the introduction of shouldComponentUpdate was to prevent the caret position from changing. However, the current design doesn't allow new instances of callback components to be passed down, therefore you can get into a case where an old callback instance is called with a stale closure scope, leading to difficult to debug issues and strange behaviour.

To try and alleviate this I created my own wrapping component which manages refs of each callback and then passes down the callbacks down.

I've had good results from this as a temporary measure. My longer term hope is that we can revisit the internal design of this component as a community and move it forward.

Here is my wrapping component (click to view)
You can now write your code as described how not to do it in within the OP.

i.e. This is fine now:

function Demo() {
  const [text, setText] = React.useState('Woot! Hooks working');

  const handleChange = evt => {
    setText(evt.target.value);
  };

  const handleBlur = () => {
    console.log(text); // 👍 correct value
  };

  return (
    <ContentEditable html={text} onBlur={handleBlur} onChange={handleChange} />
  );
}

Demo here

typescript version of the wrapping component:

import React, {useEffect, useRef} from 'react';
import ReactContentEditable, {
  ContentEditableEvent,
} from 'react-contenteditable';

interface ContentEditableProps {
  onChange?: (event: ContentEditableEvent) => void;
  onBlur?: (event: React.FormEvent<HTMLDivElement>) => void;
  onInput?: (event: React.FormEvent<HTMLDivElement>) => void;
  onKeyPress?: (event: React.KeyboardEvent<HTMLDivElement>) => void;
  onKeyDown?: (event: React.KeyboardEvent<HTMLDivElement>) => void;
  html: string;
}

export const ContentEditable: React.FC<ContentEditableProps> = ({
  onChange,
  onInput,
  onBlur,
  onKeyPress,
  onKeyDown,
  ...props
}) => {
  const onChangeRef = useRef(onChange);
  const onInputRef = useRef(onInput);
  const onBlurRef = useRef(onBlur);
  const onKeyPressRef = useRef(onKeyPress);
  const onKeyDownRef = useRef(onKeyDown);

  useEffect(() => {
    onChangeRef.current = onChange;
  }, [onChange]);
  useEffect(() => {
    onInputRef.current = onInput;
  }, [onInput]);
  useEffect(() => {
    onBlurRef.current = onBlur;
  }, [onBlur]);
  useEffect(() => {
    onKeyPressRef.current = onKeyPress;
  }, [onKeyPress]);
  useEffect(() => {
    onKeyDownRef.current = onKeyDown;
  }, [onKeyDown]);

  return (
    <ReactContentEditable
      {...props}
      onChange={
        onChange
          ? (...args) => {
              if (onChangeRef.current) {
                onChangeRef.current(...args);
              }
            }
          : () => {}
      }
      onInput={
        onInput
          ? (...args) => {
              if (onInputRef.current) {
                onInputRef.current(...args);
              }
            }
          : undefined
      }
      onBlur={
        onBlur
          ? (...args) => {
              if (onBlurRef.current) {
                onBlurRef.current(...args);
              }
            }
          : undefined
      }
      onKeyPress={
        onKeyPress
          ? (...args) => {
              if (onKeyPressRef.current) {
                onKeyPressRef.current(...args);
              }
            }
          : undefined
      }
      onKeyDown={
        onKeyDown
          ? (...args) => {
              if (onKeyDownRef.current) {
                onKeyDownRef.current(...args);
              }
            }
          : undefined
      }
    />
  );
};

@ambroiseRabier
Copy link

ambroiseRabier commented Oct 17, 2021

Hi, the documentation proposition for hook and class isn't working as I expected:

const text = useRef('');

const handleChange = evt => {
    text.current = evt.target.value;

   // /!\ Won't be colored !
   // text.current = `<span style="color: red;"> ${evt.target.value}</span>`

   // /!\ Completly ignored
   // text.current = `override`
};

const handleBlur = () => {
    console.log(text.current);
};

return <ContentEditable html={text.current} onBlur={handleBlur} onChange={handleChange} />

Using setText with useState work will color the text.
But the caret will jump at the end of the text at each edit.
Same caret issue when using the class component.

@peduarte
Copy link

peduarte commented Mar 9, 2023

Seems to work fine with Hooks, am I missing something? https://codesandbox.io/s/shy-snow-rc1md6?file=/src/App.js

egonSchiele added a commit to egonSchiele/chisel that referenced this issue Aug 19, 2023
The issue is in onBlur. I *think* the issue is similar to
lovasoa/react-contenteditable#161

where an outdated version is firing with the wrong chapterid. Bug can be
repro-d by something like:
1. go to chapter A.
2. click into the title and copy.
3. don't blur, just go to another chapter.
This should then cause a blur and cause a bug.

If you save using Enter or cmd+s, this seems to not happen.
@MalyugaSensei
Copy link

MalyugaSensei commented Nov 15, 2023

Seems to work fine with Hooks, am I missing something? https://codesandbox.io/s/shy-snow-rc1md6?file=/src/App.js

I think the issue lies in the number of renders that occur with each state change, and this behavior is normal. By using useRef, we can eliminate the 'extra' renders. I've added useRef to your example so you can visually see how it works: https://codesandbox.io/s/fragrant-water-7pg4zw?file=/src/App.js

@ghost
Copy link

ghost commented Dec 19, 2023

I don't think the comment in the README is the best workaround. With useRef, when you set text.current, a rerender is not triggered. This is fine if you only want to read the current value, but if you do postprocessing/sanitization in onChange, your new value is not immediately reflected.

I think @KevinSheedy's solution is right:

  • keep using useState
  • store a ref to the ContentEditable
  • read the current text using ref.current.innerHTML

@teniolafatunmbi
Copy link

teniolafatunmbi commented Jan 23, 2024

I don't think the comment in the README is the best workaround. With useRef, when you set text.current, a rerender is not triggered. This is fine if you only want to read the current value, but if you do postprocessing/sanitization in onChange, your new value is not immediately reflected.

I think @KevinSheedy's solution is right:

* keep using `useState`

* store a `ref` to the ContentEditable

* read the current text using `ref.current.innerHTML`

Yea! Alternatively:

  • Get the value of the current input value from your onChange handler
  • Pass that value around if needed.
const [text, setText] = useState();

const updateTextFn = (text) => {
   // will use the current text value passed :)
}

const handleChange = evt => {
    const currentVal= evt.target.value;
    setText(currentVal);
    
    updateTextFn(currentVal)
};

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

No branches or pull requests