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

ATF image in root directory isnot excluded from LL with specific markup #6536

Closed
Mai-Saad opened this issue Apr 1, 2024 · 6 comments · Fixed by #6556
Closed

ATF image in root directory isnot excluded from LL with specific markup #6536

Mai-Saad opened this issue Apr 1, 2024 · 6 comments · Fixed by #6556
Assignees
Labels
effort: [XS] < 1 day of estimated development time priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@Mai-Saad
Copy link

Mai-Saad commented Apr 1, 2024

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

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

Describe the bug
Having atf image in the root directory with the following markup , is not excluded from LL
<img src="test.png" alt="test1" width="200" height="500"> or

<picture>
<img src="test.png">
</picture>

To Reproduce
Steps to reproduce the behavior:

  1. install/activate 3.16 + activate lazyload feature
  2. permalinks without /
  3. page template with image in the root directory
  4. visit the page template
  5. clear cache and revisit the page

Expected behavior
A clear and concise description of what you expected to happen.
ATF/LCP is excluded from LL

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

  • adding / before the image will work

Acceptance Criteria (for WP Media team use only)
Clear instructions for developers, to be added before the grooming

@Mai-Saad Mai-Saad added type: bug Indicates an unexpected problem or unintended behavior lcp labels Apr 1, 2024
@Mai-Saad Mai-Saad changed the title image in root directory isnot excluded from lLL with specific markup ATF image in root directory isnot excluded from LL with specific markup Apr 1, 2024
@piotrbak piotrbak added this to the 3.16 milestone Apr 1, 2024
@Mai-Saad Mai-Saad added priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available labels Apr 1, 2024
@Tabrisrp
Copy link
Contributor

Tabrisrp commented Apr 8, 2024

What is the related DB data entry for this case looks like?

@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

I was able to reproduce the problem with the steps below:

  1. install/activate 3.16 + activate lazyload feature
  2. page template with image in the root directory without a forward slash(<img src="image.jpg" alt="image" />)
  3. visit the page template
  4. clear cache and revisit the page
  5. Check the markup, you'll still find lazyload applied: <img src="image.jpg" data-lazy-src="image.jpg" data-ll-status="loaded" class="entered lazyloaded">
    When you log the entries of excluded patterns with the filter - rocket_lazyload_excluded_src You'll get:
Array
(
    [0] => /wpcf7_captcha/
    [1] => timthumb.php?src
    [2] => woocommerce/assets/images/placeholder.png
    [3] => /id/10/2500/1667.jpg
    [4] => /image.jpg
)

Identify the root cause ✅

The reason for this behaviour is because wp_parse_url returns a path with a forward slash like so - /image.jpg here:

$exclusion = wp_parse_url( $exclusion );

With that there would be a mismatch when matching /image.jpg with image.jpg to exclude.

Scope a solution ✅

We can ltrim the forward slash off the returned paths here:

$exclusion = wp_parse_url( $exclusion );

Estimate the effort ✅

[XS]

@MathieuLamiot
Copy link
Contributor

@jeawhanlee Have you performed basic checks to ensure the code does not break on happy path and that the reported failing cases are fixed? In which case, we can merge so that we only have to work on the feature branch. OK? Thanks 🙏

@MathieuLamiot
Copy link
Contributor

@jeawhanlee Kind reminder. It seems like an issue that could be "Test by devs" @wp-media/qa-team no? In which case, if @jeawhanlee validated already, we could merge and close I think :)

@jeawhanlee
Copy link
Contributor

I have validated and adapted tests that works.

@MathieuLamiot
Copy link
Contributor

Let's merge then! 🚀

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. severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants