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 <shelley.vohr@gmail.com>

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.

shell/browser/api/electron_api_view.cc

@@ -181,6 +181,26 @@ View::~View() {
     delete view_;
 }
 
+void View::ReorderChildView(gin::Handle<View> 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<ptrdiff_t>(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<View> child,
                           std::optional<size_t> maybe_index) {
   // TODO(nornagon): !view_ is only for supporting the weird case of
@@ -199,6 +219,15 @@ void View::AddChildViewAt(gin::Handle<View> 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<View> 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;

shell/browser/api/electron_api_view.h

@@ -57,6 +57,8 @@ class View : public gin_helper::EventEmitter<View>, public views::ViewObserver {
   void set_delete_view(bool should) { delete_view_ = should; }
 
  private:
+  void ReorderChildView(gin::Handle<View> child, size_t index);
+
   std::vector<v8::Global<v8::Object>> child_views_;
 
   bool delete_view_ = true;

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]);
+  });
 });

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