-
Notifications
You must be signed in to change notification settings - Fork 116
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
[User model] [Fix] macOS Safari prompting and subscribing #1062
[User model] [Fix] macOS Safari prompting and subscribing #1062
Conversation
This check is no longer needed as the session manager and model repo handle these checks. The original motivation to remove this is to address a macOS Safari notification permission prompting issue where you can't await on I/O, otherwise the prompting is flagged not done from an end-user interaction. The isPushNotificationsEnabled() does disk I/O which we removed in this commit.
Since we no longer support the os.tc workaround there is no need to check the the indexDb for user preferences. Native notification permission status is checked later making this unnecessary as well from normal HTTPS setups.
Corrected missing logic changes that PR #1002 introduced in main but were not accounted for in the last rebase. Tested on masOS 13.3.1 with Safari 16.4 to confirm the browser subscribes to push correctly with the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditionally approved
/* | ||
Within the same page navigation (the same session), do not register for | ||
push if the user is already subscribed, otherwise the user will have its | ||
session count incremented on each page refresh. However, if the user is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you confirm the session count is not incrementing even after the refresh? What is the impact on other browsers? From first glance it doesn't appear this code was Safari-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the "Tests" section in this PR to provide more details on this.
Description
1 Line Summary
Fix macOS Safari error "Push notification prompting can only be done from a user gesture." when prompting for notification permission and registering for push.
Details
window.safari.pushNotification.requestPermission()
) doesn't allow awaiting for any I/O which was causing the prompting error. Commits:Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of array
syntax. PreferforEach
or usemap
context
if possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.context
Screenshots
Info
Checklist
Related Tickets
This change is