fix(platform-browser): Fixes IsolatedShadowDom encapsulation method #63131
Conversation
|
@thePunderWoman et al 👋 - me again! A follow up to the I've done a bit of a refactor, providing an alternative way for This is very much a RFC, or even better if you have capacity to have a quick test? It's a bit of a grey area for me in regards to the shadowdom spec, which doesn't explicitly talk about child component styling (and Angular muddies the waters a bit with different encapsulation methods). However this feels like the best of both worlds, parent styles cannot leak/be added into the shadowdom, but child component styles are correctly applied. |
5039a7e to
367d39d
Compare
bba0484 to
8813776
Compare
0b8d24e to
42511ea
Compare
42511ea to
19ea9b8
Compare
|
👋 - any chance of getting this merged (stops me having to keep the branch up to date) 😅 ? Would be good to get this in a |
a0c6ebe to
3b5f324
Compare
We're hoping to take a deeper look at it soon! Thanks for the effort keeping this up to date. |
dgp1130
left a comment
There was a problem hiding this comment.
Apologies for the slow review @ryan-bendel, thanks for pushing through this.
Just some minor comments, main thing is just trying to understand the lifecycle of SharedStylesHost and how shadow roots are tracked there.
|
@dgp1130 @thePunderWoman - Addressed the comments on the PR. I did encounter a bug though, which meant i've had to do a bit of a refactor (and arguably add a bit more complexity) I had only tested a single My fix now tracks usage, and copies the styles into both app & shadowroot, so styles persist across usages. The one "gotcha", which I don't think we need to fix (i think it'll exist today) - is if you put a "none" encapsulation component inside a shadowdom, you get into css specificity issues, as there is not a unique identifier in ShadowDom/None like there is with Emulated. Hard to articulate exactly what i mean, hopefully the below image helps. These components are all Lastly, i'm unable to really test SSR in practice (i don't really use it so don't feel qualified enough to thoroughly test it) - if one of you would have any capacity to check there's no regressions (even outside of SSR), that would be much appreciated! The more i touch this the less confident i get 🤣 
|
1dabb83 to
34391f7
Compare
dgp1130
left a comment
There was a problem hiding this comment.
I wouldn't worry too much about SSR given that we don't support declarative shadow DOM just yet. AFAICT, it's just broken today as we seem to put component styles into the <head> tag, which is necessary for initial render but also leaks those styles out of the shadow root.
IsolatedShadowRoot should probably throw when used in SSR until we support declarative shadow DOM. So should ShadowRoot frankly, but I'll check with the rest of the team to see if that's worth the breaking change at the moment.
39be8de to
f0a0dc5
Compare
7755745 to
f8deb43
Compare
9f3ae01 to
a4d94cd
Compare
|
Thanks for the follow up, I haven't had a chance to look at it just yet, but FYI, I'm gonna be out for a couple weeks and will look at this when I get back. |
| this.shadowRoot = (hostEl as HTMLElement).attachShadow({mode: 'open'}); | ||
| } else { | ||
| // In SSR or environments without shadow DOM support, throw | ||
| throw new Error( |
There was a problem hiding this comment.
Bug: Won't this throw for standard ViewEncapsulation.ShadowDom? I think we need to maintain the existing behavior and only throw for IsolatedShadowDom.
There was a problem hiding this comment.
@dgp1130 - maybe i'm misremembering, but standard Shadowdom doesn't work in SSR either, right? Which is what this would catch. I could put in a condition to only throw for Isolated, but i'm not clear on what standard would do in a server environment here....
|
Looks like I found a usecase on angular.dev for that feature once the fix lands |
1439753 to
94d7b2e
Compare
I've refactored IsolatedShadowDom now to use native Removed dom walking in isolated_style_scope and replaced with using Angulars LView hierarchy which is much nicer. Demo if you want to have play (if you uncomment |
032f709 to
8530d0e
Compare
| "dist/main.js": 153915, | ||
| "dist/polyfills.js": 34023, | ||
| "dist/open-close.component-[hash].js": 1190 | ||
| "dist/main.js": 159053, |
There was a problem hiding this comment.
Since this is quite an increase, we should investigate better tree shaking when the feature isn't used
There was a problem hiding this comment.
I shall take a look @JeanMeche - but i am struggling to run the integration tests locally (i bumped all the size.json from the output in the gh test runner output),
I get:
ERROR Failed to switch pnpm to v10.23.0. Looks like pnpm CLI is missing at "/var/folders/kj/cqmyhbyx0zv7mrtvpdlj19dw0000gp/T/ng-integration-test7nTJJM/tmp-env-0/Library/pnpm/.tools/pnpm/10.23.0/bin" or is incorrect
spawnSync /var/folders/kj/cqmyhbyx0zv7mrtvpdlj19dw0000gp/T/ng-integration-test7nTJJM/tmp-env-0/Library/pnpm/.tools/pnpm/10.23.0/bin/pnpm ENOENT
Command failed: `pnpm install`
🤔
2c2deab to
60acf6d
Compare
7cdd447 to
c0d2fb9
Compare
Implement IsolatedStyleScopeService. Refactor IsolatedShadowDom implementation to fix various bugs. Uses native slot instead of ng-content.
4254fca to
2471f4e
Compare
|
Sorry for the slow responses on my end, I've been meaning to take another look here but have just been held up. FYI, I made a related PR #66048 to fix bootstrapping Angular inside of a shadow root and found myself rewriting most of the styling system to better support |
|
Looking at this PR again, I'm thinking the changes to Where there is still value is in the changes around that. Specifically the errors for running |
@dgp1130 Apologies for the slow reply, i was having a bit of a digital detox over the holidays, so just catching up! Happy to close this, if you could tag me in the other PR when it gets merged I can then start again with anything that's missing for Isolated 😄 |
|
Closing awaiting #66048 to be merged to pick up again |
This fixes an issue where a child component inside of IsolatedShadowDom would get it's styles moved to the root style object, causing it to be unstyled. This new approach hoists the child styles to the shadowroot. This change only applies to IsolatedShadowDom
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?
After introducing IsolatedShadowDom, it was raised on the PR that any child components using other encapsulation methods would have their styles hoisted to the root styles of the app. This would cause the child component to be unstyled as it is inside a shadowroot.
What is the new behavior?
The new behaviour hoists the styles of any child component into the head of the shadowroot.
Does this PR introduce a breaking change?
Other information
Spun up a test project:
emulated, with a button styled redComparisons:
Standard/old
Shadowdomencapsulation with style leakage:IsolatedShadowDompre-fix (styles are moved to root so child component loses styling):IsolatedShadowDompost-fix (styles are moved to shadowhost, fixing child component styling):As you can see from the last screenshot, the styles are now correctly applied to the child (
Emulated) component