-
Notifications
You must be signed in to change notification settings - Fork 55
Fixes #38 #95
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
Fixes #38 #95
Conversation
…entById` to find referenced elements
src/auto-complete-element.ts
Outdated
| // eslint-disable-next-line custom-elements/no-dom-traversal-in-connectedcallback | ||
| const input = this.querySelector('input') | ||
| const results = document.getElementById(listId) | ||
| const results = this.getRootNode().getElementById(listId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break because getRootNode outside of the ShadowDOM returns the element. You'll need to do some more checks. Perhaps it's worth also falling back to light dom if we cannot find the id in the shadowdom?
| const results = this.getRootNode().getElementById(listId) | |
| const shadowRoot = this.getRootNode() | |
| let results | |
| if (shadowRoot instanceof ShadowRoot) { | |
| results = root.getElementById(listId) | |
| } | |
| if (!results) this.ownerDocument.getElementById(listId) |
I also think this is a good point to start adding tests that confirm the desired behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as element is connected getRootNode will return either a ShadowRoot or document. As we call it inside connectedCallback, it should be safe. I'll add this.isConnected check in the callback.
To squash the typescript error I'll cast rootNode to Document, as Document and ShadowRoot has no common ancestor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good clarification, thanks. What do you think about resolving outside of the shadowdom if the element isn't found within the shadowdom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think no. auto-complete being inside shadowDOM strongly implies it's being used in the internals of another custom element where encapsulation inside a shadowDOM is the whole point. Better to fail consistently than risk surprising behavior.
Adds .isconnected check in connectedCallback
Usage of
document.getElementByIdcauses autocomplete to fail to find needed elements ifauto-completeis created in javascript or is in the shadowDOM of another element.This fixes the issue by replacing usage of
document.getElementByIdwithquerySellector.