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>
This commit is contained in:
trop[bot] 2024-05-13 11:48:51 +02:00 committed by GitHub
parent e282ff6e4a
commit 8d41e6ea9b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 87 additions and 2 deletions

View file

@ -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.

View file

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

View file

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

View file

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

View file

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