Skip to content
This repository has been archived by the owner on Jan 22, 2022. It is now read-only.

enable html5 notifications #464

Open
wants to merge 9 commits into
base: xenial
Choose a base branch
from
Open

Conversation

balcy
Copy link
Collaborator

@balcy balcy commented Feb 18, 2021

No description provided.

Copy link
Member

@UniversalSuperBox UniversalSuperBox left a comment

Choose a reason for hiding this comment

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

Hmm. This seems like a huge review without much ability for me to give useful feedback. But I did find a couple of things.

src/app/NotificationsAccessDialog.qml Outdated Show resolved Hide resolved
src/app/notifications-proxy.cpp Outdated Show resolved Hide resolved
Copy link
Member

@UniversalSuperBox UniversalSuperBox left a comment

Choose a reason for hiding this comment

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

Thanks, that's the static analysis done.

Note that this DOES NOT enable webpush. At all. Attempting to use the Microsoft Edge Web Push demo throws the error Registration failed - push service not available in the browser console.

Instead, this only enables the Notifications API. The notifications API allows an active tab to send a notification via JavaScript. If a tab becomes suspended due to memory pressure, it will be unable to send notifications any more. This limits the feature's usefulness for now, but could lead to better things in the future.

I tested the feature with https://www.davidwalsh.name/demo/notifications-api.php.

When a website tries to show a notification, a permission request appears.

Permission Request: https://davidwalsh.name/; this page wants to create notifications. Checkbox to remember decision. Provided options: Allow, Deny

If allowed, the single notification that the website tried to send appears.

Notification title: "Title". Notification body: "I am the body text!"

If the website tries to send another notification, the permission request appears again. I believe this should be changed. It is not useful to be asked whether to allow or deny a push notification every time a website tries to send it... you might as well have just allowed the notification in the first place. Either the "Remember decision" checkbox should be checked by default or, probably more useful, removed and the options changed to "Always Allow" and "Always Deny"

@UniversalSuperBox
Copy link
Member

Also interesting: Tapping on a notification opens the website attached to the notification in a new tab. I think that the browser should switch to the site's open tab instead of opening a new tab. That's the behavior of other browsers, at least.

@balcy
Copy link
Collaborator Author

balcy commented May 30, 2021

for the first issue (authentication asked twice):
Was it during the same session (without closing morph), that the page asked for the confirmation again ? I thought it would remember it like the audio permission.

I think getting rid of the "allow permanently" checkbox is not possible, because in incognito mode it is not enabled because no permanent data about domains is saved then.

The second one seems a bit trickier: We would need to change the external URL handling and not open it in another tab, if there is already an open tab with that URL. You're right other browser do it like that.

@UniversalSuperBox
Copy link
Member

Was it during the same session (without closing morph), that the page asked for the confirmation again?

Yes. I didn't leave the page. I just clicked the button to send a notification, said "no" to the prompt, then clicked the button to send a notification again.

I think getting rid of the "allow permanently" checkbox is not possible, because in incognito mode it is not enabled because no permanent data about domains is saved then.

That seems fine. Firefox silently changes this behavior in Private Mode, the "Allow" button changes from enabling "Allowed" to "Allowed temporarily".

We would need to change the external URL handling and not open it in another tab

True. I think it's required, though. Take Firefox on https://www.davidwalsh.name/demo/notifications-api.php as an example: if you close the tab, the notification is removed from Gnome! The notifications aren't meant to outlive a tab, and they aren't meant to have any other context than that.

@balcy
Copy link
Collaborator Author

balcy commented Jun 1, 2021

ah ok, that's true the "no" was not saved, but the "yes" is.
Ok that should now work with commit ca01e97

For the last issue (focus tab instead of opening the link in the new tab):
On webapps it is already correct I think, because the app is opened (or focused if already open)

For morph as the default URL handler we could generally change the logic like that:

  • Does a tab with that URL exist ? -> focus that tab, otherwise open the URL in a new tab.

Would it be a problem, if all URL links (e.g. links clicked in apps) are handled that way by morph-browser ?

@balcy
Copy link
Collaborator Author

balcy commented Jun 1, 2021

that would be different to an URL passed on the desktop via firefox <url> (if firefox is already running) or sensible-browser <url> : Those open a new tab with that URL

@balcy
Copy link
Collaborator Author

balcy commented Aug 8, 2021

ok with the latest commit, morph switches to the (already open) tab with the url from the notification

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

Successfully merging this pull request may close these issues.

2 participants