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

Closes #6269: Lazyload CSS background image compatibility issue with Chrome 119+ #6300

Merged
merged 10 commits into from
Jan 1, 2024

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented Dec 1, 2023

Description

Fix the issue of compatibility with several browser including Chrome 119+

Fixes #6269

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).

Is the solution different from the one proposed during the grooming?

No

Checklists

Generic development checklist

  • My code follows the style guidelines of this project, with adapted comments and without new warnings.
  • I have added unit and integration tests that prove my fix is effective or that my feature works.
  • The CI passes locally with my changes (including unit tests, integration tests, linter).
  • Any dependent changes have been merged and published in downstream modules.
  • If applicable, I have made corresponding changes to the documentation. Provide a link to the documentation.

Test summary

  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I validated all Acceptance Criteria of the related issues. (If applicable, provide proof).
  • I validated all test plan the QA Review asked me to.

@Miraeld Miraeld added type: bug Indicates an unexpected problem or unintended behavior 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting module: lazyload background css images labels Dec 1, 2023
@Miraeld Miraeld requested a review from a team December 1, 2023 14:13
@Miraeld Miraeld self-assigned this Dec 1, 2023
@Mai-Saad Mai-Saad self-requested a review December 6, 2023 09:39
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Dec 6, 2023

@Miraeld Thanks for the PR.
When tried to use the PR on the test page https://new.rocketlabsqa.ovh/ll-bgi-oct-ar/ and enable LLBGI, images were not displayed and had 403 console error for those images. can you please check 🙏
Screenshot from 2023-12-06 11-45-35

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Dec 7, 2023

Test plan
1- Execute the e2e smoke tests for LLBG https://github.com/wp-media/wp-rocket-e2e => if any test failed, try to execute the same test manually to see if the problem in test or plugin side.
2- Manually, check the main issue mentioned on the ticket on Chrome 119 , version before and version after if any => related test case here https://wpmediaqa.testrail.io/index.php?/runs/view/794&group_by=cases:section_id&group_order=asc
3-Check if the feature is working with WPML (directory base, domain base and qs base) => related test here https://wpmediaqa.testrail.io/index.php?/runs/view/794&group_by=cases:section_id&group_order=asc
@Miraeld please let me know which tests you did, to check the rest if any, beside the exploratory test 🙏

@Miraeld Miraeld requested a review from a team December 7, 2023 14:05
@Miraeld
Copy link
Contributor Author

Miraeld commented Dec 8, 2023

@Mai-Saad ,

Test plan
1- Execute the e2e smoke tests for LLBG https://github.com/wp-media/wp-rocket-e2e => if any test failed, try to execute the same test manually to see if the problem in test or plugin side.
2- Manually, check the main issue mentioned on the ticket on Chrome 119 , version before and version after if any => related test case here https://wpmediaqa.testrail.io/index.php?/runs/view/794&group_by=cases:section_id&group_order=asc
3-Check if the feature is working with WPML (directory base, domain base and qs base) => related test here https://wpmediaqa.testrail.io/index.php?/runs/view/794&group_by=cases:section_id&group_order=asc
@Miraeld please let me know which tests you did, to check the rest if any, beside the exploratory test 🙏

1- This repo is broken for me, but I've successfully tested it manually.
2- I did test manually on a Chrome 119 and 118. Everything is ok
3- I did not check as I can't setup WPML in local properly.

@Mai-Saad
Copy link
Contributor

@Miraeld Thanks for the update. Please note that some images are not displayed now using the test page https://new.rocketlabsqa.ovh/ll-bgi-oct-ar/ (some have console error) => this is a regression and working fine on the trunk. can you please check 🙏
PR merged to dev
Screenshot from 2023-12-11 09-40-41
Trunk
Screenshot from 2023-12-11 09-43-23

@Miraeld Miraeld force-pushed the fix/6269-lazyload-css-bg-compatibility-chrome-119 branch from 77d5cc3 to 1710d1e Compare December 14, 2023 10:46
@Miraeld Miraeld force-pushed the fix/6269-lazyload-css-bg-compatibility-chrome-119 branch from 1710d1e to 8f28fe8 Compare December 14, 2023 11:11
@remyperona remyperona added this to the 3.15.7 milestone Dec 15, 2023
@@ -178,9 +178,6 @@ public function has( $key ) {
*/
public function generate_url( string $url ): string {
$path = $this->generate_path( $url );
if ( ! $this->filesystem->exists( $path ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Miraeld is it intended to remove this check? (it seems was here to guard against error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an intentional decision.

While addressing the URL problem you mentioned earlier, I encountered another issue. The url I received turned out to be a relative path. In other words, it was attempting to load images using URLs like https://domain.com/var/www/html/wp-content/rocket-test-data/image.jpg.

To resolve this, I introduced the transform_relative_to_absolute function. Its purpose is to convert a relative path to an absolute one. However, constructing an absolute path requires knowledge of the source URL. In my case, the URL doesn't originate from the original CSS file but rather from the cached version. Unfortunately, the function responsible for this process was bailing out prematurely, making it impossible to obtain the URL of the cached version, which is saved at the end of the process.

If the function doesn't bail out, you can retrieve the URL of the cached version CSS. Subsequently, you can build an absolute URL and, consequently, display the image with the correct URL.

@Mai-Saad
Copy link
Contributor

@Miraeld Thanks for the update. from the smoke test, no console errors now 🎉 , but the image (mentioned here https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/butterfly.avif ) isnot displayed on the optimized page compared to nowprocket. can you please check?

@Miraeld
Copy link
Contributor Author

Miraeld commented Dec 21, 2023

@Mai-Saad , well, I found out why that image wasn't displayed.
It seems like in this PR, in the JS we've been using the object Image and as tests are running with Chromium, it isn't compatible with .tiff images which means it couldn't do img.onload()

I've changed it back so it's working fine now, and e2e tests are all passing & green for me.

@Mai-Saad
Copy link
Contributor

fixed on chrome. but still having multiple 404 errors on firefox and safari
Screenshot from 2023-12-22 13-36-25
Screenshot from 2023-12-22 13-29-12

@Miraeld
Copy link
Contributor Author

Miraeld commented Dec 22, 2023

@piotrbak & @Mai-Saad , as we've been checking with @CrochetFeve0251 , it seems like on Firefox & Safari, each time there is a global CSS selector, it will reload / recompile the CSS which cause multiple 404 messages on these browsers.

In our case and on the test page, that statement is [title ~= "wp-rocket"].
We couldn't find a way to fix it on Firefox & Safari as it's at the browser level, and we can't find a solution for neither of these two at the moment.

@piotrbak
Copy link
Contributor

@Miraeld @Mai-Saad okay, let's skip it then and move it to separate issue for visibility in the future

@Miraeld
Copy link
Contributor Author

Miraeld commented Dec 29, 2023

I've performed e2e llcssbg test and everything pass

4 scenarios (4 passed)
31 steps (31 passed)

@Mai-Saad Mai-Saad added this pull request to the merge queue Jan 1, 2024
Merged via the queue into develop with commit 29697ac Jan 1, 2024
8 checks passed
@Mai-Saad Mai-Saad deleted the fix/6269-lazyload-css-bg-compatibility-chrome-119 branch January 1, 2024 10:11
wordpressfan pushed a commit that referenced this pull request Mar 15, 2024
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 module: lazyload background css images type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazyload CSS background image compatibility issue with Chrome 119+
5 participants