ampproject / ampproject/amphtml

[amp-story-player] Replace iframe.onload() for storyContentLoaded

Open
#32,436 2 comments 2 reactions 1 assignee Claimed by @gmajoulet View on GitHub
Stale Type: Bug WG: stories
Dominant language
JavaScript
Stars
14.9k
Forks
4.1k
PR merge metrics
No merged PRs in 30d

Description

Currently the iframes inside the player have a handler for the `onload()` event which sets some loading-related CSS classes (they show the poster image / loading spinner).

https://github.com/ampproject/amphtml/blob/e7798f4fbd1128a5d3785ab1f098bf2ea2d4c782/src/amp-story-player/amp-story-player-impl.js#L637-L641

The issue with using `iframe.onload()` is that it could be fired twice (once when the element is attached to the DOM and a second time when the content loads ([see codepen](https://codepen.io/gmajoulet/pen/poNjVYm)).

We also have an existing native event from `amp-story` called `storyContentLoaded`. It is fired once meaningful story content has loaded, so we should wait for this event instead.

While we wait for this event, we should display the poster and/or loading spinner until the current story dispatched its `storyContentLoaded` on initial loading of the player and on navigation.

### Alternative solutions considered

This could be solved by attaching the `onload` event to the iframe after it's appended to the DOM or by always setting the `src` on the iframe before appending it to the DOM [see codepen](https://codepen.io/maenrique/pen/poNgJMb). Also by checking the [event.target.src](https://stackoverflow.com/questions/10781880/dynamically-created-iframe-triggers-onload-event-twice/38459639#38459639) on the iframe `onload` event.

But this will just make the player code harder to read and it's not optimized for the existing story heuristics (`storyContentLoaded`).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.