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

@facebook-github-bot facebook-github-bot commented Jul 24, 2018

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!

Loading

@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

@facebook-github-bot facebook-github-bot commented Jul 24, 2018

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

Loading

@Slowyn Slowyn force-pushed the feature/fix-option branch from 0367120 to 388028e Jul 24, 2018
'<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.

Loading

Copy link
Contributor

@nhunzaker nhunzaker Jul 26, 2018

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).

Loading

Copy link
Contributor

@nhunzaker nhunzaker Jul 26, 2018

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?

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Can you revert scripts/rollup/results.json?

Other than that, this looks good.

Loading

'<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

@nhunzaker nhunzaker Jul 26, 2018

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).

Loading

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
Member

@gaearon gaearon Jul 26, 2018

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.

Loading

Slowyn added 2 commits Jul 26, 2018
- Remove meaningless comments
- revert scripts/rollup/results.json
@Slowyn
Copy link
Contributor Author

@Slowyn 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.

Loading

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

@nhunzaker nhunzaker Jul 26, 2018

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

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

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

Loading

@Slowyn
Copy link
Contributor Author

@Slowyn Slowyn commented Jul 26, 2018

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

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jul 26, 2018

Loading

Copy link
Member

@gaearon gaearon left a comment

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.

Loading

@Slowyn
Copy link
Contributor Author

@Slowyn 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.

Loading

@Slowyn
Copy link
Contributor Author

@Slowyn Slowyn commented Jul 26, 2018

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

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Jul 26, 2018

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

Loading

@Slowyn
Copy link
Contributor Author

@Slowyn 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.

Loading

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

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

@gaearon gaearon Jul 31, 2018

Choose a reason for hiding this comment

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

This is very vague.

Loading

Copy link
Contributor Author

@Slowyn Slowyn Jul 31, 2018

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?

Loading

Copy link
Contributor Author

@Slowyn Slowyn Jul 31, 2018

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?

Loading

@gaearon
Copy link
Member

@gaearon 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.

Loading

gaearon
gaearon approved these changes Aug 1, 2018
Copy link
Member

@gaearon gaearon left a comment

I think this is good. Thanks for digging to the bottom of this. I made a few tweaks so that it warns during SSR hydration too.

Loading

@gaearon gaearon merged commit 0182a74 into facebook:master Aug 1, 2018
1 check passed
Loading
segoddnja added a commit to segoddnja/react that referenced this issue 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
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 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 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

Loading

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

Successfully merging this pull request may close these issues.

6 participants