browser-compat-data icon indicating copy to clipboard operation
browser-compat-data copied to clipboard

Mark iOS Safari's implementation of Notification partial because it requires the site to be saved to the home screen

Open captainbrosset opened this issue 1 year ago • 2 comments

Summary

Add partial_implementation: true for the implementation of the Notification API in Safari iOS.

The reason for this is that those BCD keys already have a note saying: "Notifications are supported in web apps saved to the home screen". Requiring the site to be installed as an app on the iOS home screen is a big requirement for users, which means that most users will not see website notifications.

The right thing to do, to me, is to add the partial_implementation: true flag to make it more visible that the feature doesn't work in Safari for iOS, like it does in other browsers.

captainbrosset avatar Oct 14 '24 12:10 captainbrosset

That sounds reasonable to me. I was asking myself the same question, and just went with what was already there, assuming that this was how we did things. @Elchi3 can you help me understand the best practice for sub-features? Is it fine to mark the top-level feature as partially implemented with a note, and just leave the sub-features clean? Or should the partial status and note be repeated individually for each?

Thinking more about it, repeating does have an importance, for people visiting MDN pages that are only about a sub-feature.

Thoughts?

captainbrosset avatar Oct 16 '24 09:10 captainbrosset

The inconsistencies get even clearer when looking at https://developer.mozilla.org/en-US/docs/Web/API/Notification#browser_compatibility.

For Chrome Android, we can see that Notifications is partially implemented with a note ("Notifications in Chrome for Android are only available through service workers."), but several sub-features are marked as fully implemented without a note nevertheless.

I think that's okay, and just warrants a warning in the BCD table of the sub-feature (https://developer.mozilla.org/en-US/docs/Web/API/Notification/actions#browser_compatibility), e.g. "The parent feature is only partially supported.".

The alternative would be to require that if a feature is partially implemented, then all sub-features must be at most partially implemented as well.

PS: Yet another solution here would be to mark the note on the parent feature to apply to all sub-features. This would require note statements like suggested here.

caugner avatar Oct 16 '24 14:10 caugner

This web-features discussion is related to this PR: https://github.com/web-platform-dx/web-features/pull/1678#discussion_r1808256629.

We would like to create a feature in the web-features' repository that's about "notifications that you can use from a normal webpage, without having to use a service worker or install the app first". The problem we're facing is that there's no BCD key that encapsulates this meaning right now.

In that PR, I'm, suggesting the following:

Create a new BCD key in Notification.json named something like api.Notification.website_main_thread_support.

For Chrome Android, this key would have the following support data:

"chrome_android": {
  "version_added": false,
  "notes": "On Chrome for Android, notifications are only available through service workers."
}

And for Safari iOS:

"safari_ios": {
  "version_added": false,
  "notes": "On Safari for iOS, notifications are only supported in web apps saved to the home screen."
}

If we do this, then I guess the need for marking subfeatures as partially implemented is less important. Users would see where notifications are supported, because there's now a entire BCD key for it.

captainbrosset avatar Oct 21 '24 07:10 captainbrosset

If a sub feature can encapsulate the compat information once and for all, and we can then clean up the notes on the many sub features, it seems like a very promising way forward to me.

Elchi3 avatar Oct 21 '24 10:10 Elchi3

We discussed this on the BCD call today. I suggested that it would be much easier to know whether to create a subfeature or site the partial implementation note in one place, if someone did a little investigation into what the failure mode is for this feature. How they don't work makes a big difference (e.g., no exposure, throwing, silently not showing notifications).

ddbeck avatar Oct 22 '24 10:10 ddbeck

On Chrome for Android, constructing a new Notification object throws an error:

image

On Safari for iOS, I'm seeing the following errors when requesting the permission and then constructing a Notification object:

image

captainbrosset avatar Oct 23 '24 12:10 captainbrosset

OK, in that case here's how I think we should approach this:

Chrome for Android

  • api.Notification.Notification — Set two notes.
    • The first would say something like "This constructor always throws a TypeError exception."
    • The second note would copy the "only available through service workers" note from the parent Notification interface.
  • api.Notification — Drop the "notifications do not work in incognito mode" note, since it's meaningless.
  • api.Notification.requestPermission_static — Still not certain about this one. What does it do? I suspect it's:
    • Set "partial_implementation": true.
    • Add a note saying something like "The permission may be granted, but no notifications may be constructed."
    • Copy the "only available through service workers" note from the parent Notification interface.
  • api.Notification.* (recursively, excluding the constructor and requestPermission method) — Set { "version_added": false }. It doesn't matter if Notification.prototype has all this stuff, if you can never get an instance object.

Safari for iOS

  • api.Notification — Set { "partial_implementation": true } and change the note to say "The Notification interface is undefined unless the web app is saved to the home screen."
  • api.Notification.* (recursively) — Change the notes as described in the previous point.

ddbeck avatar Oct 28 '24 13:10 ddbeck

Thanks Daniel.

Regarding api.Notification.requestPermission_static: we need to keep this as fully supported. This is the code you write when asking users for their permission to send notifications: Notification.requestPermission().then(...). It works from the main thread (no need for a service worker).

captainbrosset avatar Oct 29 '24 13:10 captainbrosset

After more testing, I realized that, on Safari iOS, not only do you need the site to be installed on the home screen, but you also need a manifest to set the app's display mode to standalone (maybe some other values work too, but not the default one), and you can only then send notifications from the service worker, not by using the Notification constructor.

captainbrosset avatar Oct 29 '24 14:10 captainbrosset

Regarding api.Notification.requestPermission_static: we need to keep this as fully supported. This is the code you write when asking users for their permission to send notifications: Notification.requestPermission().then(...). It works from the main thread (no need for a service worker).

When you say it works, does that mean it actually prompts the user for permission to send notifications? Or does it work in that it always resolves? I'm happy with it being marked as non-partial, if it's truly doing something normal.

That said, I'd still like to note for developers on that feature (regardless of how normal it looks), on the theory that they might see the support data for Notification.requestPermission() before they would get to Notification(). They should know it's not a path forward, however normal Notification.requestPermission() appears.

and you can only then send notifications from the service worker, not by using the Notification constructor

Ah! So do I understand correctly that Safari for iOS never exposes the Notification interface, even installed as standalone? Or is there some additional nuance (e.g., it throws with an error like Chrome for Android)?

ddbeck avatar Oct 29 '24 15:10 ddbeck

Re api.Notification.requestPermission_static

When you say it works, does that mean it actually prompts the user for permission to send notifications? Or does it work in that it always resolves? I'm happy with it being marked as non-partial, if it's truly doing something normal.

Notification.requestPermission is the way to request permissions, on all browsers. On Safari, however, it appears that you can only call it when the app is installed to the home screen (with a non-default display mode). This stuff is hard to test and easy to get mixed up about. So, apologies if I mentioned that it worked before. What I meant was that this is the way to request permissions, there's no alternate way to do it with a different API from the service worker. You have to use the Notification interface. It works fine on Chrome for Android by the way. It's just Safari iOS that first requires the app to be installed.

That said, I'd still like to note for developers on that feature (regardless of how normal it looks), on the theory that they might see the support data for Notification.requestPermission() before they would get to Notification(). They should know it's not a path forward, however normal Notification.requestPermission() appears.

Yes, that makes sense. I'll add a note.

Do I understand correctly that Safari for iOS never exposes the Notification interface, even installed as standalone? Or is there some additional nuance (e.g., it throws with an error like Chrome for Android)?

From my tests, it seems like the Notification interface is undefined when the app isn't installed. When the app is installed as standalone, then the interface is defined. You can use it to do Notification.requestPermission or Notification.permission. It appears that you can even do new Notification("foo"), but that doesn't do anything.

captainbrosset avatar Oct 29 '24 15:10 captainbrosset

Notification.requestPermission is the way to request permissions, on all browsers. On Safari, however, it appears that you can only call it when the app is installed to the home screen (with a non-default display mode). This stuff is hard to test and easy to get mixed up about. So, apologies if I mentioned that it worked before. What I meant was that this is the way to request permissions, there's no alternate way to do it with a different API from the service worker. You have to use the Notification interface.

Oh, so do I understand correctly then that even if you're using service worker registration notifications, you still have to use Notification.requestPermission to get permission to do that? I did not grasp this nuance before! I'll re-review with this distinction in mind.

ddbeck avatar Oct 29 '24 15:10 ddbeck

Oh, so do I understand correctly then that even if you're using service worker registration notifications, you still have to use Notification.requestPermission to get permission to do that? I did not grasp this nuance before!

Correct. Before you can send a notification from your SW's registration, you have to request permission first, with Notification.requestPermission.

captainbrosset avatar Oct 29 '24 16:10 captainbrosset

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Nov 04 '24 13:11 github-actions[bot]

I will get back to this PR at some point this week.

captainbrosset avatar Nov 04 '24 16:11 captainbrosset

OK, I think this is very close now. There are two things I'm left concerned about:

First, it looks like the merge pulled in some spurious changes to some unrelated interfaces. Can you back out the changes to everything except Notification.json and ServiceWorkerRegistration.json?

Second, I now understand that I gave you bad advice in https://github.com/mdn/browser-compat-data/pull/24705#issuecomment-2441532754, where I suggested setting the instance properties and methods to false for Chrome for Android and Safari. This was a bad mistake on my part: service workers can get Notification objects through the getNotifications() method. Since this is my fault, I've made a commit to fix it (https://github.com/captainbrosset/browser-compat-data/compare/safari-notification-app...ddbeck:browser-compat-data:undo-recurse), which I can push to your branch or you can merge it into yours, if that looks reasonable to you.

ddbeck avatar Nov 06 '24 12:11 ddbeck

First, it looks like the merge pulled in some spurious changes to some unrelated interfaces. Can you back out the changes to everything except Notification.json and ServiceWorkerRegistration.json?

Sorry about that. I didn't check the Files tab after pushing. My last conflicts merge went a bit sideways, and I must have made a mistake there. I'll clean this up.

captainbrosset avatar Nov 06 '24 15:11 captainbrosset

Second, I now understand that I gave you bad advice in https://github.com/mdn/browser-compat-data/pull/24705#issuecomment-2441532754, where I suggested setting the instance properties and methods to false for Chrome for Android and Safari. This was a bad mistake on my part: service workers can get Notification objects through the getNotifications() method. Since this is my fault, I've made a commit to fix it (https://github.com/captainbrosset/browser-compat-data/compare/safari-notification-app...ddbeck:browser-compat-data:undo-recurse), which I can push to your branch or you can merge it into yours, if that looks reasonable to you.

Thanks. I've pushed your commit to my PR branch.

captainbrosset avatar Nov 06 '24 15:11 captainbrosset

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Nov 18 '24 12:11 github-actions[bot]

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions[bot] avatar Nov 18 '24 16:11 github-actions[bot]