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

_belongsTo error due to element being null #274

Closed
davidmaxwaterman opened this issue Jun 2, 2015 · 8 comments · Fixed by #275
Closed

_belongsTo error due to element being null #274

davidmaxwaterman opened this issue Jun 2, 2015 · 8 comments · Fixed by #275

Comments

@davidmaxwaterman
Copy link
Contributor

Hi,

I'm developing some features on an app, and I have them working on Chrome, but now I am testing on Firefox I am getting an error.

The issue seems to be in the _belongsTo() function which is recursive. After several iterations, it calls itself with the element parameter as null - ie element.parentNode is null.

I've not figured out what is wrong, but I am suspecting it is something to do with the webcomponents implementation of shadow-dom - or lack thereof - that is certainly one significant difference between chrome and firefox, and since thie _belongsTo() function seems to be ascending the DOM tree, then perhaps it is related.

It'd be great to get some opinion of how this problem can be worked-around.

In the meantime, I'll see if I can reduce my code into a simple test case.

Thanks,

Max.

webcomponentsjs#0.5.2
mousetrap#1.5.2
polymer#0.5.5

stack trace :

console.trace():
debugger eval code:1
_belongsTo() mousetrap.js:421
_belongsTo() mousetrap.js:421
_belongsTo() mousetrap.js:421
_belongsTo() mousetrap.js:421
_belongsTo() mousetrap.js:421
_belongsTo() mousetrap.js:421
Mousetrap.prototype.stopCallback() mousetrap.js:970
_fireCallback() mousetrap.js:601
Mousetrap/self._handleKey() mousetrap.js:666
Mousetrap.prototype.handleKey() mousetrap.js:983
_handleKeyEvent() mousetrap.js:726
invoke() webcomponents.js:1070
dispatchBubbling() webcomponents.js:1024
dispatchEvent() webcomponents.js:998
dispatchOriginalEvent() webcomponents.js:953

@davidmaxwaterman
Copy link
Contributor Author

It's a little odd...looking into it some more, it seems to ascend the DOM tree until it gets to the document, but the test :

        if (element === document) {

fails.

If I test element, I get :

Object { __impl4cf1e782hg__: HTMLDocument → app, parentNode_: undefined, firstChild_: undefined, lastChild_: undefined, nextSibling_: undefined, previousSibling_: undefined, treeScope_: Object, __observer: Object, stagingDocument_: Object, _pgListeners: 1, 2 more… }

and document is :

HTMLDocument → http://localhost:8282/app

They both seem to be 'document' in some form, but :

> element === document
false

I am concluding that shadow-dom isn't an issue in this problem.
Any ideas?

Max.

@davidmaxwaterman
Copy link
Contributor Author

FWIW, this seems to fix the problem :

413        if (element.parentNode === null) {

Do you see any problem with making that change?

@ccampbell
Copy link
Owner

Yeah you are totally right... there needs to be a check at the top of _belongsTo

if (element === null) {
    return false;
}

Feel free to send a pull request or I will try to get this out later tonight

davidmaxwaterman pushed a commit to davidmaxwaterman/mousetrap that referenced this issue Jun 2, 2015
Return false if element is null to handle webcomponentsjs polyfill.
davidmaxwaterman pushed a commit to davidmaxwaterman/mousetrap that referenced this issue Jun 2, 2015
@davidmaxwaterman
Copy link
Contributor Author

I hope that PR is as you intended...I checked against document too, since that'll happen one iteration sooner, iinm.

@davidmaxwaterman
Copy link
Contributor Author

Any hope of getting this fix in and released soon? I have a PR on my own s/w that is waiting on it, so it'd be great to get #275 merged.

@davidmaxwaterman
Copy link
Contributor Author

FYI, we'll be switching to using my fork so we can continue to move forward (and I don't keep having to rebase my branch) - the power of open source :)

@ccampbell
Copy link
Owner

Okay. Yes, I will try to get it released within the next couple days, but good that you are not held up on it.

@devinivy
Copy link

devinivy commented Jul 2, 2015

This is affecting us too! Would love to see #275 merged– it looks good.

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 a pull request may close this issue.

3 participants