Skip to content
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

Svg icons don't work under shadow dom #9190

Closed
VladB93 opened this issue Mar 24, 2021 · 5 comments · Fixed by #10202
Closed

Svg icons don't work under shadow dom #9190

VladB93 opened this issue Mar 24, 2021 · 5 comments · Fixed by #10202
Assignees
Labels
🐛 bug Any issue that describes a bug 🏃 priority: high version: 11.1.x version: 12.0.x ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.

Comments

@VladB93
Copy link
Contributor

VladB93 commented Mar 24, 2021

Description

Describe the issue.

  • igniteui-angular version: 11.1.1
  • browser: any

Steps to reproduce

https://stackblitz.com/edit/yu-gi-oh?file=src/app/combo-main/combo-main.component.ts

The above sample encapsulates the outlet of the combo in a shadow dom. At this point the svg icon "case-sensitive" doesn't appear.
The icon is in the dom but the svg is not visible.

Expected result

Svg icons should work under shadom dom encapsulation.

@github-actions
Copy link

There has been no recent activity and this issue has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Jun 23, 2021
@pmoleri
Copy link

pmoleri commented Sep 22, 2021

Re-bumping this.

Here is another repro stackblitz:
https://stackblitz.com/edit/github-eabljg-vbghbk?file=src/app/app.component.ts
image

Tagging @zdrawku

@pmoleri pmoleri reopened this Sep 22, 2021
@pmoleri
Copy link

pmoleri commented Sep 23, 2021

I found this userland workaround:
https://stackblitz.com/edit/github-eabljg-ss3j2f?file=src/app/my-shadow-dom/my-shadow-dom.component.ts

It consists in:

  • Providing a brand new instance of the IgxIconService scoped to the shadow root owner
  • Providing a replacement for the DOCUMENT object, which is a Proxy that forwards the necessary properties to the shadow root, and keeps the rest in the main Document.

I'm sure that @rkaraivanov can do a better job setting up the injection providers than me.

This is a very basic example, for a more complex app overwriting the DOCUMENT can introduce other side effects, in that case what we can do is manually create the IgxIconService passing the proxied DOCUMENT only to it, something like:
{ provider: IgxIconService, useFactory: createIconService, deps: [DomSanitizer, HttpClient, Document, ElementRef] }
and create the shadow proxy inside the factory function.

I guess a solution can be for the IgxIconService to accept an optional ElementRef and check if it contains a shadowRoot, that would make the whole thing a lot easier to set up.

@zdrawku zdrawku added 🏃 priority: high and removed status: inactive Used to stale issues and pull requests labels Sep 23, 2021
@damyanpetev
Copy link
Member

@pmoleri Yeah, you can totally override what's behind the IgxIconService as a token and the factory will do. So will straight-up overriding the deps on the original or going to through an extended class (prob the better option, both with factory give some type safety). https://stackblitz.com/edit/github-eabljg-sqvkdv?file=src/app/my-shadow-dom/my-shadow-dom.component.ts

The way the service is currently provided in root means it can't get element ref on it's own, so going for that will force the API methods to take one which seems a bit clunky. This could be the approach for some components though, but for the service I believe we have a different direction in mind - not to use the DOM to cache the SVGs at all.

@pmoleri
Copy link

pmoleri commented Sep 24, 2021

Nice. The extended class approach is nicer because I don't have to manually declare the provider deps.

not to use the DOM to cache the SVGs at all

Yes, it seems the only sane route for SVG in shadow DOM, otherwise you'll get a cache copy in each shadow DOM and you can have many. It could be made optional, like if shadowDomSupport: true -> cache: off.

@simeonoff simeonoff added 🛠️ status: in-development Issues and PRs with active development on them and removed 🆕 status: new labels Sep 27, 2021
@simeonoff simeonoff added ❌ status: awaiting-test PRs awaiting manual verification ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged. and removed 🛠️ status: in-development Issues and PRs with active development on them ❌ status: awaiting-test PRs awaiting manual verification labels Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Any issue that describes a bug 🏃 priority: high version: 11.1.x version: 12.0.x ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants