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

LazyLoad Inline CSS isn't removed on pages generated by AMP plugin #2523

Closed
4 tasks done
GeekPress opened this issue Apr 7, 2020 · 11 comments · Fixed by #2763
Closed
4 tasks done

LazyLoad Inline CSS isn't removed on pages generated by AMP plugin #2523

GeekPress opened this issue Apr 7, 2020 · 11 comments · Fixed by #2763
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [XS] < 1 day of estimated development time priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@GeekPress
Copy link
Contributor

GeekPress commented Apr 7, 2020

Describe the bug
Since 1.50. AMP plugin introduced a new template mode: Transitional

When our LazyLoad is active, the inline CSS related to this feature isn't removed from the AMP page version.

It will cause an error during the AMP validation process

To Reproduce
Steps to reproduce the behavior:

  1. Install AMP plugin
  2. In settings select « Transitional » template mode. The AMP URL should use a query string like mashingcoding.com/2012/01/07/template-sticky/?amp
  3. Activate all Lazyload options into WP Rocket
  4. See the inline CSS in the source code
<style id='rocket-lazyload-inline-css' type='text/css'>
.rll-youtube-player{position:relative;padding-bottom:56.23%;height:0;overflow:hidden;max-width:100%;}.rll-youtube-player iframe{position:absolute;top:0;left:0;width:100%;height:100%;z-index:100;background:0 0}.rll-youtube-player img{bottom:0;display:block;left:0;margin:auto;max-width:100%;width:100%;position:absolute;right:0;top:0;border:none;height:auto;cursor:pointer;-webkit-transition:.4s all;-moz-transition:.4s all;transition:.4s all}.rll-youtube-player img:hover{-webkit-filter:brightness(75%)}.rll-youtube-player .play{height:72px;width:72px;left:50%;top:50%;margin-left:-36px;margin-top:-36px;position:absolute;background:url(http://smashingcoding.com/wp-content/plugins/wp-rocket/assets/img/youtube.png) no-repeat;cursor:pointer}.wp-has-aspect-ratio .rll-youtube-player{position:absolute;padding-bottom:0;width:100%;height:100%;top:0;bottom:0;left:0;right:0}
</style>

And the <noscript> tag which trigger the error during the validation process:
<noscript><style id="rocket-lazyload-nojs-css">.rll-youtube-player, [data-lazy-src]{display:none !important;}</style></noscript>

Expected behavior
No inline CSS related to LazyLoad on AMP pages when Transitional mode is selected.

Screenshots
https://d33v4339jhl8k0.cloudfront.net/inline/18928/f09382920cc782ff56d82f2c30d831ea76bae97c/76e2fbb098efe191837410b679ff62852b011295/image-1.png

Additional context
After deactivated the LazyLoad iframe, the <noscript> tag which contains a small inline CSS is still present.

This code is removed after turning off all LazyLoad options.

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@GeekPress GeekPress added type: bug Indicates an unexpected problem or unintended behavior 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting priority: high Issues which should be resolved as quickly as possible needs: grooming labels Apr 7, 2020
@Tabrisrp
Copy link
Contributor

I tried to replicate this issue with transitional mode on the AMP plugin version 1.5.3, with Storefront and TwentyTwenty themes, and the lazyload inline code is not present on the pages.

@GeekPress
Copy link
Contributor Author

I was able to reproduce the issue on smashingcoding before to submit the issue.

But I can't reproduce it anymore... Let's close it, and re-open it if a new case will be detected.

@GeekPress GeekPress added can't reproduce Issues that can't be reproduced and removed needs: grooming labels Apr 23, 2020
@branko-md
Copy link

branko-md commented May 13, 2020

Hello, we updated official AMP plugin and WP-Rocket plugin on websites that we have amp set up on.
And we get this issue:
screenshot of WordPress amp validator: https://prnt.sc/sfxom8 "The parent tag of tag 'style amp-custom' is 'noscript', but it can only be 'head'."
screenshot of google amp validator: https://prnt.sc/sfxot5
Issue seems to be in this style tag that has the id "rocket-lazyload-inline-css".

Now, if we go and disable this lazy-load option "Enable for iframes and videos", in wp rocket settings https://prnt.sc/sfxs18 , validator error goes away and page is valid, https://prnt.sc/sfxtcy .

So, at this moment we can't have lazyloading for iframes and videos enabled on websites that we have amp on.
Additional notes, Amp transitional mode is enabled.
Any help is appreciated.

@piotrbak
Copy link
Contributor

I have one customer with the same problem. On his live site, there are two occurrences of:
<noscript><style id="rocket-lazyload-nojs-css">.rll-youtube-player, [data-lazy-src]{display:none !important;}</style></noscript>

However, the problem doesn't exist on exactly the same page on WP Stagecoach.

@piotrbak
Copy link
Contributor

It looks like this is our fault. In most cases, AMP plugin (most likely) is clearing it on its own and changing our <noscript> tags to the <!--noscript-->:
<!--noscript--><!--/noscript--><!--noscript--><!--/noscript-->
https://jmp.sh/oe0vOmO
vs
<style id="rocket-lazyload-nojs-css">.rll-youtube-player, [data-lazy-src]{display:none !important;}</style></noscript><noscript><style id="rocket-lazyload-nojs-css">.rll-youtube-player, [data-lazy-src]{display:none !important;}</style></noscript>
https://jmp.sh/iakes4r

However, for some reason, it sometimes might not happen and we have a problem then.

That's because we're not filtering do_rocket_lazyload_iframes in AMP compatibility, is that expected?

We're passing this if on AMP pages then:

if ( ! $this->can_lazyload_images() && ! $this->can_lazyload_iframes() ) {

@piotrbak piotrbak reopened this May 14, 2020
@GeekPress
Copy link
Contributor Author

That's because we're not filtering do_rocket_lazyload_iframes in AMP compatibility, is that expected?

@Tabrisrp If you can help us here :)

@Tabrisrp
Copy link
Contributor

It was not an issue before, so it's something we didn't add, but now we can 👍

@GeekPress
Copy link
Contributor Author

Ok, let's filter do_rocket_lazyload_iframes when we are on AMP to be sure to avoid any issues.

@Tabrisrp Do you have enough info to be able to make a fix?

@JohnTorvik
Copy link

JohnTorvik commented May 23, 2020

@GeekPress GeekPress added needs: grooming and removed can't reproduce Issues that can't be reproduced labels Jun 10, 2020
@GeekPress GeekPress added this to the 3.6.1 milestone Jun 10, 2020
@Tabrisrp
Copy link
Contributor

Scope a solution
Add add_filter( 'do_rocket_lazyload_iframes', '__return_false' ); to the AMP compatibility method disable_options_on_amp()

Estimate the effort
Effort [XS], small update to existing tests

@Tabrisrp Tabrisrp added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Jun 10, 2020
Tabrisrp added a commit that referenced this issue Jun 12, 2020
)

* disable iframes lazyload on AMP pages
* update tests
iCaspar pushed a commit that referenced this issue Jun 16, 2020
)

* disable iframes lazyload on AMP pages
* update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [XS] < 1 day of estimated development time priority: high Issues which should be resolved as quickly as possible type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants