search

anseki / plain-draggable

The simple and high performance library to allow HTML/SVG element to be dragged.
https://anseki.github.io/plain-draggable/
MIT License
773 stars 99 forks source link

Plain Draggable incompatble with shadow dom #36

Closed NathanaelA closed 4 years ago

NathanaelA commented 5 years ago

If you attach the render tree of elements to a shadow dom that you are trying to use PD with; the following line fails: https://github.com/anseki/plain-draggable/blob/master/src/plain-draggable.js#L338 as apparently anything inside the shadow dom is considered disconnected:

Test code:

<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
</head>
<body>

<test-shadow></test-shadow>

</body>
<script>
    customElements.define('test-shadow',
        class extends HTMLElement {
            constructor() {
                super();
                this._container = document.createElement('div');
                this._shadowContainer = document.createElement('div');
                this.appendChild(this._container);
                let shadow = this.attachShadow({mode: 'open'});
                shadow.appendChild(this._shadowContainer);
            }

            connectedCallback() {
                console.log("Connected");
                console.log(this._container.compareDocumentPosition(document));
                console.log(this._shadowContainer.compareDocumentPosition(document));
           }
         }
    );
</script>

You will see the _container which is NOT attached to the shadow dom returns 10. You will see the _shadowContainer which is attached to the shadow dom return 35 or 37 depending on the browser...

Deleting this single check seems to allow it to fully work inside the shadow dom...

anseki commented 5 years ago

Hi @NathanaelA, thank you for the comment. Sorry, my English is poor. I couldn't understand what you want well. So, what is "PD"?

NathanaelA commented 5 years ago

Sorry, PD = Plain Draggable. :grinning:

Here is the issue; I was trying to put a class that uses Plain Draggable to do drag and drop with a group of elements. This worked great as a standalone project.

Once I put it inside a the WebComponent (customElements.define) and tried to use the ShadowDom ( this.attachShadow({mode: 'open'}); ) PlainDraggable failed with an error about a bad element.

This error was caused by this check: (element.compareDocumentPosition(document) & Node.DOCUMENT_POSITION_DISCONNECTED) is TRUE when the draggable items are inside the ShadowDOM. Your code expects this to be false statement.

It is called from here for the initial failure: https://github.com/anseki/plain-draggable/blob/master/src/plain-draggable.js#L1561 which calls the code above here: https://github.com/anseki/plain-draggable/blob/master/src/plain-draggable.js#L338

By removing this part of the check and making this function look like this:

function isElement(element) {
  return !!(element &&
    element.nodeType === Node.ELEMENT_NODE &&
    // element instanceof HTMLElement &&
    typeof element.getBoundingClientRect === 'function');
}

PluginDraggable will work inside a shadowDOM.

anseki commented 5 years ago

Ah... I heard the PD that means PlainDraggable for the first time. :laughing:

Maybe you misunderstood some points. The shadow DOM is used for encapsulation typically. That divides DOM tree. And, the Node.DOCUMENT_POSITION_DISCONNECTED means elements are not in the same tree. PlainDraggable requires the elements that are in same tree, otherwise it may not work. Therefore, that checking and that error are correct behavior.

You can use PlainDraggable in divided document (e.g. frame) for encapsulation.

anseki commented 5 years ago

For example:

    customElements.define('test-shadow',
        class extends HTMLElement {
            constructor() {
                super();
                this.appendChild(this.window = document.createElement('iframe'));
                this.document = this.window.contentDocument;
                this.targetElement = this.document.createElement('div');
                this.document.body.appendChild(this.targetElement);
            }

            connectedCallback() {
                console.log("Connected");
                console.log('this.targetElement.compareDocumentPosition(this.document): ' + this.targetElement.compareDocumentPosition(this.document));
                console.log('this.targetElement.compareDocumentPosition(document): ' + this.targetElement.compareDocumentPosition(document));
           }
         }
    );
NathanaelA commented 5 years ago

I understand the reason for the check. :grinning: And you are correct, the ShadowDom is technically a partially different tree than the document root object.

However, everything I am doing is 100% encapsulated inside the shadowdom. So all elements that are being used, dragged, created, are part of the exact same tree. So this check will break in this use case.

I have no issue deleting the check in my copy; which is what I'm doing right now. But I figured you might want to be aware of it -- as their are two solutions I can think of that would allow your check to work with only some minor changes to the code base.

  1. We could pass in a shadowDom flag and you use that to find the shadowroot element.
  2. Or we pass in the shadowdom element itself as a optional parameter.

Then instead of comparing document to the element you compare the shadowRoot element to the element. This allows you to detect if all the objects passed in are still part of the proper root element. If the flag/shadowroot isn't passed in; you use the document object as normal.

This code is fairly trivial.

function findCompareElementShadow
            // Find shadowRoot if it exists...
            let this._compareElement = document;
            let parent = this._parentElement;  // _parentElement is the very root
            let hasShadow = false;
            while (parent != null) {
                if (parent instanceof ShadowRoot) { hasShadow = true; break; }
                parent = parent.parentNode;
            }
           if (hasShadow) {  this._compareDocument = parent; }

Then you can use _compareDocument inside the isElement function and the check should still work properly in all use-cases. :grinning:

Btw, Using a iframe will complicate thing.... So I'd rather just delete the check in my copy of PluginDraggable as I already did, if this is not something that you want to change/fix. :grinning:

anseki commented 5 years ago

Thank you for the suggestion. However, that should not be implemented because PlainDraggable can work in shadow DOM by divided document as I said. I think the option is unnecessary. Even if you don't like that, many popular libraries (e.g. YouTube movie, Facebook Like button, Twitter Tweet button, etc.) are using <iframe> element to add a web component safely. PlainDraggable also work fine in shadow DOM by general technique. Generally speaking, a library should not consider about the outside of own world. That is, the document that runs the library should be prepared by you, not the library. The example I indicated may help you.

NathanaelA commented 5 years ago

Unfortunately using a iframe would be one of the worst solutions in my case. Unlike those examples that typically have a single simple iframe added. My dom for the component is fairly complex and dynamically generated. Since their are still major performance issues with iframe ( https://jsperf.com/iframe-performance-overhead ), and in my case this widget could potentially have dozens' of these components on the page. That would be unacceptable speed hit while dynamically generating the massive amounts of items attached to an iframe.

As I stated before, I have no issue removing the check in my copy of the library that I will distribute with the rest of the code for my library. :grinning: I just figured you might want to be aware of this issue and fix it with around 10 or so lines of code. If you want a PR -- I'd be willing to send you a PR which detects the shadowdom and uses it in the compare step so that you code is as safe as it was before -- and works inside the shadow dom properly. :grinning:

anseki commented 5 years ago

As everyone knows, the shadow DOM is awesome new technology but it is not perfect yet. Then, many popular libraries are using <iframe> element to add a web component safely for now.

Anyway, as I said, PlainDraggable requires the elements that are in same tree (i.e. current document). This means that PlainDraggable is not designed for shadow DOM. Therefore it checks that. This is of course correct behavior. Since your suggestion doesn't consider mechanism of the library at all, the removing the checking may break other many apps even if it is good for your app luckily. Please understand that PlainDraggable has to be changed more to support shadow DOM, not only the removing the checking. Your suggestion breaks some functions. That means that you open the gate by force before it is ready.

You'd better use another library rather than PlainDraggable that is unsuited for your app. Or, of course you can fork the library and use the special version for your app only. However, of course it is not supported, and I don't recommend that.

Thank you for the suggestion. :smile:

NathanaelA commented 5 years ago

As everyone knows, the shadow DOM is awesome new technology but it is not perfect yet.

@anseki - You seem to be a little misinformed on Shadow Dom; it has been around for many years, and is part of the formal html spec. They did make some changes from Shadow Dom v0 and Shadow Dom v1.0 -- but even the current v1.0 shipped 2 years ago in chrome and safari and a over year ago in Firefox. It also has a fully working polyfill for any browser that doesn't support it naively . Basically v1 has been supported in pretty much all major browsers for well over a year now on desktop and mobile. And v0 for something like 6 years now.

Then, many popular libraries are using