-
Notifications
You must be signed in to change notification settings - Fork 56
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
Iframe Removal Monitoring not Working with Custom Elements #62
Comments
This patch fixes Aaronius#62.
Thanks for finding and reporting this @lpellegr! I saw your two pull requests and I'll dig deeper into them ASAP. Regarding being able to configure iframe removal monitoring, could you explain the use case for that? Thanks again. |
Thanks for your quick reply. I have 2 use cases: the first is about disabling iframe removal monitoring completely since I destroy connections properly and the window that loads the app does not aim to remain open a long time. The second is about using Penpal in a third-party widget. In that case, I prefer to keep the feature enabled but a check every hour is more than enough. |
This patch fixes Aaronius#62.
@lpellegr Is the current monitoring causing issues, are you worried about performance, or something else? It's a very inexpensive check, so I don't see performance being a concern. |
Hey, I haven't tested this yet and I'm having a bit of difficulty finding understanding this from documentation, but you may know the answer. If the iframe is added to the web component but the web component is not added to the window's document, will |
@Aaronius The answer to your question is https://jsfiddle.net/lpellegr/qxupLb0y/87/ Regarding the iframe removal monitoring option it's more about having control over what happens. I agree with you the performance hit for such a check made every minute is not critical but when embedded inside third-party pages by other developers, it becomes very hard to argue and you may lose trust when it is noticed you have code running periodically that is not required or could run much less often. Besides, in the case of an issue with Web components like pointed or whatever else involving iframe removal monitoring, you can simply disable the feature as a temporary workaround. The PR does not change the default behavior and the changes are minimal. Although I think that could benefit other users of the library, I understand if that's something you prefer not to add. In that case, I will use a fork. |
Thanks for setting up that test and explaining your reasoning @lpellegr. Thanks for the PRs as well. I'm going decline the monitoring configuration one until I see a more compelling reason to make it configurable. I'll do my best to get the other two merged and released tonight. Thanks! |
The |
Let's say you have a single-page Web app that makes use of web components (for instance a lit-element Web app). The page is rendered through a custom element that has an iframe in its Shadow DOM. This custom element interacts with this iframe using Penpal.
In this scenario, the connection between the parent and child is automatically destroyed after 60 seconds.
After investigation, I identified the problem is caused by a condition that assumes the iframe is not hosted within a custom element (see line 17 below):
penpal/src/parent/monitorIframeRemoval.ts
Lines 14 to 26 in da8df79
On line 17,
document
always references the window document. However, with the scenario described the iframe is hosted in the Shadow DOM of the custom element and not the window document. As a consequence, as soon as the monitoring code triggers, the condition!document.contains(iframe)
evaluates totrue
and the connection is destroyed.A simple solution that I tested with success locally is to replace
!document.contains(iframe)
by!iframe.isConnected
. The property isConnected considers the right context object depending on whether the iframe is in normal DOM or shadow DOM. It seems to also fit the browsers supported by the library (i.e. only IE11 has no support for this property).Would you accept a PR to fix the issue with the proposed solution?
More generally, would you accept another PR that allows disabling iframe removal monitoring?
The text was updated successfully, but these errors were encountered: