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: fire close event upon closing modal BrowserWindow in macOS #19014

Merged
merged 3 commits into from Jul 1, 2019

Conversation

@erickzhao
Copy link
Member

erickzhao commented Jun 27, 2019

Description of Change

Fixes #10674. This bug was originally reported for Electron 1.x.x, but still occurs on master. See #10674 (comment) for screenshot and Fiddle link.

Adds a small function call to windowShouldClose(_:) to emit the close event upon closing a modal window.

Context

For modals on macOS, we use NSWindow close() instead of NSWindow performClose(_:) to close the modal window because the latter doesn't get along with the Sheet implementation of modal windows.

According to the docs:

The close method differs in two important ways from the performClose(_:) method:

  • It does not attempt to send a windowShouldClose(_:) message to the window or its delegate.

  • It does not simulate the user clicking the close button by momentarily highlighting the button.

The windowShouldClose(_:) function in atom_ns_window_delegate.mm eventually triggers an Emit("close").

Other information

I added the call manually inside the control flow right before [window_ close_]. I also tried adding it into the CloseImmediately() function first (see first commit in this branch), but that didn't work.

Pings

cc @codebytere @deermichel (and @zcbenz who worked on the original lines of code)

Checklist

Release Notes

Notes: Fixed bug where the close event would not emit upon closing modal window on macOS.

@erickzhao erickzhao requested a review from codebytere Jun 27, 2019
@erickzhao erickzhao changed the title fix: fire close event upon fix: fire close event upon closing modal BrowserWindow in macOS Jun 27, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 28, 2019
@zcbenz
zcbenz approved these changes Jul 1, 2019
@zcbenz zcbenz merged commit cc223d7 into master Jul 1, 2019
13 checks passed
13 checks passed
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190628.21 succeeded
Details
electron-arm64-testing Build #20190628.21 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 1, 2019

Release Notes Persisted

Fixed bug where the close event would not emit upon closing modal window on macOS.

@zcbenz zcbenz deleted the intern/fix-modal-close-event-mac branch Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.