Skip to content

firefox: Enable userChrome in about:config#5670

Open
MrQubo wants to merge 1 commit intonix-community:masterfrom
MrQubo:firefox-userChrome-enable-in-settings
Open

firefox: Enable userChrome in about:config#5670
MrQubo wants to merge 1 commit intonix-community:masterfrom
MrQubo:firefox-userChrome-enable-in-settings

Conversation

@MrQubo
Copy link
Copy Markdown
Contributor

@MrQubo MrQubo commented Jul 25, 2024

Description

Enable "toolkit.legacyUserProfileCustomizations.stylesheets" in about:config if userChrome is not empty.
Resolves #5666

Change is not backwards compatible. It will break configs where userChrome is set to non-empty string and settings."toolkit.legacyUserProfileCustomizations.stylesheets" is set to false. It seems unlikely but I've added assertion nevertheless.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@rycee @kira-bruneau

@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from 5307a99 to b73f112 Compare July 25, 2024 04:06
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Jul 25, 2024

Here's a more friendly diff without indentation changes:

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Jul 25, 2024

I've tried running tests with nix-shell --pure tests -A run.all but I'm getting error error: executing shell '/nix/store/p5441mp6wwpdh65qamaixvd092cwr45h-bash-interactive-5.2p26/bin/bash': Argument list too long. :/

EDIT: nix develop --ignore-environment '.#firefox-profile-settings' worked.

@MrQubo MrQubo marked this pull request as draft July 25, 2024 04:18
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from b73f112 to fc80b16 Compare July 25, 2024 04:31
@MrQubo MrQubo marked this pull request as ready for review July 25, 2024 04:34
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from fc80b16 to e68817e Compare July 25, 2024 04:38
Comment thread modules/programs/firefox.nix Outdated
Copy link
Copy Markdown
Contributor

@IldenH IldenH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like this change as it is confusing having the stylesheets not work despite being set.

Comment thread tests/modules/programs/firefox/profile-settings.nix Outdated
Comment thread modules/programs/firefox.nix Outdated
Comment thread modules/programs/firefox.nix Outdated
Comment thread modules/programs/firefox.nix Outdated
Comment thread tests/modules/programs/firefox/profile-settings.nix Outdated
Comment thread tests/modules/programs/firefox/profile-settings.nix
Comment thread modules/programs/firefox.nix Outdated
Comment thread modules/programs/firefox.nix Outdated
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from e68817e to e286638 Compare July 29, 2024 18:05
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Jul 29, 2024

I hate github, I was replying you, but it was "pending" so I think you didn't see any of that. ;_;

@kira-bruneau
Copy link
Copy Markdown
Contributor

It looks like this needs to rebased since: #5128

@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from e286638 to 4205c1f Compare July 30, 2024 22:32
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Jul 30, 2024

Rebased, I've made some additional changes, could you review again? Completely unrelated change, but the containers test was bugging me.

id = 5;
containers = {
"shopping" = {
id = 6;
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.

Wait what was wrong with the containers test? Each container should have its own unique id set, so I think it makes sense to keep one explicitly set in the test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bit confusing, because it's container id, not profile id. It looks like someone set it to 6 because it's the next number after 5, but these ids are unrelated. I feel like there's no need to set container id here (apart from making this test failproof against changing the default container id), and it's not testing anything.

I could set it explicitly to 1, but using 6 here is a bit confusing, because it looks like it's related to this 5.

Comment on lines +829 to +830
{option}`${userChromeAttr}` won't work with
{option}`settings."toolkit.legacyUserProfileCustomizations.stylesheets"` set to false.
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.

I'm not sure if you saw my other comment on this since it was in a resolved thread, but {option}`x` shouldn't be used in assertions, because they don't get rendered like the docs do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I left the backticks characters.

Comment thread modules/programs/firefox/mkFirefoxModule.nix Outdated
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch 3 times, most recently from d1aee34 to e9e0e61 Compare August 7, 2024 00:27
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from e9e0e61 to 2682ad9 Compare August 8, 2024 14:26

"${profilesPath}/${profile.path}/user.js" = mkIf (profile.settings != { }
|| profile.extraConfig != "" || profile.bookmarks != [ ]) {
"${cfg.profilesPath}/${profile.path}/user.js" = mkIf (profile.settings
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it hurts readability how ./format broke the line before !=. :/

@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from 2682ad9 to 615a348 Compare August 8, 2024 14:32
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Aug 8, 2024

I (hopefully) fixed the tests on macos.

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Aug 8, 2024

Maybe it would be easier to fix the tests in #5698 and keep this PR self-contained.

@kira-bruneau
Copy link
Copy Markdown
Contributor

kira-bruneau commented Aug 8, 2024

Maybe it would be easier to fix the tests in #5698 and keep this PR self-contained.

Thanks @MrQubo! I'll push the fix to #5698 if it passes here.

@kira-bruneau
Copy link
Copy Markdown
Contributor

It looks like it's still failing here

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Aug 16, 2024

@kira-bruneau I can't understand why it's failing.

@stale
Copy link
Copy Markdown

stale bot commented Nov 17, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Nov 17, 2024
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from 615a348 to c4707bf Compare November 19, 2024 01:26
@stale stale bot removed the status: stale label Nov 19, 2024
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from c4707bf to 1cd9311 Compare November 19, 2024 01:35
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Nov 19, 2024

Seems like macos tests where fixed in master.

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Nov 19, 2024

Now ubuntu tests are broken in master. ;_;

Enable "toolkit.legacyUserProfileCustomizations.stylesheets" in
about:config if userChrome or userContent is not empty.
@MrQubo MrQubo force-pushed the firefox-userChrome-enable-in-settings branch from 1cd9311 to 008a165 Compare November 20, 2024 22:57
@MrQubo MrQubo requested review from kira-bruneau and rycee November 20, 2024 23:07
@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Nov 20, 2024

The code moved from default.nix to mkFirefoxModule.nix, but the changes are still the same.

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Nov 20, 2024

No idea what is the error now.
It says "NOTE: If your Manual build passes locally and you see this message in CI, you probably need a rebase", but I already rebased onto master.

@nyabinary
Copy link
Copy Markdown

No idea what is the error now. It says "NOTE: If your Manual build passes locally and you see this message in CI, you probably need a rebase", but I already rebased onto master.

Is this working at its current state, or are there still errors?

@MrQubo
Copy link
Copy Markdown
Contributor Author

MrQubo commented Feb 24, 2025

Tests on this PR need fixing, see: #6403

@stale
Copy link
Copy Markdown

stale bot commented Jun 18, 2025

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable userChrome in about:config if userChrome is used

7 participants