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

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

Merged
merged 16 commits into from Aug 24, 2017

Conversation

Projects
None yet
5 participants
@felixrieseberg
Member

felixrieseberg commented Aug 9, 2017

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 😉

@felixrieseberg felixrieseberg requested a review from poiru Aug 9, 2017

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

This comment has been minimized.

@mgc

mgc Aug 10, 2017

Contributor

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?

@poiru

poiru approved these changes Aug 10, 2017

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

@@ -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);

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

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.

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

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

Is this needed?

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

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

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

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

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

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.

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

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

Maybe this could be in a:

#ifdef DEBUG_DRAG_REGIONS
...
#endif

with a comment at the top:

// Uncomment to...
//#define DEBUG_DRAG_REGIONS
@@ -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

This comment has been minimized.

@poiru

poiru Aug 10, 2017

Member

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

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Aug 15, 2017

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

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 19, 2017

@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

This comment has been minimized.

Member

felixrieseberg commented Aug 21, 2017

@CharlieHess

This comment has been minimized.

Contributor

CharlieHess commented Aug 23, 2017

@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

This comment has been minimized.

Member

felixrieseberg commented Aug 23, 2017

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

This comment has been minimized.

Member

MarshallOfSound commented Aug 23, 2017

@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

This comment has been minimized.

Member

felixrieseberg commented Aug 23, 2017

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

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

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 24, 2017

Member

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 😀

@MarshallOfSound MarshallOfSound merged commit 7ecac42 into master Aug 24, 2017

0 of 5 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-mas-x64 Build #4936 in progress...
Details
electron-osx-x64 Build #4923 in progress...
Details
electron-win-ia32 Build #3953 in progress...
Details

@MarshallOfSound MarshallOfSound deleted the drag-browser-view branch Aug 24, 2017

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

Merge pull request #10232 from electron/drag-browser-view
Add -webkit-app-region support to BrowserView

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment