Skip to content

Conversation

@CGNonofr
Copy link
Contributor

@CGNonofr CGNonofr commented Aug 5, 2025

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

Copy link
Member

@bpasero bpasero left a 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
@CGNonofr
Copy link
Contributor Author

CGNonofr commented Aug 6, 2025

On a glance the direction seems legit, but the implementation is not working for me:

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?

  • we need to scope this to only apply for firefox (via isFirefox) and not always

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?)

  • 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

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?)

@bpasero
Copy link
Member

bpasero commented Aug 6, 2025

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.

@CGNonofr CGNonofr force-pushed the fix-firefox-theme-update-external-window branch from ea7c216 to 69bc77b Compare August 6, 2025 08:39
@CGNonofr
Copy link
Contributor Author

CGNonofr commented Aug 6, 2025

What about now?

@bpasero
Copy link
Member

bpasero commented Aug 6, 2025

@CGNonofr better, but I made some changes on top, though I cannot push them back to your branch. I created a new PR at #260037

@CGNonofr CGNonofr force-pushed the fix-firefox-theme-update-external-window branch from 69bc77b to d4cf2f4 Compare August 6, 2025 12:16
@CGNonofr
Copy link
Contributor Author

CGNonofr commented Aug 6, 2025

@CGNonofr better, but I made some changes on top, though I cannot push them back to your branch. I created a new PR at #260037

Sorry, forgot about the UUID tools in VSCode. I've replaced my commit with yours. Feel free to merge either one or the other

Copy link
Member

@bpasero bpasero left a 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

@bpasero bpasero added this to the August 2025 milestone Aug 6, 2025
@CGNonofr
Copy link
Contributor Author

CGNonofr commented Aug 6, 2025

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

@CGNonofr
Copy link
Contributor Author

CGNonofr commented Aug 6, 2025

Oh thanks, nice feature :)

@bpasero bpasero enabled auto-merge (squash) August 6, 2025 14:34
@bpasero bpasero merged commit 5a03ce4 into microsoft:main Aug 6, 2025
17 checks passed
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Sep 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants