octachrome / treason

A clone of the card game Coup written in Node.js
Other
139 stars 79 forks source link

feat: add notification actions #45

Closed KatherineWinter closed 4 years ago

KatherineWinter commented 4 years ago

Most major browsers can have notifications actions. To support notification actions, a service worker has to be registered. The service worker activates the the client tab, and sends back the action event the user picked.

JackieNiebling commented 4 years ago

I had a look at this as I was interested in how it worked and I found a few things that I thought I should bring to your attention. There is still an optional chaining operator hiding on line 31 in service-worker.js.

When testing this on Firefox 76.0.1, I found that the actions are not coming through on this browser. I also found that clicking on the notification tries to open a new tab, rather than going to the existing one. This only seems to happen when the windows is not active. There is a known bug where the URL redirection via a hash doesn't work, so this new tab also gets an error anyway. If it did manage to open a new tab and join anyway, it would join as a new player (same user though, this is supported), so that's not desirable.

In Chrome, I seem to be getting multiple notifications about having won a game. To reproduce, start a new game, say with FF and Chrome. Have the FF user leave the session. The Chrome user now gets a notification that they have won, as expected. If the Chrome user now joins the lobby, they get the same notification again. If the Chrome user creates a new game, they get the notification a third time. If the FF user joins this game, the Chrome user gets 3 notifications that they have won.

The notifications appear to not work in Edge.

KatherineWinter commented 4 years ago

I also found that clicking on the notification tries to open a new tab, rather than going to the existing one.

Cannot repro.

I seem to be getting multiple notifications about having won a game.

This would be an existing bug given nothing changed on when it notifies the user. Only how it notify the user.

Feel free to open a PR with changes. I will try to dig later, but not for a while.

octachrome commented 4 years ago

This would be an existing bug given nothing changed on when it notifies the user.

Seems unlikely to be an existing bug. I would be inclined to suspect this code that you added:

// If site is cannot be opened, open in new window with url
if (self.clients.openWindow) {
  self.clients.openWindow(targetUrl);
}

I will revert the merge for now so we can fix anything that needs fixing first.

octachrome commented 4 years ago

I have verified that clicking the notification in Firefox 76 opens a new tab unexpectedly as @JackieNiebling reported. I tested this on Linux.

I also tested on Firefox 70 on Linux, and notifications do not work at all after this merge (I verified that they do work before the merge). This error is shown in the console:

ServiceWorker registration failed:  TypeError: "ServiceWorker script at http://localhost:8080/service-worker.js for scope http://localhost:8080/ encountered an error during installation."

@JackieNiebling it is expected that the action buttons do not appear in Firefox, because Firefox does not support this part of the notifications API yet.

octachrome commented 4 years ago

I just confirmed that the issue where you get multiple "you have won" notifications is indeed an existing issue. Sorry about my reply earlier, I was confused about which issue was being discussed as existing. So we can ignore this issue.

I also found that the ServiceWorker registration failed is caused by that last ?. operator left in service-worker.js - removing it fixes notifications on Firefox 70.

So the open issues as I understand it are:

JackieNiebling commented 4 years ago

First point: Yes, notifications are not working in Edge, and it is indeed because of the opt.chain operator. Removing the operator makes notifications work with actions and all.

The new tab opening unexpectedly is also happening in Edge. You need to actually click on the notification in Firefox to trigger it to opening the new tab. In Edge, the new tab will open just by clicking on any of the actions (Allow, Challenge), when the above bug is fixed.

JackieNiebling commented 4 years ago

I have done some debugging and found out more about why Edge and Firefox are triggering the opening of a new window rather than finding the current one. It seems that both Firefox (76.0.1) and Edge (44.18362.449.0, EdgeHTML 18.18363) are handling the URL wrong. Both of them are returning the clientWindow.url without the game name fragment in some cases. If you start your session from localhost:8080/ then navigate to the game (for instance by clicking the Quick Play button), then the hash fragment is not included in the clientWindow.url that is compared against the targetUrl. If you start your session directly by going to the game with hash fragment, localhost:8080/#17, then this clientWindow.url does include the hash fragment.

I suspect there is a bug in both FF and Edge where the client(window) that registers to a service worker has a fixed URL that does not change, even when navigating that window (to a fragment). This behaviour can be observed in the Edge development tools, where the clients are registered and displayed with the URL they initially loaded with.

I suspect Chromium has a special hook to update this client window URL when fragment navigation happens and the other two probably only does it on a full page load.

JackieNiebling commented 4 years ago

I was about to raise behaviour as a bug in the Mozilla Bugzilla, when I decided to dig around and try and read the actual spec for this. According to the spec:

The url attribute must return the context object’s associated service worker client's serialized creation URL.

I raised this issue with the good folks over there to see if they could clarify.