From 8d41e6ea9be261ca0fb8c011e8e4340d976f0d9e Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Mon, 13 May 2024 11:48:51 +0200 Subject: [PATCH] fix: View reordering on re-addition to same parent (#42116) Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr --- docs/api/view.md | 3 +++ shell/browser/api/electron_api_view.cc | 31 ++++++++++++++++++++++++- shell/browser/api/electron_api_view.h | 2 ++ spec/api-view-spec.ts | 32 ++++++++++++++++++++++++++ spec/api-web-contents-view-spec.ts | 21 ++++++++++++++++- 5 files changed, 87 insertions(+), 2 deletions(-) diff --git a/docs/api/view.md b/docs/api/view.md index 97417137caa1..2f2ec00b2e23 100644 --- a/docs/api/view.md +++ b/docs/api/view.md @@ -48,6 +48,9 @@ Objects created with `new View` have the following instance methods: * `index` Integer (optional) - Index at which to insert the child view. Defaults to adding the child at the end of the child list. +If the same View is added to a parent which already contains it, it will be reordered such that +it becomes the topmost view. + #### `view.removeChildView(view)` * `view` View - Child view to remove. diff --git a/shell/browser/api/electron_api_view.cc b/shell/browser/api/electron_api_view.cc index fb35345e0955..4a454652c3d0 100644 --- a/shell/browser/api/electron_api_view.cc +++ b/shell/browser/api/electron_api_view.cc @@ -181,6 +181,26 @@ View::~View() { delete view_; } +void View::ReorderChildView(gin::Handle child, size_t index) { + view_->ReorderChildView(child->view(), index); + + const auto i = base::ranges::find(child_views_, child.ToV8()); + DCHECK(i != child_views_.end()); + + // If |view| is already at the desired position, there's nothing to do. + const auto pos = std::next( + child_views_.begin(), + static_cast(std::min(index, child_views_.size() - 1))); + if (i == pos) + return; + + if (pos < i) { + std::rotate(pos, i, std::next(i)); + } else { + std::rotate(i, std::next(i), std::next(pos)); + } +} + void View::AddChildViewAt(gin::Handle child, std::optional maybe_index) { // TODO(nornagon): !view_ is only for supporting the weird case of @@ -199,6 +219,15 @@ void View::AddChildViewAt(gin::Handle child, size_t index = std::min(child_views_.size(), maybe_index.value_or(child_views_.size())); + + // If the child is already a child of this view, just reorder it. + // This matches the behavior of View::AddChildViewAtImpl and + // otherwise will CHECK if the same view is added multiple times. + if (child->view()->parent() == view_) { + ReorderChildView(child, index); + return; + } + child_views_.emplace(child_views_.begin() + index, // index isolate(), child->GetWrapper()); // v8::Global(args...) #if BUILDFLAG(IS_MAC) @@ -221,7 +250,7 @@ void View::RemoveChildView(gin::Handle child) { return; if (!child->view()) return; - auto it = std::find(child_views_.begin(), child_views_.end(), child.ToV8()); + const auto it = base::ranges::find(child_views_, child.ToV8()); if (it != child_views_.end()) { #if BUILDFLAG(IS_MAC) ScopedCAActionDisabler disable_animations; diff --git a/shell/browser/api/electron_api_view.h b/shell/browser/api/electron_api_view.h index 1080333c2f5f..708f966c6ba7 100644 --- a/shell/browser/api/electron_api_view.h +++ b/shell/browser/api/electron_api_view.h @@ -57,6 +57,8 @@ class View : public gin_helper::EventEmitter, public views::ViewObserver { void set_delete_view(bool should) { delete_view_ = should; } private: + void ReorderChildView(gin::Handle child, size_t index); + std::vector> child_views_; bool delete_view_ = true; diff --git a/spec/api-view-spec.ts b/spec/api-view-spec.ts index 020f3ce0d6a0..5af674785c82 100644 --- a/spec/api-view-spec.ts +++ b/spec/api-view-spec.ts @@ -22,4 +22,36 @@ describe('View', () => { w.contentView.addChildView(w.contentView); }).to.throw('A view cannot be added as its own child'); }); + + it('does not crash when attempting to add a child multiple times', () => { + w = new BaseWindow({ show: false }); + const cv = new View(); + w.setContentView(cv); + + const v = new View(); + w.contentView.addChildView(v); + w.contentView.addChildView(v); + w.contentView.addChildView(v); + + expect(w.contentView.children).to.have.lengthOf(1); + }); + + it('correctly reorders children', () => { + w = new BaseWindow({ show: false }); + const cv = new View(); + w.setContentView(cv); + + const v1 = new View(); + const v2 = new View(); + const v3 = new View(); + w.contentView.addChildView(v1); + w.contentView.addChildView(v2); + w.contentView.addChildView(v3); + + expect(w.contentView.children).to.deep.equal([v1, v2, v3]); + + w.contentView.addChildView(v1); + w.contentView.addChildView(v2); + expect(w.contentView.children).to.deep.equal([v3, v1, v2]); + }); }); diff --git a/spec/api-web-contents-view-spec.ts b/spec/api-web-contents-view-spec.ts index cbe310b9e9d6..70447ad39969 100644 --- a/spec/api-web-contents-view-spec.ts +++ b/spec/api-web-contents-view-spec.ts @@ -36,6 +36,25 @@ describe('WebContentsView', () => { v.removeChildView(wcv); }); + it('correctly reorders children', () => { + const w = new BaseWindow({ show: false }); + const cv = new View(); + w.setContentView(cv); + + const wcv1 = new WebContentsView(); + const wcv2 = new WebContentsView(); + const wcv3 = new WebContentsView(); + w.contentView.addChildView(wcv1); + w.contentView.addChildView(wcv2); + w.contentView.addChildView(wcv3); + + expect(w.contentView.children).to.deep.equal([wcv1, wcv2, wcv3]); + + w.contentView.addChildView(wcv1); + w.contentView.addChildView(wcv2); + expect(w.contentView.children).to.deep.equal([wcv3, wcv1, wcv2]); + }); + function triggerGCByAllocation () { const arr = []; for (let i = 0; i < 1000000; i++) { @@ -79,7 +98,7 @@ describe('WebContentsView', () => { expect(await v.webContents.executeJavaScript('initialVisibility')).to.equal('hidden'); }); - it('becomes visibile when attached', async () => { + it('becomes visible when attached', async () => { const v = new WebContentsView(); await v.webContents.loadURL('about:blank'); expect(await v.webContents.executeJavaScript('document.visibilityState')).to.equal('hidden');