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

Remove or explain usages of unload and beforeunload listeners #23634

Closed
cvializ opened this issue Aug 1, 2019 · 8 comments

Comments

@cvializ
Copy link
Contributor

commented Aug 1, 2019

These listeners prevent the browser from adding documents to the back/forward cache when they are added to the window object.

@cvializ cvializ added the Type: Bug label Aug 1, 2019

@cvializ cvializ self-assigned this Aug 1, 2019

@wassgha wassgha self-assigned this Aug 16, 2019

@cvializ cvializ referenced this issue Aug 16, 2019
2 of 5 tasks complete
@wassgha

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

After doing some research on this, unload / beforeunload can be replaced with the pagehideand the event.persisted property. We need to figure out what this means for analytics and metrics since pages will now be able to unload/load from the cache instead of a full load (so if we simply use pagehide instead of unload and pageshow instead of load then metrics that relied on these events to measure load time for example would be skewed). The two options are:

  • Define unload as pagehide with !event.persisted and define load as pageshow with !event.persisted
  • Change the metrics to make use of a compound event (or two separate events) composed of both the pagehide/pageshow event and whether the page was loaded from the cache or not.
    I am investigating what metrics/analytics rely on load/unload/beforeunload and will update this.

/cc @sparhami

@wassgha

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

More context on this.

The analytics usage was only added on Safari and will be removed through #24065 since it is not critical (see context in #24071 .

The viewer usage – on the Google AMP Viewer side at least – was added to detect whether an AMP document unloads inside a viewer (and it shouldn't) and displays an error.

Based on my limited understanding, there is no usage of BFCache inside the viewer and since the beforeunload listener for this case is added through the amp-viewer-integration extension (which is only added by AMP caches as far as I know), it shouldn't affect pages that are not cached. We should however detect whether the page is served from a viewer and gate the listener in order to enable BFCache on pages that are cached by an AMP Cache but not served inside a viewer. Another alternative is to remove the listener all together since this is a rare case.

/cc @cvializ @sparhami and Eric Steinlauf for comments

@ericfs

This comment has been minimized.

Copy link

commented Aug 20, 2019

I think @wassgha's analysis is correct. The unload event is used specifically in the case of an AMP Viewer. While it is extremely rare, it really shouldn't be allowed since The AMP Viewer does not expect to contain non-AMP content.

I'm not familiar with the use of beforeunload in amp-viewer-integration.

@wassgha

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

@ericfs do you know anyone other than @chenshay who would be more familiar with amp-viewer-integration? We can tag them to comment on this

@ericfs

This comment has been minimized.

Copy link

commented Aug 20, 2019

I'm not sure. Can you point me to the code you're talking about and I can take a look?

@ericfs

This comment has been minimized.

Copy link

commented Aug 22, 2019

Oh, ok. My explanation above in #23634 (comment) does apply to that. I thought you were saying there is an additional listener for beforeunload. If there is, that's the one I'm not familiar with.

@wassgha

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

Closed by #24172

@wassgha wassgha closed this Aug 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.