React DOM crashes when <option> contains three interpolated value if one is a conditional.

[ZhangYiJiang]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

React DOM crashes when <option> contains three interpolated value if one is a conditional.

Reproduction: https://jsfiddle.net/0opjvycp/

1. Change the value of the <select>
2. React crashes with NotFoundError: Node was not found

From what I can tell, the conditional value is necessary, and it must be three interpolated values.

What is the expected behavior?

React should not crash.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React DOM 16.2 and 16.0. I think this worked in 15.6 - https://jsfiddle.net/mrwkmuqc/ does not crash

[gaearon]

Tagging as good first issue.

[mannanali413]

@gaearon i can take this up :)

[jorrit]

Could this be related to #11602 ?

[gaearon]

@mannanali413 Any progress?

[kevinzwhuang]

@gaearon I experienced this regression myself yesterday when upgrading from 15.4.2 to 16.2.0.
Here's a simple repro of the problem I was seeing:
https://jsfiddle.net/qkc4eyqe/1/

Same issue - different error message:
Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

I'm not sure if @mannanali413 is still working on a fix for this, but I can take a closer look later today too.

---

To anyone else experiencing this problem, the only workaround I've found so far is to remove the conditional logic (at least until a fix is found).

[gaearon]

@kevinzwhuang Would you like to work on figuring out the fix?

[kevinzwhuang]

Sure @gaearon I can work on finding a fix.

Did some digging in devtools
Here's some of my findings:

---

parentInstance: Note the childNodes is a single element of the flattened text

child to be removed - Since there's a mismatch with the flattened childNodes,

---

What we should be seeing is 4 elements in the childNodes array, instead of a single flattened text element - this might have something to do with #11602 like @jorrit mentioned

[avinashdvv]

@kevinzwhuang can you make a video or a blog post about how did u solve this issue once u solved this.
try to make a video if possible

[kevinzwhuang]

Getting closer to finding the root of the problem by running this code with v15 - it looks like what changed from 15/stack to 16/fiber for this operation is that:
in v15, it would execute setTextContent to update the <option> text for all cases.
in v16, it executes removeChild or insertBefore, or whatever variation of the operation where ReactDOM assumes text nodes should be added or removed - when in fact there shouldn't be more than 1 child node because it's flattened in ReactDOMFiberOption (option text nodes were also flattened before in v15, so flattening might be not the issue here)
(whereas this is the normal set of operations for generic components in 15 and 16 because text nodes aren't flattened in other components)

@gaearon - I think the solution has something to do with going back to the old behavior of doing setTextContent for <option> updates - I'm not sure yet how to go about doing this yet, but this is a step closer to finding it.

[gaearon]

Sounds good. I really haven't dug into that code but happy to review a PR once you have something that works.

[stephenkingsley]

I think function insertBefore has something strange.

insertBefore: function (parentInstance, child, beforeChild) {
  parentInstance.insertBefore(child, beforeChild);
},

That is correct , if I use parentInstance.childNodes[0] instead of beforeChild. Like this:

insertBefore: function (parentInstance, child, beforeChild) {
  parentInstance.insertBefore(child, parentInstance.childNodes[0]);
},

So, in my options, the problem is parentInstance.childNodes not contain beforeChild.

My solution is

// in ReactDOMFiberOption.js file
function flattenChildren(children) {
  let content = document.createElement('span');
  // Flatten children and warn if they aren't strings or numbers;
  // invalid types are ignored.
  // We can silently skip them because invalid DOM nesting warning
  // catches these cases in Fiber.
  React.Children.forEach(children, function(child) {
    if (child == null) {
      return;
    }

    if (typeof child === 'string' || typeof child === 'number') {
      if (typeof children === 'string') {
        content += child;
      } else {
        if (!content) {
          content = document.createElement('span');
        }
        const textNode = document.createTextNode(child);
        content.appendChild(textNode);
      }
      // content += child;
    }
  });

  return content;
}

[stephenkingsley]

@gaearon would you please code review?

[cvlab]

I have the same problem with this code

<select>
	{ this.state.option === 'first' ?
		<option>{ '' } a</option>
		:
		<option></option>
	}
</select>

or

<option><br/> a</option>
:
<option></option>

because React try to delete each node from option

@kevinzwhuang my workaround is to use unique keys
https://jsfiddle.net/qkc4eyqe/5/ or https://jsfiddle.net/qkc4eyqe/8/

[itssumitrai]

Got the same issue with <span>, any update on the fix ?

[dgrcode]

I'm hitting this too

[gaearon]

@itssumitrai This issue is about option. If you have a problem with some other tag please file a new issue with a reproducing example.

@dgrcode Knowing that you hit it too doesn't add anything to this conversation. Comments notify everybody subscribed to this thread so it's best to avoid commenting if it doesn't add new info. If you'd like to share more details, or help get it fixed, please let us know!

[gaearon]

If someone wants to take another look at #12078 and specifically #12078 (comment) you're most welcome to do it. That will likely lead us closer to fixing this.

[Slowyn]

@gaearon Hi Dan! I would like to investigate into this problem and try to help with fix.

[gaearon]

Sounds great! This should be a good starting point: #12078 (comment)

[Slowyn]

@gaearon Hi Dan!
I did some research and that's what I have at the moment. Basically, I just confirm @kevinzwhuang conclusions.

1. A fiber of text inside of option contains stateNode without a parentNode.
2. It happens because of flattening children into an option.
3. this bug isn't reproduced if I replace children flattening with filtering an array. But this produce new bugs and tests are failed, so it doesn't fit as a decision.
4. Also, the bug isn't reproduced when you use a construction like this {condition ? 'renderString' ''}. I mean if not use null or false ({condition && 'renderString'}).

I think the easiest solution would be a changing of flattening (or replacing) in a right way. As I get it an option expects one string as a child, but I'm not sure why we need it now when fiber landed 🤔

By the way, it's cool to dip into react sources :)

[gaearon]

I think the easiest solution would be a changing of flattening (or replacing) in a right way.

Sounds good to me, wanna try it?

[Slowyn]

Yeah, of course!

[Slowyn]

I have tried to change flattening, but it didn't fix all problems and became a reason for new issues.
Flattening cause different states. DOM has flattened children, while fiber contains all children.
The other possible solution is to allow react to set text content to option as it does to a textarea. It solves all problems, but there is no warning about incorrect children inside because validateDomNesting is called.

[Slowyn]

@gaearon Hi, Dan!
I've created a PR that solves the issue. I've added example into fixtures so it would be easy to test.
Let's discuss this solution :)

[gaearon]

Fixed by @Slowyn

[krishnaTORQUE]

@gaearon I experienced this regression myself yesterday when upgrading from 15.4.2 to 16.2.0.
Here's a simple repro of the problem I was seeing:
https://jsfiddle.net/qkc4eyqe/1/

Same issue - different error message:
Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

I'm not sure if @mannanali413 is still working on a fix for this, but I can take a closer look later today too.

To anyone else experiencing this problem, the only workaround I've found so far is to remove the conditional logic (at least until a fix is found).

after update 16.13.1
I am getting the same error

does anyone found any solution ?