Add -webkit-app-region support to BrowserView #10232

felixrieseberg · 2017-08-09T22:18:30Z

This PR adds support for -webkit-app-region to the experimental BrowserView API. Major downside right now: This PR ensures that the draggable regions of the HTML content in the window are respected, it does not implement draggable regions for the content of the BrowserView.

NSView Implementation Overview

Before
3️⃣ BrowserView (not draggable)
2️⃣ Multiple exclusion zones
1️⃣ Window's RenderView (contains a NSView that's draggable)
0️⃣ Window

After
5️⃣ Multiple exclusion zones
4️⃣ Draggable NSView
3️⃣ BrowserView (not draggable)
2️⃣ Multiple exclusion zones
1️⃣ Window's RenderView (contains a NSView that's draggable)
0️⃣ Window

This PR uses performWindowDragWithEvent where supported (10.11 and up) and good ol' window moving code on versions below. This PR also didn't have to do anything on Windows, where the window's webkit-app-region are already respected regardless of a BrowserView presence.

Why the window's draggable regions?

Despite my best efforts, I was completely unable to get the -webkit-app-region-regions for the content of the actual BrowserView. In addition, the BrowserView uses inspectable_web_contents instead of web_contents, which has a minor difference in the precise NSView class that is being returned from GetNativeView(). Long story short: I was unable to make it work, but this alternative ensures that you can use the BrowserView and still build apps with a custom frame and webkit-app-region.

Also: This mirrors the behavior on Windows, where the window's drag regions also take control over dragging in the BrowserView. Truly cross-platform 😉

mgc · 2017-08-10T01:36:44Z

atom/browser/native_browser_view_mac.mm

+       ++iter) {
+    base::scoped_nsobject<NSView> controlRegion(
+        [[ExcludeDragRegionView alloc] initWithFrame:NSZeroRect]);
+    [controlRegion setFrame:NSMakeRect(iter->x(),


There is a good chance I'm missing something, but just in case: is this assuming the browser view bounds are 100% of the containing window? Should use view.frame and view.superview.frame to handle the case where the browserview.SetBounds() has been used to position it in the bottom right corner?

into drag-browser-view

poiru

Awesome, thanks so much for fixing this! I left a few comments, but LGTM otherwise.

poiru · 2017-08-10T17:30:08Z

atom/browser/native_browser_view.h

@@ -38,6 +40,10 @@ class NativeBrowserView {
  virtual void SetBounds(const gfx::Rect& bounds) = 0;
  virtual void SetBackgroundColor(SkColor color) = 0;

+  // Called when the window needs to update its draggable region.
+  virtual void UpdateDraggableRegions(
+      std::vector<gfx::Rect> system_drag_exclude_areas);


Please use const std::vector<gfx::Rect& to avoid copying.

Could you also use {} and get rid of the change to native_browser_view.cc.

poiru · 2017-08-10T17:31:48Z

atom/browser/native_browser_view_mac.mm

@@ -8,10 +8,100 @@
 #include "skia/ext/skia_utils_mac.h"
 #include "ui/gfx/geometry/rect.h"

+using base::scoped_nsobject;


Is this needed?

poiru · 2017-08-10T17:33:34Z

atom/browser/native_browser_view_mac.mm

+                                    webViewHeight)];
+
+  // Then, on top of that, add "exclusion zones"
+  for (std::vector<gfx::Rect>::const_iterator iter =


You can use auto instead of std::vector<gfx::Rect>::const_iterator.

poiru · 2017-08-10T18:19:42Z

atom/browser/native_browser_view_mac.mm

+  NSRect screenFrame = [[NSScreen mainScreen] frame];
+  NSRect windowFrame = [self.window frame];
+
+  currentLocation = [NSEvent mouseLocation];


Maybe we could move these up so that we don't have to leave currentLocation and newOrigin uninitailized. You could also you NSMakeRect to initialize newOrigin in one line.

poiru · 2017-08-10T18:21:11Z

atom/browser/native_browser_view_mac.mm

+}
+
+// Debugging tips:
+// Uncomment the following four lines to color DragRegionView bright red


Maybe this could be in a:

#ifdef DEBUG_DRAG_REGIONS ... #endif

with a comment at the top:

// Uncomment to... //#define DEBUG_DRAG_REGIONS

poiru · 2017-08-10T18:21:38Z

atom/browser/native_window_mac.mm

@@ -1767,6 +1767,12 @@ static bool FromV8(v8::Isolate* isolate, v8::Handle<v8::Value> val,
  std::vector<gfx::Rect> system_drag_exclude_areas =
      CalculateNonDraggableRegions(regions, webViewWidth, webViewHeight);

+  // Call down to a BrowserView, if it exists, and inform it about the


I think this comment is not needed, the code is sufficiently clear.

felixrieseberg · 2017-08-15T23:13:43Z

Thanks a ton for the review, @poiru! I implemented all of it, this should now be "good to go":tm:

MarshallOfSound · 2017-08-19T03:56:46Z

@felixrieseberg You've got a few cpp linting errors

atom/browser/native_browser_view.h:10:  Include "atom/common/draggable_region.h" not in alphabetical order  [build/include_alpha] [4]
atom/browser/native_browser_view.h:11:  Found C++ system header after other header. Should be: native_browser_view.h, c system, c++ system, other.  [build/include_order] [4]
atom/browser/native_browser_view.h:45:  You don't need a ; after a }  [readability/braces] [4]
atom/browser/native_browser_view_mac.h:12:  Include "atom/browser/native_browser_view.h" not in alphabetical order  [build/include_alpha] [4]
atom/browser/native_browser_view_mac.h:25:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
atom/browser/native_browser_view_mac.h:25:  Add #include <vector> for vector<>  [build/include_what_you_use] [4]

felixrieseberg · 2017-08-21T23:53:30Z

@MarshallOfSound: Fixed!

CharlieHess · 2017-08-23T00:36:40Z

@felixrieseberg still got some weirdness here:

obj/atom/browser/electron_lib.native_browser_view_mac.o
../../atom/browser/native_browser_view_mac.mm:42:18: error: instance method '-performWindowDragWithEvent:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
    [self.window performWindowDragWithEvent:event];
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~

felixrieseberg · 2017-08-23T22:58:32Z

Hm, this one is tough. I'd love to use the method, but I'm not sure how to convince the build machines that I want to only use it if it's available.

We can get by without, but the native drag feel is superior.

MarshallOfSound · 2017-08-23T23:12:24Z

@felixrieseberg You'd probably have to use a similar technique to the touchbar forward declarations. Issue is the build machiens SDK is less than 10.11 (I think it's 10.9) so it doesn't know anything about that method. You can forward declare though and then just wrap your method call in a version check

felixrieseberg · 2017-08-23T23:13:53Z

I gave interface extension a try, that should work. @MarshallOfSound Thanks, if that doesn't stick, I'll study your code 👍

MarshallOfSound · 2017-08-24T00:18:38Z

atom/browser/native_browser_view.cc


 #include "atom/browser/api/atom_api_web_contents.h"
+#include "atom/browser/native_browser_view.h"


Suprised linting doesn't complain about this, standard afaik is to have the header file for the current cc file as the first thing in the file (like it was originally). If you put this back at the top I'll approve it 😀

Add -webkit-app-region support to BrowserView

Backport #10232 (drag-browser-view)

felixrieseberg added 4 commits August 8, 2017 17:00

🔧 Draggable (using the window’s regions)

df66223

🔧 Support older versions of macOS

fdd0d67

🔧 Make work with complex shapes

ebe058e

🔧 Properly derive

ae5c8e6

felixrieseberg requested a review from poiru August 9, 2017 22:18

felixrieseberg added 4 commits August 9, 2017 15:21

🔧 Draggable (using the window’s regions)

a5dfb09

🔧 Support older versions of macOS

a55015d

🔧 Make work with complex shapes

7bfece1

🔧 Properly derive

47d652b

mgc reviewed Aug 10, 2017

View reviewed changes

felixrieseberg added 3 commits August 10, 2017 08:00

🔧 Handle offset BrowserView

d913b53

Merge branch 'drag-browser-view' of https://github.com/electron/electron

4c48908

into drag-browser-view

🔧 Spaces for the Spacelord

b4bb008

poiru approved these changes Aug 10, 2017

View reviewed changes

🔧 Implement feedback

6191e6e

felixrieseberg force-pushed the drag-browser-view branch from 67fb3ff to 6191e6e Compare August 15, 2017 23:15

felixrieseberg added component/BrowserView platform/macOS labels Aug 17, 2017

🔧 Linter fascism

f068058

🔪 Bonus colon

cd5cd25

🔧 Extend interface

a44c2d5

MarshallOfSound reviewed Aug 24, 2017

View reviewed changes

🔧 Respect the header

fdfbf3a

MarshallOfSound approved these changes Aug 24, 2017

View reviewed changes

MarshallOfSound merged commit 7ecac42 into master Aug 24, 2017

MarshallOfSound deleted the drag-browser-view branch August 24, 2017 00:31

felixrieseberg pushed a commit that referenced this pull request Aug 28, 2017

Merge pull request #10232 from electron/drag-browser-view

fb7661d

Add -webkit-app-region support to BrowserView

felixrieseberg mentioned this pull request Aug 28, 2017

Backport #10232 (drag-browser-view) #10374

Merged

jkleinsc pushed a commit that referenced this pull request Aug 29, 2017

Merge pull request #10374 from electron/backport-drag-browser-view

2a536d2

Backport #10232 (drag-browser-view)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add -webkit-app-region support to BrowserView #10232

Add -webkit-app-region support to BrowserView #10232

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		#include "atom/browser/api/atom_api_web_contents.h"
		#include "atom/browser/native_browser_view.h"

Add -webkit-app-region support to BrowserView #10232

Add -webkit-app-region support to BrowserView #10232

Uh oh!

Conversation

Uh oh!

NSView Implementation Overview

Why the window's draggable regions?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!