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

TW-1887: improve notification permission request on web #1954

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

Conversation

Te-Z
Copy link
Contributor

@Te-Z Te-Z commented Jul 17, 2024

Ticket

Related issue

Root cause

If this is a bug, please provide a brief description of the root cause of the issue
On web the notification permission was asked when the app starts. But for some browsers it should be on an action of the user.
Also sound can't be played on some browsers causing exception before displaying the notification.

Solution

Outline the implemented solution, detailing the changes made and how they address the issue
For sound a try catch has been added to avoid to be stuck.
The notification permission is asked when clicked on chat list item but still also when starting the app for browsers who can handle it.

But the notification display conditions changes from one browser to an other.

Test recommendations

Recommendations for how to test this, or anything else you are worried about?

  • Test on all browsers
  • With the browser on foreground without being on the room where you will receive message
  • With the browser on background

Pre-merge

Does anything else need to be done before merging?
no

Resolved

Attach screenshots or videos demonstrating the changes

  • Web:
    Opera (only sounds, notification display when the browser is closing.. will try to handle this later)
ici.webm

Chromium

Capture.video.du.17-07-2024.08.52.16.webm

Chrome

Capture.video.du.17-07-2024.08.50.16.webm

Edge

Capture.video.du.17-07-2024.08.39.49.webm

Firefox

Capture.video.du.17-07-2024.08.37.15.webm

Copy link

This PR has been deployed to https://linagora.github.io/twake-on-matrix/1954

@nqhhdev
Copy link
Member

nqhhdev commented Jul 18, 2024

  • Pls check case: When open link /rooms/roomId => User can click on chat screen then request permission as well.

Copy link
Contributor

@KhaledNjim KhaledNjim left a comment

Choose a reason for hiding this comment

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

Tested on edge, firefox and chrome worked well in my case

@nqhhdev
Copy link
Member

nqhhdev commented Jul 19, 2024

I tested on Firefox, can't show pop-up request notification permission

Screen.Recording.2024-07-19.at.13.31.26.mov

@Te-Z
Copy link
Contributor Author

Te-Z commented Jul 19, 2024

I tested on Firefox, can't show pop-up request notification permission

Screen.Recording.2024-07-19.at.13.31.26.mov

Yes @nqhhdev this is because notifications are already allowed on your test.
But I can confirm it works

image

@hoangdat
Copy link
Member

It is an improvement for notification in web only, not a fix for receive notificaiton. IMO, please update the title to more precise to prevent confuse when we need to trace back.
Moreover, we can not sure the problem of notification in mobile will not comeback. @Te-Z

@Te-Z Te-Z changed the title TW-1887: cannot receive notifications TW-1887: improve notification permission request on web Jul 19, 2024
@nqhhdev
Copy link
Member

nqhhdev commented Jul 25, 2024

Hi @Te-Z pls check all comments

@Te-Z
Copy link
Contributor Author

Te-Z commented Aug 19, 2024

done @nqhhdev @sherlockvn

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.

5 participants