ember-basic-dropdown icon indicating copy to clipboard operation
ember-basic-dropdown copied to clipboard

Shadow DOM support, implements #617

Open lolmaus opened this issue 3 years ago • 2 comments

This is a proof-of-concept PR that implements basic Shadow DOM support, closing #617. Please see the issue for explanation of why Shadow DOM may be necessary.

This PR misses Shadow DOM-specific tests and documentation. I would like to receive a general approval and guidelines from ember-basic-dropdown maintainers before investing moore time into it.

Existing tests pass, and we already use this branch in a project.

This PR does not cover all possible cases of arbitrarily creating Shadow DOM containers and arbitrarily positioning the wormhole container. Supporting all such cases would be tricky and would like require a generic hack like https://github.com/Georgegriff/query-selector-shadow-dom which is undesirable.

But this PR does cover the most basic and demaded case: wrapping your whole Ember app with one Shadow DOM container. It can be done effortlessly with our https://github.com/kaliber5/ember-embedded-snippet/ addon.

In order to avoid leaking app styles into the host website, stylesheets are referenced from inside Shadow DOM. This means that a wormholed dropdown content would appear unstyled if the wormhole is outside of the Shadow DOM.

Thus, a requirement of using ember-basic-dropdown in Shadow DOM is to set up a wormhole container in application.hbs and override ['ember-basic-dropdown'].destination to prevent the addon from emitting a default one into {{content-for "body-footer"}}.

CC @simonihmig

lolmaus avatar May 31 '21 12:05 lolmaus

Thank you for this work. My team has been maintaining an internal fork of the 1.x line with similar shadow DOM support. (We only recently became Octane-ready.)

We were able to upgrade to this fork with no issues.

apellerano-pw avatar Jun 16 '21 17:06 apellerano-pw

This PR introduces ember-ref-bucket to the package.json but doesn't look to be using it. The npm install also may have been run with npm 7 even though this lib is still using npm 6, as it upgrades the entire package-lock.json to the v2 lockfile format.

Could these npm changes be divorced from the rest of the shadow dom implementation? That might make it easier for upstream to accept the PR.

apellerano-pw avatar Jul 13 '21 21:07 apellerano-pw

@lolmaus any luck we can push this forward? Do you need any help with it?

herzzanu avatar Feb 13 '23 20:02 herzzanu

@herzzanu That's a question to addon maintainers. 😬

lolmaus avatar Feb 14 '23 14:02 lolmaus

Thanks @lolmaus

@cibernox this one looks promising. How can we help to move it forward? 🙏

herzzanu avatar Feb 17 '23 22:02 herzzanu

I have a branch here based on the last version and inspired by the changes in this PR https://github.com/EmberExperts/ember-basic-dropdown/tree/shadow-dom

herzzanu avatar Mar 13 '23 22:03 herzzanu

I have a branch here based on the last version and inspired by the changes in this PR https://github.com/EmberExperts/ember-basic-dropdown/tree/shadow-dom

I was able to upgrade to this newer fork with no issues. We've been using the kaliber5 fork for years

apellerano-pw avatar Oct 09 '23 20:10 apellerano-pw

Oh I found an issue in the EmberExperts fork. Inside component:basic-dropdown-content, it still uses the old way of registering the triggerElement so that handleRootMouseDown expects it to exist at this.args.triggerElement. However, the newer version of ember-basic-dropdown no longer uses a component arg. Instead it looks up the trigger element just in time using a data attribute. (Which would need to be shadow DOM aware.)

I'll see if I can get an example commit tomorrow

apellerano-pw avatar Oct 10 '23 23:10 apellerano-pw

This commit on top of the EmberExperts fork addresses the issue when clicking the trigger to close the dropdown content.

I scanned for any other dangling document.bleh element lookups and found 2 more, made sure they are trying out owner.rootElement as well.

https://github.com/ProsperWorks/ember-basic-dropdown/commit/21d4c7dbe72b2b43e8ba4b999870a30e1ecd6fd6

apellerano-pw avatar Oct 11 '23 16:10 apellerano-pw

I would like to close/merge this PR... can sombody bring this PR up to date? Unfortunately there is not anymore mergable...

mkszepp avatar Feb 11 '24 07:02 mkszepp

I've made my fixes to the EmberExpert fork available here https://github.com/EmberExperts/ember-basic-dropdown/pull/1

Maybe @herzzanu can incorporate my changes and open a new PR here?

apellerano-pw avatar Feb 12 '24 15:02 apellerano-pw

Thanks for your contribution @apellerano-pw 🙏 I've merged that PR and created a PR here to incorporate the changes back into this repo. We need to fix the conflicts 🙈 and should be good to go.

herzzanu avatar Feb 14 '24 09:02 herzzanu

I see, the changes don't apply cleanly because the addon has been migrated to v2 format. I'm not enough of a git plumber to know the easy way to approach this. I'll try to re-apply our commits onto HEAD and open a PR

edit: see convo here for updates https://github.com/cibernox/ember-basic-dropdown/pull/789#issuecomment-1945159662 tl;dr: v8 is not a trivial port, but the diff applies cleanly to v7. we should consider a v7 branch and doing one more release on v7

apellerano-pw avatar Feb 14 '24 17:02 apellerano-pw

closed by #791

mkszepp avatar Mar 03 '24 06:03 mkszepp