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

fix: ensure BrowserView bounds are always relative to window #39851

Merged
merged 1 commit into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions shell/browser/api/electron_api_base_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -773,10 +773,20 @@ void BaseWindow::AddBrowserView(gin::Handle<BrowserView> browser_view) {
browser_view->SetOwnerWindow(nullptr);
}

// If the user set the BrowserView's bounds before adding it to the window,
// we need to get those initial bounds *before* adding it to the window
// so bounds isn't returned relative despite not being correctly positioned
// relative to the window.
auto bounds = browser_view->GetBounds();

window_->AddBrowserView(browser_view->view());
window_->AddDraggableRegionProvider(browser_view.get());
browser_view->SetOwnerWindow(this);
browser_views_[browser_view->ID()].Reset(isolate(), browser_view.ToV8());

// Recalibrate bounds relative to the containing window.
if (!bounds.IsEmpty())
browser_view->SetBounds(bounds);
}
}

Expand Down
5 changes: 3 additions & 2 deletions shell/browser/api/electron_api_browser_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ class BrowserView : public gin::Wrappable<BrowserView>,
BrowserView(const BrowserView&) = delete;
BrowserView& operator=(const BrowserView&) = delete;

gfx::Rect GetBounds();
void SetBounds(const gfx::Rect& bounds);

protected:
BrowserView(gin::Arguments* args, const gin_helper::Dictionary& options);
~BrowserView() override;
Expand All @@ -78,8 +81,6 @@ class BrowserView : public gin::Wrappable<BrowserView>,

private:
void SetAutoResize(AutoResizeFlags flags);
void SetBounds(const gfx::Rect& bounds);
gfx::Rect GetBounds();
void SetBackgroundColor(const std::string& color_name);
v8::Local<v8::Value> GetWebContents(v8::Isolate*);

Expand Down
22 changes: 12 additions & 10 deletions shell/browser/native_browser_view_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,27 @@
auto* iwc_view = GetInspectableWebContentsView();
if (!iwc_view)
return;

auto* view = iwc_view->GetNativeView().GetNativeNSView();
auto* superview = view.superview;
const auto superview_height = superview ? superview.frame.size.height : 0;
view.frame =
NSMakeRect(bounds.x(), superview_height - bounds.y() - bounds.height(),
bounds.width(), bounds.height());
const auto superview_height =
view.superview ? view.superview.frame.size.height : 0;
int y_coord = superview_height - bounds.y() - bounds.height();

view.frame = NSMakeRect(bounds.x(), y_coord, bounds.width(), bounds.height());
}

gfx::Rect NativeBrowserViewMac::GetBounds() {
auto* iwc_view = GetInspectableWebContentsView();
if (!iwc_view)
return gfx::Rect();

NSView* view = iwc_view->GetNativeView().GetNativeNSView();
const int superview_height =
(view.superview) ? view.superview.frame.size.height : 0;
return gfx::Rect(
view.frame.origin.x,
superview_height - view.frame.origin.y - view.frame.size.height,
view.frame.size.width, view.frame.size.height);
view.superview ? view.superview.frame.size.height : 0;
int y_coord = superview_height - view.frame.origin.y - view.frame.size.height;

return gfx::Rect(view.frame.origin.x, y_coord, view.frame.size.width,
view.frame.size.height);
}

void NativeBrowserViewMac::SetBackgroundColor(SkColor color) {
Expand Down
45 changes: 45 additions & 0 deletions spec/api-browser-view-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,41 @@ describe('BrowserView module', () => {
view.setBounds({} as any);
}).to.throw(/conversion failure/);
});

it('can set bounds after view is added to window', () => {
view = new BrowserView();

const bounds = { x: 0, y: 0, width: 50, height: 50 };

w.addBrowserView(view);
view.setBounds(bounds);

expect(view.getBounds()).to.deep.equal(bounds);
});

it('can set bounds before view is added to window', () => {
view = new BrowserView();

const bounds = { x: 0, y: 0, width: 50, height: 50 };

view.setBounds(bounds);
w.addBrowserView(view);

expect(view.getBounds()).to.deep.equal(bounds);
});

it('can update bounds', () => {
view = new BrowserView();
w.addBrowserView(view);

const bounds1 = { x: 0, y: 0, width: 50, height: 50 };
view.setBounds(bounds1);
expect(view.getBounds()).to.deep.equal(bounds1);

const bounds2 = { x: 0, y: 150, width: 50, height: 50 };
view.setBounds(bounds2);
expect(view.getBounds()).to.deep.equal(bounds2);
});
});

describe('BrowserView.getBounds()', () => {
Expand All @@ -156,6 +191,16 @@ describe('BrowserView module', () => {
view.setBounds(bounds);
expect(view.getBounds()).to.deep.equal(bounds);
});

it('does not changer after being added to a window', () => {
view = new BrowserView();
const bounds = { x: 10, y: 20, width: 30, height: 40 };
view.setBounds(bounds);
expect(view.getBounds()).to.deep.equal(bounds);

w.addBrowserView(view);
expect(view.getBounds()).to.deep.equal(bounds);
});
});

describe('BrowserWindow.setBrowserView()', () => {
Expand Down