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

[Popover] Invariant Violation: findComponentRoot(...) from unclosed <noscript> #3586

Closed
echenley opened this issue Mar 3, 2016 · 15 comments · Fixed by #3647
Closed

[Popover] Invariant Violation: findComponentRoot(...) from unclosed <noscript> #3586

echenley opened this issue Mar 3, 2016 · 15 comments · Fixed by #3647
Assignees
Labels
component: Popover The React component.

Comments

@echenley
Copy link
Contributor

echenley commented Mar 3, 2016

I have a universal app with HMR in development. I noticed something very strange when using an <IconMenu />.

My code:

<IconMenu
  iconButtonElement={ <IconButton><MoreVertIcon color="#fff" /></IconButton> }
  targetOrigin={ { horizontal: 'right', vertical: 'top' } }
  anchorOrigin={ { horizontal: 'right', vertical: 'top' } }
>
  <MenuItem
    primaryText="Logout"
    onClick={ () => actions.logout() }
  />
</IconMenu>

The rendered HTML:

screen shot 2016-03-03 at 4 01 52 pm

If you look at the highlighted part, there is an extra, unclosed <noscript> in there. I believe this is what's causing issues.

Edit:
Looks like it's coming from the react-event-listener library: https://github.com/callemall/material-ui/blob/master/src/popover/popover.jsx#L384

@oliviertassinari

Versions

Material UI Version: 0.15.0-alpha.1
React: 0.14.7
Browser: Chrome

@echenley echenley changed the title IconMenu potentially causing Invariant Violation: findComponentRoot with HMR [IconMenu] Invariant Violation: findComponentRoot(...) from unclosed <noscript> Mar 3, 2016
@echenley echenley changed the title [IconMenu] Invariant Violation: findComponentRoot(...) from unclosed <noscript> [Popover] Invariant Violation: findComponentRoot(...) from unclosed <noscript> Mar 3, 2016
@alitaheri alitaheri added this to the 0.15.0 Release milestone Mar 4, 2016
@vasyas
Copy link

vasyas commented Mar 4, 2016

Spotted the same issue with DropDownMenu

@oliviertassinari
Copy link
Member

@echenley I don't manage to reproduce this issue. Can you help me?

@echenley
Copy link
Contributor Author

echenley commented Mar 7, 2016

@oliviertassinari It looks like this might be a bug in React. It doesn't properly handle nested <noscript /> tags.

If you inspect the source here, you'll see the same thing: http://codepen.io/anon/pen/BKjVpZ

I'm not sure if that's intentional. In the meantime you might have to use an empty <div /> instead of <noscript /> here? https://github.com/callemall/material-ui/blob/master/src/popover/popover.jsx#L383

@oliviertassinari
Copy link
Member

I'm not sure if that's intentional. In the meantime you might have to use an empty <div /> instead of <noscript /> here?

That sounds like a reasonable fix. Do you want to submit a PR?
I think that it's no longer an issue with React 15.0.0.

@stueynet
Copy link

stueynet commented Mar 9, 2016

Testes with react 15.0.0-alpha.1 and it still happens

@echenley
Copy link
Contributor Author

echenley commented Mar 9, 2016

@oliviertassinari Yes, working on a PR!

echenley added a commit to echenley/material-ui that referenced this issue Mar 9, 2016
@mbrookes
Copy link
Member

@echenley

I believe this is what's causing issues.

What issues specifically?

The fix broke nested menus: #3802

@echenley
Copy link
Contributor Author

@mbrookes In React 0.14, nested <noscript> tags render invalid HTML, which caused invariant violations when hot-reloading. Check out this issue: facebook/react#6204. It is apparently fixed in React 15.

@mbrookes
Copy link
Member

Odd - I'm not getting any invariant violation here when I revert the change in #3647 (React 0.14.7).

But I don't understand the use of <noscript> here in the first place TBH.

@echenley
Copy link
Contributor Author

It looks like react-event-listener isn't used in too many places. Could it be re-implemented as a higher order component in such a way that the <noscript> wrapper isn't necessary? @oliviertassinari

@oliviertassinari
Copy link
Member

I could re-implement react-event-listener as a HoC but I don't see how this would help with this issue.
<RenderToLayer /> is also returning null in the render method.

@echenley
Copy link
Contributor Author

The <noscript /> wrapper in Popover is only necessary because there's two children. If react-event-listener were an HoC, there would only be a single child and no need for the explicit <noscript> wrapper.

@mbrookes
Copy link
Member

Sure, but why <noscript>?

@echenley
Copy link
Contributor Author

I'm not entirely sure, but React renders <noscript> when a render function returns null. I'd imagine since both <EventListener /> and <RenderToLayer /> render null, that's why it was chosen? If it weren't for the bug in React 0.14, it would be working fine if I'm not mistaken.

@mbrookes
Copy link
Member

Maybe. Anyway, rather than revert to <noscript> now that react-event-listener appears to be fixed, I'll just make the <div>: <div style={{display: 'none'}}> instead.

@zannager zannager added the component: Popover The React component. label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants