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.

Sign up for GitHub

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

Jump to bottom

fix: context-menu event with BaseWindows #44940

Merged
merged 1 commit into from
Dec 4, 2024
Merged

fix: context-menu event with BaseWindows #44940

merged 1 commit into from
Dec 4, 2024
+79 −27

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Dec 3, 2024 •
edited
Loading

Description of Change

Closes #44898.
Closes #44896.
Closes #44943.

Fixes an possible crash when using draggable regions and BaseWindows to get the context-menu event. My previous fix did not take into account that the WebContentsView was only added as as a sibling view in a BrowserWindow context. On an implementation level menu.popup([options]) already takes a BaseWindows, so also update documentation to reflect this.

Tested with https://gist.github.com/09dd58e289571d3956cc95ea92b7d73a

Checklist

Release Notes

Notes: Fixed an possible crash when using draggable regions and BaseWindows to get the context-menu event.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/31-x-y PR should also be added to the "31-x-y" branch. target/32-x-y PR should also be added to the "32-x-y" branch. target/33-x-y PR should also be added to the "33-x-y" branch. target/34-x-y PR should also be added to the "34-x-y" branch. labels Dec 3, 2024
@codebytere codebytere requested review from ckerr, deepak1556 and a team December 3, 2024 10:55
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 3, 2024
@nikwen nikwen mentioned this pull request Dec 3, 2024
Closed
3 tasks
@cptpcrd cptpcrd mentioned this pull request Dec 3, 2024
Closed
3 tasks
@nikwen
Copy link
Contributor

nikwen commented Dec 3, 2024

In this issue, someone said they still experience a crash despite applying this patch to their local build: #44943

@codebytere
Copy link
Member Author

Ugh yeah it's because of the vibrant view in that issue - i'll figure out a more robust solution.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 4, 2024
@codebytere codebytere marked this pull request as draft December 4, 2024 11:20
@codebytere codebytere requested a review from ckerr December 4, 2024 14:17
@codebytere codebytere marked this pull request as ready for review December 4, 2024 14:17
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 4, 2024
@VerteDinde VerteDinde merged commit 208dc56 into main Dec 4, 2024
57 checks passed
@VerteDinde VerteDinde deleted the fix-context-crash branch December 4, 2024 21:35
@release-clerk Release Clerk
Copy link

release-clerk bot commented Dec 4, 2024

Release Notes Persisted

Fixed an possible crash when using draggable regions and BaseWindows to get the context-menu event.

@trop
Copy link
Contributor

trop bot commented Dec 4, 2024

I was unable to backport this PR to "31-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/31-x-y and removed target/31-x-y PR should also be added to the "31-x-y" branch. labels Dec 4, 2024
This was referenced Dec 4, 2024
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Dec 4, 2024

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

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

trop bot commented Dec 4, 2024

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

@trop
Copy link
Contributor

trop bot commented Dec 4, 2024

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

@trop trop bot added in-flight/33-x-y in-flight/32-x-y and removed target/33-x-y PR should also be added to the "33-x-y" branch. target/32-x-y PR should also be added to the "32-x-y" branch. labels Dec 4, 2024
@nikwen
Copy link
Contributor

nikwen commented Dec 4, 2024

The original patch didn't make it into v31, so no v31 backport is required. See #44761.

@trop trop bot added merged/34-x-y PR was merged to the "34-x-y" branch. merged/33-x-y PR was merged to the "33-x-y" branch. merged/32-x-y PR was merged to the "32-x-y" branch. and removed in-flight/34-x-y in-flight/33-x-y in-flight/32-x-y labels Dec 5, 2024
@samuelmaddock samuelmaddock mentioned this pull request Dec 9, 2024
Closed
3 tasks
This was referenced Dec 21, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VerteDinde VerteDinde VerteDinde approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@ckerr ckerr ckerr approved these changes

@deepak1556 deepak1556 Awaiting requested review from deepak1556

Assignees
No one assigned
Labels
merged/32-x-y PR was merged to the "32-x-y" branch. merged/33-x-y PR was merged to the "33-x-y" branch. merged/34-x-y PR was merged to the "34-x-y" branch. needs-manual-bp/31-x-y new-pr 🌱 PR opened in the last 24 hours semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
5 participants
@codebytere @nikwen @ckerr @jkleinsc @VerteDinde