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

Fix a crash when using dynamic children in <option> tag #13261

Merged
merged 10 commits into from
Aug 1, 2018

Conversation

Slowyn
Copy link
Contributor

@Slowyn Slowyn commented Jul 24, 2018

fix #11911

I had to move validateDOMNesting into the flattening process so it causes a different warning message. I'm not sure if this is a right approach.

The problem

After the investigation, I got that the reason of crash was desynchronization of Fiber and real node.
<option /> flattens its children, while Fiber does not. So we can pass into <option /> an array of children e.g.

['some', <div>5</div>, null, ' string']

It flattens to a string

'some string'

But Fiber keeps links to all children. Let's assume that null depends on some state. Fiber will try to update children using insertBefore, but as we don't have a real node in DOM, it caused a runtime error.

Solution

<option />'s children should be a string and we have to filter children. The simplest solution was to allow react to set content as a direct text child. It solves the issue and makes a behavior of <option /> like in React v.15... Unfortunately, this solution leads us to skip validateDOMNesting.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@Slowyn Slowyn changed the title Make option children a text content by default [#11911] Make option children a text content by default Jul 24, 2018
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

'<div> cannot appear as a child of <option>.\n' +
' in div (at **)\n' +
' in option (at **)',
'<div> cannot appear as a child of <option>.\n' + ' in option (at **)',
Copy link
Contributor Author

@Slowyn Slowyn Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavior change and I'm not sure how to avoid this with this solution. But it's only warning, so shouldn't be critical.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay. Particularly since the problem is still identified in the message (even if the nest doesn't show up).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aweary or @gaearon Do you anticipate any problems with changing this warning text?

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert scripts/rollup/results.json?

Other than that, this looks good.

'<div> cannot appear as a child of <option>.\n' +
' in div (at **)\n' +
' in option (at **)',
'<div> cannot appear as a child of <option>.\n' + ' in option (at **)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay. Particularly since the problem is still identified in the message (even if the nest doesn't show up).

if (__DEV__) {
// We do not have HostContext here
// but we can put some parent information at least
// Also this cause a bit different message than it was previously
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: “a bit different message” would become meaningless after merging this PR. Let’s ensure any comments are helpful to future readers.

- Remove meaningless comments
- revert scripts/rollup/results.json
@Slowyn
Copy link
Contributor Author

Slowyn commented Jul 26, 2018

My IDE automatically insert an empty row at the end of file -_-
I've "reverted" scripts/rollup/results.json and removed the unnecessary comment.

}
if (__DEV__) {
// We do not have HostContext here
// but we can put some parent information at least
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could you reformat this like:

    // We do not have HostContext here, but we can at least
    // put some parent information

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this behavior cause an issue in JSDOM too? I'm curious if the manual fixture could be moved into a unit test.

@Slowyn
Copy link
Contributor Author

Slowyn commented Jul 26, 2018

I can try to write a test. I think It's a case for DOMSelect-test, right?

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 26, 2018 via email

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why "we don't have host context here".

This function is called from getHostProps which is called from setInitialProperties which is called from finalizeInitialChildren which definitely has host context. Pass it through.

@Slowyn
Copy link
Contributor Author

Slowyn commented Jul 26, 2018

@gaearon You are right, I've totally missed it. But after a quick look, it seems that sometimes hostContext is undefined. I'm checking now.

@Slowyn
Copy link
Contributor Author

Slowyn commented Jul 26, 2018

image
It seems like it's a consequence of this changes.

@gaearon
Copy link
Collaborator

gaearon commented Jul 26, 2018

Can you push the code that doesn't work so I can take a look?

@Slowyn
Copy link
Contributor Author

Slowyn commented Jul 26, 2018

Sorry, I wasn't able to push right away.
I forgot to pass ancestorInfo when was making the screenshot., but passing ancestorInfo wouldn't change the result.

@@ -545,6 +545,48 @@ describe('ReactDOMSelect', () => {
expect(node.options[2].selected).toBe(false); // gorilla
});

it('should not fail', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very vague.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, It's some temporary "name" until we get a full working version.
Going to change it to some concrete form 😄

Have you taken a look on anccestorInfo in current case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this description: "Options with stateful children; it should change its state properly and shouldn't fail"
What do you think?

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2018

Seems like the reason this doesn't work is because the host context we get is from option's parent, not option itself.

segoddnja pushed a commit to segoddnja/react that referenced this pull request Aug 1, 2018
* Make option children a text content by default

fix facebook#11911

* Apply requested changes

- Remove meaningless comments
- revert scripts/rollup/results.json

* remove empty row

* Update comment

* Add a simple unit-test

* [WIP: no flow] Pass through hostContext

* [WIP: no flow] Give better description for test

* Fixes

* Don't pass hostContext through

It ended up being more complicated than I thought.

* Also warn on hydration
@gaearon gaearon mentioned this pull request Sep 5, 2018
@exogen
Copy link

exogen commented Jan 18, 2019

FYI, I just upgraded React and I suspect this change (or one of the other <option> PRs) caused a backwards incompatibility within the 16.x channel.

Despite the HTML <option> tag only allowing strings for labels, all that should matter in React is what makes it to the final HTML output. For instance, these should (and used to) work:

<option><React.Fragment>Label A</React.Fragment></option>

import { FormattedMessage } from 'react-intl';
<option><FormattedMessage>{msg => "Label B"}</FormattedMessage></option>

const MyLabel = () => "Label C";
<option><MyLabel /></option>

All three of the above worked in 16.4.x, but will render [object Object] as the option label now. Due to using the middle technique above in our app, some of our forms unfortunately started doing this.

Edit: looks like this was already discussed in #13586

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2022

This works again in 18 if you specify value for <option>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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