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

refactor: use views NonClientHitTest for draggable regions on mac #35603

Merged
merged 13 commits into from
Oct 12, 2022

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Sep 8, 2022

Description of Change

This changes the implementation of draggable regions on macOS to use the
built-in Views tools for it.

See here for some more context.

Checklist

Release Notes

Notes: none

@nornagon nornagon added no-backport semver/patch backwards-compatible bug fixes labels Sep 8, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 8, 2022
@nornagon
Copy link
Member Author

nornagon commented Sep 8, 2022

This supersedes #35007, I think.

@nornagon nornagon requested review from a team as code owners September 8, 2022 01:45
@zcbenz
Copy link
Member

zcbenz commented Sep 8, 2022

/cc @daniel-koc

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 9, 2022
@daniel-koc
Copy link
Contributor

Overall looks great.

  1. in shell/browser/api/electron_browser_window.cc the following calls seem unnecessary
#if BUILDFLAG(IS_MAC)
  UpdateDraggableRegions(draggable_regions_);
#endif

In such way we could remove overrides for AddBrowserView, RemoveBrowserView, SetTopBrowserView, ResetBrowserViews from BrowserWindow
2. After change:

diff --git a/shell/browser/api/electron_api_browser_window_mac.mm b/shell/browser/api/electron_api_browser_window_mac.mm
index a789647f1a..97c4af3bd3 100644
--- a/shell/browser/api/electron_api_browser_window_mac.mm
+++ b/shell/browser/api/electron_api_browser_window_mac.mm
@@ -19,6 +19,9 @@ namespace electron::api {
 
 void BrowserWindow::UpdateDraggableRegions(
     const std::vector<mojom::DraggableRegionPtr>& regions) {
+  if (window_->has_frame())
+    return;
+
   if (&draggable_regions_ != &regions && web_contents()) {
     draggable_regions_ = mojo::Clone(regions);
   }

The following definitions:

  SkRegion* GetDraggableRegion() const { return draggable_region_.get(); }
  void UpdateDraggableRegions(
      const std::vector<mojom::DraggableRegionPtr>& regions);
  std::unique_ptr<SkRegion> draggable_region_;

we could just move from native_window_mac.h and native_window_views.h into native_window.h

@nornagon
Copy link
Member Author

@samuelmaddock I've dropped one of the tests you added for the focus event here because it doesn't seem to work correctly when using BridgedContentView, which is a part of macOS Views. Based on analysis from @zcbenz, this may be a Chrome bug with the OnWebContentsFocused method, which is fairly lightly used in Chromium itself.

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

This looks really good. I have a couple of questions & minor suggestions but both the idea and impl look solid.

shell/browser/native_window.h Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Show resolved Hide resolved
nornagon and others added 3 commits September 27, 2022 14:59
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Comment on lines -900 to -921

it('is triggered when BrowserWindow is focused', async () => {
const window1 = new BrowserWindow({ show: false });
const window2 = new BrowserWindow({ show: false });

await Promise.all([
window1.loadURL('about:blank'),
window2.loadURL('about:blank')
]);

const focusPromise1 = emittedOnce(window1.webContents, 'focus');
const focusPromise2 = emittedOnce(window2.webContents, 'focus');

window1.showInactive();
window2.showInactive();

window1.focus();
await expect(focusPromise1).to.eventually.be.fulfilled();

window2.focus();
await expect(focusPromise2).to.eventually.be.fulfilled();
});
Copy link
Member Author

Choose a reason for hiding this comment

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

heads up @samuelmaddock, this removes a test that you added because it no longer passes. I'm pretty sure this is due to a Chrome bug that doesn't properly emit the web contents focused event when the window is focused on Mac/Views—the event is emitted if I click on the WebContents, just not when the window is only focused. Nothing else seems to be problematic in terms of using the WebContents though.

@jkleinsc jkleinsc merged commit 8a926ff into main Oct 12, 2022
@jkleinsc jkleinsc deleted the dragregions branch October 12, 2022 16:05
@release-clerk
Copy link

release-clerk bot commented Oct 12, 2022

No Release Notes

MarshallOfSound added a commit that referenced this pull request Feb 21, 2023
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
…ectron#35603)

* refactor: use views NonClientHitTest for draggable regions on mac

* iwyu

* add backport of 9bb5f0316

* chore: update patches

* remove some unneeded functions

* remove test for triggering when BW is focused

* chore: update patches

* simplify views/mac split now that the draggable logic is the same

* Apply suggestions from code review

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* Update shell/browser/native_window.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix build

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants