Skip to content

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 5, 2025

Safari has a bug where if you put a block element inside an inline element and the inline element has a view-transition-name assigned it finds it as duplicate names.

https://bugs.webkit.org/show_bug.cgi?id=290923

This adds a warning if we detect this scenario in dev mode.

For the case where it renders into a single block, we can model this by making the parent either block or inline-block automatically to fix the issue. So we do that to automatically cover simple cases like <a><div>...</div></a>. This unfortunately causes layout/styling thrash so we might want to delete it once the bug has been fixed in enough Safari versions.

@sebmarkbage sebmarkbage requested a review from eps1lon April 5, 2025 04:57
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Apr 5, 2025
@react-sizebot
Copy link

Comparing: 6a7650c...9bba396

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 = 516.15 kB 516.12 kB = 91.87 kB 91.87 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.28% 619.52 kB 621.25 kB +0.31% 109.51 kB 109.85 kB
facebook-www/ReactDOM-prod.classic.js +0.25% 650.76 kB 652.42 kB +0.25% 114.94 kB 115.23 kB
facebook-www/ReactDOM-prod.modern.js +0.26% 641.04 kB 642.70 kB +0.25% 113.38 kB 113.67 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js +0.46% 65.07 kB 65.37 kB +0.73% 16.28 kB 16.40 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.28% 619.52 kB 621.25 kB +0.31% 109.51 kB 109.85 kB
oss-experimental/react-dom/cjs/react-dom-client.development.js +0.28% 1,121.36 kB 1,124.45 kB +0.36% 186.85 kB 187.52 kB
oss-experimental/react-dom/cjs/react-dom-profiling.development.js +0.27% 1,137.75 kB 1,140.84 kB +0.35% 189.68 kB 190.35 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.27% 1,137.90 kB 1,140.99 kB +0.35% 190.54 kB 191.21 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.js +0.27% 633.93 kB 635.66 kB +0.29% 113.10 kB 113.42 kB
facebook-www/ReactDOM-prod.modern.js +0.26% 641.04 kB 642.70 kB +0.25% 113.38 kB 113.67 kB
facebook-www/ReactDOM-dev.modern.js +0.26% 1,176.28 kB 1,179.29 kB +0.31% 195.05 kB 195.67 kB
facebook-www/ReactDOM-prod.classic.js +0.25% 650.76 kB 652.42 kB +0.25% 114.94 kB 115.23 kB
facebook-www/ReactDOM-dev.classic.js +0.25% 1,185.42 kB 1,188.44 kB +0.31% 196.82 kB 197.44 kB
facebook-www/ReactDOMTesting-dev.modern.js +0.25% 1,192.81 kB 1,195.83 kB +0.32% 198.83 kB 199.47 kB
facebook-www/ReactDOMTesting-prod.modern.js +0.25% 655.44 kB 657.10 kB +0.25% 116.98 kB 117.27 kB
oss-experimental/react-dom/cjs/react-dom-profiling.profiling.js +0.25% 684.23 kB 685.95 kB +0.28% 119.30 kB 119.63 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.25% 1,201.95 kB 1,204.97 kB +0.31% 200.61 kB 201.23 kB
facebook-www/ReactDOMTesting-prod.classic.js +0.25% 665.16 kB 666.82 kB +0.24% 118.61 kB 118.90 kB
facebook-www/ReactDOM-profiling.modern.js +0.23% 714.78 kB 716.44 kB +0.22% 123.68 kB 123.95 kB
facebook-www/ReactDOM-profiling.classic.js +0.23% 722.82 kB 724.48 kB +0.21% 125.00 kB 125.27 kB

Generated by 🚫 dangerJS against 9bba396

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this introduce layout trash for every browser because of Safari?

@sebmarkbage
Copy link
Collaborator Author

Yes unless we want to do UA detection but that would also suffer from inconsistencies in some edge case where our application of the patch wouldn't yield exactly the same result.

The worst bit is in gestures because to read the computed style we have to compute styling for the clones and currently that pass is interleave with inserting the clones so it needs to computes the styling again and again even if display: inline is not used. We might have to do a second gesture pass to avoid this.

@sebmarkbage sebmarkbage merged commit 365c031 into facebook:main Apr 7, 2025
243 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 7, 2025
Safari has a bug where if you put a block element inside an inline
element and the inline element has a `view-transition-name` assigned it
finds it as duplicate names.

https://bugs.webkit.org/show_bug.cgi?id=290923

This adds a warning if we detect this scenario in dev mode.

For the case where it renders into a single block, we can model this by
making the parent either `block` or `inline-block` automatically to fix
the issue. So we do that to automatically cover simple cases like
`<a><div>...</div></a>`. This unfortunately causes layout/styling thrash
so we might want to delete it once the bug has been fixed in enough
Safari versions.

DiffTrain build for [365c031](365c031)
github-actions bot pushed a commit that referenced this pull request Apr 7, 2025
Safari has a bug where if you put a block element inside an inline
element and the inline element has a `view-transition-name` assigned it
finds it as duplicate names.

https://bugs.webkit.org/show_bug.cgi?id=290923

This adds a warning if we detect this scenario in dev mode.

For the case where it renders into a single block, we can model this by
making the parent either `block` or `inline-block` automatically to fix
the issue. So we do that to automatically cover simple cases like
`<a><div>...</div></a>`. This unfortunately causes layout/styling thrash
so we might want to delete it once the bug has been fixed in enough
Safari versions.

DiffTrain build for [365c031](365c031)
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.

4 participants