Skip to content

Fix theme not being synchronized with external windows on firefox - #259839

Merged
Benjamin Pasero (bpasero) merged 1 commit into
microsoft:mainfrom
CodinGame:fix-firefox-theme-update-external-window
Aug 6, 2025
Merged

Fix theme not being synchronized with external windows on firefox#259839
Benjamin Pasero (bpasero) merged 1 commit into
microsoft:mainfrom
CodinGame:fix-firefox-theme-update-external-window

Conversation

@CGNonofr

Copy link
Copy Markdown
Contributor

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

@bpasero Benjamin Pasero (bpasero) left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

Benjamin Pasero (bpasero) commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

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
Loïc Mangeonjean (CGNonofr) force-pushed the fix-firefox-theme-update-external-window branch from ea7c216 to 69bc77b Compare August 6, 2025 08:39
@CGNonofr

Copy link
Copy Markdown
Contributor Author

What about now?

@bpasero

Copy link
Copy Markdown
Contributor

Loïc Mangeonjean (@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
Loïc Mangeonjean (CGNonofr) force-pushed the fix-firefox-theme-update-external-window branch from 69bc77b to d4cf2f4 Compare August 6, 2025 12:16
@CGNonofr

Copy link
Copy Markdown
Contributor Author

Loïc Mangeonjean (@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

@bpasero Benjamin Pasero (bpasero) left a comment

Copy link
Copy Markdown
Contributor

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 Benjamin Pasero (bpasero) added this to the August 2025 milestone Aug 6, 2025
@CGNonofr

Copy link
Copy Markdown
Contributor Author

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

@bpasero

Copy link
Copy Markdown
Contributor

@CGNonofr

Copy link
Copy Markdown
Contributor Author

Oh thanks, nice feature :)

Comment thread src/vs/workbench/services/themes/browser/workbenchThemeService.ts
@bpasero
Benjamin Pasero (bpasero) enabled auto-merge (squash) August 6, 2025 14:34
@bpasero
Benjamin Pasero (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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Firefox: changes to global stylesheets are not applied live

4 participants