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

webxdc sendUpdate does not work in onclose/visibility changed event handler on closing the app #3321

Open
Simon-Laux opened this issue Jul 20, 2023 · 9 comments
Labels
bug Something isn't working webxdc

Comments

@Simon-Laux
Copy link
Member

Simon-Laux commented Jul 20, 2023

webxdc sendUpdate does not work in onclose/visibility changed even handler on closing the app.

window.on("visibilitychange", (ev)  => {
 window.webxdc.sendUpdate(update, descr);
})

In this case desktop does not manage to send the update. I suspect it is because sendUpdate uses the async electron ipc invoke-function.

I tried using a the synchronous ipc functions (without changing that sendUpdate returns a promise, it uses jsonrpc under the hood in the back-end and JS has no ::block_on(future/promise) function, so the promise is necessary), but I was not successful in my first try (but I was also very tired and had a hard time concentrating, so maybe I could still make it work, but I don't see very high chances.)

At the moment I would recommend the following workaround, but I haven't tested if closing the window works with standard browser apis in our case:

window.on("visibilitychange", (ev)  => {
 ev.preventDefault(); // prevent the closing of the window
 await window.webxdc.sendUpdate(update, descr);
 // somehow close window via JS //maybe with window.close() ?
})

Maybe it could also make sense to add a close callback api to the webxdc spec, where you can set a custom close handler that is run on closing and returns a promise, when it is resolved or after a timeout the app is then closed by the host app (in our case DC).

@Simon-Laux Simon-Laux added bug Something isn't working webxdc labels Jul 20, 2023
@WofWca
Copy link
Collaborator

WofWca commented Jul 21, 2023

Also see

Also the correct code for the workaround you suggested would be

function sendUpdateAndPreventCloseUntilSent(e) {
    webxdc.sendUpdate({ payload: 'bar' }, '').then(() => {
        window.removeEventListener('beforeunload', sendUpdateAndPreventCloseUntilSent);
        // Close the parent since we're in an `<iframe>`.
        window.parent.close();
    })

    e.preventDefault();
    e.returnValue = '';
    return '';
}
window.addEventListener('beforeunload', sendUpdateAndPreventCloseUntilSent);

, as in https://codeberg.org/webxdc/editor/commit/9d56ab4167ad9d72f79baecde50401dc036bc0dd

@WofWca
Copy link
Collaborator

WofWca commented Jul 21, 2023

Something notable. I'm trying webxdc/webxdc-test@4add6f7, and it appears that sendUpdate works inside of beforeunload even without event.preventDefault().

image

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

@Simon-Laux
Copy link
Member Author

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

Maybe would need a way to prevent sending duplicate updates?

@r10s
Copy link
Member

r10s commented Sep 11, 2023

So the workaround would be to add a beforeunload listener besides the visibilitychange listener?

why not simply call visibilitychange from the beforeunload listener. beforeunload is known to not working on all platforms and is discouraged anyways - that in mind, we can ignore the issue if an app sets beforeunload on its own and the handler does not get called (as maybe overwritten by us).
(if visibilitychange gets fired twice, that's seems okayish as it is known to be fired often)

iirc, i did a small test some time ago, and that worked out.

that way, visibilitychange would work on all platforms. and we can avoid the costs of adding new api on all platforms (eg. android/ios/xmpp do not have the issue, but it is easy to imagine that new pop up with new api :) - still a pity that we did not find a way to make sendUpdate() non-async as localStorage.setItem() - maybe someone else can have a look there as well, see Simon's comments above)

@WofWca

This comment was marked as outdated.

@r10s

This comment was marked as outdated.

@WofWca
Copy link
Collaborator

WofWca commented Sep 14, 2023

I'm sorry, I still don't get your idea. Are you suggesting a workaround? How is it better than mine? Can you show some code?

@r10s
Copy link
Member

r10s commented Sep 20, 2023

I'm sorry, I still don't get your idea. Are you suggesting a workaround? How is it better than mine? Can you show some code?

i have overseen or misinterpreted your comment "So the workaround would be to add a beforeunload listener besides the visibilitychange listener?" that time, sorry.

i think, we are talking about the same thing. however, to be clear, i'd not add beforeunload to some/all webxdc but to desktop, to webxdc.js if in doubt, - so that visibilitychange is guaranteed to work on all platforms

@WofWca
Copy link
Collaborator

WofWca commented Nov 21, 2023

Something to note: this is only the case inside of the <iframe> that we contain the apps in. If you open the console, choose the top context and execute

window.addEventListener('visibilitychange', () => {
    webxdc.sendUpdate({
        payload: 'dummyPayload',
        info: 'sent!' + Math.random()
    }, '');
});

it will send a message, whereas if you choose the app's context, it wont.

So, another workaround:

+const targetWindow = window.parent;
-window.addEventListener('visibilitychange', () => {
+targetWindow.addEventListener('visibilitychange', () => {
+  // if (targetWindow.document.visibilityState ===...
     webxdc.sendUpdate({
         payload: 'dummyPayload',
         info: 'sent!' + Math.random()
     }, '');
 });

Update: I implemented the hack here https://codeberg.org/WofWca/webxdc-yjs-provider/commit/d129908920f9f21bb2bc6495b6cfffc17393a382

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working webxdc
Projects
None yet
Development

No branches or pull requests

3 participants