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

Fix for Shadow DOM #470

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix for Shadow DOM #470

wants to merge 4 commits into from

Conversation

txsmith
Copy link

@txsmith txsmith commented Feb 17, 2017

This PR makes dragula work inside Shadow DOM.

I've implemented @kevinpschaaf suggestion to use composedPath() if an event target has a shadowRoot, but I'm not sure if this is the best way since composedPath() seems quite experimental (See #418).

The tests that touch the DOM are modified to run against document.body as well as an element inside multiple shadow roots.

I've also put together a wrapper element using Polymer with several demos that run successfully with shadowDom enabled.

Suggestions are very welcome!

Fixes #418

@ergo
Copy link

ergo commented Feb 27, 2017

@txsmith Awesome stuff - I was working on this element (https://www.webcomponents.org/element/ergo/polymer-dragula) but I also encountered issues with shadow dom :-) I can try the wrapper with updated version.

@davidmaxwaterman
Copy link

davidmaxwaterman commented Mar 2, 2017

I've tried this commit and I don't think it is quite enough. I could be wrong, but I'd like to see it work - is there a demo of it working?
When I try it, I find that I need to duplicate the dragged element's style into the body's style, else it isn't even in the correct position, let alone styled correctly. The style needs adjustment slightly too - the copy seems to have its dimensions set to include the padding, so that needs removing so it remains the same overall size; but that makes its children too close to the edge of it, meaning they need some margin adding to compensate. It all gets a bit messy.
I am also now trying to find a solution to the fact that, once the item is positioned correctly, it seems to prevent it from being dropped (it works when it isn't positioned under the cursor).

ref: ergo/polymer-dragula#7

@davidmaxwaterman
Copy link

OK, I had to add '.gu-hide { display: none !important }' into a style in the document's head too...then it seems to work (it seems dragula applies that class to hide the element being dragged when searching down the tree to find the element under the cursor).
I'm not sure if any of this needs to be added into dragula, or perhaps it should simply be in polymer-dragula (it could add the appropriate classes into a style in the document.head for the user). Perhaps I'll work on that and submit a PR.

@txsmith
Copy link
Author

txsmith commented Mar 7, 2017

David, did you try the demo's in here? These should work on Chrome but I've not made an effort to make the element compatible with Firefox or any other browser yet.

If you did try my demo, then the styling problems you have appear to be caused by a missing import for a style module. Make sure you have <style include="dragula-styles"> and the corresponding html import for dragula-styles.html. You might have to add this style include in multiple places. Check the demo source code to see where I placed them.

I tried my patch of dragula on ergo/polymer-dragula but ran into some problems because subContainerSelector breaks under Shadow DOM.

@davidmaxwaterman
Copy link

@txsmith I somehow hadn't seen that element so, no, but it seems to be doing essentially the same as what I concluded was necessary (albeit including all the styles in the root document rather than just the ones that are strictly necessary, iinm).

However, those demos don't use dom-repeat, which was where I needed to make most changes to polymer-dragula. It'd be awesome if you could have a demo of one of those, since dom-repeat is surely an extremely common use-case.

Anyway, I have a working solution, even if it is a hacked together one, so I'm happy for the time being. If dragula-element could be shown to work with dom-repeat, and this PR be merged, I'll switch to all released versions :)

It should be noted that I didn't have to make any changes to this patch, so I'd say it is working just fine. Any chance this PR can be merged?

[1] https://github.com/txsmith/dragula-element/tree/master/demo

@davidmaxwaterman
Copy link

@bevacqua any chance this PR can be merged? It'd be a step towards us only having to use released versions.

@txsmith
Copy link
Author

txsmith commented Mar 7, 2017

Thanks for the confimation! I'll add a dom-repeat demo later today.

@davidmaxwaterman
Copy link

@txsmith 's dragula-element works perfectly in my app.

It'd be really awesome to get this PR merged (looking at @bevacqua 's activity, it looks like he might be on holiday or sick).

@ergo
Copy link

ergo commented Apr 27, 2017

Hello, are there any news on this?
It would be awesome if we could get the changes merged back in.

@ergo
Copy link

ergo commented Jun 8, 2017

@bevacqua any news on getting this merged?

@ergo
Copy link

ergo commented Jul 31, 2017

@txsmith any news on this?

@hariprasadiit
Copy link

any update on this? waiting for this to be merged to use it with polymer 2

@rjcorwin
Copy link

rjcorwin commented Feb 8, 2018

Also waiting for this to be merged :)

@txsmith
Copy link
Author

txsmith commented Feb 26, 2018

First of all: I'm sorry it took me so long to get back to this.

I've implemented the suggestion from @ergo to flip the or for getParent. As far as I'm aware this is guaranteed to work since anchors always have the parentNode property defined. The only way this could break would be if an <a> is the top-level element on a page, which can never happen as anchors are only allowed inside a <body>.

@ergo
Copy link

ergo commented Feb 26, 2018

Than you so much, maybe we can get it merged then :-)

@ergo
Copy link

ergo commented Mar 10, 2018

@bevacqua @lange Any chance we can get new release with this?

@ergo
Copy link

ergo commented Mar 21, 2018

Hello @bevacqua, any chance this can be merged in?

@dthorne
Copy link

dthorne commented Apr 17, 2018

We are looking for this as well. @bevacqua I'd like to help get this up to snuff but I don't see any outstanding complaints with it.

@noncototient
Copy link

Would be nice to finalise this and finally merge it 🙌

@ergo
Copy link

ergo commented Aug 29, 2018

@bevacqua any news on this, does this package currently have a maintainer?

@caridy
Copy link

caridy commented Jan 10, 2019

PR seems to be good enough, we did some extensibly review of the code for Lightning Web Components, and it seems to be correct. We also validated with our own implementation of synthetic shadow, and it works too. Can we get this merge?

@ScottMGerstl
Copy link

Is this good to go? When do you anticipate to merge and publish this?

@ergo
Copy link

ergo commented Apr 26, 2019

I'm not sure dragula is maintained anymore.

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.

Shadow DOM Support
9 participants