Skip to content

Commit

Permalink
Check the parent fiber in appendChildToContainer
Browse files Browse the repository at this point in the history
Fixes facebook#14535
Alternative to facebook#13778

As @gaearon already noted, we can not rely on a container node having a
`_reactRootContainer` to detect a React Root since the `createRoot()`
API will not set it.

Furthermore, the `createRoot()` API is currently only setting a property
on the container in DEV mode.

We could:

 1. Set a property in prod as well.
 2. Pass in more information into the `appendChildToContainer()` config.

This PR is an attempt to implement 2. to visualize the outcome. It feels
bad to do this though since non of the other renderers need that
property and I’m not sure how stable the reconciler API is (i.e if we
can just add properties like this).

Let me know if you prefer 1. or have another idea. 🙂
  • Loading branch information
philipp-spiess committed Jan 23, 2019
1 parent 6cb2677 commit d494deb
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 8 deletions.
2 changes: 1 addition & 1 deletion packages/react-art/src/ReactARTHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ export function appendChild(parentInstance, child) {
child.inject(parentInstance);
}

export function appendChildToContainer(parentInstance, child) {
export function appendChildToContainer(parentInstance, child, parentIsRoot) {
if (child.parentNode === parentInstance) {
child.eject();
}
Expand Down
16 changes: 15 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2688,7 +2688,21 @@ describe('ReactDOMComponent', () => {
expect(typeof portalContainer.onclick).toBe('function');
});

it('does not add onclick handler to the React root', () => {
it('does not add onclick handler to a React root', () => {
const container = document.createElement('div');

function Component() {
return <div onClick={() => {}} />;
}

const root = ReactDOM.unstable_createRoot(container);
root.render(<Component />);
jest.runAllTimers();

expect(typeof container.onclick).not.toBe('function');
});

it('does not add onclick handler to a legacy React root', () => {
const container = document.createElement('div');

function Component() {
Expand Down
7 changes: 2 additions & 5 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ export function appendChild(
export function appendChildToContainer(
container: DOMContainer,
child: Instance | TextInstance,
parentIsRoot: boolean,
): void {
let parentNode;
if (container.nodeType === COMMENT_NODE) {
Expand All @@ -378,11 +379,7 @@ export function appendChildToContainer(
// This is why we ensure that non React root containers have inline onclick
// defined.
// https://github.com/facebook/react/issues/11918
const reactRootContainer = container._reactRootContainer;
if (
(reactRootContainer === null || reactRootContainer === undefined) &&
parentNode.onclick === null
) {
if (!parentIsRoot && parentNode.onclick === null) {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((parentNode: any): HTMLElement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ export function appendChild(
export function appendChildToContainer(
parentInstance: Container,
child: Instance | TextInstance,
parentIsRoot: boolean,
): void {
const childTag = typeof child === 'number' ? child : child._nativeTag;
UIManager.setChildren(
Expand Down
1 change: 1 addition & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
function appendChildToContainer(
parentInstance: Container,
child: Instance | TextInstance,
parentIsRoot: boolean,
): void {
if (typeof parentInstance.rootID !== 'string') {
// Some calls to this aren't typesafe.
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,8 @@ function commitPlacement(finishedWork: Fiber): void {
}
} else {
if (isContainer) {
appendChildToContainer(parent, node.stateNode);
const parentIsRoot = parentFiber.tag === HostRoot;
appendChildToContainer(parent, node.stateNode, parentIsRoot);
} else {
appendChild(parent, node.stateNode);
}
Expand Down

0 comments on commit d494deb

Please sign in to comment.