refactor: use views NonClientHitTest for draggable regions on mac by nornagon · Pull Request #35603 · electron/electron

nornagon · 2022-09-08T01:03:33Z

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

PR description included and stakeholders cc'd
npm test passes
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: none

nornagon · 2022-09-08T01:03:58Z

This supersedes #35007, I think.

zcbenz · 2022-09-08T05:39:25Z

/cc @daniel-koc

daniel-koc · 2022-09-09T03:54:03Z

Overall looks great.

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 · 2022-09-14T21:05:13Z

@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.

ckerr

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

shell/browser/native_window_mac.mm

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

nornagon · 2022-09-27T20:02:43Z

spec/api-web-contents-spec.ts

-
-      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();
-      });


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.

release-clerk · 2022-10-12T16:05:49Z

No Release Notes

… mac (#35603)" This reverts commit 8a926ff.

…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>

nornagon added no-backport semver/patch backwards-compatible bug fixes labels Sep 8, 2022

electron-cation bot added the new-pr 🌱 PR opened recently label Sep 8, 2022

nornagon added the wip ⚒ label Sep 8, 2022

nornagon requested review from a team as code owners September 8, 2022 01:45

electron-cation bot removed the new-pr 🌱 PR opened recently label Sep 9, 2022

nornagon mentioned this pull request Sep 13, 2022

feat: replace BrowserView with WebContentsView #35658

Merged

10 tasks

codebytere self-requested a review September 13, 2022 18:32

nornagon and others added 6 commits September 14, 2022 13:54

refactor: use views NonClientHitTest for draggable regions on mac

1a6391b

iwyu

da36195

add backport of 9bb5f0316

b74915d

chore: update patches

248ec6f

remove some unneeded functions

5e2b4f8

remove test for triggering when BW is focused

53122fd

nornagon force-pushed the dragregions branch from d0b8ea0 to 53122fd Compare September 14, 2022 21:04

patchup bot and others added 2 commits September 14, 2022 21:18

chore: update patches

7cdb67b

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

0083fee

ckerr approved these changes Sep 14, 2022

View reviewed changes

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

Merge branch 'main' into dragregions

7dc830b

Apply suggestions from code review

2dcc364

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

Update shell/browser/native_window.h

6a52ce4

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

nornagon commented Sep 27, 2022

View reviewed changes

nornagon removed the wip ⚒ label Sep 27, 2022

nornagon mentioned this pull request Sep 27, 2022

chore: Move draggable regions implementation from NativeBrowserView into InspectableWebContentsView #35007

Merged

5 tasks

Merge remote-tracking branch 'origin/main' into dragregions

a26d640

fix build

6c354fb

zcbenz approved these changes Oct 12, 2022

View reviewed changes

jkleinsc approved these changes Oct 12, 2022

View reviewed changes

jkleinsc merged commit 8a926ff into main Oct 12, 2022

jkleinsc deleted the dragregions branch October 12, 2022 16:05

nornagon mentioned this pull request Nov 1, 2022

refactor: move draggable regions to WebContents #36230

Merged

5 tasks

commit-lint bot mentioned this pull request Feb 8, 2023

[pull] main from electron:main Blaquez-home/electron#3

Open

codebytere mentioned this pull request Feb 9, 2023

[Bug]: Draggable regions prevent webContents interaction with customButtonsOnHover #37210

Closed

3 tasks

MarshallOfSound added a commit that referenced this pull request Feb 21, 2023

Revert "refactor: use views NonClientHitTest for draggable regions on…

780ab9c

… mac (#35603)" This reverts commit 8a926ff.

MarshallOfSound mentioned this pull request Feb 21, 2023

fix: restore original draggable regions behavior pre-35603 #37372

Closed

This was referenced Feb 23, 2023

fix: allow contextmenu event in draggable regions #37386

Merged

[Bug]: context menus cannot be displayed when "-webkit-app-region: drag" is enabled #37301

Closed

codebytere mentioned this pull request Feb 27, 2023

fix: handle closing webContents in BrowserViews #37420

Merged

4 tasks

This was referenced Mar 1, 2023

[Bug]: Frameless window is not draggable when running MAS build #37414

Closed

fix: draggable regions on MAS #37466

Merged

codebytere mentioned this pull request Apr 12, 2023

fix: swipe event emission on macOS #37946

Merged

3 tasks

zcbenz mentioned this pull request May 23, 2023

chore: remove dead draggable region code #38404

Merged

codebytere mentioned this pull request Feb 14, 2024

chore: remove unused anonymous namespace methods #41333

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

refactor: use views NonClientHitTest for draggable regions on mac#35603
jkleinsc merged 13 commits intomainfrom
dragregions

nornagon commented Sep 8, 2022

Uh oh!

nornagon commented Sep 8, 2022

Uh oh!

zcbenz commented Sep 8, 2022

Uh oh!

daniel-koc commented Sep 9, 2022

Uh oh!

nornagon commented Sep 14, 2022

Uh oh!

ckerr left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nornagon Sep 27, 2022

Uh oh!

release-clerk bot commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nornagon commented Sep 8, 2022

Description of Change

Checklist

Release Notes

Uh oh!

nornagon commented Sep 8, 2022

Uh oh!

zcbenz commented Sep 8, 2022

Uh oh!

daniel-koc commented Sep 9, 2022

Uh oh!

nornagon commented Sep 14, 2022

Uh oh!

ckerr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nornagon Sep 27, 2022

Choose a reason for hiding this comment

Uh oh!

release-clerk bot commented Oct 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants