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: View reordering on re-addition to same parent #42085

Merged
merged 1 commit into from
May 10, 2024

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented May 8, 2024

Description of Change

Closes #42077.
Closes #41974.
Closes #42061.

When the same view is added multiple times, we should match Chromium's behavior in View::AddChildViewAtImpl and re-order the view such that the view becomes the topmost view. Also adds tests.

Checklist

Release Notes

Notes: Fixed a crash when the same WebContentsView is added via addChildView multiple times.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels May 8, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 8, 2024
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB https://issues.chromium.org/issues/41447071 intends to change this behavior in Chromium, but since this neatly fixes #42061 with no API change, seems fine. We should perhaps consider directly exposing ReorderChildView in a followup.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 9, 2024
@codebytere codebytere force-pushed the match-upstream-view-reordering branch from 0728b7f to d1d49f2 Compare May 9, 2024 17:21
@codebytere codebytere merged commit 3bd807b into main May 10, 2024
16 checks passed
@codebytere codebytere deleted the match-upstream-view-reordering branch May 10, 2024 08:16
Copy link

release-clerk bot commented May 10, 2024

Release Notes Persisted

Fixed a crash when the same WebContentsView is added via addChildView multiple times.

@trop
Copy link
Contributor

trop bot commented May 10, 2024

I have automatically backported this PR to "30-x-y", please check out #42115

@trop trop bot added in-flight/30-x-y and removed target/30-x-y PR should also be added to the "30-x-y" branch. labels May 10, 2024
@trop
Copy link
Contributor

trop bot commented May 10, 2024

I have automatically backported this PR to "31-x-y", please check out #42116

@trop trop bot added in-flight/31-x-y merged/31-x-y PR was merged to the "31-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. and removed target/31-x-y PR should also be added to the "31-x-y" branch. in-flight/31-x-y in-flight/30-x-y labels May 10, 2024
Mrnikifabio pushed a commit to Mrnikifabio/electron that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/30-x-y PR was merged to the "30-x-y" branch. merged/31-x-y PR was merged to the "31-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
2 participants