Skip to content
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

firefox, thunderbird, librewolf: Enable wayland support by default #201359

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Nov 15, 2022

Please read the commit message for details.

TL;DR: Always set MOZ_ENABLE_WAYLAND=1 to prevent XWayland usage on Wayland.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Fixes #163206

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/firefox-all-black-when-first-launched-after-login/21143/15

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Nov 15, 2022
@bjornfor
Copy link
Contributor

So using MOZ_ENABLE_WAYLAND=1 also on X11 is fine?

Does anyone know why firefox doesn't enable this by default itself?

I've run with MOZ_ENABLE_WAYLAND=1 for a while on Wayland, and it does fix the occasionally black screen / crash on startup, so +1 from me.

vcunat
vcunat previously requested changes Nov 15, 2022
pkgs/applications/networking/browsers/firefox/wrapper.nix Outdated Show resolved Hide resolved
@Tungsten842
Copy link
Member

So using MOZ_ENABLE_WAYLAND=1 also on X11 is fine?

Does anyone know why firefox doesn't enable this by default itself?

I've run with MOZ_ENABLE_WAYLAND=1 for a while on Wayland, and it does fix the occasionally black screen / crash on startup, so +1 from me.

Some bugs supposedly:
https://bugzilla.mozilla.org/show_bug.cgi?id=1752398

@vcunat
Copy link
Member

vcunat commented Nov 15, 2022

Funnily, there are both kinds in there – bugs in running on Xwayland ("Blocks") and bugs in running on Wayland ("Depends on").

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation labels Nov 16, 2022
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

On the one hand, firefox-wayland is superior to the experience on xwayland and I've been using it because of that but OTOH Mozilla explicitly decided against enabling wayland by default and it's kind of their decision whether they deem their wayland support to be production-ready.

What Fedora did here was to enable wayland by default in firefox but offer a firefox-x11. I think we should do the same.

@lovesegfault
Copy link
Member

Ah, right, this might have licensing implications since we'd be distributing a branded non-standard build.

Sigh.

@rhendric
Copy link
Member

All this does is set an environment variable in the wrapper script we use to invoke Firefox; does that really constitute changing the build for purposes of licensing?

@mweinelt
Copy link
Member Author

I don't think this affects licensing unless we severely degrade the UX.

@Atemu
Copy link
Member

Atemu commented Nov 16, 2022

Well, the thing is, we could severely degrade UX by enabling wayland. You and I are comfortable with that but some of our users wouldn't even know that the terrible bug they just experienced could have been caused by wayland being enabled or that that's even the case.

I don't think Mozilla will be coming after us in any capacity if we did a minor thing like this (even if they'd rather we didn't) but, as a packager, it's a general courtesy to follow upstream projects explicit UX decisions wherever possible.

Fedora stoke a nice balance I think but, from Mozilla's perspective, they'd probably rather have us keep it the way it was (convenient yet explicit wayland opt-in).

@piegamesde
Copy link
Member

I was baffled to find out that I'd been using Firefox with XWayland all the time. As of 2022, I'd expect applications to use Wayland by default if they support it. Since this is just an environment variable and not a compile flag, and that setting it does not affect X11 support, I am not concerned about licensing stuff.

However, I think we should still provide a way to opt-out of Wayland, i.e. to force the usage of XWayland if desired. Since we are setting the environment variable in the wrapper, setting MOZ_ENABLE_WAYLAND=0 won't work. Having a firefox-x11 package as discussed above is a viable option.

An alternative would be to set this environment variable globally, for example as part of a programs.firefox.wayland option that defaults to true. The obvious disadvantage is that this would then only apply to NixOS systems.

@squalus
Copy link
Member

squalus commented Nov 16, 2022

An alternative would be to set this environment variable globally, for example as part of a programs.firefox.wayland option that defaults to true. The obvious disadvantage is that this would then only apply to NixOS systems.

A nixpkgs config option would work, and apply to non-NixOS systems.

@ajs124
Copy link
Member

ajs124 commented Nov 16, 2022

Since we are setting the environment variable in the wrapper, setting MOZ_ENABLE_WAYLAND=0 won't work

if we use --set-default instead of --set, this won't be an issue

@jtojnar
Copy link
Member

jtojnar commented Nov 16, 2022

I was baffled to find out that I'd been using Firefox with XWayland all the time. As of 2022, I'd expect applications to use Wayland by default if they support it.

Then that should be taken up with Mozilla.

Since this is just an environment variable and not a compile flag, and that setting it does not affect X11 support, I am not concerned about licensing stuff.

The method does not really matter. GTK_USE_PORTAL was also an environment variable and it lead to a massive slowdown of some users’ boot that was difficult to pinpoint. The issue is that we would be potentially making a non-trivial change in behaviour.

And since Mozilla only allows us to use the Firefox trademark as long as our changes do not break their product, it is our responsibility to make sure that any changes are in fact non-breaking, if we want to keep using the Firefox branding.

At minimum, we should probably find out why exactly it is not enabled by default. But final decision whether the change would be fine with Mozilla can only be done by Nixpkgs Firefox maintainers, hoping that their mental model of Mozilla is good enough.

@mweinelt mweinelt changed the title firefox, thunderbird, librewolf: Enable wayland unconditionally firefox, thunderbird, librewolf: Enable wayland support by default Nov 16, 2022
@zhaofengli

This comment was marked as resolved.

@colemickens
Copy link
Member

(fwiw, I generally agree with @jtojnar)

@mweinelt
Copy link
Member Author

mweinelt commented Nov 16, 2022

The basic change here is that nobody will use XWayland by default anymore. Nobody will be able to pick the wrong firefox package anymore. XWayland is most certainly the variant that gets the least maintenance at this point. Accessibility features like the onscreen keyboard don't seem to work under XWayland.

I think keeping a firefox-x11 variant around is pretty much pointless, when Firefox with Wayland enabled will gracefully fallback to X11 when the session is based on X11. There is also the possibility to force the usage of X11 by overriding MOZ_ENABLE_WAYLAND as outlined by @ajs124.

We're not the first distro to enable Wayland by default, Ubuntu went first.

I do not expect any licensing issue with this.

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

I believe this PR is good as it is now. The variable is there.

I've also been using this PR on pure X11 since the moment it was open. The wayland variant has been heavily tested by other nixers for a long time, so it's not anything new.

The variable override sounds sufficient at this point, as so far there's no evidence that it would be commonly needed and it works across all the packages using the wrapper.

Enabling Wayland support by default prevents use of XWayland on Wayland
systems, while correctly falling back to X11 when Wayland is
unavailable in the current session.

With the current packaging many people unnecessarily rely on the
`firefox` attribute, which is suggested by nixos-generate-config, which
in turn makes their Firefox use XWayland, when it shouldn't, which
causes bugs with GNOME on Wayland:

https://discourse.nixos.org/t/firefox-all-black-when-first-launched-after-login/21143

Using the Wayland-enabled Firefox was tested on pure X11 systems by
contributors on the #nix-mozilla:nixos.org room and we are confident
this change will not cause severe regressions.

Even better, people can now toggle `MOZ_ENABLE_WAYLAND=<0|1>` in their
environment to override this decision, should they feel the need to do
so.
pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

Have been using this for years, works great.

@vcunat vcunat merged commit 8ab030e into NixOS:master Nov 18, 2022
@mweinelt mweinelt deleted the firefox-always-wayland branch November 18, 2022 11:58
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/16

r-vdp added a commit to r-vdp/nixos-config that referenced this pull request Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox - consider enabling Wayland by default