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

renderBodyContextMenu gets rendered outside of React root #1121

Closed
mscolnick opened this issue May 18, 2017 · 8 comments · Fixed by #5489
Closed

renderBodyContextMenu gets rendered outside of React root #1121

mscolnick opened this issue May 18, 2017 · 8 comments · Fixed by #5489

Comments

@mscolnick
Copy link
Contributor

for blueprintjs/table

Bug report

  • Package version(s): blueprintjs/table: 1.14.0
  • Browser and OS versions: n/a

Steps to reproduce

Create a table with renderBodyContextMenu. View the context menu in the React dev console and see that its react context is empty

Actual behavior

renderBodyContextMenu gets rendered outside of the React hierarchy that the Table is in so the context menu does not have access to my React context

Expected behavior

context menu has React context

@giladgray
Copy link
Contributor

seems like the crux of this issue is using ReactDOM.render instead of ReactDOM.unstable_renderSubtreeIntoContainer. The latter preservers context; the former creates new. Makes sense to refactor to the latter as we're re-implementing Portal functionality.

@mscolnick
Copy link
Contributor Author

thanks!

@tgreenwatts
Copy link
Contributor

Does this require work on blueprint's side? Sorry I'm unclear

@speletz
Copy link

speletz commented Sep 20, 2017

Any update on this?

@adidahiya
Copy link
Contributor

no, I don't think anyone has looked into this. would be open to PRs (with tests included). I believe this is the line in question that @giladgray is referring to:

contextMenu = ReactDOM.render(<ContextMenu />, contextMenuElement) as ContextMenu;

@giladgray
Copy link
Contributor

ugh this is not easily fixable. i'm not sure that using unstable_... would preserve context.

@giladgray giladgray removed this from the 3.0.0 milestone Apr 17, 2018
@giladgray giladgray removed their assignment Apr 17, 2018
@JoonsungUm
Copy link
Contributor

JoonsungUm commented Apr 19, 2018

@giladgray The root cause of these issues seems to be that the ReactDOM.render() method, which you mentioned, creates a new tree. The problem is that the new tree cannot access the context shared within the existing tree.

What do you think about re-implementing the ContextMenu? When I re-implemented the core of the declarative part using the Portal, I could get the following benefits:

  • React Context is preserved.
  • If this.state and this.props are changed, The contents of ContextMenu will be re-rendered.

I know that you are currently working on a fix for ContextMenu. After you complete the work, I will be able to reimplement the Imperative API.

Attached a partial implementation.

contextMenu.tsx

import { Portal } from "../portal/portal";

// ...

export interface IContextMenuProps {
    // Move fields in state to props so that the fields can be passed by ContextMenuTarget components
    menu?: JSX.Element;
    offset?: IOffset;
    // ...

}

// ...

export class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuState> {

    // ...

    public render() {
        const content = <div onContextMenu={this.cancelContextMenu}>{this.props.menu}</div>;

        // ...
    
        return (
            // Wrapping with Portal
            <Portal>
                <div className={Classes.CONTEXT_MENU_POPOVER_TARGET} style={this.props.offset}>
                    <Popover
                        
                    // ...

                    </Popover>
                </div>
            </Portal>
        );
    }

// ...

contextMenuarget.tsx

import { ContextMenu } from "./contextMenu";

// ...

    public render() {

// ...

        const onContextMenu = (e: React.MouseEvent<HTMLElement>) => {
            e.persist();
            e.preventDefault();
            this.setState({ e } as any);
        };

        const menu = this.state.e ? this.renderContextMenu(this.state.e) : null;
        const root = React.cloneElement(element, { onContextMenu });
        return (
            <React.Fragment>
                {root}
                {menu && (
                    <ContextMenu menu={menu} offset={{ left: this.state.e.clientX, top: this.state.e.clientY }} />
                )}
            </React.Fragment>
        );
    }
}

// ...

@badams
Copy link
Contributor

badams commented Apr 26, 2018

@js-um @giladgray Would be great to get the menu re-rendering when state/props changes.
Is there any way with the current implementation to achieve this?

@adidahiya adidahiya added this to the 5.0.0 milestone Mar 3, 2022
@adidahiya adidahiya self-assigned this Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants