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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 [Story animation] Fix error thrown when user pauses before animation runs. #35161

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Jul 8, 2021

Closes ampproject/error-reporting#94

When the story animations are not initialized (eg: because the story page is not visible yet, due to prerender mode), calling pause will throw an error because this.players_ == null.

There was a try/catch that was catching this error that was removed in #34466 that is needed.

https://github.com/ampproject/amphtml/pull/34466/files#diff-c5a86c80a77ae2a9abb4ac5ff1e9ef10f58c0e9262baa64273be1b4424d7161bL383-L388

We can re-introduce this try-catch and prevent these errors from surfacing, which is ok since the story is not initialized when the pause method is called. Changing the comment to more clearly explain why the method call can fail

An alternative is to change the behavior of pause by doing if (!this.players_) {return;} (silently fail the pause) but it will alter the API. Another alternative is to call init before calling pause but it would be extra work for initializing the animations when users pause the story, which could introduce delays in the rest of the pausing code.

@mszylkowski mszylkowski self-assigned this Jul 8, 2021
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jul 8, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/animation.js

@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Jul 8, 2021
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM since this matches the old implementation that was, as far as we know, working perfectly.
I'm very surprised by the amount of error reports if the issue only happens when a user sees the story with a prerender mode, as this should never happen.

@mszylkowski mszylkowski enabled auto-merge (squash) July 9, 2021 22:10
@mszylkowski mszylkowski merged commit 5a8717f into ampproject:main Jul 9, 2021
wg-stories Sprint automation moved this from In progress to Done Jul 9, 2021
@mszylkowski mszylkowski deleted the animation_prerenderpause branch July 11, 2021 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

馃毃 Error: Cannot read property 'forEach' of null
3 participants