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 and JetPack Compare Images block compatibility #3383

Closed
4 tasks
NataliaDrause opened this issue Dec 3, 2020 · 4 comments · Fixed by wp-media/rocket-lazyload-common#92 or #3497
Closed
4 tasks
Assignees
Labels
effort: [XS] < 1 day of estimated development time priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@NataliaDrause
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version - yes
  • Used the search feature to ensure that the bug hasn’t been reported before -yes

Describe the bug
Images are not being displayed when added with the JetPack Compare Image block.
The markup is cut, so only the pixel svg is visible when I inspect the element, but not the image source.
Screenshots:
With LazyLoad disabled: https://jmp.sh/O7wBHsF
With LazyLoad enabled: https://jmp.sh/ZOZZB7N

To Reproduce
Steps to reproduce the behavior:

  1. Create a post with a Compare Image block.
  2. Enable LazyLoad in WP Rocket
  3. Open the page and the images are not displayed as in the screenshot here: https://jmp.sh/RrOeolq

Expected behavior
The images have to be displayed as here: https://jmp.sh/14xZcAi

Current solution
Exclude class name class="image-compare from LazyLoad option.
Block markup in source code before LL is applied: https://jmp.sh/HnYusFT

Additional context
The Compare block works with JetPack lazy-load and with WordPress core lazy-load because images are actually not being lazyloaded, i.e. excluded.

Related ticket: https://secure.helpscout.net/conversation/1351509800/217772/
Slack: https://wp-media.slack.com/archives/G01BGNQU7AP/p1606920475010500

Possibly adding this LazyLoad exclusion automatically would be a solution from our end?
Or the compatibility needs to be done by JetPack?

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@arunbasillal arunbasillal added module: lazyload needs: grooming priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior labels Dec 3, 2020
@Tabrisrp
Copy link
Contributor

Reproduce the issue ✅

Reproduced on local test site

Identify the root cause ✅

The code generated from the plugin itself is correct, as can be seen in the source code:

<img id="1756" src="data:image/svg+xml,%3Csvg%20xmlns='http://www.w3.org/2000/svg'%20viewBox='0%200%20801%20801'%3E%3C/svg%3E" alt="" width="801" height="801" class="image-compare__image-before" data-lazy-src="https://tests.local/wp-content/uploads/2019/05/tshirt.jpg"/>
<noscript>
    <img id="1756" src="https://tests.local/wp-content/uploads/2019/05/tshirt.jpg" alt="" width="801" height="801" class="image-compare__image-before"/>
</noscript>
<img id="1757" src="data:image/svg+xml,%3Csvg%20xmlns='http://www.w3.org/2000/svg'%20viewBox='0%200%20801%20800'%3E%3C/svg%3E" alt="" width="801" height="800" class="image-compare__image-after" data-lazy-src="https://tests.local/wp-content/uploads/2019/05/vneck-tee.jpg"/>
<noscript>
    <img id="1757" src="https://tests.local/wp-content/uploads/2019/05/vneck-tee.jpg" alt="" width="801" height="800" class="image-compare__image-after"/>
</noscript>

The issue is happening after when the comparing script kicks in, and doesn't work correctly with the lazyloaded markup.

Scope a solution ✅

Adding image-compare__ to the exclusion in RocketLazyload\Images::getExcludedAttributes() will exclude those images from lazyload and solve the problem.

This fixture data can be added to our automated tests for validation and regression prevention.

Estimate the effort ✅

Effort [XS]

@Tabrisrp Tabrisrp added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Dec 17, 2020
@NataliaDrause
Copy link
Contributor Author

Related: https://secure.helpscout.net/conversation/1372584429/224759/
adding image-compare__ to the exclusion list resolved the issue.

@arunbasillal
Copy link
Contributor

@engahmeds3ed @piotrbak This might be ready for 3.8.3 for release next week? Shall I update the milestone?

@piotrbak
Copy link
Contributor

@arunbasillal From my point of view, I'm going to start QA in a couple of minutes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time priority: medium Issues which are important, but no one will go out of business. type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
5 participants