-
Notifications
You must be signed in to change notification settings - Fork 37.2k
Fix theme not being synchronized with external windows on firefox #259839
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
Fix theme not being synchronized with external windows on firefox #259839
Conversation
bpasero
left a comment
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.
On a glance the direction seems legit, but the implementation is not working for me:
- we need to scope this to only apply for firefox (via
isFirefox) and not always - computing the SHA seems overkill, I would do a pragmatic approach and set some UUID or even easier a counter that just increments, this may produce false positives but I think is fair enough for Firefox
by not working for you you mean that you tried it and it didn't work, or are you just unhappy with the way it's implemented?
I was mainly for the sake of simplicity, and also because I was anticipating it may also impact some other browsers I'm unable to test (like safari?)
I had multiple ideas on how to implement it 🤔 I've started with using a simple counter, an UUID, and at the end I was thinking using a hash was the most elegant way to implement it (Also I'm not sure how to name the attribute if using an UUID 🤔) I'm aware of the additional computation required, but computing a hash is pretty light, and also it's only done when switching the theme so... Btw it's really not a big deal, I just want that issue fixed and wanted to help do it quickly by suggesting a way to implement it, feel free to refactor it the way you want. I can also do it with clean indications (what attribute name for the UUID? Are you sure to scope it for Firefox only?) |
|
It works fine for me in all chrome based browsers and safari, so I think its an isolated issue with firefox only. I can do a fix as well but would probably start fresh then. I am having trouble actually verifying that your fix works running out of sources because somehow NO CSS loads for me in this case. I need to create a build with the fix to verify it. |
ea7c216 to
69bc77b
Compare
|
What about now? |
69bc77b to
d4cf2f4
Compare
bpasero
left a comment
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.
Thank you, this is not a trivial contribution, great work. I confirmed it seems fixed with the change on insiders.vscode.dev
I've just tried and it doesn't seem to be deployed 🤔 is there some kind of gradual rollout? I'm still on commit c661d0e |
|
Oh thanks, nice feature :) |
fix #259835
Firefox doesn't allow to watch style element content using a MutationObserver. A MutationObserver is used to synchronize main windows styles with external windows. As a consequence, the theme is not properly updated on external windows on Firefox.
As a workaround, also add a data-hash attribute on the style elements, and watch it as well in the MutationObserver