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 generic Chromium support #4281

Closed
MichaIng opened this issue Oct 4, 2020 · 9 comments · Fixed by #4287
Closed

Add generic Chromium support #4281

MichaIng opened this issue Oct 4, 2020 · 9 comments · Fixed by #4287
Labels
4. to release enhancement feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients
Milestone

Comments

@MichaIng
Copy link
Member

MichaIng commented Oct 4, 2020

Is your feature request related to a problem? Please describe.
Nextcloud Talk tells me that my browser is not supported, even that it of course, as it's Opera, based on Chromium and works nicely with all aspects of the app. Worse, the banner makes the generally wrong information true by hiding parts of the apps UI, making them unusable:

I think the blue hidden button at the top right there was to toggle the participants list, but I am unable to toggle it now 😢.

image

Describe the solution you'd like

  • Support all Chromium-based browsers in a generic way, which, aside of Opera, would include Vivaldi, Brave, Iron and some others that deserve more attention instead of feel like being forced to use one of the big four...
  • Add a little [x] hide button to hide the notification, probably even remember that to not bother users again with it.

Describe alternatives you've considered

  • If it's insisted to only support original Chrome and Chromium Edge as Chromium-based browsers and wanted to keep showing this to users on every access, at least let's make it a notification behind the bell instead of a banner that blocks parts of the UI, the banner does not make it's own message become true.

Additional context
Many thanks for the great conference and value speeches and presentations btw! I hope it's okay that parts of the conversation is included in the screen above, right after Julius Härtl finished great demonstrations of Nextcloud Flow and Dashboard widget development 👏.

@nickvergessen
Copy link
Member

Opera is not supported by the webrtc lib. The hidden button is the call button. But you should be able to close the error popover there should be an X in the right end. 🤔

@MichaIng
Copy link
Member Author

MichaIng commented Oct 4, 2020

But you should be able to close the error popover there should be an X in the right end. 🤔

Interesting, the close button is there, but the icon is missing, it still works when I click on the empty area at the right side. Checked on my own server and see that it is searching at the wrong location:

GET https://my.domain.org/js/close.svg?ba1f38e8d96ebd8b9b9561c893e2c86a 404 (Not Found)
  • Actually my Nextcloud is located in nextcloud/ sub dir. I think I remember seeing this behaviour somewhere else already 🤔.
  • Apache2 with all rewrite rules and pretty URLs enabled, working all nice, otherwise.

On NC conference Talk I cannot see that error but a bunch of others. I left the conversation to clean things up and here is what is left, nothing that looks related to me:

Browser console log
sentry.js?v=a05d0f88-1:16 Please do NOT wait for the DOMContentLoaded before registering your viewer handler
(anonymous) @ sentry.js?v=a05d0f88-1:16
handlers @ Viewer.vue:214
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ viewer.js:15
r @ sentry.js?v=a05d0f88-1:16
sentry.js?v=a05d0f88-1:16 The following mime is already registered text/csv {__ob__: De}
(anonymous) @ sentry.js?v=a05d0f88-1:16
(anonymous) @ Viewer.vue:508
registerHandler @ Viewer.vue:505
handlers @ Viewer.vue:217
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ viewer.js:15
r @ sentry.js?v=a05d0f88-1:16
sentry.js?v=a05d0f88-1:16 The following mime is already registered image/svg+xml {__ob__: De}
(anonymous) @ sentry.js?v=a05d0f88-1:16
(anonymous) @ Viewer.vue:508
registerHandler @ Viewer.vue:505
handlers @ Viewer.vue:217
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ viewer.js:15
r @ sentry.js?v=a05d0f88-1:16
sentry.js?v=a05d0f88-1:16 The following mime is already registered text/plain {__ob__: De}component: (...)group: (...)id: (...)mimes: (...)__ob__: De {value: {…}, dep: le, vmCount: 0}get component: ƒ ()set component: ƒ (t)get group: ƒ ()set group: ƒ (t)get id: ƒ ()set id: ƒ (t)get mimes: ƒ ()set mimes: ƒ (t)__proto__: Object
(anonymous) @ sentry.js?v=a05d0f88-1:16
(anonymous) @ Viewer.vue:508
registerHandler @ Viewer.vue:505
handlers @ Viewer.vue:217
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ viewer.js:15
r @ sentry.js?v=a05d0f88-1:16
sentry.js?v=a05d0f88-1:16 Please do NOT wait for the DOMContentLoaded before registering your viewer handler
(anonymous) @ sentry.js?v=a05d0f88-1:16
handlers @ Viewer.vue:214
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ main.js:26
r @ sentry.js?v=a05d0f88-1:16
sentry.js?v=a05d0f88-1:16 The following mime is already registered application/pdf {__ob__: De}component: (...)id: (...)mimes: (...)__ob__: De {value: {…}, dep: le, vmCount: 0}get component: ƒ ()set component: ƒ (t)get id: ƒ ()set id: ƒ (t)get mimes: ƒ ()set mimes: ƒ (t)__proto__: Object
(anonymous) @ sentry.js?v=a05d0f88-1:16
(anonymous) @ Viewer.vue:508
registerHandler @ Viewer.vue:505
handlers @ Viewer.vue:217
fr.run @ vue.runtime.esm.js:4568
cr @ vue.runtime.esm.js:4310
(anonymous) @ vue.runtime.esm.js:1980
Xe @ vue.runtime.esm.js:1906
Promise.then (async)
Ye @ vue.runtime.esm.js:1933
tt @ vue.runtime.esm.js:1990
(anonymous) @ vue.runtime.esm.js:4402
fr.update @ vue.runtime.esm.js:4544
le.notify @ vue.runtime.esm.js:730
(anonymous) @ vue.runtime.esm.js:882
value @ Viewer.js:78
(anonymous) @ main.js:26
r @ sentry.js?v=a05d0f88-1:16

Opera is not supported by the webrtc lib.

Hmm, then this should be reported to the WebRTC lib, since it fully supports WebRTC like any other Chromium-based browser I know. I use Nextcloud Talk, not often but regularly, now I wonder how I found the call button then 😄, maybe UI changed a bid with v10, and in my instance I still can click the button via small part showing above the banner.

@nickvergessen nickvergessen self-assigned this Oct 5, 2020
@nickvergessen nickvergessen added this to the 💔 Backlog milestone Oct 5, 2020
@nickvergessen nickvergessen added the feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients label Oct 5, 2020
@nickvergessen nickvergessen removed their assignment Oct 5, 2020
@nickvergessen
Copy link
Member

#4287 brings back the X to close the error.

Reopening for future "support more chrome based browsers"

@nickvergessen nickvergessen reopened this Oct 5, 2020
@MichaIng
Copy link
Member Author

MichaIng commented Oct 5, 2020

Many thanks for the quick solution. If browser support is a matter of an external library, give me a hint/link and I can do a start and open an issue there.

Hmm:

@nickvergessen
Copy link
Member

It's more about https://github.com/webrtcHacks/adapter

@MichaIng
Copy link
Member Author

MichaIng commented Oct 5, 2020

The browser detection of both modules looks quite similar: https://github.com/webrtcHacks/adapter/blob/master/src/js/utils.js#L163-L192

Opera is listed in the webkit prefix block, but Chromium is not based on webkit for a loong time. That issue is somehow related: webrtcHacks/adapter#764

Looks like a generic Blink browser engine detection is missing and not all Chromium-based browsers still support those webkit prefixed variables (anymore).

@nickvergessen
Copy link
Member

Opera is now supported as the lib mentions it explicitly

@MichaIng
Copy link
Member Author

MichaIng commented Feb 5, 2021

Just tested and verified with NC21 RC1 and related alpha Talk version, not showing the warning now 👍.

@fippo
Copy link

fippo commented Feb 13, 2021

The browser detection of both modules looks quite similar

That is not a coincidence ;-)
In general I would recommend getting rid of webrtcsupport, see https://github.com/simplewebrtc/SimpleWebRTC/pull/559/files for some of the mechanics.

Opera is listed in the webkit prefix block, but Chromium is not based on webkit for a loong time

Opera still supports webkitGetUserMedia. Should you encounter a chromium-based browser that doesn't please add a note to the adapter issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release enhancement feature: WebRTC 🚡 WebRTC connection between browsers and/or mobile clients
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants