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: Windows Toast notification dismissal from Action Center #40197

Merged
merged 3 commits into from Oct 17, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 13, 2023

Description of Change

Closes #40133

Inspiration taken from upstream

This PR closes a longstanding issue whereby Windows Toast notifications could only be dismissed and prevented from living in the Action Center until removed by the user if .close is called on them while the Notification is visible to the user. On an API level, When ToastNotifier.Hide is called, the ToastNotification is removed from the screen if it is currently visible, and it is not sent to the Action Center. A ToastNotification is considered dismissed per underlying callbacks if it leaves the screen, and so our existing logic destroyed and garbage collected the underlying notification as soon as the notification left the screen regardless of whether it was dismissed programmatically, by the user, or by timeout.

If it was dismissed by the user or by timeout, it entered the Action Center, and notification.close() then appeared to have no effect because the notification in JS-land was still alive. In order to address this problem, and ensure that the notification's behavior aligned more closely with 1) our documentation and 2) other platforms, we need to make some changes to garbage collection behavior as well as which APIs are called depending on whether the ToastNotification has already entered the Action Center. If it is dismissed by timeout or sent to the Action Center by the user, we want to notify the user via event, but now, we do not want to immediately destroy the underlying notification. We instead want to wait until the notification is destroyed programmatically or removed from the action center to destroy the underlying ToastNotification. We use the ToastNotificationHistory class to remove it from the Action Center.

Checklist

Release Notes

Notes: Fixed an issue where Windows Toast notifications weren't properly dismissed from the Action Center on notification.close() if they'd previously been dismissed.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. labels Oct 13, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 13, 2023
@codebytere codebytere force-pushed the fix-notifications-remove-after-close branch 2 times, most recently from ae8c5a3 to f013c0d Compare October 13, 2023 09:09
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 14, 2023
@codebytere codebytere marked this pull request as ready for review October 16, 2023 08:12
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 16, 2023
@codebytere codebytere marked this pull request as draft October 16, 2023 09:38
@codebytere codebytere marked this pull request as ready for review October 16, 2023 12:50
docs/api/notification.md Outdated Show resolved Hide resolved
shell/browser/notifications/notification.h Show resolved Hide resolved
@codebytere codebytere force-pushed the fix-notifications-remove-after-close branch from c18de9b to 6a637fa Compare October 17, 2023 10:16
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 17, 2023
@jkleinsc
Copy link
Contributor

Merging as CI failure is unrelated to PR changed and is fixed via #40241.

@jkleinsc jkleinsc merged commit 666907d into main Oct 17, 2023
17 of 18 checks passed
@jkleinsc jkleinsc deleted the fix-notifications-remove-after-close branch October 17, 2023 23:33
@release-clerk
Copy link

release-clerk bot commented Oct 17, 2023

Release Notes Persisted

Fixed an issue where Windows Toast notifications weren't properly dismissed from the Action Center on notification.close() if they'd previously been dismissed.

@trop
Copy link
Contributor

trop bot commented Oct 17, 2023

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

@trop trop bot removed the target/26-x-y PR should also be added to the "26-x-y" branch. label Oct 17, 2023
@trop
Copy link
Contributor

trop bot commented Oct 17, 2023

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

@trop
Copy link
Contributor

trop bot commented Oct 17, 2023

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

@trop trop bot added in-flight/28-x-y in-flight/27-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed target/28-x-y PR should also be added to the "28-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. in-flight/27-x-y in-flight/28-x-y in-flight/26-x-y labels Oct 17, 2023
felixrieseberg pushed a commit to felixrieseberg/electron that referenced this pull request Oct 25, 2023
…n#40197)

* fix: Windows Toast notification dismissal from Action Center

* docs: note Toast behavior in  event

* chore: address feedback from review
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
…n#40197)

* fix: Windows Toast notification dismissal from Action Center

* docs: note Toast behavior in  event

* chore: address feedback from review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: notification.close() does not work in Windows after the popup has disappeared
3 participants