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

Add Declarative Web Push #385

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from
Open

Add Declarative Web Push #385

wants to merge 3 commits into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Aug 26, 2024

This introduces a new feature whereby push messages conforming to a certain JSON format directly create an end user notification and show it (possibly preceded by an enhanced push event).

In addition to showing a notification, the app badge can be updated as well.

This builds on whatwg/notifications#213 which adds URL members to notifications.

Exposing PushManager outside of service workers is #393.


The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementer support:

  • Chromium
  • Gecko
  • WebKit

Preview | Diff

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see more explanatory text attached to this sort of change. (In the spec, not the PR, for avoidance of doubt.)

From what I can see, a message is opportunistically parsed as JSON. If it parses and there is a "web_push": 9001 (?!) attribute, the browser attempts to make a notification. If that works, the notification is shown.

There is also a mutable attribute attached, which would allow the SW the option of intercepting the notification and tweaking it before it is shown. That would somewhat negate the benefits from a purely declarative notification, so it seems an unnecessary feature (the app could save the "web_push": 9001 bytes and just make a notification for itself).

index.html Outdated
Comment on lines 1537 to 1521
If a notification has been shown through {{showNotification()}} at this
point, then abort these steps.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems non-ideal. I don't know how this is supposed to work, but this requires a trap in showNotification to track, but you aren't monkey-patching that. Does preventDefault not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this statement is monkey patching that. Certainly seems reasonable to tidy it up though. (preventDefault() wouldn't guarantee that a notification is shown.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once whatwg/notifications#227 lands this can be checked here instead of showNotification(). Does that seem reasonable? Anything else remaining?

This introduces a new feature whereby push messages conforming to a certain JSON format directly create an end user notification and show it (possibly preceded by an enhanced push event).

In addition to showing a notification, the app badge can be updated as well.

This builds on whatwg/notifications#213 which adds URL members to notifications.

Exposing PushManager outside of service workers is handled by #393.
</p>
</dd>
<dt>
<code>require_interaction</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a strong reason to go for snake_case? I find the case is the only deviation from NotificationOptions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whatwg/infra#642 goes into the high-level thinking behind this. https://w3ctag.github.io/design-principles/#casing-rules has the relevant guideline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If the casing was same the parsing step could just say "parse it as NotificationOptions" after extracting title first. And then updating notification options wouldn't require updating both specs.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I did that initially and then based on feedback ended up here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorta understandable from infra point but I guess the callers would have to deal with this casing in that case, like:

onpush = async (ev) => {
  // Err, our little cute browser doesn't support declarative web push,
  // so we are here for nice progressive web.
  const { notification } = await ev.data.json();
  
  // This will mostly work until you use requireInteraction.
  // await registration.showNotification(notification.title, notification);
  
  const notificationOptions = {
    ...notification,
    requireInteraction: notification.require_interaction, // 😅
  };
  await registration.showNotification(notification.title, notificationOptions);
};

I wonder it would make sense to have an exception here to prevent extra boilerplate and head-scratching?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can escalate, but I'm inclined to accept this inconvenience: w3ctag/design-principles#554.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that as a server library tracking this proposal, my adoption documentation looks similarly to @saschanaz's, and I'd similarly push for aligning the property names so they could be passed as-is to showNotification(), if only to easy the migration story the community would need to push for to get this adopted:

self.addEventListener('push', function(event) {
    const data = event.data?.json() ?? {};
    event.waitUntil((async () => {
        const notification = data.notification ?? {}
        /// Try parsing the data, otherwise use fallback content. DO NOT skip sending the notification, as you must display one for every push message that is received or your subscription will be dropped.
        const title = notification.title ?? "Your App Name";
        const body = notification.body ?? "New Content Available!";
        
        await self.registration.showNotification(title, { 
            body,
            requireInteraction: notification.require_interaction ?? false,
            ...notification,
        });
    })());
});

</p>
</dd>
<dt>
<code>require_interaction</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If the casing was same the parsing step could just say "parse it as NotificationOptions" after extracting title first. And then updating notification options wouldn't require updating both specs.)

Copy link
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split mutable out of this PR for now?

@@ -56,7 +56,8 @@
};
</script>
</head>
<body data-cite="service-workers FILEAPI secure-contexts hr-time permissions ECMASCRIPT">
<body data-cite=
"service-workers FILEAPI secure-contexts hr-time permissions ECMASCRIPT NOTIFICATIONS BADGING">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, is this case sensitive or can we make things consistent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with ReSpec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoscaceres, do you know?

</p>
</dd>
<dt>
<code>require_interaction</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorta understandable from infra point but I guess the callers would have to deal with this casing in that case, like:

onpush = async (ev) => {
  // Err, our little cute browser doesn't support declarative web push,
  // so we are here for nice progressive web.
  const { notification } = await ev.data.json();
  
  // This will mostly work until you use requireInteraction.
  // await registration.showNotification(notification.title, notification);
  
  const notificationOptions = {
    ...notification,
    requireInteraction: notification.require_interaction, // 😅
  };
  await registration.showNotification(notification.title, notificationOptions);
};

I wonder it would make sense to have an exception here to prevent extra boilerplate and head-scratching?

</dt>
<dd>
<p>
Any JSON value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how many existing callers set binary data (arraybuffer/blob etc) here? One can still use base64 though (and Gecko serializes it to base64 anyway).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but there should be a variety of workarounds indeed.

</li>
<li>
<p>
If |notificationShown| is false, then run the [=notification show steps=]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed during an internal discussion; this would involve cookies as current implemented in Firefox, and clearing site data would mean the cookie jar would become empty, which may be significant if the image URL can only be fetched with proper cookie (imagine an image attachment from a DM). Should we accept such breakage? It seems like that should be somehow covered in the spec at least. (related: whatwg/notifications#5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants