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

dstNode is undefined in function prepareNodeCopyAsDragImage #111

Closed
z2oh opened this issue Aug 30, 2017 · 7 comments
Closed

dstNode is undefined in function prepareNodeCopyAsDragImage #111

z2oh opened this issue Aug 30, 2017 · 7 comments

Comments

@z2oh
Copy link

z2oh commented Aug 30, 2017

In the function prepareNodeCopyAsDragImage I am running into an error that causes dragging and dropping to break. In an attempt to debug this issue I found that dstNode was sometimes undefined, which would break the script on

dstNode.style.setProperty(...);

Sometimes, dstNode was a text node (dstNode.nodeType === 3) which would also break on the above line, as the style object was undefined. I was able to 'fix' this by wrapping all the problem lines in

if(dstNode && dstNode.style) {
    ...
}

but I'm not sure if this just fixes a symptom of some other root cause.

I am using Polymer to polyfill webcomponents, which has made it difficult to locate exactly what HTML was causing this to break (and difficult to post steps to reproduce). I was able to narrow it down to the <paper-icon-button> element, which is mostly just a wrapper around an SVG. When I isolate the SVG, the dragging works fine without my hotfix so I am stumped as to what undefined node is causing this to break. Any ideas on how I can debug this further? I am happy to open a PR once I have a better understanding of the underlying issue.

@reppners
Copy link
Collaborator

Thanks for the detailed report and your effort in investigation. I don't have time right now to go deep but recommend investigating further on the result of https://github.com/timruffles/ios-html5-drag-drop-shim/blob/b485129b36fb875a072743df1a1d641b4183c34d/src/index.ts#L1303

Because in your case obviously the cloneNode() method does not return a result that is a matching clone I'd loop over srcNode and dstNode in one loop just printing the tree and highlighting the differences. This might indicate on which parts of the srcNode tree cloning did fail.

Maybe there is some bug or edge cases regarding the cloneNode() method that is rather undocumented and the polyfill trips over it.

@reppners reppners added the bug label Aug 30, 2017
@z2oh
Copy link
Author

z2oh commented Aug 31, 2017

Using your suggestion I isolated where cloneNode() failed.

I cloned the following node:

<paper-icon-button icon="close" ...>
    <iron-icon ...>
        <svg ...>...</svg>
    </iron-icon>
</paper-icon-button>

and got:

<paper-icon-button icon="close" ...>
    <iron-icon ...>
    </iron-icon>
</paper-icon-button>

The ... were extra attributes I omitted for brevity.

The issue here is clear, the svg node was not cloned. The svg was added automatically by the <paper-icon-button> web component. I tried pasting the dom manually in my source (replacing the elements with divs) and the clone worked successfully.

It looks like perhaps cloneNode() does not work well with shadow dom? If that's the case it is unfortunate but it seems like it is not something that can be easily fixed by this polyfill. The automatic drag image generation with the normal HTML5 drag and drop works fine, but I'm guessing their implementation does not utilize the cloneNode() function.

With my 'hotfix' above though, the svg appears in the drag image though... now that's got me confused. My 'fix' only fixes a symptom of an incomplete or incorrect cloneNode() function which doesn't clone the entire node, yet when I ignore the undefineds from cloneNode() the entire node appears to be there!

Any ideas? It seems like the actual bug (or just missing functionality) here is in the cloneNode() function, so feel free to close this issue if you agree. It is a shame though as I will probably have to end up using a fork of this library with that hotfix above.

@reppners
Copy link
Collaborator

reppners commented Sep 1, 2017

Thanks for posting your findings.

I'd rather push for a solution that works with webcomponents/shadowdom for everybody.

So based on more research I think we can find a solution that can be included into the library. Don't have any knowledge about cloning and shadow dom - maybe there's some documentation. Maybe its also the fault of a webcomponents polyfill? Are you using any?

Another approach could be to attach the node to the DOM with visibility: hidden or display:none so it initializes and only after that transfer the styles.

@reppners
Copy link
Collaborator

reppners commented Sep 3, 2017

I think the problem originates from ShadyDOM and cloning without using the ShadyDOM API. Here is a related issue Polymer/polymer#1888

@z2oh
Copy link
Author

z2oh commented Sep 5, 2017

It does seem related to that issue, although when replacing https://github.com/timruffles/ios-html5-drag-drop-shim/blob/0f2c6426d2618fadd1e9dfa4816fa0384b0291a0/release/index.js#L604 with

var dragImage = Polymer.dom(sourceNode).cloneNode(true);

I run into the same issue. When running through the chrome debugger I notice that all the ShadyDOM nodes are there. However, I stepped through the code and found this (notice the evaluated srcNode and dstNode):

textnode

It would seem the svg node is incorrectly cloned as a text node? Text nodes do not have a style property so an error occurs on line 590 in the screenshot above.

I will do some research to see if this issue still occurs when there are no SVGs in the ShadyDOM.

@z2oh
Copy link
Author

z2oh commented Sep 5, 2017

I tried nesting some components and making the parent draggable, and the drag image shows up just fine including all the child elements that are in ShadyDOM. It looks like this issue may be somehow isolated to SVGs in ShadyDOM specifically.

@reppners
Copy link
Collaborator

API additions to enable custom dragImageSetup released as of v2.3.0-rc.0. Closing this one as the polyfill can't yet implement a general purpose solution for ShadowDOM/ShadyDOM.

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

No branches or pull requests

2 participants