Popout! compatibilty (Fixes #3044) #3048
Conversation
|
Also I am not particularly attached to the current implementation of the fix. I know there is a fair amount of code duplication, and one might want to implement the CSS caching as free functions or as a mixin class and attach them to the respective custom elements. But I wanted to keep the changes minimal to show what was necessary for the fix and nothing more. |
Posnet
changed the title
Posnet
force-pushed
the
Posnet/popout-3.x.x-fix
branch
from
2c8967b to
fc156cd
Compare
For custom elements that use constructed CSS, we have to implement the adoptedCallback method and manually reconstruct and reattach a new CSS object to the shadow DOM. This is because when a custom element is moved to a new document, any constructed CSS objects that it has adopted already are silently stripped. We also need to update how we construct our CSS objects. Specifically we need to ensure we are accessing the current owner document of our custom element, and not bind the root document from which ever window we defined our custom element in. We also can no longer use a static variable to store our CSS obj, since in Chrome based browsers the static element is shared across windows (though not the case in firefox). So we need to keep a map from document to CSS obj singleton and do the lookup manually.
Posnet
force-pushed
the
Posnet/popout-3.x.x-fix
branch
from
fc156cd to
f9a038b
Compare
|
You're getting there! Thank you for working this. Still seems to have problems with some of the objects scaling/Sizing? In the effects tab and the Death Save Skull are examples. |
|
Thanks for doing this work, I think it's worthwhile to make our custom elements agnostic of their owning Document if at all possible. I've factored out your suggested changes into a mixin to hopefully keep all the logic in one place. Even with your original changes though, I wasn't able to get this working with PopOut! in Firefox. Maybe I need a pre-release version of PopOut! to test? I'm unsure. The errors seemed to be related to |
| fill: var(--filigree-border-color, var(--dnd5e-color-gold)); | ||
| z-index: -1; | ||
| /** @inheritDoc */ | ||
| _buildCSS(sheet) { |
There was a problem hiding this comment.
I think this could be a simple static CSS = , but I was trying to avoid using statics at all due to this note:
We also can no longer use a static variable to store our CSS obj, since in Chrome based browsers the static element is shared across windows (though not the case in firefox). So we need to keep a map from document to CSS obj singleton and do the lookup manually.
Even avoiding it though, it seems it was not working in Firefox. If we can get to the bottom of what's happening there, we can revisit this design hopefully.
|
@Fyorl I'll have a go at making a fix that uses that uses inline styles if you prefer, which would be simpler than the current fix. The reason this this fix doesn't work on Firefox is due to browser implementation differences, that being moving custom elements (and even some normal elements) is essentially broken, the element in the new window will be missing all of it's non static methods and member variables. I stopped testing or supporting Firefox with PopOut! because of this and because there are parts of jQuery that have the same issue. So if a system or module is using custom html elements/web components or is using jQuery extensively it will never work on Firefox with PopOut! Also because Foundry uses Electron so generally is better tested on blink based browsers. I actually prefer Firefox for normal web browsing, but have to use chrome with Foundry. |
|
I have updated my gist showing the broken behavior https://gist.github.com/Posnet/9d87f790d4f3c64ed468559600c76302 |
Now you've clarified this part below, I think it's fine to use what we have for now. I'll make some additional tweaks to use the static CSS, which should simplify things further, and then we can have the performance benefits of adopted stylesheets for whatever they're worth at least.
I didn't realise it was non-static methods. It sounds like Firefox is a dead-end then here. Do we know if this is a bug or some security sandboxing on Firefox's part such that this will never work? |
Looks like a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1502814 |
For details of the actual buggy behavior see: League-of-Foundry-Developers/fvtt-module-popout#109
TL;DR - Moving a custom (or otherwise) HTML element to a new window involves adopting it into the new windows document. However any use of constructed CSS (i.e. new CSSStyleSheet) will be silently stripped or fail outright unless we are careful to construct new copies of the CSSStyleSheet object via the new window's constructor.
For custom elements that use constructed CSS, we have to implement the adoptedCallback method and manually reconstruct and reattach a new CSS object to the shadow DOM. This is because when a custom element is moved to a new document, any constructed CSS objects that it has adopted already are silently stripped.
We also need to update how we construct our CSS objects. Specifically we need to ensure we are accessing the current owner document of our custom element, and not bind the root document from which ever window we defined our custom element in.
We also can no longer use a static variable to store our CSS obj, since in Chrome based browsers the static element is shared across windows (though not the case in firefox). So we need to keep a map from document to CSS obj singleton and do the lookup manually.