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: handle Notification requireInteraction option #36477

Merged
merged 1 commit into from Nov 30, 2022
Merged

fix: handle Notification requireInteraction option #36477

merged 1 commit into from Nov 30, 2022

Conversation

jeremyspiegel
Copy link
Contributor

@jeremyspiegel jeremyspiegel commented Nov 29, 2022 •
edited

Description of Change

This makes it so that HTML5 Notifications with the requireInteraction flag will not timeout/disappear automatically, taking advantage of the work done in #20153 for Linux/Windows. cc @codebytere @MarshallOfSound

Checklist

Release Notes

Notes: Changed HTML5 Notifications created with the requireInteraction option to not timeout on Linux and Windows.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 29, 2022
@jeremyspiegel
Copy link
Contributor Author

Windows build failed with:

Uploading artifact mksnapshot.zip (80,469,231 bytes)...10%Error uploading artifact to the storage: One or more errors occurred. (The SSL connection could not be established, see inner exception. Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host..)
Command "appveyor PushArtifact out\Default\mksnapshot.zip" failed with exit code 2. Retrying 2 of 3
Uploading artifact mksnapshot.zip (80,469,231 bytes)...10%Error uploading artifact to the storage: One or more errors occurred. (The SSL connection could not be established, see inner exception. Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host..)
Command "appveyor PushArtifact out\Default\mksnapshot.zip" failed with exit code 2. Retrying 3 of 3
Uploading artifact mksnapshot.zip (80,469,231 bytes)...10%Error uploading artifact to the storage: One or more errors occurred. (The SSL connection could not be established, see inner exception. Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host..)
Sorry, we tried running command for 3 times and all attempts were unsuccessful!
Command exited with code 2

The macOS tests are failing due to a couple fullscreen state tests that fail before my change as well.

nornagon
nornagon approved these changes Nov 29, 2022
View reviewed changes
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.

seems legit! looks like this still isn't supported on macOS, right?

@nornagon nornagon added target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. semver/patch backwards-compatible bug fixes labels Nov 29, 2022
@jeremyspiegel
Copy link
Contributor Author

No this still doesn't support macOS. I did some digging and I believe that Chrome supports this Notification option on macOS by sending banner (non-sticky) notifications through the main browser app and alert (sticky) notifications through a Google Chrome Helper (Alerts).app helper whose Info.plist contains a NSUserNotificationAlertStyle of alert. That helper was added in this change as part of this bug. It is not built automatically by Chromium content embedders like Electron, but is instead in the main Chrome build.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 30, 2022
@jkleinsc jkleinsc merged commit 665cf03 into electron:main Nov 30, 2022
@release-clerk
Copy link

release-clerk bot commented Nov 30, 2022

Release Notes Persisted

Changed HTML5 Notifications created with the requireInteraction option to not timeout on Linux and Windows.

@trop trop bot mentioned this pull request Nov 30, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Nov 30, 2022

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

@trop trop bot mentioned this pull request Nov 30, 2022
Merged
@trop trop bot removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Nov 30, 2022
@trop trop bot mentioned this pull request Nov 30, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Nov 30, 2022

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

@trop
Copy link
Contributor

trop bot commented Nov 30, 2022

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

@trop trop bot added in-flight/21-x-y and removed target/23-x-y PR should also be added to the "23-x-y" branch. target/21-x-y PR should also be added to the "21-x-y" branch. labels Nov 30, 2022
trop bot added a commit that referenced this pull request Dec 1, 2022
@trop @jeremyspiegel
fix: handle Notification requireInteraction option (#36477)
202d91e 
Co-authored-by: Jeremy Spiegel <jeremy.spiegel@frontapp.com>
trop bot added a commit that referenced this pull request Dec 1, 2022
@trop @jeremyspiegel
fix: handle Notification requireInteraction option (#36477)
70b3e12 
Co-authored-by: Jeremy Spiegel <jeremy.spiegel@frontapp.com>
@trop trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Dec 5, 2022
@trop trop bot added merged/22-x-y PR was merged to the "22-x-y" branch. and removed in-flight/22-x-y labels Jan 10, 2023
@trop trop bot added merged/23-x-y PR was merged to the "23-x-y" branch. and removed in-flight/23-x-y labels Feb 7, 2023
@Soltus Soltus mentioned this pull request Feb 16, 2023
Closed
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@jeremyspiegel @khalwa
fix: handle Notification requireInteraction option (electron#36477)
d357fe3
@commit-lint commit-lint bot mentioned this pull request Mar 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

Assignees
No one assigned
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jeremyspiegel @nornagon @jkleinsc