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: React cannot render in ShadowRoot #15894

Merged
merged 3 commits into from Aug 17, 2020
Merged

Fix: React cannot render in ShadowRoot #15894

merged 3 commits into from Aug 17, 2020

Conversation

@Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Jun 15, 2019

Fixes #19558

This pr fix:

  • ReactDOM.findDOMNode not working well with a ShadowRoot node
  • ReactDOM ensureListening work in this senario
<custom-element>
    #ShadowRoot: open
        <div>
            <!-- React mount on this node, not the ShadowRoot -->
        </div>
</custom-element>
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jun 15, 2019

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!

@sizebot
Copy link

@sizebot sizebot commented Jun 15, 2019

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: c40075a...3fe3666

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.1% +0.1% 115 KB 115.07 KB 36.27 KB 36.29 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 917.98 KB 918.52 KB 203.84 KB 204.01 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.34 KB 19.34 KB 7.23 KB 7.23 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.95 KB 10.95 KB 4.01 KB 4.01 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.71 KB 10.71 KB 3.95 KB 3.94 KB NODE_PROD
react-dom.development.js +0.1% +0.1% 894.87 KB 895.41 KB 203.45 KB 203.61 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 111.28 KB 111.34 KB 35.81 KB 35.85 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 114.72 KB 114.78 KB 36.87 KB 36.89 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 889.16 KB 889.69 KB 201.94 KB 202.1 KB NODE_DEV
react-dom-server.node.development.js 0.0% 0.0% 133.8 KB 133.8 KB 35.43 KB 35.43 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 111.36 KB 111.42 KB 35.34 KB 35.37 KB NODE_PROD
react-dom-server.node.production.min.js 0.0% 0.0% 20.11 KB 20.11 KB 7.54 KB 7.54 KB NODE_PROD
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 372.27 KB 372.47 KB 68.28 KB 68.32 KB FB_WWW_PROD
ReactDOM-profiling.js +0.1% +0.1% 378.6 KB 378.8 KB 69.63 KB 69.67 KB FB_WWW_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 131.88 KB 131.88 KB 34.89 KB 34.89 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.74 KB 10.74 KB 3.68 KB 3.68 KB UMD_PROD
ReactDOMServer-prod.js 0.0% 0.0% 46.67 KB 46.67 KB 10.74 KB 10.74 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.85 KB 3.85 KB 1.5 KB 1.5 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.48 KB 10.48 KB 3.58 KB 3.58 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.1 KB 1.1 KB 668 B 667 B NODE_PROD

Generated by 🚫 dangerJS

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jun 15, 2019

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

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jun 15, 2019

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

@sravannerella
Copy link

@sravannerella sravannerella commented Nov 4, 2019

Is this going to fix the input event propagation of shadow DOM?

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Nov 5, 2019

Is this going to fix the input event propagation of shadow DOM?

Let me have a try. And I also need a rebase on the react codebase

@codesandbox
Copy link

@codesandbox codesandbox bot commented Nov 5, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 822541c:

Sandbox Source
React Configuration
@sizebot
Copy link

@sizebot sizebot commented Nov 5, 2019

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 822541c

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Nov 5, 2019

Is this going to fix the input event propagation of shadow DOM?

I don't get your meaning by "fix the input event propagation". My change fix the following example. Checkout if it is what you mean

<html>

<body>
  <script src="../../../build/node_modules/react/umd/react.development.js"></script>
  <!-- The latest version on unpkg won't work on this example. -->
  <!-- <script src="https://unpkg.com/react-dom/umd/react-dom.development.js"></script> -->
  <script src="../../../build/node_modules/react-dom/umd/react-dom.development.js"></script>
  <script src="https://unpkg.com/babel-standalone@6/babel.js"></script>
  <div id="container"></div>
  <custom-element>
    #ShadowRoot: open
    <div>
      <!-- React mount on this node, not the ShadowRoot -->
    </div>
  </custom-element>
  <script>
    var s = document.querySelector('custom-element').attachShadow({ mode: 'open' })
    var d = document.createElement('div')
    s.appendChild(d)
  </script>
  <script type="text/babel">
    function Comp() {
      const [s, ss] = React.useState('input')
      return <span>{s}<input value={s} onChange={e => ss(e.target.value)} /></span>
    }
      ReactDOM.render(
        <Comp />,
        d
      );
    </script>
</body>

</html>
@stale
Copy link

@stale stale bot commented Feb 3, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale label Feb 3, 2020
@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Feb 3, 2020

Don't close it.

@stale stale bot removed the Resolution: Stale label Feb 3, 2020
@Jack-Works Jack-Works requested review from gaearon and bvaughn Feb 3, 2020
@sizebot
Copy link

@sizebot sizebot commented Feb 3, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 822541c

@Jack-Works Jack-Works requested a review from necolas Feb 3, 2020
@bvaughn bvaughn requested a review from trueadm Feb 3, 2020
@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 3, 2020

cc @trueadm on this one

Copy link
Member

@trueadm trueadm left a comment

These changes look promising. Thank you for taking the time to do them. I've added some suggested changes, but it would be great if you could add a Jest test too, so we can ensure this doesn't regress in the future (both for events and findDOMNode).

packages/react-dom/src/client/ReactDOMComponent.js Outdated Show resolved Hide resolved
packages/react-dom/src/client/ReactDOMComponent.js Outdated Show resolved Hide resolved
packages/react-dom/src/client/ReactDOMComponent.js Outdated Show resolved Hide resolved
packages/react-dom/src/client/ReactDOMComponent.js Outdated Show resolved Hide resolved
@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Feb 4, 2020

It seems like jsdom doesn't support shadow dom ( jsdom/jsdom#1030 ) so I can't write a test for that

@Jack-Works Jack-Works requested a review from trueadm Feb 17, 2020
@trueadm
Copy link
Member

@trueadm trueadm commented Feb 17, 2020

Looking at this again, it clashes with some of the work we're looking into now in regards to moving event registration away from the document and to roots instead. I'd rather come back to this PR in the future (if needed).

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Feb 17, 2020

Looking at this again, it clashes with some of the work we're looking into now in regards to moving event registration away from the document and to roots instead. I'd rather come back to this PR in the future (if needed).

fc01ed9 if so, can you cherry pick this commit at least?

Copy link

@shivaylamba shivaylamba left a comment

Also Fix the merge conflicts

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Mar 12, 2020

In 62861bb#diff-adeaf9850a891faf89a84c73308e0e0bR284 @trueadm mentioned:

We do not want to register events to document fragments or documents with the modern plugin event system.

Why? Rejecting document fragments will disable the ability that React to render in a ShadowRoot.

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Mar 12, 2020

@shivaylamba @trueadm I have rebased this PR.

Due to conflict with React new event system, the other 3 commits are removed. Only ce0f272 (fix findDOMNode think DocumentFragment is a "ReactComponent") left.

Again, why throws on document fragments container? It's useful when trying to wrap the React component into a web component.

@stale
Copy link

@stale stale bot commented Aug 2, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 3, 2020

findDOMNode is effectively deprecated in modern code. There is no need to make changes to it.

Material needs to migrate away from it; we shouldn't be adding more code to support it.

@Jack-Works Jack-Works changed the title Fix findDOMNode does not work with ShadowRoot Fix: React cannot render in ShadowRoot Aug 4, 2020
@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 4, 2020

@trueadm I made a fix, but I'm not sure if this fix covers all usage with ShadowRoot. I have tried the following mounting ways:

<!-- Working! -->
#ShadowRoot: open (React Root)
    <div onClick={x => console.log(x)}>Content</div>

<!-- Working! -->
#ShadowRoot: open
    <div> (React Root)
        <div onClick={x => console.log(x)}>Content</div>
    </div>

<!-- Working! -->
#ShadowRoot: open (React Portal Root)
    <div onClick={x => console.log(x)}>Content</div>

<!-- Working! -->
#ShadowRoot: open
    <div> (React Portal Root)
        <div onClick={x => console.log(x)}>Content</div>
    </div>

But I found a strange problem:

function App() {
  return <h1 onClick={console.log}>h1</h1>;
  // console.log is replaced by "disableLog" so I can't log any event.
  return <h1 onClick={x => console.log(x)}>h1</h1>;
  // have to do this
}
@trueadm
Copy link
Member

@trueadm trueadm commented Aug 4, 2020

console.log is replaced by "disableLog" so I can't log any event.

That's to be expected, React replaces console.log during render. Just keep with the callback approach, as in reality the render should be side-effect free and console.log is actually a form of side-effect (and one only used really for debugging).

I made a fix, but I'm not sure if this fix covers all usage with ShadowRoot. I have tried the following mounting ways:

This looks good and the implementation is how I would have gone about it too. It appears you need to fix some Flow issues. It would also be great to have a test fixture for ShadowRoots, maybe using the above cases as code examples (see https://github.com/facebook/react/tree/master/fixtures/dom/src for reference).

Thanks for this :)

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 4, 2020

The console replacement part is confusing to me. You can't cancel all side effects after all, this is making people(me) confused and doesn't really resolve the side effect problem.

I'll resolve the CI error and add tests later this week!

@trueadm
Copy link
Member

@trueadm trueadm commented Aug 4, 2020

The console replacement part is confusing to me. You can't cancel all side effects after all, this is making people(me) confused and doesn't really resolve the side effect problem.

This is something we do in DEV to avoid confusion around double renders (to catch side-effects that might cause problems for concurrent mode). Last time I checked, in regards to console.log, we override it so you don't get duplicate console logs. Duplicate console logs might be even more confusing otherwise.

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 4, 2020

@trueadm is correct. React only overrides the console methods for the second render, which it only does in DEV. This prevents duplicate logs from being in the console and confusing people. We understand that it might be confusing as well to see a console.log statement that doesn't print anything– but we think that's a less common case.

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 5, 2020

React only overrides the console methods for the second render, which it only does in DEV. This prevents duplicate logs from being in the console and confusing people.

Actually, duplicate logs will warn the developer about they shouldn't do side effect things out of useEffect. It actually helps developers to figure out what is "double render" or things about the concurrent mode. I think React should at lease preserve the log and warn the developer instead of drop everything they logged, it's super confusing

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 5, 2020

Actually, duplicate logs will warn the developer about they shouldn't do side effect things

We test changes like this within Facebook before we deploy them to open source. From early testing experience, duplicate logs confuse developers into thinking that either React or their application code is broken and triggering multiple renderers. Silencing the log before the second render dramatically reduced the reports we got from confused devs.

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 5, 2020

Silencing the log dramatically reduces this.

It's not resolving the problem. It's hiding the problem. And people (me) start to thinking return <h1 onClick={console.log}>h1</h1>; why the event is not fired.

Another approach might be, if it is not in the rendering stage, let disableLog function behave like a normal log, this will reduce the harm.

@trueadm
Copy link
Member

@trueadm trueadm commented Aug 5, 2020

I feel like this discussion had gone off track. If you have concerns about the behavior around the console.log heuristics, maybe it's best to put these concerns in another issue?

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 5, 2020

Discussion about this moved to #19537

@eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Aug 8, 2020

This PR fixes #19558

By the way: Material-UI is only using findDOMNode for backwards compatibility. If you're not using class components with v4 then everything should be fine. v5 will drop findDOMNode completely. You can test this with v5.0.0-alpha.4 and above.

@Jack-Works
Copy link
Contributor Author

@Jack-Works Jack-Works commented Aug 8, 2020

@trueadm Hi I have fixed the flow type problem, but I have no idea about how to add tests.

The basic idea is

const x = document.querySelector('#root');
const shadow = x.attachShadow({mode: 'open'});

ReactDOM.unstable_createRoot(shadow).render(<App />);
function App() {
  return <h1 onClick={console.trace}>h1</h1>;
}

If the click invokes the function, the test passes.

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2020

Apparently we broke this before and it was last fixed in #6462.

@@ -256,7 +257,7 @@ if (__DEV__) {
}

export function ensureListeningTo(
rootContainerInstance: Element | Node,
rootContainerInstance: Element | Node | ShadowRoot,

This comment has been minimized.

@gaearon

gaearon Aug 17, 2020
Member

This is unnecessary. ShadowRoot is already a Node. (So is element, fwiw)

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2020

I pushed a slightly different fix:

  • The comment right above this code was stale and repeated the code, so I removed it
  • There is no need for this to be a production error, so I changed it to a DEV warning
    • This also means we don't have to pay for this bugfix with production bytes
  • Removed the type changes since this doesn't affect anything else and we'd like to keep types simpler
@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2020

Btw sorry this took long to review; I misunderstood the report in #19558 and thought this was already broken in 16 because both links led to the same sandbox.

@gaearon gaearon merged commit 1287670 into facebook:master Aug 17, 2020
32 checks passed
32 checks passed
Facebook CLA Check Contributor License Agreement is valid!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_persistent Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: sizebot_stable Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Aug 17, 2020

Btw sorry this took long to review; I misunderstood the report in #19558 and thought this was already broken in 16 because both links led to the same sandbox.

My bad, sorry. I have to be more careful when forking codesandbox for different React versions

@gaearon
Copy link
Member

@gaearon gaearon commented Aug 17, 2020

Haha no problem, I do that all the time

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.

9 participants
You can’t perform that action at this time.