Allow transitions and animations to work within iframes #3625
Conversation
Svelte works great in iframes except for transitions and animations. This fixes that issue. See #3624.
| type DocStyles = [CSSStyleSheet, Record<string, true>]; | ||
|
|
||
| let activeDocs = new Set<Document>(); | ||
| let docStyles = new WeakMap<Document, DocStyles>(); |
There was a problem hiding this comment.
We're not using WeakMap anywhere else currently, and I think we probably want that to continue being the case since it's basically impossible to polyfill in environments that don't support it. Reckon we could get away with this being a Map instead? In the vast majority of cases we're only going to have a single key, and it's always likely to be stable, so maybe it doesn't matter if we hang on to references?
| doc.head.appendChild(style); | ||
| stylesheet = style.sheet as CSSStyleSheet; | ||
| docStyles.set(doc, [ stylesheet, current_rules ]); | ||
| } |
There was a problem hiding this comment.
This is obnoxious code-golfing, but if you wanted to save 18 bytes (after minification) you could do this...
const doc = node.ownerDocument;
activeDocs.add(doc);
const [stylesheet, current_rules] = (docStyles.has(doc) ? docStyles : docStyles.set(doc, [
(doc.head.appendChild(element('style') as HTMLStyleElement).sheet as CSSStyleSheet),
{}
])).get(doc) as DocStyles;Can we change docStyles to doc_styles?
| @@ -53,14 +59,19 @@ export function delete_rule(node: Element & ElementCSSInlineStyle, name?: string | |||
| ) | |||
| .join(', '); | |||
|
|
|||
| if (name && !--active) clear_rules(); | |||
| active = Math.max(0, active - 1); | |||
There was a problem hiding this comment.
This feels like it's masking a bug — is the behaviour different when iframes are involved, or is this unrelated to iframes?
There was a problem hiding this comment.
Yes, it is masking a bug. Toggling a transition on and off multiple times has a memory leak.
| while (i--) stylesheet.deleteRule(i); | ||
| docStyles.set(doc, [ stylesheet, {} ]); | ||
| }); | ||
| activeDocs.clear(); |
There was a problem hiding this comment.
oh! we can definitely use a Map instead of a WeakMap if we're clearing activeDocs (can we change it to active_docs?)
| @@ -53,14 +55,19 @@ export function delete_rule(node: Element & ElementCSSInlineStyle, name?: string | |||
| ) | |||
| .join(', '); | |||
|
|
|||
| if (name && !--active) clear_rules(); | |||
| active = Math.max(0, active - 1); | |||
| if (name && !active) clear_rules(); | |||
There was a problem hiding this comment.
I've been working off of this branch (since my Svelte app needs iframe support) and noticed that this change causes a previously working transition of mine to get cut short mid-transition.
It's more noticeable with a longer transition duration. For example, with duration=5s, the first 2s or so of the transition work as expected, but then it jumps to the final position suddenly. This is the case both in and outside of an iframe.
My use case is pretty simple, just: <div transition:slide={...}>. Reverting just the above 3 lines fixes the issue for me, although then the leak is still present of course.
I will add a ticket to document the leak.
`doc_styles` was a leak as it was keeping old iframe docs in memory. By adding the stylesheet info on the document object it can be cleaned up (poor man's weakmap). `delete_rule` can potentially be called many times and if called without the `name` value it may remove all the rules but not track it correctly. The logic is updated to clear rules when there are no active transitions left.
|
I've updated the PR to latest Svelte, fixed memory leaks by solidifying the logic in @jtormey I would love it you could test this in your situation. If it cuts transitions short, it is because of a bug in Ready for another review @Rich-Harris. :D |
|
Just retested with your latest changes, the transition no longer gets cut short! 🎉 |
|
Thanks @Conduitry! 🎉 |
Svelte works great in iframes except for transitions and animations. This fixes that issue.
Fixes #3624.