Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Flag to adhere to standard HTML Shadow Dom style encapsulation #59838

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
Loading
from

Conversation

lmartorella
Copy link

@lmartorella lmartorella commented Feb 3, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix / performance improvement

What is the current behavior?

Currently the styles of any live Angular component with None and Emulated style encapsulation type are bleeding into the live Angular component's instance of ShadowDom ones.

This is bad for two main reasons:

  • This violates the standard HTML concept about Shadow Dom style encapsulation, making them fragile and prone to CSS injection. In addition, the current behavior is counter-intuitive for HTML developers, that are expecting real isolation when shadow Dom is involved: https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_shadow_DOM#shadow_dom_and_custom_elements
  • This silently introduces a performance degradation when many instances of Angular components are composed in an application, mixing "ShadowDom" and other encapsulation types. The degradation is roughly of O(M*N) magnitude, where M and N are the component instance counts of None+Emulated type and ShadowDom type respectively.

Issue Number: #57801
Issue Number: #35039
Issue Number: #5059

What is the new behavior?

The new behavior should reflect exactly the HTML spec about Shadow DOM: any component's private style must be injected as before, but no other component's style must be loaded inside the Shadow DOM.
Contrarily to the documentation, that states that the shared styles are injected to avoid "unexpected styles" when nested Angular component are used inside a Shadow-DOM ones, the developer must/can:

  • Use other Shadow-DOM based components with their own private encapsulated styles
  • Use <slot> to permit external style to be used when the component is a "container".

However, the behavior cannot be changed without the dev's consensus.

The proposed PR exposes a global DI flag that the application can enabled (so app-wide) to enable the new behavior.

However, a more granular approach should be used. Other possibilities, more user friendly, are:

  • A new enum flag of the ViewEncapsulation type (something like IsolatedShadowDom) that explicitly denies shared style injection;
  • A new Component annotation property;
  • A module-level config DI object.

Does this PR introduce a breaking change?

  • Yes
  • No

@angular-robot angular-robot bot added the area: elements Issues related to Angular Elements label Feb 3, 2025
@ngbot ngbot bot added this to the Backlog milestone Feb 3, 2025
@lmartorella lmartorella force-pushed the fix/shared-style-in-shadow-dom branch from 8576904 to a85e0a3 Compare February 3, 2025 13:36
@lmartorella lmartorella marked this pull request as ready for review February 3, 2025 13:37
@pullapprove pullapprove bot requested a review from thePunderWoman February 3, 2025 13:37
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

This seems reasonable to me. Looks like you have some conflicts. If you can resolve those, we can land this. Thanks for your work and your patience waiting on a review.

@thePunderWoman
Copy link
Contributor

@lmartorella Also could you please squash that second "linted" commit? Thanks!

@thePunderWoman thePunderWoman added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Apr 2, 2025
@lmartorella lmartorella force-pushed the fix/shared-style-in-shadow-dom branch from 3cfe4c0 to 78021bb Compare April 3, 2025 16:05
@angular-robot angular-robot bot requested a review from thePunderWoman April 3, 2025 16:05
@thePunderWoman thePunderWoman added action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker labels Apr 10, 2025
@thePunderWoman
Copy link
Contributor

@lmartorella Looks like this has some legitimate failing tests that you'll need to address.

@jtuds
Copy link

jtuds commented Apr 17, 2025

@lmartorella I think the issue you are seeing in your test failures is coming from the renderer_factory_spec.ts file where DomRendererFactory2 is being constructed; it has not been amended to take in the isolatedShadowDom boolean so it does not have the number of expected arguments. TLDR: add extra boolean parameter to new DomRendererFactory2() here DomRendererFactory2

@lmartorella
Copy link
Author

Hi, trying to build to run the tests, but yarn build is giving a strange error.

npm ERR! errno ENOTFOUND
npm ERR! network request to https://registry.yarnpkg.com/esbuild/-/esbuild-0.15.17.tgz failed, reason: getaddrinfo ENOTFOUND registry.yarnpkg.com
npm ERR! network This is a problem related to network connectivity.

No proxy, no VPN.
Using nvm targeting node 20.11.1, cleaned node_modules and yarn.lock, yarn install ran just fine. Not a yarn user so not sure what I can do to restore the local build.

@jtuds
Copy link

jtuds commented Apr 22, 2025

Runs ok for me locally using Node 22.14.0 and Yarn 1.22.22 - which Yarn version are you using? It might be worth trying the latest Node LTS which is 22.14.0

Added new API to let ShadowDom encapsulation to be isolated and avoid injection of shared styles
Updated golden files
@lmartorella lmartorella force-pushed the fix/shared-style-in-shadow-dom branch from 78021bb to ce30113 Compare April 22, 2025 13:46
@pullapprove pullapprove bot requested review from atscott, kirjs and mmalerba April 22, 2025 13:46
@angular-robot angular-robot bot requested a review from thePunderWoman April 22, 2025 13:46
@lmartorella
Copy link
Author

Runs ok for me locally using Node 22.14.0 and Yarn 1.22.22 - which Yarn version are you using? It might be worth trying the latest Node LTS which is 22.14.0

Great, node 20 was the issue, on node 22 everything is OK.
Updated the tests and the golden files.

Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing the tests.

reviewed-for: fw-general, public-api

@jtuds
Copy link

jtuds commented Apr 23, 2025

There's still a failing test but not sure exactly why. Seems to be Protractor has failed to even run? E/launcher - Error: Error: Timeout waiting for server to start

@JeanMeche
Copy link
Member

That's a flaky test, usually a re-run fixes it.

@jtuds
Copy link

jtuds commented Apr 24, 2025

Looks like it's flaked again - could we trigger a re run?

@jtuds
Copy link

jtuds commented Apr 28, 2025

Is anyone able to assist in getting through the final pending checks?

Copy link
Contributor

@kirjs kirjs left a comment

Choose a reason for hiding this comment

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

reviewed-for: fw-general, public-api

@jtuds
Copy link

jtuds commented May 7, 2025

Just needs labels removing to pass the ci/angular test

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels May 7, 2025
@ryan-bendel
Copy link

@JeanMeche @alxhub @thePunderWoman - Any update on when this is likely to get merged/released?

This issue is causing considerable bloat in our app, so would like to get it resolved a.s.a.p

@alxhub alxhub self-assigned this May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: discuss area: elements Issues related to Angular Elements target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.