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

consider using beforeunload event sparsely #8595

Open
akosyakov opened this issue Oct 7, 2020 · 3 comments
Open

consider using beforeunload event sparsely #8595

akosyakov opened this issue Oct 7, 2020 · 3 comments
Labels
core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves

Comments

@akosyakov
Copy link
Member

akosyakov commented Oct 7, 2020

Using unload and beforeunload events is not recommended: https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-beforeunload-event
In order to detect last page interaction one can use pagehide and visibilitychange events instead: https://developers.google.com/web/updates/2018/07/page-lifecycle-api#advice-hidden
beforeunload event should be installed only when there is unsaved changes and removed when there are not

@akosyakov akosyakov added core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves labels Oct 7, 2020
@akosyakov
Copy link
Member Author

Bad effects which it is causing: gitpod-io/gitpod#1959

@akosyakov
Copy link
Member Author

In order to reproduce with Theia:

  • do dirty changes
  • stop the server
  • try to reload the page
  • see confirmation dialogs about which user cannot do anything

@kittaakos
Copy link
Contributor

I did a prototype, it's looking very promising, but it does not really work in electron. Although calling addUnsavedChanges on the lifecycle does prevent the electron window to reload/close, I could not customize the underlying beforeunload listener, so I was not able to show a custom modal dialog in electron. It works in the browser. I tried it with Brave. There is another, generic issue I have noticed; I never received the expected hidden state on page reload. I tried it with both browser and electron. Maybe I have overlooked something.

I used this service to wrap the lifecycle:

import { injectable } from 'inversify';
import { Event as TheiaEvent, Emitter } from '../common';

const lifecycle: Lifecycle = require('page-lifecycle/dist/lifecycle.es5');

/**
 * The API documentation is [here](https://github.com/GoogleChromeLabs/page-lifecycle#api).
 */
export interface Lifecycle {
    readonly state: LifecycleState;
    readonly pageWasDiscarded: boolean;
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    addEventListener(type: 'statechange', listener: (event: LifecycleEvent) => any): void;
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    removeListener(type: 'statechange', listener: (event: LifecycleEvent) => any): void;
    addUnsavedChanges(id: Object | Symbol): void;
    removeUnsavedChanges(id: Object | Symbol): void;
}

/**
 * See https://developers.google.com/web/updates/2018/07/page-lifecycle-api.
 */
export type LifecycleState = 'active' | 'passive' | 'hidden' | 'frozen' | 'terminated' | 'discarded';

export interface LifecycleEvent {
    readonly oldState: LifecycleState;
    readonly newState: LifecycleState;
    readonly originalEvent: Event;
}

@injectable()
export class PageLifecycle {

    protected readonly instance = lifecycle;
    protected readonly stateChangedEmitter = new Emitter<LifecycleEvent>();

    constructor() {
        this.instance.addEventListener('statechange', this.onStateChange.bind(this));
    }

    protected onStateChange(event: LifecycleEvent): void {
        this.stateChangedEmitter.fire(event);
    }

    get state(): LifecycleState {
        return this.instance.state;
    }

    get onStateChanged(): TheiaEvent<LifecycleEvent> {
        return this.stateChangedEmitter.event;
    }

    addUnsavedChanges(id: Object | Symbol): void {
        this.instance.addUnsavedChanges(id);
    }

    removeUnsavedChanges(id: Object | Symbol): void {
        this.instance.removeUnsavedChanges(id);
    }

}

and this simple logic to track dirty widgets and call addUnsavedChanges/removeUnsavedChanges accordingly:

protected trackDirtyState(widget: Widget): void {
    const saveable = Saveable.get(widget);
    if (!saveable) {
        return;
    }
    const toDisposeOnWidgetClose = new DisposableCollection();
    const widgetDisposeListener = () => toDisposeOnWidgetClose.dispose();
    toDisposeOnWidgetClose.pushAll([
        Disposable.create(() => widget.disposed.disconnect(widgetDisposeListener)),
        Disposable.create(() => this.pageLifecycle.removeUnsavedChanges(widget)),
        saveable.onDirtyChanged(() => {
            if (saveable.dirty) {
                this.pageLifecycle.addUnsavedChanges(widget);
            } else {
                this.pageLifecycle.removeUnsavedChanges(widget);
            }
        })
    ]);
    widget.disposed.connect(() => widgetDisposeListener);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves
Projects
None yet
Development

No branches or pull requests

2 participants