Send a 'close' action when the widget is ready to close #3011
Conversation
|
I bumped the TypeScript |
robintown
force-pushed
the
robin/close-action
branch
from
c602dfa to
a08d75d
Compare
robintown
force-pushed
the
robin/close-action
branch
from
a08d75d to
7566bc4
Compare
By keeping 'hangup' and 'close' as separate actions, we can allow Element Call widgets to stay on an error screen after the user has been disconnected without the widget completely disappearing from the host's UI. We don't have to request any additional capabilities to use a custom widget action like this one.
robintown
force-pushed
the
robin/close-action
branch
from
7566bc4 to
a75952c
Compare
src/rtcSessionHelpers.ts
Outdated
| await widget.api.transport.send(ElementWidgetActions.HangupCall, {}); | ||
| // On a normal user hangup we can shut down and close the widget. But if an | ||
| // error occurs we should keep the widget open until the user reads it. | ||
| if (cause === "user") { | ||
| await widget.api.transport.send(ElementWidgetActions.Close, {}); | ||
| widget.api.transport.stop(); | ||
| PosthogAnalytics.instance.logout(); |
There was a problem hiding this comment.
As mentioned in the VoIP room I think that the general approach of two actions is correct. (It is the minimal set of information we need).
I do think we should not change the meaning of ElementWidgetActions.HangupCall however.
Changing it here is not an issue. But this change implies a change in EX and EW where we now do NOT close a widget anymore on receiving the hangup action. This make EC and EX incompatible with older EC versions which could be an issue on self hosted instances.
Also Jitsi might break.
I think the better would be to keep the action as they are and introduce the following:
- a general purpose close action best with an MSC.
org.matrix.mscXXXX.closeaction that any widget can use for requesting to get closed (This was sth that seemed like I wanted it for the matrixRTC demo also) - A dedicated element call hangup action
io.element.hangupthat is only used for element call hangups and does not imply any widget updating but only makes the call render as not connected. (Can you also update me again on the case in which this is different to the room state not including our own rtc memebrship anymore? I think that is not tracked yet in text) - replace
HangupCall = "im.vector.hangup",from heElementWidgetActionsenum withio.element.hangup
There was a problem hiding this comment.
I do think we should not change the meaning of
ElementWidgetActions.HangupCallhowever. Changing it here is not an issue.
I'm still confused by this. This PR doesn't change the meaning of im.vector.hangup at all, it only adds more information.
But this change implies a change in EX and EW where we now do NOT close a widget anymore on receiving the hangup action. This make EC and EX incompatible with older EC versions which could be an issue on self hosted instances.
It's always been the case that we expect EC+EW deployments to be on the latest EC release at the time of the EW release, otherwise it becomes unnecessarily hard to make forward progress when it comes to URL parameters or widget communication. I agree that this isn't the best situation, as admins might forget about this hidden assumption, but I suggest we continue accepting this for now since the embedding work (#2994 / element-hq/element-web#23908) is going to make the dependency explicit, resolving it for good.
We do need to keep the EW/EX PRs blocked until we're confident that we can release a new EC version though, that's a good thing to remember here.
Also Jitsi might break.
Good thing to flag but it doesn't break, Jitsi video rooms are handled by a different class in Element Web and I've tested them.
I think the better would be to keep the action as they are and introduce the following:
- a general purpose close action best with an MSC.
org.matrix.mscXXXX.closeaction that any widget can use for requesting to get closed (This was sth that seemed like I wanted it for the matrixRTC demo also)
I have some thoughts about this:
- Does this close action become part of the minimum implementation requirements of the widget API, or does it need to be requested as a capability?
- The benefit, I guess, of standardizing a close action would be to allow arbitrary, untrusted widgets to request their own closure. It doesn't change anything for deep widget integrations like EW/EX + EC, which could just as easily use proprietary actions.
- If you're a client, and you're presenting an untrusted widget, you have to either show your own close button in addition to the widget's, which could look awkward, or else risk that the widget won't offer any way of allowing the user to close it, permanently intruding the user's screen.
So generally it seems a bit hairy: see for example the restrictions that the Web platform places on window.close(). I think sticking to a proprietary action here may just be a good thing.
- A dedicated element call hangup action
io.element.hangupthat is only used for element call hangups and does not imply any widget updating but only makes the call render as not connected. (Can you also update me again on the case in which this is different to the room state not including our own rtc memebrship anymore? I think that is not tracked yet in text)- replace
HangupCall = "im.vector.hangup",from heElementWidgetActionsenum withio.element.hangup
How would such an action be any different from im.vector.hangup?
| @@ -21,10 +21,11 @@ import { getUrlParams } from "./UrlParams"; | |||
| import { Config } from "./config/Config"; | |||
| import { ElementCallReactionEventType } from "./reactions"; | |||
|
|
|||
| // Subset of the actions in matrix-react-sdk | |||
| // Subset of the actions in element-web | |||
| export enum ElementWidgetActions { | |||
| JoinCall = "io.element.join", | |||
| HangupCall = "im.vector.hangup", | |||
There was a problem hiding this comment.
| HangupCall = "im.vector.hangup", | |
| HangupCall = "io.element.hangup", |
| export enum ElementWidgetActions { | ||
| JoinCall = "io.element.join", | ||
| HangupCall = "im.vector.hangup", | ||
| Close = "io.element.close", |
There was a problem hiding this comment.
| Close = "io.element.close", | |
| Close = "org.matrix.mscXXXX.close", |
src/rtcSessionHelpers.ts
Outdated
| // On a normal user hangup we can shut down and close the widget. But if an | ||
| // error occurs we should keep the widget open until the user reads it. | ||
| if (cause === "user") { | ||
| await widget.api.transport.send(ElementWidgetActions.Close, {}); |
There was a problem hiding this comment.
Can we add a way to also emit this with a button on the error screen?
This is unnecessary for EW since we have the close button there. But on EX this is a better UX since we then can close the widget by pressing sth like "Ok" or "Close" inside the IFrame.
(for now this screen would of course not be visible but once we update the rust sdk (if we go this route where a rust sdk udpate is required) we dont introduce the isse that the user cannot exit the error screen except when using the os activity manager)
There was a problem hiding this comment.
I would love to do that, yeah, but in a separate PR.
By keeping 'hangup' and 'close' as separate actions, we can allow Element Call widgets to stay on an error screen after the user has been disconnected without the widget completely disappearing from the host's UI. We don't have to request any additional capabilities to use a custom widget action like this one.