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

Allow notifications and actions to specify a navigable URL #213

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Jul 10, 2024

This makes notifications more declarative by not requiring explicit handling of the clicks by the web application.


This change is part of a larger Declarative Web Push effort, but can hopefully land in isolation as a small incremental step toward that goal.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

notifications.bs Outdated Show resolved Hide resolved
@annevk annevk marked this pull request as ready for review July 17, 2024 09:45
@annevk
Copy link
Member Author

annevk commented Jul 17, 2024

@saschanaz @beverloo thoughts?

@saschanaz

This comment was marked as resolved.

@annevk

This comment was marked as resolved.

@saschanaz

This comment was marked as resolved.

@annevk

This comment was marked as resolved.

@saschanaz
Copy link
Member

1 or 2 minor changes are fine but getting more starts to be annoying, especially if we get multiple review rounds... 😛

Should we also fold in some part of #204 here? Non-persistent notification with a URL certainly has a valid action to do even if the page is closed.

@saschanaz
Copy link
Member

(Interesting that we are currently not telling what action is invoked for non-persistent notifications, per the existing steps)

@saschanaz
Copy link
Member

A parallel question, is WebKit going to implement actions?

@annevk
Copy link
Member Author

annevk commented Jul 17, 2024

@saschanaz
Copy link
Member

Some extra questions:

  1. Should we still give a chance for the webpage to handle this, so that existing active tab can e.g. open notification inbox without opening a new tab? Doing that might be a bit tricky though:

    1. Tab A triggers a notification with URL
    2. Tab A goes away
    3. User opens tab B and C
    4. User clicks notification
    5. Now which tab should respond? 🤷‍♀️

    cc @smaug----

  2. We silently ignore URL parsing error here (probably for consistency with other fields), but should we throw explicitly for this one? Clicking notification with incorrect URL wouldn't open anything, and such thing probably should fail early without being shown.

@annevk
Copy link
Member Author

annevk commented Aug 12, 2024

  1. We could potentially offer that in a future version, but I don't think we need to for v1. Let's see how much adoption there will be for Declarative Web Push first.
  2. Hmm, per 0f8a7b6 it seems historically we didn't really define what happened on failure and just copy-and-pasted our way forward from there (with "minor" changes along the way such as adopting the WHATWG URL parser). I suppose we could start breaking with that precedent now and start throwing for new URL members. Not sure I lean particularly strongly either way.

@annevk annevk added the agenda+ To be discussed at a triage meeting label Aug 21, 2024
@smaug----
Copy link

Having a way to bound to a tab might affect the API usage quite a bit. Needing to open a tab all the time even if the site was already open would be a bit annoying.

But perhaps you've had some other types of use cases in mind. In which way do you imagine one to use only the URL + new top level navigable?

@annevk
Copy link
Member Author

annevk commented Aug 23, 2024

This feature is part of w3c/push-api#360 to enable fully declarative push notifications. Perhaps it should not be allowed for new Notification(), but it also seemed kinda harmless. And perhaps we should also offer a variant that ends up navigating an existing tab instead, if any, but that seems quite a bit more involved.

cc @beidson

@saschanaz
Copy link
Member

We should probably also consider not always opening a new tab for declarative push, btw.

@annevk
Copy link
Member Author

annevk commented Aug 23, 2024

Yeah we'll think about it and are of course also happy to consider concrete proposals. I suspect that anything we decide to do can easily go on top of this, so I'm not sure I'd consider it blocking.

@annevk
Copy link
Member Author

annevk commented Aug 23, 2024

Brady pointed out to me that we already reuse an existing tab if there is one. I don't think HTML has a good primitive for that, but I'll see about adjusting the wording to allow for that. Thanks for pointing it out! (I guess that raises the question as to whether there should be a feature to force a new window, but that's probably best deferred.)

@saschanaz
Copy link
Member

saschanaz commented Aug 23, 2024

Hmmmm, in that case I would be interested to learn more about the tab selection algorithm.

annevk added a commit to w3c/push-api that referenced this pull request 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 a new pushnotification 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 will be done in a separate change.
@beverloo
Copy link
Member

beverloo commented Sep 5, 2024

Hey Anne, this seems good to us. I've two thoughts -

  1. I don't love the url property name, but I can't really come up with anything much better either. Using either navigationUrl or activationUrl is much longer, and inconsistent with other properties on the notification objects.
  2. +1 to @saschanaz that some sort of "how to open this" policy is necessary. We see three categories, (1) always open a new tab, (2) re-use an existing tab, and (3) re-use an existing tab unless it's been modified, e.g. dirty form fields.

Perhaps navigationUrl is an option to consider as it would work with navigationPolicy, which then could then be an enum of sorts. Doesn't have to be for this PR either, and developer feedback might highlight additional options to consider.

@beverloo
Copy link
Member

beverloo commented Sep 5, 2024

Also - we don't have good metrics on use between Clients.openWindow() and WindowClient.navigate() (or WindowClient.focus()) unfortunately, which I can reintroduce but it'll be a while before we have data.

@annevk
Copy link
Member Author

annevk commented Sep 6, 2024

I think the policy we end up using in Safari is roughly reuse a tab with the same URL. There's a number of corner cases to imagine where such a vague notion could fall apart, but I'm also not sure we should try to nail it down in detail as I think we want this to be a little flexible to allow changes to it in the future and also because it fundamentally is also part of the overall end user experience.

I could see renaming url to navigate so it's analogous with the Navigation API. @domenic thoughts?

(Also, thanks a lot for sharing your thoughts Peter, much appreciated! And I hope all is well.)

@domenic
Copy link
Member

domenic commented Sep 7, 2024

I could see renaming url to navigate so it's analogous with the Navigation API. @domenic thoughts?

No strong feelings, but that (or navigationURL) seems reasonable.

For the internal slot, it does seem that something more specific than "URL" would be helpful for clarity.

@past past removed the agenda+ To be discussed at a triage meeting label Sep 11, 2024
annevk added a commit to whatwg/html that referenced this pull request Oct 22, 2024
@asutherland
Copy link

I think the policy we end up using in Safari is roughly reuse a tab with the same URL. There's a number of corner cases to imagine where such a vague notion could fall apart, but I'm also not sure we should try to nail it down in detail as I think we want this to be a little flexible to allow changes to it in the future and also because it fundamentally is also part of the overall end user experience.

Now that URLPattern is an available primitive, one could imagine letting the site provide a (bounded) list of URLPatterns in precedence order to help with the tab selection.

@saschanaz
Copy link
Member

saschanaz commented Oct 22, 2024

Might want to block some tabs from being navigated, in case the state can't be recovered there. (Some form with no backing storage, or some in-progress page either for authentication or payment, etc.)

Edit: Or a safelist, maybe passing a safelist can be a signal for an explicit opt-in for existing tab navigation (and open a new tab by default without it)

@annevk
Copy link
Member Author

annevk commented Oct 22, 2024

Those seem like reasonable ideas, but I think we should only consider adding complexity once we see significant web developer demand for more fine-grained control.

@saschanaz
Copy link
Member

saschanaz commented Oct 22, 2024

dictionary NotificationOptions {
  navigate: {
    url: (URL),
    onExistingTab: (boolean) // initially, and in the future give URLPattern?
  } 
}

Might make sense to make navigate a nested dictionary in case more options come.

@annevk
Copy link
Member Author

annevk commented Oct 22, 2024

That seems a little premature? We could always overload once we establish we need the complexity, no? "Baby steps" and all that.

@saschanaz
Copy link
Member

If we are going to only ship one extra member, sure. I wonder we should make the tab selection optional if we are going to do an implementation detailed magic there though.

domenic pushed a commit to whatwg/html that referenced this pull request Oct 23, 2024
<var>navigationURL</var> with <a for=url/equals><i>exclude fragments</i></a> set to true, then
<a for=/>navigate</a> <var>traversable</var> to <var>navigationURL</var> and return. If there are
multiple <a for=/>top-level traversables</a> that satisfy the condition the user agent has to
pick one in an <a>implementation-defined</a> manner.
Copy link
Member

@saschanaz saschanaz Oct 24, 2024

Choose a reason for hiding this comment

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

Given "losing the data by accidental navigation" is an existing problem with existing solution, Mozilla is against letting this fully implementation defined. We prefer to giving some basic heuristic to prevent it (using beforunload as an opt-out signal, for example), unless there's a proof that selecting random traversable will not cause a problem.

Here's our suggested revision:

  1. Let |traversables| be the user agent’s top-level traversable set whose active document’s URL equals |navigationURL| with exclude fragments set to true. (This part is copied from the PR, but see the above comment)
  2. Let |selected| be null.
  3. For each |traversable| in |traversables|,
    1. If |traversable| needs beforeunload, continue.
    2. Let |activeDocument| be |traversable|'s active document.
    3. Let |focusState| be the result of running the [has focus steps] with |activeDocument| as the argument.
    4. If |focusState| is true, let |selected| be |traversable| and break.
    5. If |activeDocument|'s visibility state is "visible", let |selected| be |traversable| and break.
  4. If |selected| is null and |traversables| is not empty, let |selected| be the first item of |traversables|.
  5. If |selected| is null,
    1. Create a fresh top-level traversable given |navigationURL| and return.
  6. Navigate |selected| to |navigationURL|.
  7. Run the focusing steps with |selected|.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the website is not robust against navigation, wouldn't they simply mint a new URL?

<li><p>If there is an existing <a for=/>top-level traversable</a> <var>traversable</var> within
the user agent's <a for="user agent">top-level traversable set</a> whose
<a for="navigable">active document</a>'s <a for=Document>URL</a> <a for=url>equals</a>
<var>navigationURL</var> with <a for=url/equals><i>exclude fragments</i></a> set to true, then
Copy link
Member

@saschanaz saschanaz Oct 24, 2024

Choose a reason for hiding this comment

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

Wouldn't this always compare the full path? Should we have some base path to compare instead? Otherwise I think each click would effectively always open a new tab unless the user already has a tab for the exact same URL. (e.g. think of Mastodon where each notification has a different URL for each post)

Having a base URL would be useful for Slack example, as the URL is https://app.slack.com/client/(workspace)/(channel), and the notification would pass https://app.slack.com/client/(workspace) as the base url to only match the same workspace.

(It could use URLPattern as @asutherland already mentioned)

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 think Slack would probably opt to handle the notification in its service worker if there is one, using this as fallback. Perhaps over time we can address more use cases here, but having some deployment experience first would help a lot with that.

@saschanaz
Copy link
Member

Now I'm confused, I was assuming that the new intention is to reuse the existing tabs, and the full URL comparison was accidental. It seems the intention is actually to reuse only if the URL is exactly the same of one of the existing tabs and open new tab for anything else. Not sure how that's strictly useful than just open new tab for everything for V1, given a lot of existing use cases assign a new URL for each notification because a notification frequently means there's a new content (e.g. Mastodon, Twitter, YouTube, Slack, Instagram, etc.. Chat apps may have higher frequency of having same URL but still with severely limited chance.)

dizhang168 pushed a commit to dizhang168/html that referenced this pull request Oct 28, 2024
annevk added a commit to w3c/push-api that referenced this pull request Nov 5, 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 a new pushnotification 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 will be done in a separate change.
This makes notifications more declarative by not requiring explicit handling of the clicks by the web application.

This is part of the Declarative Web Push effort: w3c/push-api#360.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants