Rich text crashes when clicked multiple times in a row

[nbelyh]

Category

[ ] Enhancement
[x] Bug
[ ] Question

Version

Please specify what version of the library you are using: [3.5.0]

Expected / Desired Behavior / Question

Nothing happens.

Observed Behavior

The application crashes (react crashes), with message:

Uncaught Invariant Violation: Maximum update depth exceeded. 
This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. 
React limits the number of nested updates to prevent infinite loops.

Crashes here, at the row 867, this.setState:

sp-dev-fx-controls-react/src/controls/richText/RichText.tsx

Lines 854 to 873 in 2f28397

	private handleChangeSelection = (range, oldRange, source) => {
	const quill = this.getEditor();
	try {
	if (quill) {
	// Get the selected text
	const selectedText = quill.getText(range);
	// Get the current format
	const formats = quill.getFormat(range);
	// Get the currently selected url
	const selectedUrl = formats.link ? formats.link : undefined;
	this.setState({
	selectedText: selectedText,
	selectedUrl: selectedUrl,
	formats: formats
	});
	if (this._propertyPaneRef && this.state.morePaneVisible) {

Steps to Reproduce

Quickly (milliseconds) click in the RichText text control several times, like five or so (using left mouse button).
The rich text should not be empty (should contain some formatted text, such as a hyperlink)

It looks like the handler just enters the infinite loop, maybe a safeguard around it could solve it.

[ghost]

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

[nbelyh]

After investigating this a bit, there is an infinite loop (seems to be caused by fixing #666)
Setting the control value resets it's selection that is causing selection change event, then selection change event again resets the value with setState(), this causes the whole thing to loop.

This commit:
4e60d63#diff-e75aee2bee84699e37e09dd949a4ecbe46936e63f4d7fe912a159c45f0e51f49L622

[joelfmrodrigues]

@nbelyh many thanks for investigating it and providing more details. Would you be available to fix it and send a PR?

[nbelyh]

@joelfmrodrigues unfortunately I am not sure how to fix this properly... 😢 The issue looks a bit complicated for me. There were already several attempts to fix the core thing as far as I understand (linked issue), i.e. the <ReactQuill value={....}> thing. There are even comments in the code related to it 😄 For myself, I just went for the workaround of replacing the value={} with defaultValue={} thing and updating the text manually in my code in onChange event. But this won't work in general case, described in the linked issue. I mean here:

sp-dev-fx-controls-react/src/controls/richText/RichText.tsx

Line 630 in 2f28397

value={text || ''} //property value causes issues, defaultValue does not

This comment is a sad truth. Unfortunately, if you replace value with defaultValue, you'll get the "evil" issue 666 come back again. I've decided just to live with that one for now... waiting for a hero 😄 I would say the author of that comment may be a decent candidate 😄

I could revert the fix for 666, I think having 666 is better than infinite loop and app crash anyway? I would probably ask @AJIXuMuK to look at this when he has time, since he already dealt with this thing?

[AJIXuMuK]

I will look at it for sure.
Little business: find appropriate block of time :)