[Wayland][OpenGL] CSD has sharp corners (drawn over GTK rounded border/shadow frame)
Categories
(Core :: Graphics, defect, P2)
Tracking
()
People
(Reporter: val, Assigned: emilio)
References
(Blocks 2 open bugs)
Details
(Keywords: perf-alert)
Attachments
(10 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
(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.
Updated•6 years ago
|
The same thing happens on linux also. https://www.enlightenment.org/ss/display.php?image=e-5c1b7b13e6d7e3.27335093.png
Updated•5 years ago
|
Comment 2•5 years ago
|
||
I'm not sure how to fix that on Wayland as it does not support XShape mask which we use on X11.
Reporter | ||
Comment 3•5 years ago
|
||
huh, the corners were just masked out? But who draws the non-transparent gray pixels there in the first place?
Comment 4•5 years ago
|
||
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.
Comment 5•5 years ago
|
||
Hi, the issue still exists on Firefox 74.0 on Fedora (Wayland). Any idea how to fix this?
Updated•4 years ago
|
I can confirm this issue still exists in Fedora 34, Firefox 89, Wayland.
Comment 7•3 years ago
|
||
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?
Updated•3 years ago
|
Comment 8•3 years ago
|
||
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.
Assignee | ||
Comment 9•3 years ago
|
||
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
Comment 10•3 years ago
|
||
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.
Comment 11•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 13•3 years ago
|
||
(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 | ||
Comment 14•3 years ago
|
||
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>
Assignee | ||
Comment 15•3 years ago
|
||
Mostly plumbing.
Depends on D128679
Assignee | ||
Comment 16•3 years ago
|
||
Depends on D128680
Assignee | ||
Comment 17•3 years ago
|
||
We always use alpha visual for WebRender, and appearance: none is
unnecessary (root element has no intrinsic appearance).
Depends on D128681
Assignee | ||
Comment 18•3 years ago
|
||
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
Comment 19•3 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)
Created attachment 9246290 [details]
Bug 1509931 - Remove -moz-gtk-csd-transparent-background. r=stranskyWe 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).
Assignee | ||
Comment 20•3 years ago
|
||
(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?
Comment 22•3 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/81ca2d28c253 Simplify titlebar buttons CSS. r=desktop-theme-reviewers,dao
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 23•3 years ago
|
||
This seemed possible/maybe worth doing, but let me know if you'd rather
keep the 10 hard-coded.
Assignee | ||
Comment 24•3 years ago
|
||
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.
Comment 25•3 years ago
|
||
(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
- caused problems on Mutter back then
- crashes the GPU process on Nvidia: bug 1730991
- causes performance problems according to the Chromium bugtracker: bug 1733094 comment 18,https://bugs.chromium.org/p/chromium/issues/detail?id=1198080
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.
Assignee | ||
Comment 26•3 years ago
|
||
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
Comment 27•3 years ago
|
||
(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).
Comment 28•3 years ago
|
||
Comment 29•3 years ago
|
||
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.
Assignee | ||
Comment 30•3 years ago
|
||
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?
Comment 31•3 years ago
|
||
(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.
Comment 33•3 years ago
|
||
bugherder |
Comment 34•3 years ago
|
||
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
Comment 35•3 years ago
|
||
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
Comment 36•3 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/2be88317a6c0 Fix rebase mistake.
Comment 37•3 years ago
|
||
bugherder |
Comment 38•3 years ago
|
||
bugherder |
Comment 39•3 years ago
|
||
Emilio, I tested the patches and it looks great!
Comment 40•3 years ago
|
||
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?
Comment 41•3 years ago
|
||
May nsWindow::GetTitlebarRadius() be called from render thread? (this is sw-wr).
Comment 42•3 years ago
|
||
We have a technical debt here as we don't use titlebar rendering in testsuite - I'll look at it.
Updated•3 years ago
|
Assignee | ||
Comment 43•3 years ago
|
||
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.
Comment 44•3 years ago
|
||
(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.
Assignee | ||
Comment 45•3 years ago
|
||
Oh, great, let me know if I can help. Probably a static atomic integer in nsLookAndFeelGtk is slightly easier to invalidate in practice.
Comment 47•3 years ago
|
||
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
Comment 48•3 years ago
|
||
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
Assignee | ||
Comment 49•3 years ago
|
||
(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.
Comment 50•3 years ago
|
||
(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.
Comment 51•3 years ago
|
||
bugherder |
Comment 52•3 years ago
|
||
== 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
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 53•3 years ago
|
||
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!
Comment 55•3 years ago
|
||
Thanks! Then I will set the flags accordingly.
Comment 56•3 years ago
|
||
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?
Description
•