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.

Sign up for GitHub

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

Jump to bottom

fix: Wayland general CSD fixes #34955

Merged
merged 5 commits into from Aug 3, 2022
Merged

fix: Wayland general CSD fixes #34955

merged 5 commits into from Aug 3, 2022

Conversation

msizanoen1
Copy link
Contributor

@msizanoen1 msizanoen1 commented Jul 18, 2022 •
edited

Description of Change

General Wayland client-side decoration fixes. Check individual commits for more information.

Supersedes #34945, #34948
Fixes #34820, fixes #33161

Checklist

Release Notes

Notes: Fix support for Wayland client-side decorations

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 18, 2022
@ckerr ckerr mentioned this pull request Jul 18, 2022
Closed
3 tasks
@fontivan fontivan mentioned this pull request Jul 18, 2022
Closed
5 tasks
@msizanoen1 msizanoen1 marked this pull request as ready for review July 19, 2022 02:41
@msizanoen1 msizanoen1 requested review from a team as code owners July 19, 2022 02:41
@msizanoen1 msizanoen1 marked this pull request as draft July 19, 2022 02:48
@msizanoen1 msizanoen1 marked this pull request as ready for review July 19, 2022 03:18
@msizanoen1 msizanoen1 changed the title fix: Wayland general fixes fix: Wayland general CSD fixes Jul 19, 2022
zcbenz
zcbenz reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

I'm good with this change, only with one question: do you know whether Chrome browser has the same problem? If it has, can you try to submit this patch to Chromium as well? If it does not have, do you know why?

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/19-x-y labels Jul 19, 2022
@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Jul 19, 2022 •
edited

I'm good with this change, only with one question: do you know whether Chrome browser has the same problem? If it has, can you try to submit this patch to Chromium as well? If it does not have, do you know why?

This only affects BrowserFrameViewLinuxNative on upstream Chromium, but it doesn't affect any part of the browser itself because the browser currently doesn't use BrowserFrameViewLinuxNative, but draws its own decorations and only uses the toolkit for styling information. Non-browser windows don't use the toolkit at all but uses its own custom (really outdated) decorations.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 20, 2022
@msizanoen1 msizanoen1 force-pushed the wayland-general-fixes branch 2 times, most recently from 9713e08 to 54e6ddc Compare July 20, 2022 15:28
@GrantMoyer GrantMoyer mentioned this pull request Jul 20, 2022
Closed
@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Jul 21, 2022 •
edited

Rebasing for conflict resolution broke backporting so the branches suitable for backporting are now wayland-general-fixes-19 and wayland-general-fixes-20 for version 19 and 20 respectively.

@msizanoen1 msizanoen1 force-pushed the wayland-general-fixes branch 4 times, most recently from 0e2309a to e273cc6 Compare July 22, 2022 16:11
@msizanoen1 msizanoen1 requested a review from zcbenz July 22, 2022 16:12
@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Jul 22, 2022 •
edited

@zcbenz I changed the fix for unclickable window button in 3d7b7fd, please re-review.

@trop
Copy link
Contributor

trop bot commented Aug 3, 2022

I was unable to backport this PR to "20-x-y" cleanly;
you will need to perform this backport manually.

@msizanoen1 msizanoen1 deleted the wayland-general-fixes branch August 3, 2022 08:52
@msizanoen1 msizanoen1 mentioned this pull request Aug 3, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Aug 3, 2022

@msizanoen1 has manually backported this PR to "20-x-y", please check out #35206

@msizanoen1 msizanoen1 mentioned this pull request Aug 3, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Aug 3, 2022

@msizanoen1 has manually backported this PR to "19-x-y", please check out #35207

@skoruppa
Copy link

skoruppa commented Aug 11, 2022 •
edited

Finally working windows decorations on Gnome with Wayland! Thank you :) @msizanoen1, do you know windows controls (maximize, minimize, close) are drawn on the left when my whole system has them on the right? Is that a bug?
image

@msizanoen1
Copy link
Contributor Author

This looks like your system is misconfigured. Window decorations always has the correct position on my machine with the window controls position configured using gnome-tweaks.

@skoruppa
Copy link

Thank you again :) It was configured to the right side in the gnome-tweaks, but after changing to left, and back to right, controls are now on the correct side for the electron apps as well

@satmandu satmandu mentioned this pull request Aug 11, 2022
Closed
2 tasks
@r2binx r2binx mentioned this pull request Sep 12, 2022
Merged
@commit-lint commit-lint bot mentioned this pull request Oct 12, 2022
Open
@SilverMight
Copy link

Are the windows not fully maximizing issue still a thing? I tried electron 22 and I still get the issue, on GNOME 43.

@msizanoen1
Copy link
Contributor Author

Are the windows not fully maximizing issue still a thing? I tried electron 22 and I still get the issue, on GNOME 43.

Are you sure you are using the correct Electron binary with the --ozone-platform=wayland --enable-features=WaylandWindowDecorations flags? I can't reproduce it in a GNOME OS Nightly VM using the Electron 22 Alpha binary from GitHub Releases.

@SilverMight
Copy link

Are the windows not fully maximizing issue still a thing? I tried electron 22 and I still get the issue, on GNOME 43.

Are you sure you are using the correct Electron binary with the --ozone-platform=wayland --enable-features=WaylandWindowDecorations flags? I can't reproduce it in a GNOME OS Nightly VM using the Electron 22 Alpha binary from GitHub Releases.

I figured out the issue - every other theme works but the Dracula GTK theme I'm using, for some reason, so that must be a problem there. Thanks regardless!

@SilverMight SilverMight mentioned this pull request Oct 22, 2022
Open
schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
@msizanoen1 @schetle
fix: Wayland general CSD fixes (electron#34955)
5bc59a6 
* fix: broken wayland window decorations due to botched chromium update

The `GetTitlebarBounds().height()` is obviously intended to be placed in
the `top` parameter, which used to be the second one before upstream
removed multi-parameter `gfx::Rect::Inset`, but it's the first parameter
for `gfx::Insets::TLBR`, which was intended to replace the removed
`Inset` function. However, whoever updated Chromium kept the parameter
unchanged, causing the title bar height to be passed to the `left`
parameter, causing the window title bar to be unclickable.

* fix: wayland window top bar buttons unclickable

Use NonClientFrameView::TargetForRect for the ClientFrameViewLinux
implementation because the default inherited from FramelessView blocks
any non-HTCLIENT events.

* fix: add maximized parameter to LinuxUI::GetWindowFrameProvider

* fix: pass frame_->IsMaximized() to GetWindowFrameProvider

This ensures that the toolkit renders the window decorations in maximized mode
while the window is maximized to ensure that there is no empty space around the window.
@sarim sarim mentioned this pull request Nov 13, 2022
Closed
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@msizanoen1 @khalwa
fix: Wayland general CSD fixes (electron#34955)
21a127b 
* fix: broken wayland window decorations due to botched chromium update

The `GetTitlebarBounds().height()` is obviously intended to be placed in
the `top` parameter, which used to be the second one before upstream
removed multi-parameter `gfx::Rect::Inset`, but it's the first parameter
for `gfx::Insets::TLBR`, which was intended to replace the removed
`Inset` function. However, whoever updated Chromium kept the parameter
unchanged, causing the title bar height to be passed to the `left`
parameter, causing the window title bar to be unclickable.

* fix: wayland window top bar buttons unclickable

Use NonClientFrameView::TargetForRect for the ClientFrameViewLinux
implementation because the default inherited from FramelessView blocks
any non-HTCLIENT events.

* fix: add maximized parameter to LinuxUI::GetWindowFrameProvider

* fix: pass frame_->IsMaximized() to GetWindowFrameProvider

This ensures that the toolkit renders the window decorations in maximized mode
while the window is maximized to ensure that there is no empty space around the window.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@ckerr ckerr Awaiting requested review from ckerr

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
5 participants
@msizanoen1 @zcbenz @deepak1556 @skoruppa @SilverMight