-
Notifications
You must be signed in to change notification settings - Fork 26.2k
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
base: main
Are you sure you want to change the base?
Conversation
8576904
to
a85e0a3
Compare
There was a problem hiding this 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.
@lmartorella Also could you please squash that second "linted" commit? Thanks! |
3cfe4c0
to
78021bb
Compare
@lmartorella Looks like this has some legitimate failing tests that you'll need to address. |
@lmartorella I think the issue you are seeing in your test failures is coming from the |
Hi, trying to build to run the tests, but
No proxy, no VPN. |
Runs ok for me locally using Node |
Added new API to let ShadowDom encapsulation to be isolated and avoid injection of shared styles Updated golden files
78021bb
to
ce30113
Compare
Great, node 20 was the issue, on node 22 everything is OK. |
There was a problem hiding this 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
There's still a failing test but not sure exactly why. Seems to be Protractor has failed to even run? |
That's a flaky test, usually a re-run fixes it. |
Looks like it's flaked again - could we trigger a re run? |
Is anyone able to assist in getting through the final pending checks? |
There was a problem hiding this 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
Just needs labels removing to pass the ci/angular test |
@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 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently the styles of any live Angular component with
None
andEmulated
style encapsulation type are bleeding into the live Angular component's instance ofShadowDom
ones.This is bad for two main reasons:
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:
<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:
ViewEncapsulation
type (something likeIsolatedShadowDom
) that explicitly denies shared style injection;Does this PR introduce a breaking change?