Skip to content

fix(platform-browser): Fixes Shadowdom encapsulation to not include external styles #62357

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix(platform-browser): Fixes Shadowdom encapsulation to not include external styles #62357

wants to merge 5 commits into from
+112 −38

Conversation

ryan-bendel
Copy link

@ryan-bendel ryan-bendel commented Jun 28, 2025 •
edited
Loading

Fixes Shadowdom encapsulation to not include external styles that are not declared by the component. Prevents style bleed, so components are scoped correctly. Now adheres to the spec for Shadowdom

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: 57801

The current Shadowdom implementation is incorrect, and does not adhere to the spec. Shadowdom components should be encapsulated with no style leakage, but currently, external styles are added that are completely unrelated to the shadow component. This causes significant bloat.

Before:

Screenshot 2025-06-27 at 15 32 02

After:

Screenshot 2025-06-28 at 07 12 54

What is the new behavior?

The new behaviour removes the sharedStylesHost from the domrenderer for ShadowDom. There should be NO shared styles polluting the shadowdom.

Does this PR introduce a breaking change?

  • Yes
  • No

Now that it follows the spec, shadowdom does not now add <style> tags unrelated/not imported/defined by the component. If someone has built ui off of (incorrectly) expecting styles not imported by the shadow component to be there, they will need to update their components to declare the necessary styles for that component.

Other information

I have tested this in an enterprise application (over 6000 components), with multiple shadowdom components - each use of one of these components previously added ~50kb bloat, per component. After this change, only the styles that have been declared inside the component are added to it, fixing the bug and making Angular's shadowdom implementation adhere to the spec.

Ideally this would be available as a patch version in both v19 and v20

Your own documentation is currently slightly at odds with the current behaviour. This fix will make the documentation accurate/correct.

ViewEncapsulation.ShadowDom
This mode scopes styles within a component by using the web standard Shadow DOM API. When enabling this mode, Angular attaches a shadow root to the component's host element and renders the component's template and styles into the corresponding shadow tree.

This mode strictly guarantees that only that component's styles apply to elements in the component's template. Global styles cannot affect elements in a shadow tree and styles inside the shadow tree cannot affect elements outside of that shadow tree.

@ryan-bendel
fix(platform-browser): Fixes Shadowdom to not include external styles…
9cf47d9 
… that are not declared by the component. Prevents style bleed, so components are scoped correctly. Now adheres to the spec for Shadowdom
@pullapprove pullapprove bot requested a review from mmalerba June 28, 2025 07:07
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Jun 28, 2025
@ngbot ngbot bot added this to the Backlog milestone Jun 28, 2025
@ryan-bendel ryan-bendel mentioned this pull request Jun 28, 2025
Open
@ryan-bendel ryan-bendel marked this pull request as draft June 28, 2025 09:37
@ryan-bendel ryan-bendel marked this pull request as ready for review June 28, 2025 10:00
@JeanMeche
Copy link
Member

JeanMeche commented Jun 28, 2025 •
edited
Loading

Ideally this would be available as a patch version in both v19 and v20
As per our policies, we only land feature fixes on the current version.

Also, even if this could considered a fix (to respect the spec), the fact the applications can have relied on the current implementation, changing the behavior is a breaking change as such cannot land in a minor. (see https://www.hyrumslaw.com/)

@ryan-bendel
Copy link
Author

ryan-bendel commented Jun 28, 2025 •
edited
Loading

Ideally this would be available as a patch version in both v19 and v20
As per our policies, we only land feature fixes on the current version.

Also, even if this could considered a fix (to respect the spec), the fact the applications can have relied on the current implementation, changing the behavior is a breaking change as such cannot land in a minor. (see https://www.hyrumslaw.com/)

No problem 😊

I mean that's kind of on them for misusing the purpose of the shadowdom 😂 but I take your point

Seeing as you cannot predict what additional stylesheets were getting inserted into the component before, I'd be amazed (actually horrified) if people have built ui around expecting things to be there that shouldn't be.

I'll take a nosey at the tests later on, ran the tests for platform-browser before and they passed, so will dig into the failures

Edit - I have updated tests and added extra test cases, marked as breaking change

@ryan-bendel
fix(platform-browser): Fixes Shadowdom so it meets spec
1dec08e 
BREAKING CHANGE: Fixes Shadowdom to not include external styles that are not declared by the component. Prevents style bleed, so components are scoped correctly. Now adheres to the spec for Shadowdom. Adds test cases.
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Jun 29, 2025
@ryan-bendel
fix(platform-browser): Fixes Shadowdom so it meets spec
acb5c9b 
Refactor of test cases to not use content projection. Sets up child components of shadow component.
@ryan-bendel
test(platform-browser): Fixes linting
ec83ad2 
This commit removes a log that I left in by mistake
@ryan-bendel
test(platform-browser): Fixes integration
065f4f7 
This commit updates expected defer uncompressed main size
@JeanMeche
Copy link
Member

I ran a global test on Google's codebase and confirm that this is a breakage change that we won't land outside of a breaking change window.

@ryan-bendel
Copy link
Author

ryan-bendel commented Jun 30, 2025 •
edited
Loading

I ran a global test on Google's codebase and confirm that this is a breakage change that we won't land outside of a breaking change window.

No problem, that makes sense. Thanks for verifying.

I've fixed the lint issue and the integration test btw that was checking the bundle size.

I'm curious @JeanMeche - what was the issue in the codebase? Were some shadowdom components relying on global styles (that were previously being inserted into the shadowdom)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmalerba mmalerba Awaiting requested review from mmalerba

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
action: discuss area: core Issues related to the framework runtime breaking changes detected: breaking change PR contains a commit with a breaking change
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@ryan-bendel @JeanMeche