Closed Bug 1509931 Opened 6 years ago Closed 3 years ago

[Wayland][OpenGL] CSD has sharp corners (drawn over GTK rounded border/shadow frame)

Categories

(Core :: Graphics, defect, P2)

Desktop
Linux
defect

Tracking

()

VERIFIED FIXED
96 Branch
Tracking Status
firefox65 --- wontfix
firefox96 --- verified
firefox97 --- verified

People

(Reporter: val, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert)

Attachments

(10 files)

Attached image firefox-wayland-corner.png (deleted) —
(continuing from bug 1507608) When GL compositing is used on the Wayland backend, the GL subsurface that actual Firefox content is rendered to does not have rounded corners. It overdraws the parent GTK surface that contains the border and shadow. The Default Firefox theme actually draws the Adwaita rounded corner, but the surface is still not transparent in the corners, so there's an ugly gray corner.
Blocks: wayland
Priority: -- → P3

I'm not sure how to fix that on Wayland as it does not support XShape mask which we use on X11.

huh, the corners were just masked out? But who draws the non-transparent gray pixels there in the first place?

This almost works for me, and the behavior is the same whether basic layers or WR is used.

As I understand, the way CSD works here is that the window uses a GTK window style which only draws the shadow around an inner widget drawn by Firefox. Adwaita gives us a soft shadow and a 1px semitransparent outline.

The Default theme has a slight glitch where the corners are not as transparent as they should be. I think the tab bar's "headerbar" styling is trying to draw a window shadow here which gets overlaid with the shadow drawn by the window. We need to either somehow remove the shadow from the headerbar style applied to the tab bar, or somehow mask out the drawing of the window shadow so that it only draws outside the inner widget.

The Light and Dark themes have no "headerbar" styling, so the tab bar has sharp corners which clash with the rounded outline from the window's Adwaita theme.

Hi, the issue still exists on Firefox 74.0 on Fedora (Wayland). Any idea how to fix this?

No longer blocks: wayland
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: FreeBSD → Linux

I can confirm this issue still exists in Fedora 34, Firefox 89, Wayland.

This just became even more prominent in 95 with colorways. We need a way to support this with alpha visuals as XShape is neither available on Wayland nor on X11 with HW Webrender - i.e. it's only available in legacy fallback paths. As the default theme supports this (even though with a grey corner), I suppose this is not really a graphics but rather a theme issue.

Emilio, do you happen to know how the default theme gets its corners and what would need to be done to make it work for other themes as well?

Flags: needinfo?(emilio)

Wayland case seems to be a bit special as it produces gray corner even with Gtk theme (which works OK on X11/Alpha).
We may want to draw alpha over corners when custom theme is used and main window is not maximized or tiled.

I'm not quite familiar with how this works, I think Martin is the right person to ask about it.

I can dig if you want but I'd guess that this kind of stuff (the :not(:-moz-lwtheme) bits and such) is related: https://searchfox.org/mozilla-central/rev/9bc5dcea99c59dc18eae0de7064131aa20cfbb66/browser/themes/linux/browser.css#389-399

Flags: needinfo?(emilio)

Thanks emilio, that's a good hint. The comment says

It may cause performance issues so let's put it under a preference and enable it for desktop environment which do that by default.

This is clearly outdated and from a pre Webrender time.

Emilio, you're absolutely right :) There's original Bug 1408360 for it.

For custom themes we need to apply alpha component only and keep original color from theme (i.e. paint over alpha from theme).

Emilio, how is the best way how to do it? We can introduce a new appearance (-moz-window-titlebar-alpha for instance) which contains only alpha component from titlebar and paint it over theme color or use existing one (-moz-window-titlebar) but I'm not very experienced in js/css styling so I'll appreciate any help here.

Flags: needinfo?(emilio)
Priority: P3 → P2

(In reply to Martin Stránský [:stransky] (ni? me) from comment #11)

Emilio, how is the best way how to do it? We can introduce a new appearance (-moz-window-titlebar-alpha for instance) which contains only alpha component from titlebar and paint it over theme color or use existing one (-moz-window-titlebar) but I'm not very experienced in js/css styling so I'll appreciate any help here.

I don't think that's super-great, I think I have an idea which should be relatively simple, stealing.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

This bit is taken straight from D73454 (I reviewed it but I guess
another pair of eyes is ok, it's really straight-forward).

Co-authored-by: Nicklas Boman <smurfd@gmail.com>

We always use alpha visual for WebRender, and appearance: none is
unnecessary (root element has no intrinsic appearance).

Depends on D128681

There's no need to use the media query to set the default styles of the
buttons, we only need to hide them if appropriate.

Depends on D128682

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Created attachment 9246290 [details]
Bug 1509931 - Remove -moz-gtk-csd-transparent-background. r=stransky

We always use alpha visual for WebRender

KDE with disabled compositor, i3, etc. use alpha visual, but it's not transparent.
Could -moz-gtk-csd-transparent-background be defined with gdk_screen_is_composited()
and be used to forbid outer border-radius + outer shadow of main menu/addon/etc. widgets
to remove any Xshape usage (bug 1730991 comment 6)?
According to the Chromium bugtracker, Xshape also causes performance problems (bug 1730991 comment 7).

(In reply to Darkspirit from comment #19)

KDE with disabled compositor, i3, etc. use alpha visual, but it's not transparent.

WDYM, just that the background wouldn't be transparent?

Could -moz-gtk-csd-transparent-background be defined with gdk_screen_is_composited()
and be used to forbid outer border-radius + outer shadow of main menu/addon/etc. widgets
to remove any Xshape usage (bug 1730991 comment 6)?
According to the Chromium bugtracker, Xshape also causes performance problems (bug 1730991 comment 7).

I'm not sure about this (not familiar with XShape at all).

This patch stack makes Wayland look great, but on X11/XWayland I still see the compositor shadow drawn around the crisp corner when using a lightweight theme... Do you know whether that's avoidable Martin?

Flags: needinfo?(stransky)

Ah, I think I know why that might be...

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/81ca2d28c253 Simplify titlebar buttons CSS. r=desktop-theme-reviewers,dao
Flags: needinfo?(emilio)
Keywords: leave-open
Flags: needinfo?(stransky)
Attachment #9246289 - Attachment description: Bug 1509931 - Use titlebar radius on Linux and simplify native titlebar set-up. r=stransky → Bug 1509931 - Use titlebar radius on Linux and make titlebar set-up work for lightweight themes. r=stransky!,dao!

This seemed possible/maybe worth doing, but let me know if you'd rather
keep the 10 hard-coded.

I fixed the wayland overdraw and the X11 artifacts, so I think this is ready to go. I've tested this stack on KWin and GNOME, both X11 and Wayland. Will test i3 and such asap.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)
Non-composited X11 (the legacy variant of X11: i3, KDE with manually disabled compositor, etc.) does not support transparency. Everything that would usually be transparent/alpha is just black/opaque. Menus and window corners had shadows on black background (=fat black borders).
The autoscroll icon was a black square with a white circle in it.

XShapeCombineMask is implemented for software rendering only. Firefox creates a stencil (like a cookie cutter) of where transparency needs to get cut off. X11 then applies this stencil on every frame. The outer shadow of a rounded main menu, addon widget or window titlebar has transparency, so it gets cut off by X11, leaving a rounded widget without shadow.

XShapeCombineMask is not implemented for hardware rendering.
That's why SW WR is enforced for menus, addons and autoscroll icon on non-composited X11 to make use of XShapeCombineMask while the main window is still using OpenGL.

XShapeCombineMask

Could XShapeCombineMask be removed and instead menus and the autoscroll icon loose their border-radius (become rectangular) and loose their shadow (to avoid black background) if gdk_screen_is_composited() is false?
IIUC, that would reduce code complexity, improve stability and performance.

A rectangular autoscroll icon on non-composited X11 would be noticeable. But users choose non-composited X11 to get more performance than with composited X11. Considering this as well, it doesn't make sense to use XShapeCombineMask which contradicts their intent and causes above problems.

Well, perhaps, but that seems tangential to this bug, isn't it? We don't use XShapeCombineMask for the titlebar and this patch stack doesn't start doing that.

If we do want to do the cleanup described above, we might need a new media query which determines this (but it should definitely not have csd in the name, since it is not about csd at all). So I'd rather clean up the existing one since it's confusing at best.

Anyways, things I've tested:

  • KWin (Wayland/X11/XWayland)
  • GNOME (Wayland/X11/XWayland) with a variety of GTK themes
  • i3
  • bspwm

And in all cases this patch stack doesn't regress behavior (and improves it in the obvious cases). So I'd say comment 25 should be a separate bug?

If someone has an exotic setup and wants to give this a try before it lands, or wants to provide feedback, builds should be here eventually: https://treeherder.mozilla.org/jobs?repo=try&revision=327533608921f19068d1b8898b094fb3b3cdfc9d

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)

If we do want to do the cleanup described above, we might need a new media query which determines this (but it should definitely not have csd in the name, since it is not about csd at all). So I'd rather clean up the existing one since it's confusing at best.

And in all cases this patch stack doesn't regress behavior (and improves it in the obvious cases). So I'd say comment 25 should be a separate bug?

Yes. I only remembered that a query like -moz-gtk-csd-transparent-background might be useful to allow Xshape removal (if it is desired).

btw. It would be great to get border-radius from menu elements (Adwaita uses it) and use it in gecko too, for context menus for instance. But that may be a different bug.

So, querying the individual border-radius properties doesn't work. I get:

(firefox:17330): Gtk-WARNING **: 00:39:54.651: Style property "border-top-left-radius" is not gettable

And same for the other corners of course. However, my patch works, because of this code. Basically, querying border-radius on gtk3 returns the border-top-left-radius, which happens to work for us, yay :)

So given there's no way to access the other border corners, and that this works for our purposes, I think we should probably just roll with it, thoughts?

Flags: needinfo?(stransky)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #30)

So given there's no way to access the other border corners, and that this works for our purposes, I think we should probably just roll with it, thoughts?

I'd agree - should work well for almost everyone.

One question I had to think of is if we want to use rounded corners at the bottom at some point - more and more GTK4 apps do that, e.g. nautilus/files. But we can also leave that for the future.

Okay, fair enough.

Flags: needinfo?(stransky)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/257c4f510cfd Add support for chrome-only environment variables. r=stransky https://hg.mozilla.org/integration/autoland/rev/a1e159fd2136 Expose titlebar radius as a chrome-only CSS environment variable. r=stransky
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c9399923df7b Remove -moz-gtk-csd-transparent-background. r=stransky,desktop-theme-reviewers,dao https://hg.mozilla.org/integration/autoland/rev/bcdfe3daf980 Use a more precise toolbar radius. r=stransky

Emilio, I tested the patches and it looks great!

Emilio, when running testsuite with the patch (locally) I'm getting this crash:

2:05.41 GECKO(53217) Assertion failure: sInServoTraversal || NS_IsMainThread(), at /home/komat/src/objdir-opt/dist/include/mozilla/ServoUtils.h:33
2:05.41 GECKO(53217) #01: mozilla::ServoStyleSet::IsInServoTraversal() [/home/komat/src/objdir-opt/dist/include/mozilla/ServoStyleSet.h:103]
2:05.41 GECKO(53217) #02: mozilla::Preferences::InitStaticMembers() [/home/komat/src/modules/libpref/Preferences.cpp:3464]
2:05.41 GECKO(53217) #03: nsresult mozilla::Internals::GetPrefValue<int*&>(char const*, int*&, mozilla::PrefValueKind) [/home/komat/src/modules/libpref/Preferences.cpp:4439]
2:05.41 GECKO(53217) #04: mozilla::Preferences::GetInt(char const*, int*, mozilla::PrefValueKind) [/home/komat/src/modules/libpref/Preferences.cpp:0]
2:05.41 GECKO(53217) #05: nsXPLookAndFeel::GetFloatValue(mozilla::LookAndFeel::FloatID, float&) [/home/komat/src/widget/nsXPLookAndFeel.cpp:894]
2:05.41 GECKO(53217) #06: nsWindow::GetTitlebarRadius() [/home/komat/src/widget/gtk/nsWindow.cpp:6111]
2:05.41 GECKO(53217) #07: nsWindow::GetTitlebarRect() [/home/komat/src/widget/gtk/nsWindow.cpp:6438]
2:05.41 GECKO(53217) #08: mozilla::widget::GtkCompositorWidget::GetTransparentRegion() [/home/komat/src/widget/gtk/GtkCompositorWidget.cpp:0]
2:05.41 GECKO(53217) #09: mozilla::wr::RenderCompositorSWGL::AllocateMappedBuffer(mozilla::wr::Box2D<int, mozilla::wr::DevicePixel> const*, unsigned long) [/home/komat/src/gfx/webrender_bindings/RenderCompositorSWGL.cpp:149]
2:05.41 GECKO(53217) #10: mozilla::wr::RenderCompositorSWGL::StartCompositing(mozilla::wr::ColorF, mozilla::wr::Box2D<int, mozilla::wr::DevicePixel> const*, unsigned long, mozilla::wr::Box2D<int, mozilla::wr::DevicePixel> const*, unsigned long) [/home/komat/src/gfx/webrender_bindings/RenderCompositorSWGL.cpp:185]
2:05.41 GECKO(53217) #11: <webrender::compositor::sw_compositor::SwCompositor as webrender::composite::Compositor>::start_compositing [gfx/wr/webrender/src/compositor/sw_compositor.rs:1400]
2:05.41 GECKO(53217) #12: webrender::renderer::Renderer::draw_frame [gfx/wr/webrender/src/renderer/mod.rs:4582]
2:05.41 GECKO(53217) #13: webrender::renderer::Renderer::render_impl [gfx/wr/webrender/src/renderer/mod.rs:2009]
2:05.41 GECKO(53217) #14: webrender::renderer::Renderer::render [gfx/wr/webrender/src/renderer/mod.rs:0]
2:05.41 GECKO(53217) #15: wr_renderer_render [gfx/webrender_bindings/src/bindings.rs:623]
2:05.41 GECKO(53217) #16: mozilla::wr::RendererOGL::UpdateAndRender(mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::Maybe<mozilla::wr::ImageFormat> const&, mozilla::Maybe<mozilla::Range<unsigned char> > const&, bool*, mozilla::wr::RendererStats*) [/home/komat/src/gfx/webrender_bindings/RendererOGL.cpp:186]
2:05.42 GECKO(53217) #17: mozilla::wr::RenderThread::UpdateAndRender(mozilla::wr::WrWindowId, mozilla::layers::BaseTransactionId<mozilla::VsyncIdType> const&, mozilla::TimeStamp const&, bool, mozilla::Maybe<mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::Maybe<mozilla::wr::ImageFormat> const&, mozilla::Maybe<mozilla::Range<unsigned char> > const&, bool*) [/home/komat/src/gfx/webrender_bindings/RenderThread.cpp:0]
2:05.42 GECKO(53217) [Parent 53217: Main Thread]: D/Widget GetScreenBounds [7fba6021a000] 95,55 -> 1280 x 1012, unscaled 95,55 -> 1280 x 1012
2:05.42 GECKO(53217) #18: mozilla::wr::RenderThread::HandleFrameOneDoc(mozilla::wr::WrWindowId, bool) [/home/komat/src/gfx/webrender_bindings/RenderThread.cpp:355]
2:05.42 GECKO(53217) [Parent 53217: Main Thread]: D/Widget Events sent from focus in event [7fba6021a000]
2:05.42 GECKO(53217) #19: mozilla::detail::RunnableMethodImpl<mozilla::wr::RenderThread*, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), true, (mozilla::RunnableKind)0, mozilla::wr::WrWindowId, bool>::Run() [/home/komat/src/objdir-opt/dist/include/nsThreadUtils.h:1203]

Any idea what can be wrong?

Flags: needinfo?(emilio)

May nsWindow::GetTitlebarRadius() be called from render thread? (this is sw-wr).

We have a technical debt here as we don't use titlebar rendering in testsuite - I'll look at it.

Yeah, so nsWindow::GetTitlebarRadius can be called off the main thread which seems unfortunate. We can fix this easily by moving the titlebar radius to a member or such.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #43)

Yeah, so nsWindow::GetTitlebarRadius can be called off the main thread which seems unfortunate. We can fix this easily by moving the titlebar radius to a member or such.

I can fix that as a part of Bug 1736518.

Oh, great, let me know if I can help. Probably a static atomic integer in nsLookAndFeelGtk is slightly easier to invalidate in practice.

Flags: needinfo?(emilio)

I have just tested the latest nightly on Fedora 35 and it works perfectly for XWayland, however, when running Wayland the bottom corners are still not rounded and the top corners are rounded but still have a weird (barely noticeable unless you know) transparent sharp corner. Note that I am running a patched version of mutter (RPMs) that enables rounded corners in every app, however, Firefox is the only app that breaks the nice uniform look that I believe GTK4/libadwaita are also going for with the rounded corners on all sides (just like Windows 11 and macOS).

I understand that this is an edge case currently (with the patched mutter) but I foresee this becoming a "problem" when in a year or two all other apps also have rounded bottom corners and Firefox is the odd one out, I think fixing this now is a good idea to prevent technical debt.

Xwayland screenshot
Wayland screenshot
Other apps screenshot

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb8c23dda78d Use titlebar radius on Linux and make titlebar set-up work for lightweight themes. r=stransky,dao

(In reply to gschram from comment #47)

I understand that this is an edge case currently (with the patched mutter) but I foresee this becoming a "problem" when in a year or two all other apps also have rounded bottom corners and Firefox is the odd one out, I think fixing this now is a good idea to prevent technical debt.

Getting bottom corners rounded on Wayland seems worth a separate bug.

Keywords: leave-open

(In reply to Emilio Cobos Álvarez (:emilio) from comment #49)

(In reply to gschram from comment #47)

I understand that this is an edge case currently (with the patched mutter) but I foresee this becoming a "problem" when in a year or two all other apps also have rounded bottom corners and Firefox is the odd one out, I think fixing this now is a good idea to prevent technical debt.

Getting bottom corners rounded on Wayland seems worth a separate bug.

I have opened a separate bug here https://bugzilla.mozilla.org/show_bug.cgi?id=1743981 I have listed it as an enhancement.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

== Change summary for alert #32637 (as of Mon, 06 Dec 2021 09:45:12 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
5% tresize linux1804-64-shippable-qr e10s stylo webrender 18.35 -> 17.39
5% tresize linux1804-64-shippable-qr e10s stylo webrender-sw 17.60 -> 16.79
5% tresize linux1804-64-shippable-qr e10s stylo webrender-sw 17.57 -> 16.77
4% tresize linux1804-64-shippable-qr e10s fission stylo webrender 21.07 -> 20.29

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=32637

Flags: qe-verify+
Attached image dim corners.png (deleted) —

Verified on 96.0a1 and 97.0a1 that the top side is rounded and look good on Wayland session under Ubuntu 20.04. Although while getting the notification to make Firefox as default, a dim effect is active and the rounded parts have corners because of that (see attachment and zoom in). Any idea? Thank you!

Flags: needinfo?(emilio)
Blocks: 1745419

Bug 1745419 will fix that.

Flags: needinfo?(emilio)

Thanks! Then I will set the flags accordingly.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

I have posted a bug #1744496 in which I explain that on elementary OS 6.0 Odin the corners are still square. It seems on elementary OS -moz-window-titlebar doesn't work. -moz-gtk-csd-titlebar-radius does work, as you can see when the dark theme is enabled. Can anyone look at this bug?

Regressions: 1745894
Blocks: 1756903
Regressions: 1762379
Regressions: 1765685
No longer regressions: 1762379
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: