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

AMP shadows broken due AMP doc being activated on interaction #29608

Closed
oliverfernandez opened this issue Jul 31, 2020 · 6 comments · Fixed by #29631
Closed

AMP shadows broken due AMP doc being activated on interaction #29608

oliverfernandez opened this issue Jul 31, 2020 · 6 comments · Fixed by #29631

Comments

@oliverfernandez
Copy link
Contributor

This bug has been introduced when the issue #28227 has been fixed in the PR #28807

SInce now an AMP document is set to visible when the users interacts with window, those websites that use AMP shadows does not have control over the visibilityState of the ampdoc since it automatically switches to visible

I understand that this "visible on user interaction" behaviour is not necessary in AMP shadow mode.

The fix should be as easy as checking here this.ampdoc.isSingleDoc() === true before calling visibleOnUserAction_() method, and I was going to open a PR for it.

However I just saw that it seems that the method isSingleDoc is being deprecated, and there's a new ampdoc FIE that I'm not familiar with, so I'm not sure which should be the right fix.

@oliverfernandez
Copy link
Contributor Author

cc/ @dvoytenko

@dvoytenko
Copy link
Contributor

@oliverfernandez isSingleDoc can indeed be used here. I'd appreciate the pull request. The isSingleDoc is not being deprecated, it's just being renamed. The meaning will change slightly, but not for this use case. Ampdoc FIE is not relevant here - those are just ads. They've always existed, but now they are just more standardized.

@oliverfernandez
Copy link
Contributor Author

Got it!

I'll send the PR ASAP. Thanks!!

@oliverfernandez
Copy link
Contributor Author

@dvoytenko PR ready!

@dvoytenko
Copy link
Contributor

Thanks a lot! Merged!

@lannka
Copy link
Contributor

lannka commented Aug 27, 2020

A side note: this issue was also seen on amp-next-page causing more unnecessary ad requests . fixed by the same PR. thank you @oliverfernandez

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

Successfully merging a pull request may close this issue.

3 participants