Skip to content

Fix fragmentInstance#compareDocumentPosition nesting and portal cases #34069

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jackpope
Copy link
Member

Found a couple of issues while integrating FragmentInstance#compareDocumentPosition into Fabric.

  1. Basic checks of nested host instances were inaccurate. For example, checking the first child of the first child of the Fragment would not return CONTAINED_BY.
  2. Then fixing that logic exposed issues with Portals. The DOM positioning relied on the assumption that the first and last top-level children were in the same order as the Fiber tree. I added additional checks against the parent's position in the DOM, and special cased a portaled Fragment by getting its DOM parent from the child instance, rather than taking the instance from the Fiber return. This should be accurate in more cases. Though its still a guess and I'm not sure yet I've covered every variation of this. Portals are hard to deal with and we may end up having to push more results towards IMPLEMENTATION_SPECIFIC if accuracy is an issue.

@meta-cla meta-cla bot added the CLA Signed label Jul 31, 2025
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jul 31, 2025
@react-sizebot
Copy link

react-sizebot commented Jul 31, 2025

Comparing: 8de7aed...b5d1bae

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.12% 530.04 kB 530.70 kB +0.08% 93.63 kB 93.70 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.32% 654.48 kB 656.56 kB +0.23% 115.34 kB 115.61 kB
facebook-www/ReactDOM-prod.classic.js +0.30% 674.42 kB 676.45 kB +0.26% 118.68 kB 118.98 kB
facebook-www/ReactDOM-prod.modern.js +0.31% 664.84 kB 666.87 kB +0.27% 117.02 kB 117.34 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.development.js +5.34% 11.22 kB 11.82 kB +3.65% 2.44 kB 2.53 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.production.js +5.18% 9.93 kB 10.45 kB +3.57% 2.38 kB 2.47 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.32% 654.48 kB 656.56 kB +0.23% 115.34 kB 115.61 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.js +0.31% 668.90 kB 670.97 kB +0.23% 118.91 kB 119.19 kB
facebook-www/ReactDOM-prod.modern.js +0.31% 664.84 kB 666.87 kB +0.27% 117.02 kB 117.34 kB
facebook-www/ReactDOM-prod.classic.js +0.30% 674.42 kB 676.45 kB +0.26% 118.68 kB 118.98 kB
facebook-www/ReactDOMTesting-prod.modern.js +0.30% 679.24 kB 681.27 kB +0.26% 120.66 kB 120.97 kB
facebook-www/ReactDOMTesting-prod.classic.js +0.29% 688.82 kB 690.85 kB +0.24% 122.27 kB 122.57 kB
oss-experimental/react-dom/cjs/react-dom-profiling.profiling.js +0.29% 718.06 kB 720.16 kB +0.23% 124.72 kB 125.01 kB
facebook-www/ReactDOM-profiling.modern.js +0.28% 737.48 kB 739.53 kB +0.24% 126.93 kB 127.24 kB
facebook-www/ReactDOM-profiling.classic.js +0.28% 745.58 kB 747.64 kB +0.23% 128.26 kB 128.56 kB
oss-stable-semver/react-server/cjs/react-server.development.js = 197.13 kB 196.70 kB = 34.86 kB 34.76 kB
oss-stable/react-server/cjs/react-server.development.js = 197.13 kB 196.70 kB = 34.86 kB 34.76 kB
oss-experimental/react-server/cjs/react-server.production.js = 154.02 kB 153.61 kB = 26.70 kB 26.61 kB
oss-stable-semver/react-server/cjs/react-server.production.js = 136.25 kB 135.83 kB = 24.05 kB 23.97 kB
oss-stable/react-server/cjs/react-server.production.js = 136.25 kB 135.83 kB = 24.05 kB 23.97 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js = 2,072.84 kB 2,062.53 kB = 299.93 kB 298.45 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js = 2,072.67 kB 2,062.35 kB = 299.91 kB 298.42 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js = 2,072.67 kB 2,062.35 kB = 299.91 kB 298.42 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js = 2,068.37 kB 2,058.05 kB = 298.92 kB 297.43 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js = 2,068.19 kB 2,057.88 kB = 298.90 kB 297.41 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js = 2,068.19 kB 2,057.88 kB = 298.90 kB 297.41 kB

Generated by 🚫 dangerJS against b5d1bae

Found a couple of bugs when integrating this API into fabric.
1) Basic checks of nested host instances were inaccurate. For example, checking the first child of the first child of the Fragment would not return CONTAINED_BY.
2) The DOM positioning relied on the assumption that the first and last top-level children were in the same order as the Fiber tree. I added additional checks against the parent's position in the DOM, and special cased a portaled Fragment by getting its DOM parent from the child instance, rather than taking the instance from the Fiber return. This should be accurate in more cases. Though its still a guess and I'm not sure yet I've covered every variation of this. Portals are hard to deal with and we may end up having to push more results towards IMPLEMENTATION_SPECIFIC if accuracy is an issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants