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

Largest element with a background-image that is not kept (such as linear-gradient) makes the OCI optimization have no LCP #6728

Closed
MathieuLamiot opened this issue Jun 19, 2024 · 16 comments · Fixed by wp-media/rocket-scripts#33 or #7036
Assignees
Labels
module: OCI Optimize Critical Images priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@MathieuLamiot
Copy link
Contributor

Context
The issue is reported here: https://wp-media.slack.com/archives/C072P5EU5DF/p1718772180824549

Checking with WPR Inspector, I can see the beacon returns this to AJAX:
image

I added a JS file identical to the beacon in Chrome dev tools, added a log of firstElementWithInfo and found the LCP element was div.x-section.e9265-e1.m75d-0.m75d-1.hero-section.
It is indeed the largest element with a background-image.
But the background image is linear-gradient(164deg, rgb(0, 0, 0) 0%, rgb(0, 65, 85) 53%, rgb(35, 213, 171) 97%) which is not kept in the else branch of _getElementInfo (I think it does not passes the regex there).
As a result, the method _getElementInfo returns an element with an elementInfo, but it is empty. This empty element takes the place of LCP.

Expected behavior
TBC with Product team: I think we don't want to account for linear-gradient and instead, pick the next element as LCP. Is this correct?

Acceptance Criteria
TBC with Product team.

Additional information
If we want to skip elements with a background-image that is set but not as expected currently (for instance linaer-gradient), then I as able to get this behavior by just adding:

			if (element_info.bg_set.length <= 0) {
				return null;
			}

in _getElementInfo just before if (element_info.bg_set.length > 0) {

@DahmaniAdame
Copy link
Contributor

DahmaniAdame commented Jun 20, 2024

LCP elements should only be considered if there is an actual image on it. It's safe to skip to the next candidate if only the gradient is declared.

The element will still be a valid LCP if a gradient + an image is being used. Example:

background: linear-gradient(164deg,rgba(0,0,0,1) 0%,rgba(0,65,85,1) 53%,rgba(35,213,171,1) 97%), url(https://dummyimage.com/600x400/000/fff);

We should still reference https://dummyimage.com/600x400/000/fff as the image to optimize in this case.

@tristan1970
Copy link

I really need this issue to be fixed. Any idea when this can be expected?

@piotrbak
Copy link
Contributor

piotrbak commented Jul 3, 2024

@MathieuLamiot Wouldn't it be related?
#6599

@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement lcp labels Jul 3, 2024
@MathieuLamiot
Copy link
Contributor Author

A little bit. 6599 can make the LCP field being empty. This issue is to fallback on another LCP candidate in that case.

@piotrbak piotrbak added module: OCI Optimize Critical Images priority: low Issues that can wait and removed lcp labels Jul 17, 2024
@tristan1970
Copy link

Any updates? I am in dire need of having this fixed!

@juricazuanovic
Copy link
Contributor

@DahmaniAdame
Copy link
Contributor

@piotrbak can we bump the priority on this one? I'm seeing more page builders/themes affected by this behavior.
Sample for QA for testing: https://gist.github.com/DahmaniAdame/7ba7c1d11772fe33f2d6a1ad1488235b

@piotrbak piotrbak added priority: high Issues which should be resolved as quickly as possible and removed priority: low Issues that can wait labels Aug 28, 2024
@timbad2
Copy link

timbad2 commented Sep 6, 2024

I signed up for WP Rocket only recently, only to discover that this is a crucial issue holding back my site's LCP problem.
I use Thrive Theme Builder, from Thrive Themes, which has a background image just like this, and I'm currently wondering whether I wasted my money on this plugin WP Rocket.

Would you please share the projected ETA for this bugfix?

@WordPresseur
Copy link
Contributor

@timbad2
Copy link

timbad2 commented Sep 20, 2024

https://secure.helpscout.net/conversation/2688201235/509015/

Hi, I cannot access that site.
Does it say when this issue will be fixed?

If not, would someone please provide an ETA?

@viobru
Copy link
Contributor

viobru commented Sep 27, 2024

@viobru
Copy link
Contributor

viobru commented Oct 1, 2024

@piotrbak
Copy link
Contributor

piotrbak commented Oct 1, 2024

We should make sure that #6995 is fixed while fixing this one.

@piotrbak piotrbak added this to the 3.17.2 milestone Oct 6, 2024
@Miraeld Miraeld self-assigned this Oct 7, 2024
@Miraeld
Copy link
Contributor

Miraeld commented Oct 7, 2024

Reproduce the problem

To reproduce the issue, refer to #6995 as it explains how to trigger the issue.

Identify the root cause

The issue is that the _getElementInfo method in BeaconLcp.js does not properly skip elements with background-images that do not contain actual image URLs, leading to incorrect LCP detection.

Scope a solution

To solve the issue, we must update the _getElementInfo method to skip elements with background-images that do not contain actual image URLs. Additionally, ensure elements with both gradients and image URLs are still considered valid LCP candidates.
To do this, in here we can add the following:

            if (element_info.bg_set.length <= 0) {
                return null;
            }

This change ensures that elements with only gradients are evicted by checking if the bg_set array, which contains the URLs of background images, is empty. If bg_set is empty, it means there are no valid image URLs, and the element is skipped. Gradients do not have URLs, so this check effectively filters out elements with only gradients.

Also, we should change this line within _initWithFirstElementWithInfo to

const firstElementWithInfo = elements.find(item => {
            return item.elementInfo !== null && (item.elementInfo.src || item.elementInfo.srcset);
        });

in order to ensure that the firstElementWithInfo not only has a non-null elementInfo but also contains a valid src or srcset property

Development steps

  • Modify _getElementInfo function
  • Modify _initWithFirstElementWithInfo function
  • Perform tests

Effort estimation

S

@Miraeld Miraeld removed their assignment Oct 7, 2024
@Khadreal
Copy link
Contributor

Khadreal commented Oct 7, 2024

LGTM

@hanna-meda hanna-meda self-assigned this Oct 14, 2024
@hanna-meda
Copy link

hanna-meda commented Oct 14, 2024

Related Test Plan HERE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: OCI Optimize Critical Images priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet