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

feat: support attached shadow dom elements #618

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

breakthestatic
Copy link
Contributor

@breakthestatic breakthestatic commented Apr 22, 2022

PR Checklist

Please review the guidelines for contributing to this repository.

  • I am requesting to pull a topic/feature/bugfix branch (right side). In other words, not main.
  • I have run yarn test against my changes and tests pass.
  • I have added tests to prove my fix is effective or my feature works. This can be done in the form of unit tests in test/unit/ or a new or altered demo in demo/.
  • I have added or edited necessary types and generated documentation (yarn docs), or no docs changes are needed.

Description

Updates the isAttached helper to handle shadow dom elements. If the passed element's root node is a ShadowRoot, it sets parent to the ShadowRoot's host and subsequently checks if the host is attached; otherwise it falls back to the pre-existing functionality.

With additional testing, I found that the prior change was not sufficient to handle multiple nested shadow dom elements. The logic has been updated to manually traverse through the dom (handling both regular & shadow nodes) to ensure that any nesting is properly accounted for.

Fixes: #536

Original implementation did not properly handle nested shadow dom elements.

Updated the traversal to manually step through the dom to ensure that all
shadow dom ancestors are properly handled.
@breakthestatic
Copy link
Contributor Author

Hey @timmywil, I haven't really contributed to many open source projects before, so I apologize in advance if this is not the "right" way to do this - would you be able to take a look at this PR soon or provide a rough idea of when you'll be able to? I'm using the proposed fix in 2 other projects right now and would love to point to your repo instead of a local link or my fork. Let me know if there's anything else needed & thanks!

@breakthestatic
Copy link
Contributor Author

Hey @timmywil, just wanted to ping again to see if you're able to review and merge or provide feedback. Thanks!

@breakthestatic
Copy link
Contributor Author

@timmywil, can you please take a look at this and merge if acceptable (or provide guidance if you have any issues). I'd rather not permanently fork the project for this, but it's been quite a while with no movement.

@dbrnz
Copy link

dbrnz commented Jan 9, 2023

@timmywil any chance of this being merged? It all works for me. Thanks!

@timmywil
Copy link
Owner

timmywil commented Jan 9, 2023

Yes, this looks good. Sorry I haven't gotten to it sooner.

@timmywil timmywil merged commit 7428761 into timmywil:main Jan 9, 2023
@dbrnz
Copy link

dbrnz commented Jan 9, 2023

Thanks!!

A great library -- it just works and works well, compared to some others out there...

dbrnz added a commit to dbrnz/panzoom that referenced this pull request Feb 16, 2023
@dermotduffy
Copy link

@timmywil Thanks for this great little library. Would it be possible to get a new release anytime soon to include this PR? As-is the latest release is not usable by web components (without turning off shadow DOM usage entirely), and it looks like this is the only major change since the last release -- so may be an easy / low risk release. Thank you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants