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

✨ Add a defense-in-depth to the viewer in case communication fails #28807

Merged
merged 2 commits into from
Jun 15, 2020

Conversation

cramforce
Copy link
Member

@cramforce cramforce commented Jun 10, 2020

This will declare a doc to be visible if it isn't but gets a user action.

In Google Search this should never happen, because the doc isn't actually shown until the viewer handshake happened.

But other viewers do show documents right away, and we've seen cases where docs where presented to the user, but not interactive, because they never got the respective message.

Fixes #28227

@cramforce cramforce requested review from dvoytenko and gmajoulet June 10, 2020 20:12
@amp-bundle-size amp-bundle-size bot requested a review from dreamofabear June 10, 2020 20:29
@ampproject ampproject deleted a comment Jun 10, 2020
@gmajoulet
Copy link
Contributor

I really like building this at the AMP level. This looks good to me. Privacy implications have been discussed and approved by an internal Privacy Working Group.

@dvoytenko
Copy link
Contributor

Overall this makes sense. But also depends a bit on a viewer. E.g. in case of Google Search, there was a glasspain on top of the doc that blocked all actions until the doc becomes visible. This, of course, wouldn't help those cases.

This will declare a doc to be visible if it isn't but gets a user action.

In Google Search this should never happen, because the doc isn't actually shown until the viewer handshake happened.

But other viewers do show documents right away.

Fixes ampproject#28227
@cramforce cramforce force-pushed the interactive-fallback branch from 333d647 to 4fa40e7 Compare June 13, 2020 20:11
@cramforce cramforce marked this pull request as ready for review June 13, 2020 20:23
@cramforce
Copy link
Member Author

Thanks. Added tests

@cramforce cramforce merged commit 5ba7326 into ampproject:master Jun 15, 2020
mszylkowski pushed a commit to mszylkowski/amphtml that referenced this pull request Jun 17, 2020
…mpproject#28807)

This will declare a doc to be visible if it isn't but gets a user action.

In Google Search this should never happen, because the doc isn't actually shown until the viewer handshake happened.

But other viewers do show documents right away.

Fixes ampproject#28227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Stories more resilient to viewer communication initialization errors.
5 participants